Update for CUDA 12.2.2 #8

adibbley · 2023-12-18T23:37:29Z

Fixes #4

conda-forge-webservices · 2023-12-18T23:37:41Z

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 · 2023-12-18T23:43:01Z

Currently only crt is split to new outputs, nvvm also needs to be split in this PR. In an effort to be consistent with the packages originally providing these files I matched their naming and structure. Metapackages should be added to conda-forge/cuda-nvcc-feedstock#32 as cuda-crt and cuda-nvvm

recipe/meta.yaml

jakirkham

Thanks Alex! 🙏

Had some comments below

~~Also is the NVVM package being split out too?~~ Edit: Nvm this is answered above

~~We may also want to think about a metapackage for all the CRT bits combined for easier use~~ Edit: Nvm this is also answered above

recipe/meta.yaml

Co-authored-by: jakirkham <[email protected]>

recipe/meta.yaml

Co-authored-by: Bradley Dice <[email protected]>

recipe/meta.yaml

msarahan · 2023-12-20T20:44:37Z

I agree with brad on the major distinction. In my opinion, pin_subpackage is clearer about things coming from this specific recipe, but the net effect on the outcome is exactly the same. It's up to you on how you want to do it.

jakirkham · 2023-12-20T21:19:04Z

recipe/meta.yaml

@@ -129,21 +128,21 @@ outputs:
      binary_relocation: False


Probably ok to keep this. Though quick question, do we have any libraries still here to corrupt? Only see the static library below, which I think patchelf shouldn't touch

Agreed this isn't doing anything now. Left it in since the relocation only seems to break things for us.

vyasr

Have some questions and suggestions, but approving so that I'm not blocking when I leave for the holidays.

recipe/meta.yaml

vyasr · 2023-12-20T22:13:22Z

recipe/meta.yaml

+        - {{ pin_subpackage("cuda-crt-tools", exact=True) }}
+        - {{ pin_subpackage("cuda-nvvm-tools", exact=True) }}


Why do we need these in host? I would have expected them to only be run requirements, especially since they only contain binaries.

The files constrain above is grabbing bin/*. It may make sense to include these just to avoid repackaging duplicate files that either of these packages would already have

Admittedly bin/* may be too sweeping. Though it does ensure that we have gotten everything else that was in bin that is not included in any other package

vyasr · 2023-12-20T22:15:12Z

recipe/meta.yaml

@@ -72,18 +73,14 @@ outputs:
        - "*/api-ms-win-core-winrt-*.dll"        # [win]
    run_exports:
      strong:
-        - cuda-version >={{ cuda_version }},<{{ cuda_version.split(".")[0]|int + 1 }}
+        - cuda-version >={{ cuda_version }},<{{ cuda_version_next_major }}


Maybe put this whole constraint into a variable? It gets used numerous times.

vyasr · 2023-12-20T22:15:35Z

recipe/meta.yaml

+        - {{ pin_subpackage("cuda-crt-dev_" + target_platform, exact=True) }}
+        - {{ pin_subpackage("cuda-nvvm-dev_" + target_platform, exact=True) }}


Same question as above re: why this is needed.

As these are -dev packages, they would make sense to keep (especially if there is something linking to them)

recipe/meta.yaml

vyasr · 2023-12-20T22:17:28Z

recipe/meta.yaml

    requirements:
      build:
        - sysroot_{{ target_platform }} 2.17    # [linux]
      host:
        - arm-variant * {{ arm_variant_type }}  # [aarch64]
        - cuda-version {{ cuda_version }}
        - cuda-cudart-dev
+        - {{ pin_subpackage("cuda-nvvm-impl", exact=True) }}


One more possibly unnecessary host (just marking to make them easier to find in case they can be removed).

As we are only packaging one static library here that shouldn't be packaged anywhere else, this is probably safe to drop.

vyasr · 2023-12-20T22:20:59Z

recipe/meta.yaml

+        - test -d $PREFIX/bin/crt            # [linux]
+        - test -f $PREFIX/bin/crt/link.stub  # [linux]
+        - if not exist %LIBRARY_BIN%\crt\link.stub exit 1  # [win]
+    about:


Not at all required for this PR, but does conda support YAML anchors? If so, we should use them for the about rather than duplicating it once per output in these recipes. We could use an anchor override for the description/summary, which are the only fields that change. If we were to apply it, would recommend doing so to any of our packages with multiple outputs.

Here's an example: https://www.linode.com/docs/guides/yaml-anchors-aliases-overrides-extensions/

jakirkham · 2023-12-20T22:38:34Z

Instead of doing a bunch of YAML changes here would suggest someone draft a PR of the changes they would like to make and we can review separately. Let's keep this focused on the 12.2 update and related changes. Just want to keep this tightly scoped so we can get this done reasonably quickly

bdice · 2024-01-04T23:31:30Z

I agree with @jakirkham on deferring YAML simplification. However, there are a few questions from @vyasr about packages in host that might not be necessary (?). I think these are somewhat pertinent to 12.2 but I would be okay with merging for now and addressing these separately. I think the next step would be either merge, or test what happens if those packages are removed from host.

#8 (comment)
#8 (comment)
#8 (comment)

jakirkham · 2024-01-05T00:48:23Z

Would think the main reason to include a package in host that is already in run would be to avoid clobbering or to satisfy linkage (or other) checks. It seems relatively safe to have the packages in host (if a little redundant). While there is some risk in dropping them (that we wind up with duplicate copies of files in different packages and thus clobbering errors at install time)

Would be tempted to leave them as-is

If someone wants to test dropping them offline, that seems ok, but it also seems ok to punt that work into another issue handled separately from this PR

recipe/meta.yaml

jakirkham · 2024-01-05T20:26:36Z

Thanks all! 🙏

jakirkham · 2024-01-05T21:47:49Z

Needed another re-render to update the README. Done with PR ( #11 )

jakirkham · 2024-01-05T22:04:08Z

recipe/meta.yaml

+        Compiler for CUDA applications.
+      doc_url: https://docs.nvidia.com/cuda/index.html
+
+  - name: cuda-nvvm-dev_{{ target_platform }}


This needs to be noarch: generic just like cuda-crt-dev_{{ target_platform }} is

It is needed because cuda-nvcc-dev_{{ target_platform }} is noarch: generic and depends on both of these

Without noarch: generic the compiler becomes uninstallable in cross-compilation workloads

Suggested change

- name: cuda-nvvm-dev_{{ target_platform }}

- name: cuda-nvvm-dev_{{ target_platform }}

build:

noarch: generic

Fixing in PR ( #12 ), which has more context

Update for CUDA 12.2.2

517c215

adibbley commented Dec 18, 2023

View reviewed changes

recipe/meta.yaml Outdated Show resolved Hide resolved

jakirkham reviewed Dec 18, 2023

View reviewed changes

recipe/meta.yaml Show resolved Hide resolved

recipe/meta.yaml Outdated Show resolved Hide resolved

recipe/meta.yaml Outdated Show resolved Hide resolved

recipe/meta.yaml Outdated Show resolved Hide resolved

jakirkham mentioned this pull request Dec 19, 2023

Create outputs for nvvm and crt #4

Closed

adibbley and others added 7 commits December 19, 2023 10:50

Rename to cuda-crt-dev_{{ target_platform }}

454ce06

Apply suggestions from code review

22368b8

Co-authored-by: jakirkham <[email protected]>

Move bin test

586782a

Split nvvm outputs

dc54e67

Linux only

94a6ca2

Update nvvm split for windows

aa268fb

Update crt summary

faaf654

bdice reviewed Dec 20, 2023

View reviewed changes

recipe/meta.yaml Outdated Show resolved Hide resolved

recipe/meta.yaml Outdated Show resolved Hide resolved

recipe/meta.yaml Outdated Show resolved Hide resolved

recipe/meta.yaml Outdated Show resolved Hide resolved

recipe/meta.yaml Outdated Show resolved Hide resolved

bdice mentioned this pull request Dec 20, 2023

Update for CUDA 12.2.2 conda-forge/cuda-nvcc-feedstock#32

Merged

adibbley and others added 3 commits December 20, 2023 11:31

Update crt-tools description

c2b6229

Add cuda_version_next variable

22ecea9

Update recipe/meta.yaml

b1044b4

Co-authored-by: Bradley Dice <[email protected]>

bdice reviewed Dec 20, 2023

View reviewed changes

recipe/meta.yaml Outdated Show resolved Hide resolved

recipe/meta.yaml Outdated Show resolved Hide resolved

jakirkham requested review from robertmaynard, vyasr, leofang, msarahan and jakirkham December 20, 2023 20:23

msarahan approved these changes Dec 20, 2023

View reviewed changes

Rename cuda_version_next_major

047d00d

jakirkham reviewed Dec 20, 2023

View reviewed changes

adibbley added 2 commits December 20, 2023 16:25

Use string

ab93a60

Use pin_subpackage

3f5e527

bdice approved these changes Dec 20, 2023

View reviewed changes

vyasr approved these changes Dec 20, 2023

View reviewed changes

bdice marked this pull request as ready for review January 4, 2024 23:32

bdice requested a review from a team as a code owner January 4, 2024 23:32

robertmaynard reviewed Jan 5, 2024

View reviewed changes

recipe/meta.yaml Show resolved Hide resolved

robertmaynard approved these changes Jan 5, 2024

View reviewed changes

jakirkham merged commit b6f0f36 into conda-forge:main Jan 5, 2024
5 checks passed

jakirkham reviewed Jan 5, 2024

View reviewed changes

jakirkham mentioned this pull request Jan 5, 2024

Make cuda-nvvm-dev_{{ target_platform }} noarch #12

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update for CUDA 12.2.2 #8

Update for CUDA 12.2.2 #8

adibbley commented Dec 18, 2023 •

edited

Loading

conda-forge-webservices bot commented Dec 18, 2023

adibbley commented Dec 18, 2023

jakirkham left a comment •

edited

Loading

msarahan commented Dec 20, 2023

jakirkham Dec 20, 2023

adibbley Dec 20, 2023

vyasr left a comment

vyasr Dec 20, 2023

jakirkham Jan 5, 2024

vyasr Dec 20, 2023

vyasr Dec 20, 2023

jakirkham Jan 5, 2024

vyasr Dec 20, 2023

jakirkham Jan 5, 2024

vyasr Dec 20, 2023 •

edited

Loading

vyasr Dec 20, 2023

jakirkham commented Dec 20, 2023

bdice commented Jan 4, 2024

jakirkham commented Jan 5, 2024

jakirkham commented Jan 5, 2024

jakirkham commented Jan 5, 2024

jakirkham Jan 5, 2024

jakirkham Jan 5, 2024

		- {{ pin_subpackage("cuda-crt-tools", exact=True) }}
		- {{ pin_subpackage("cuda-nvvm-tools", exact=True) }}

		- {{ pin_subpackage("cuda-crt-dev_" + target_platform, exact=True) }}
		- {{ pin_subpackage("cuda-nvvm-dev_" + target_platform, exact=True) }}

Update for CUDA 12.2.2 #8

Update for CUDA 12.2.2 #8

Conversation

adibbley commented Dec 18, 2023 • edited Loading

conda-forge-webservices bot commented Dec 18, 2023

adibbley commented Dec 18, 2023

jakirkham left a comment • edited Loading

Choose a reason for hiding this comment

msarahan commented Dec 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyasr Dec 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakirkham commented Dec 20, 2023

bdice commented Jan 4, 2024

jakirkham commented Jan 5, 2024

jakirkham commented Jan 5, 2024

jakirkham commented Jan 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adibbley commented Dec 18, 2023 •

edited

Loading

jakirkham left a comment •

edited

Loading

vyasr Dec 20, 2023 •

edited

Loading