Skip to content
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

Update for CUDA 12.3.2 #5

Merged
merged 4 commits into from
Feb 6, 2024
Merged

Conversation

adibbley
Copy link
Contributor

@conda-forge-webservices
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@adibbley
Copy link
Contributor Author

@conda-forge-admin, please rerender

@adibbley adibbley marked this pull request as ready for review January 24, 2024 15:22
@adibbley adibbley requested a review from a team as a code owner January 24, 2024 15:22
@jakirkham jakirkham self-requested a review January 25, 2024 07:55
@jakirkham
Copy link
Member

Thanks Alex! 🙏

Will take a closer look at CI tomorrow

Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Thanks Alex! 🙏

Had a couple questions below

[[ -d $PREFIX/$i/$j ]] && continue
[[ $j == "compute-sanitizer" ]] && continue

patchelf --set-rpath '$ORIGIN/../lib' $PREFIX/$i/$j
Copy link
Member

Choose a reason for hiding this comment

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

Would it work if we used the same approach as we did to resolve issue ( conda-forge/cuda-feedstock#10 )?

Suggested change
patchelf --set-rpath '$ORIGIN/../lib' $PREFIX/$i/$j
patchelf --set-rpath '$ORIGIN' $PREFIX/$i/$j

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, these will turn back to warnings:

INFO (cuda-sanitizer-api,compute-sanitizer/libTreeLauncherTargetInjection.so): Needed DSO lib/libgcc_s.so.1 found in conda-forge/linux-64::libgcc-ng==13.2.0=h807b86a_3

Copy link
Member

@jakirkham jakirkham Jan 25, 2024

Choose a reason for hiding this comment

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

Wonder if we could symlink libgcc_s.so into the same directory as libTreeLauncherTargetInjection.so

Maybe that would make the patchelf call unneeded

For more context, thinking about the patchelf call with '$ORIGIN/../lib', am wondering if that will cause issues with the targets structure used in other packages (though admittedly not as relevant here)

Edit: Also not using patchelf might allow us to keep the overlinking check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see how managing that symlink could be at all preferable to what I have here.

Why are patchelf and the overlink check a concern here when we've already done that for every other cuda library package?

The libraries here are not installed to targets, or even lib, so you are right that it is not relevant here.

@@ -1,5 +1,5 @@
conda_build:
error_overlinking: true
error_overlinking: false
Copy link
Member

Choose a reason for hiding this comment

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

Are we able to revert this after the patchelf change?

Suggested change
error_overlinking: false
error_overlinking: true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we've needed this anytime we update rpath (example: https://github.com/conda-forge/libcurand-feedstock/pull/2/files#diff-478786365dd93f740eb520c2faa03d7a0623273663b75472272c8ef94297bbe2)

This will be the error:

WARNING (cuda-sanitizer-api,compute-sanitizer/libsanitizer-public.so): runpaths ['$ORIGIN/../lib'] found in /home/conda/feedstock_root/build_artifacts/cuda-sanitizer-api_1706216449799/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_pl/compute-sanitizer/libsanitizer-public.so

@jakirkham
Copy link
Member

Was trying to get a better sense of what is going on here. So downloaded the redist archive. In looking at its contents, a couple things stood out:

  1. The compute-sanitizer directory has libraries not under a lib directory (unlike the headers in include)
  2. There are 2 layers of what appear to be the same SOs

Do we know why these are happening?

Have included the abbreviated results from running tree below for clarity:

cuda_sanitizer_api-linux-x86_64-12.3.101-archive
...
├── compute-sanitizer
│   ...
│   ├── libInterceptorInjectionTarget.so
│   ├── libsanitizer-collection.so
│   ├── libsanitizer-public.so
│   ├── libTreeLauncherPlaceholder.so
│   ├── libTreeLauncherTargetInjection.so
│   ├── libTreeLauncherTargetUpdatePreloadInjection.so
│   ├── TreeLauncherSubreaper
│   ├── TreeLauncherTargetLdPreloadHelper
│   └── x86
│       ├── libInterceptorInjectionTarget.so
│       ├── libTreeLauncherPlaceholder.so
│       ├── libTreeLauncherTargetInjection.so
│       ├── libTreeLauncherTargetUpdatePreloadInjection.so
│       ├── TreeLauncherSubreaper
│       └── TreeLauncherTargetLdPreloadHelper
...

@@ -30,7 +31,9 @@ requirements:
build:
- {{ compiler("c") }}
- {{ compiler("cxx") }}
- sysroot_{{ target_platform }} 2.17 # [linux]
Copy link
Member

Choose a reason for hiding this comment

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

Interestingly was able to get the package to build locally with only this change (adding the sysroot to requirements/build)

Given this, wonder if we can drop the patchelf bits

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how you tested it, this is not sufficient when running build-locally.py

Copy link
Member

Choose a reason for hiding this comment

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

Did the following locally

docker run --rm -it quay.io/condaforge/linux-anvil-aarch64

cat >>~/.condarc <<EOF
conda-build:
  error_overlinking: true
EOF

git clone https://github.com/adibbley/cuda-sanitizer-api-feedstock -b cuda-12.3.2
cd cuda-sanitizer-api-feedstock
git reset --hard 25c14ad89edd769dfe5e4955f7d44f5313b91926

git apply -
diff --git a/recipe/meta.yaml b/recipe/meta.yaml
index 71d935e..9a0d82b 100644
--- a/recipe/meta.yaml
+++ b/recipe/meta.yaml
@@ -30,6 +30,7 @@ requirements:
   build:
     - {{ compiler("c") }}
     - {{ compiler("cxx") }}
+    - sysroot_{{ target_platform }} 2.17    # [linux]
     - arm-variant * {{ arm_variant_type }}  # [aarch64]
   host:
     - cuda-version {{ cuda_version }}
<ctrl + d>

conda build -m .ci_support/linux_aarch64_.yaml recipe/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like you tested aarch64, this error only occurred on x86_64 AFAICT. First CI from before any changes: https://github.com/conda-forge/cuda-sanitizer-api-feedstock/pull/5/checks?check_run_id=20821915045

Copy link
Member

Choose a reason for hiding this comment

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

Oops thanks for catching that. Am on an ARM machine where this is a little hard to test

Let's stick with what we have already. If we come up with a better solution, we can always do another PR

Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

This looks fine to me. I have no significant reservations about the changes here. Happy to merge once @jakirkham and @adibbley are ready.

@jakirkham
Copy link
Member

Let's go ahead and merge. We can iterate on any improvements separately

@jakirkham jakirkham merged commit f965614 into conda-forge:main Feb 6, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants