-
Notifications
You must be signed in to change notification settings - Fork 91
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.
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
Changes from 8 commits
db41d5f
16e8841
015ff8a
3b33824
ee4e25b
ca1f016
9563814
5c20c2e
fa4342e
00b642a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,10 +25,15 @@ export RAPIDS_ARTIFACTS_DIR | |
| # populates `RATTLER_CHANNELS` array and `RATTLER_ARGS` array | ||
| source rapids-rattler-channel-string | ||
|
|
||
| # Use sed to replace the RAPIDS_CUDA_VERSION string in the variants file with | ||
| # the value of that variable in this shell. | ||
| sed -i "s/RAPIDS_CUDA_VERSION/${RAPIDS_CUDA_VERSION}/g" "conda/recipes/libkvikio/variants_$(arch).yaml" | ||
|
|
||
|
Comment on lines
+28
to
+58
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 "conda/recipes/libkvikio/variants_$(arch).yaml" \ | ||
| "${RATTLER_ARGS[@]}" \ | ||
| "${RATTLER_CHANNELS[@]}" | ||
|
|
||
|
|
||
| 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" | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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") }} | ||
|
vyasr marked this conversation as resolved.
|
||
| # 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we drop these?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
|
@@ -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") }} | ||
|
|
@@ -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 | ||
|
|
@@ -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: | ||
|
|
@@ -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") }} | ||
|
vyasr marked this conversation as resolved.
Outdated
|
||
| 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 | ||
|
|
@@ -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: | ||
|
|
@@ -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") }} | ||
|
vyasr marked this conversation as resolved.
Outdated
|
||
| 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 }} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| zip_keys: | ||
| - [c_compiler_version, cxx_compiler_version, cuda_version, should_use_cufile] | ||
|
|
||
| c_compiler_version: | ||
| - 12 | ||
| - 13 | ||
|
|
||
| cxx_compiler_version: | ||
| - 12 | ||
| - 13 | ||
|
|
||
| cuda_version: | ||
| - 12.1 # The last version to not support cufile | ||
| - RAPIDS_CUDA_VERSION | ||
|
|
||
| should_use_cufile: | ||
| - false | ||
| - true | ||
|
vyasr marked this conversation as resolved.
Outdated
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| c_compiler_version: | ||
| - 13 | ||
|
|
||
| cxx_compiler_version: | ||
| - 13 | ||
|
|
||
| cuda_version: | ||
| - RAPIDS_CUDA_VERSION | ||
|
|
||
| should_use_cufile: | ||
| - true |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing that has worked well before, is to output this to a variant file
It does a similar thing admittedly. Though it can be a bit clearer about what is happening. That could just be personal preference though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that alternative! I agree it is clearer.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered this possibility as well. However, in this instance it's not quite so simple as the pynvjitlink example because we need other things to be encoded in the variants as well. The behavior on x86 and arm is different because the former is a single variant while the latter requires two, so we would need to write out variants conditionally based on that. Moreover, for arm we need to use a different compiler version for each CUDA version, so it's a longer file. If you both prefer that I can switch to it, we just wind up with two fairly long here-docs instead of this command. The flip side is that we can drop the committed variants files if we choose to go that route.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made the changes in 00b642a. If you like it we can stick with it, otherwise feel free to revert that commit.