Skip to content

Use variants to produce separate builds with and without cufile support #754

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rapids-bot merged 10 commits into from
Jun 13, 2025
Merged

Use variants to produce separate builds with and without cufile support #754

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
db41d5f
Use preexisting linux64 variable
vyasr Jun 12, 2025
16e8841
Add branching logic to recipe
vyasr Jun 12, 2025
015ff8a
First pass at creating variants
vyasr Jun 12, 2025
3b33824
Clean up some variable naming
vyasr Jun 12, 2025
ee4e25b
Style
vyasr Jun 12, 2025
ca1f016
Fix zip_keys spec
vyasr Jun 13, 2025
9563814
Avoid constexpr
vyasr Jun 13, 2025
5c20c2e
Skip tests if not built with cufile support
vyasr Jun 13, 2025
fa4342e
Apply most review comments
vyasr Jun 13, 2025
00b642a
Inline variant files via here-docs
vyasr Jun 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions ci/build_cpp.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,42 @@ export RAPIDS_ARTIFACTS_DIR
# populates `RATTLER_CHANNELS` array and `RATTLER_ARGS` array
source rapids-rattler-channel-string

# Construct the extra variants according to the architecture
if [[ "$(arch)" == "x86_64" ]]; then
cat > variants.yaml << EOF
c_compiler_version:
- 13

cxx_compiler_version:
- 13

cuda_version:
- ${RAPIDS_CUDA_VERSION}
EOF
else
cat > variants.yaml << EOF
zip_keys:
- [c_compiler_version, cxx_compiler_version, cuda_version]

c_compiler_version:
- 12
- 13

cxx_compiler_version:
- 12
- 13

cuda_version:
- 12.1 # The last version to not support cufile
- ${RAPIDS_CUDA_VERSION}
EOF
fi

Comment on lines +28 to +58
Copy link
Contributor

@gforsyth gforsyth Jun 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking, but I think this would be better to encode in individual variant_$ARCH_$CUDA_VERSION.yaml files and then the branching logic here can be to select the appropriate variant file for the build at hand.

That way the various build variants are all in the conda recipe file and not hidden away in this bash script.

I'm fine with this being done in a follow-up to get CI unblocked.

Copy link
Contributor

@gforsyth gforsyth Jun 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol, I see that was your original approach. well, not worth going back and forth over it.

My general take is "repeat yourself" if it keeps things clean, and heredocs in a random bash script isn't worth the tradeoff to me.

Copy link
Contributor Author

@vyasr vyasr Jun 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I liked the old way better too. This version feels wrong in how much of the logic is moved out of the recipe itself. I think we should reconsider what approach we like best, but I'll merge as-is to get things unblocked for now.

Copy link
Member

@jakirkham jakirkham Jun 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provided a simplification on the current approach in PR: #755

# --no-build-id allows for caching with `sccache`
# more info is available at
# https://rattler.build/latest/tips_and_tricks/#using-sccache-or-ccache-with-rattler-build
rattler-build build --recipe conda/recipes/libkvikio \
--variant-config variants.yaml \
"${RATTLER_ARGS[@]}" \
"${RATTLER_CHANNELS[@]}"

Expand Down
6 changes: 0 additions & 6 deletions conda/recipes/libkvikio/conda_build_config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,3 @@
c_compiler_version:
- 13

cxx_compiler_version:
- 13

cmake_version:
- ">=3.30.4"

Expand Down
54 changes: 40 additions & 14 deletions conda/recipes/libkvikio/recipe.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,18 @@ schema_version: 1
context:
version: ${{ env.get("RAPIDS_PACKAGE_VERSION") }}
minor_version: ${{ (version | split("."))[:2] | join(".") }}
cuda_version: ${{ (env.get("RAPIDS_CUDA_VERSION") | split("."))[:2] | join(".") }}
cuda_major: '${{ (env.get("RAPIDS_CUDA_VERSION") | split("."))[0] }}'
# We need to support three cases:
# 1. Linux x86_64, which always uses libcufile
# 2. Linux aarch64 with CUDA >= 12.2, which uses libcufile
# 3. Linux aarch64 with CUDA < 12.2, which does not use libcufile
# Each case has different cuda-version constraints as expressed below
should_use_cufile: ${{ x86_64 or (aarch64 and cuda_version >= "12.2") }}
# When reverting, instances of cuda_key_string can be replaced with cuda_major
cuda_key_string: ${{ cuda_version | replace(".", "_") }}
#cuda_version: ${{ (env.get("RAPIDS_CUDA_VERSION") | split("."))[:2] | join(".") }}
#cuda_major: '${{ (env.get("RAPIDS_CUDA_VERSION") | split("."))[0] }}'
Comment on lines +15 to +16
Copy link
Member

@jakirkham jakirkham Jun 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we drop these?

Copy link
Contributor Author

@vyasr vyasr Jun 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose to keep them to simplify reverting the changes, but if others feel like it would be cleaner to drop these I am fine with that.

date_string: '${{ env.get("RAPIDS_DATE_STRING") }}'
head_rev: '${{ git.head_rev(".")[:8] }}'
linux64: ${{ linux and x86_64 }}

recipe:
name: libkvikio-split
Expand Down Expand Up @@ -46,7 +53,7 @@ cache:
SCCACHE_REGION: ${{ env.get("SCCACHE_REGION") }}
SCCACHE_S3_USE_SSL: ${{ env.get("SCCACHE_S3_USE_SSL") }}
SCCACHE_S3_NO_CREDENTIALS: ${{ env.get("SCCACHE_S3_NO_CREDENTIALS") }}
SCCACHE_S3_KEY_PREFIX: libkvikio/${{ env.get("RAPIDS_CONDA_ARCH") }}/cuda${{ cuda_major }}
SCCACHE_S3_KEY_PREFIX: libkvikio/${{ env.get("RAPIDS_CONDA_ARCH") }}/cuda${{ cuda_key_string }}
requirements:
build:
- ${{ compiler("c") }}
Expand All @@ -59,7 +66,7 @@ cache:
host:
- cuda-version =${{ cuda_version }}
- libcurl ${{ libcurl_version }}
- if: (linux and x86_64) or (linux and aarch64 and cuda_version >= "12.2")
- if: should_use_cufile
then:
- libcufile-dev
- libnuma
Expand All @@ -72,7 +79,7 @@ outputs:
script:
content: |
cmake --install cpp/build
string: cuda${{ cuda_major }}_${{ date_string }}_${{ head_rev }}
string: cuda${{ cuda_key_string }}_${{ date_string }}_${{ head_rev }}
dynamic_linking:
overlinking_behavior: "error"
prefix_detection:
Expand All @@ -86,15 +93,25 @@ outputs:
- cuda-version =${{ cuda_version }}
- libcurl ${{ libcurl_version }}
run:
- ${{ pin_compatible("cuda-version", upper_bound="x", lower_bound="x") }}
- if: linux and x86_64
- if: x86_64
then:
- ${{ pin_compatible("cuda-version", upper_bound="x", lower_bound="x") }}
else:
- if: aarch64 and cuda_version >= "12.2"
then:
- ${{ pin_compatible("cuda-version", upper_bound="x", lower_bound="12.2.0a0") }}
else:
- ${{ pin_compatible("cuda-version", upper_bound="12.2.0a0", lower_bound="12.0") }}
- if: should_use_cufile
then:
- libcufile-dev
ignore_run_exports:
by_name:
- cuda-version
- libcufile
- libcurl
- if: should_use_cufile
then:
- libcufile
tests:
- script:
- test -f $PREFIX/include/kvikio/file_handle.hpp
Expand All @@ -107,7 +124,7 @@ outputs:
name: libkvikio-tests
version: ${{ version }}
build:
string: cuda${{ cuda_major }}_${{ date_string }}_${{ head_rev }}
string: cuda${{ cuda_key_string }}_${{ date_string }}_${{ head_rev }}
dynamic_linking:
overlinking_behavior: "error"
script:
Expand All @@ -121,20 +138,29 @@ outputs:
- ${{ pin_subpackage("libkvikio", exact=True) }}
- cuda-version =${{ cuda_version }}
- cuda-cudart-dev
- if: linux and x86_64
- if: should_use_cufile
then:
- libcufile-dev
run:
- ${{ pin_compatible("cuda-version", upper_bound="x", lower_bound="x") }}
- if: x86_64
then:
- ${{ pin_compatible("cuda-version", upper_bound="x", lower_bound="x") }}
else:
- if: aarch64 and cuda_version >= "12.2"
then:
- ${{ pin_compatible("cuda-version", upper_bound="x", lower_bound="12.2.0a0") }}
else:
- ${{ pin_compatible("cuda-version", upper_bound="12.2.0a0", lower_bound="12.0") }}
- cuda-cudart
- libcufile
ignore_run_exports:
by_name:
- cuda-cudart
- cuda-version
- libcufile
- libcurl
- libnuma
- if: should_use_cufile
then:
- libcufile
about:
homepage: ${{ load_from_file("python/libkvikio/pyproject.toml").project.urls.Homepage }}
license: ${{ load_from_file("python/libkvikio/pyproject.toml").project.license.text }}
Expand Down
4 changes: 0 additions & 4 deletions cpp/include/kvikio/shim/cufile.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,11 +128,7 @@ bool is_cufile_available() noexcept;
*
* @return The version (1000*major + 10*minor) or zero if older than 1080.
*/
#ifdef KVIKIO_CUFILE_FOUND
int cufile_version() noexcept;
#else
constexpr int cufile_version() noexcept { return 0; }
#endif
Comment on lines -133 to -135
Copy link
Contributor Author

@vyasr vyasr Jun 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is necessary because the dead code optimizer will generally elide constexpr functions from the final binary if no runtime usage is detected, but we need the symbol in the DSO for usage from other libraries (including Python extensions). For the same reason we also avoid inlining the definition here.


/**
* @brief Check if cuFile's batch API is available.
Expand Down
2 changes: 2 additions & 0 deletions cpp/src/shim/cufile.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,8 @@ int cufile_version() noexcept
return 0;
}
}
#else
int cufile_version() noexcept { return 0; }
#endif

bool is_batch_api_available() noexcept { return cufile_version() >= 1060; }
Expand Down
86 changes: 49 additions & 37 deletions python/kvikio/tests/test_cufile_driver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,47 +13,59 @@ def test_version():


@pytest.mark.cufile
def test_open_and_close():
kvikio.cufile_driver.driver_open()
kvikio.cufile_driver.driver_close()
def test_open_and_close(request):
try:
kvikio.cufile_driver.driver_open()
kvikio.cufile_driver.driver_close()
except RuntimeError as e:
if "KvikIO not compiled with cuFile.h" in str(e):
pytest.skip("KvikIO not compiled with cuFile.h, skipping cuFile tests")


@pytest.mark.cufile
def test_property_accessor():
"""Test the method `get` and `set`"""

# Attempt to set a nonexistent property
with pytest.raises(KeyError):
kvikio.cufile_driver.set("nonexistent_property", 123)

# Attempt to get a nonexistent property
with pytest.raises(KeyError):
kvikio.cufile_driver.get("nonexistent_property")

# Attempt to set a read-only property
with pytest.raises(KeyError, match="read-only"):
kvikio.cufile_driver.set("major_version", 2077)

# Nested context managers
poll_thresh_size_default = kvikio.cufile_driver.get("poll_thresh_size")
with kvikio.cufile_driver.set("poll_thresh_size", 1024):
assert kvikio.cufile_driver.get("poll_thresh_size") == 1024
with kvikio.cufile_driver.set("poll_thresh_size", 2048):
assert kvikio.cufile_driver.get("poll_thresh_size") == 2048
with kvikio.cufile_driver.set("poll_thresh_size", 4096):
assert kvikio.cufile_driver.get("poll_thresh_size") == 4096
assert kvikio.cufile_driver.get("poll_thresh_size") == 2048
assert kvikio.cufile_driver.get("poll_thresh_size") == 1024
assert kvikio.cufile_driver.get("poll_thresh_size") == poll_thresh_size_default

# Multiple context managers
poll_mode_default = kvikio.cufile_driver.get("poll_mode")
max_device_cache_size_default = kvikio.cufile_driver.get("max_device_cache_size")
with kvikio.cufile_driver.set({"poll_mode": True, "max_device_cache_size": 2048}):
assert kvikio.cufile_driver.get("poll_mode") and (
kvikio.cufile_driver.get("max_device_cache_size") == 2048
try:
# Attempt to set a nonexistent property
with pytest.raises(KeyError):
kvikio.cufile_driver.set("nonexistent_property", 123)

# Attempt to get a nonexistent property
with pytest.raises(KeyError):
kvikio.cufile_driver.get("nonexistent_property")

# Attempt to set a read-only property
with pytest.raises(KeyError, match="read-only"):
kvikio.cufile_driver.set("major_version", 2077)

# Nested context managers
poll_thresh_size_default = kvikio.cufile_driver.get("poll_thresh_size")
with kvikio.cufile_driver.set("poll_thresh_size", 1024):
assert kvikio.cufile_driver.get("poll_thresh_size") == 1024
with kvikio.cufile_driver.set("poll_thresh_size", 2048):
assert kvikio.cufile_driver.get("poll_thresh_size") == 2048
with kvikio.cufile_driver.set("poll_thresh_size", 4096):
assert kvikio.cufile_driver.get("poll_thresh_size") == 4096
assert kvikio.cufile_driver.get("poll_thresh_size") == 2048
assert kvikio.cufile_driver.get("poll_thresh_size") == 1024
assert kvikio.cufile_driver.get("poll_thresh_size") == poll_thresh_size_default

# Multiple context managers
poll_mode_default = kvikio.cufile_driver.get("poll_mode")
max_device_cache_size_default = kvikio.cufile_driver.get(
"max_device_cache_size"
)
with kvikio.cufile_driver.set(
{"poll_mode": True, "max_device_cache_size": 2048}
):
assert kvikio.cufile_driver.get("poll_mode") and (
kvikio.cufile_driver.get("max_device_cache_size") == 2048
)
assert (kvikio.cufile_driver.get("poll_mode") == poll_mode_default) and (
kvikio.cufile_driver.get("max_device_cache_size")
== max_device_cache_size_default
)
assert (kvikio.cufile_driver.get("poll_mode") == poll_mode_default) and (
kvikio.cufile_driver.get("max_device_cache_size")
== max_device_cache_size_default
)
except RuntimeError as e:
if "KvikIO not compiled with cuFile.h" in str(e):
pytest.skip("KvikIO not compiled with cuFile.h, skipping cuFile tests")