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

Adding cuda-nvcc recipe #21350

Merged
merged 46 commits into from
Apr 18, 2023
Merged

Adding cuda-nvcc recipe #21350

merged 46 commits into from
Apr 18, 2023

Conversation

adibbley
Copy link
Contributor

@adibbley adibbley commented Nov 28, 2022

Checklist

  • Title of this PR is meaningful: e.g. "Adding my_nifty_package", not "updated meta.yaml".
  • License file is packaged (see here for an example).
  • Source is from official source.
  • Package does not vendor other packages. (If a package uses the source of another package, they should be separate packages or the licenses of all packages need to be packaged).
  • If static libraries are linked in, the license of the static library is packaged.
  • Package does not ship static libraries. If static libraries are needed, follow CFEP-18.
  • Build number is 0.
  • A tarball (url) rather than a repo (e.g. git_url) is used in your recipe (see here for more details).
  • GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there.
  • When in trouble, please check our knowledge base documentation before pinging a team.

@conda-forge-linter
Copy link

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

I wanted to let you know that I linted all conda-recipes in your PR (recipes/cuda_nvcc) and found some lint.

Here's what I've got...

For recipes/cuda_nvcc:

  • The license item is expected in the about section.
  • The summary item is expected in the about section.
  • The recipe must have some tests.
  • Selectors are suggested to take a <two spaces>#<one space>[<expression>] form. See lines [2, 3, 4, 5, 13, 14, 15, 16]

@conda-forge-linter
Copy link

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

I wanted to let you know that I linted all conda-recipes in your PR (recipes/cuda_nvcc) and found some lint.

Here's what I've got...

For recipes/cuda_nvcc:

  • The license item is expected in the about section.
  • The recipe must have some tests.

@conda-forge-linter
Copy link

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

I wanted to let you know that I linted all conda-recipes in your PR (recipes/cuda_nvcc) and found some lint.

Here's what I've got...

For recipes/cuda_nvcc:

  • The top level meta keys are in an unexpected order. Expecting ['package', 'source', 'build', 'test', 'outputs', 'about', 'extra'].
  • The license item is expected in the about section.

@conda-forge-linter
Copy link

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

I wanted to let you know that I linted all conda-recipes in your PR (recipes/cuda_nvcc) and found some lint.

Here's what I've got...

For recipes/cuda_nvcc:

  • The license item is expected in the about section.

@adibbley adibbley mentioned this pull request Nov 30, 2022
49 tasks
Co-authored-by: Marius van Niekerk <[email protected]>
@conda-forge-linter
Copy link

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 (recipes/cuda_nvcc) and found it was in an excellent condition.

@conda-forge-linter
Copy link

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 (recipes/cuda-nvcc) and found it was in an excellent condition.

@jakirkham jakirkham changed the title Adding cuda_nvcc recipe Adding cuda-nvcc recipe Dec 5, 2022
@m3vaz
Copy link
Contributor

m3vaz commented Dec 8, 2022

Doesn't nvcc have a maximum supported compiler version? Is that limit monitored here?

@conda-forge-webservices
Copy link

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

I wanted to let you know that I linted all conda-recipes in your PR (recipes/cuda-nvcc) and found some lint.

Here's what I've got...

For recipes/cuda-nvcc:

  • Selectors are suggested to take a <two spaces>#<one space>[<expression>] form. See lines [21]

@conda-forge-webservices
Copy link

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 (recipes/cuda-nvcc) and found it was in an excellent condition.

@adibbley adibbley mentioned this pull request Mar 24, 2023
10 tasks
Comment on lines 65 to 66
- nvvm # [linux]
- Library\nvvm # [win]
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense for nvvm to be a separate package? I'm thinking about downstream packages like Numba who don't use nvcc but do use nvvm.

Regardless, this could be split out later if desired.

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 think this could make sense. If we can raise this as an issue on the feedstock we could align this change for all formats, rather than a conda-forge only change here.

- add cuda-version
- explicit target_names
- about sections in every output
- add arm-variant
recipes/cuda-nvcc/meta.yaml Outdated Show resolved Hide resolved
- cuda-nvcc_{{ target_platform }} =={{ version }}
- {{ pin_compatible("cuda-version", max_pin="x.x") }}
run_constrained:
- gcc_impl_{{ target_platform }} >=6,<13 # [linux]
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to do similar with clang or msvc?

Copy link
Member

Choose a reason for hiding this comment

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

Since we only use clang on macOS and that is skipped, think we don't need to worry about that.

On Windows we don't distribute compiler packages and instead rely on CI to have them. Not sure what more we can do there

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we only use clang on macOS and that is skipped, think we don't need to worry about that.

A user could presumably install it and want to use it in their environment though?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry am not following. How would they install it? This recipe isn't built on macOS

Copy link
Contributor

Choose a reason for hiding this comment

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

There's linux-64 clang builds that someone could install and use in their environment.

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok. That makes more sense. Thanks Keith! 🙏

There is a clang package. Though the package that encodes target_platform would be in clang-compiler-activation-feedstock. However this is only configured for macOS targets currently.

That said, suppose one could add Linux targets for clang. Maybe we could save this as a follow up issue (for whenever those packages get added)?

Copy link
Contributor

Choose a reason for hiding this comment

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

We can always repodata patch at that time. I'm fine to defer it to a follow up.

@@ -0,0 +1,17 @@
if not exist %PREFIX% mkdir %PREFIX%
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm unfamiliar with Windows, is this something we should be doing in all of our recipes? We didn't do that in libcublas, for instance. We're not specifically writing anything to the %LIBRARY_PREFIX% directory in this file.

done
fi
else
cp -r $i ${PREFIX}
Copy link
Contributor

Choose a reason for hiding this comment

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

Some questions:

  • I assume that we're intentionally not including the contents of bin into the targetsDir?
  • We probably want to put the nvvm contents into the targetsDir right?
  • cicc is inside nvvm/bin, but nvcc needs to be able to find it. Does that need special handling to be placed into ${PREFIX}/bin?

Copy link
Contributor

Choose a reason for hiding this comment

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

The current layout is fine. The bin and nvvm directories will be copied into ${PREFIX}/bin/ and ${PREFIX}/nvvm. Since the relative layot has stayed consistent, nvcc doesn't need to be modified to know where the nvvm directory is

Copy link
Contributor

Choose a reason for hiding this comment

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

Are we OK writing to ${PREFIX}/nvvm? My understanding was that we were avoiding writing to such top-level directories as much as possible to avoid clobbering, but perhaps we're OK treating nvvm as CUDA-specific in the same way that we are treating targets as CUDA-specific.

Do we still want to include some of these contents in targets or only put them in the top-level ${PREFIX}/* directories?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need nvvm in $PREFIX due to the relative pathing Robert mentioned. nvcc expects bin nvvm and targets to all be at the same level.

Clobbering shouldn't be a concern here as it was with things like src/example.c

Copy link
Member

Choose a reason for hiding this comment

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

We are already patching nvcc.profile, so it's not a big deal to patch a couple more lines.
If we are going with filesystem hierarchy, they should not be in $PREFIX and rather in somewhere like $PREFIX/lib/nvcc/nvvm and $PREFIX/lib/nvcc/targets just like gcc does with $PREFIX/lib/gcc

Thoughts @mbargull, @conda-forge/core?

Copy link
Member

Choose a reason for hiding this comment

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

nvcc.profile is not patched on Linux. It is only patched on Windows to address shipping different CCCL versions as requested in PR ( #21953 ). Preferably it would not be patched at all (even on Windows). Open to a better solution that doesn't require the patch on Windows (if we have ideas). Though this is probably best taken up in the feedstock since we are blocked on Windows builds atm ( #21723 (comment) ).

It's worth noting that there are already cases where we install non-traditional things in $PREFIX. For example go adds $PREFIX/go. With go, trying to move away from that model breaks the compiler in weird ways.

Typically all CUDA components are placed in the same relative paths where they are then QA’d and deployed. The proposal here suggests dividing them up between different paths that will deviate from the process. This risks introducing new issues unique to this non-standard deployment model and makes it harder to support.

Finally would add Numba is dependent on this location. So changing it here means Numba (and maybe other libraries) will need updates.

Comment on lines 51 to 52
- sysroot_{{ target_platform }} 2.17 # [linux]
- __glibc >=2.17 # [linux]
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of scope for fixing right now, but for future note: Is there no relationship between these packages? Should there be?

host:
- sysroot_{{ target_platform }} 2.17 # [linux]
- __glibc >=2.17 # [linux]
- cuda-version {{ cuda_version }}
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the benefit of including cuda-version in the host environment? Isn't the purpose to be a metapackage ensuring compatibility across dependencies, i.e. something we really only need in run? What build requirements are being constrained by including this in host?

Copy link
Contributor

Choose a reason for hiding this comment

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

Defining this in host allows us to use the pin_compatible function in the run section. There may be more context though.

run:
- sysroot_{{ target_platform }} 2.17 # [linux]
- __glibc >=2.17 # [linux]
- cuda-cudart-dev
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need a version constraint here, or is that all managed using cuda-version?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe cuda-version suffices here.

recipes/cuda-nvcc/meta.yaml Outdated Show resolved Hide resolved
@jakirkham
Copy link
Member

We were running into some issues during testing, which we were struggling to resolve here. So for now we have cut this down to the minimal working recipe. Think it will be easier to try to add back the removed functionality in the feedstock. Though please let us know if you have other thoughts on that.

@jakirkham
Copy link
Member

Planning on merging EOD Monday if no comments

Copy link
Contributor

@robertmaynard robertmaynard left a comment

Choose a reason for hiding this comment

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

Testing the package locally ( conda install -c ..../staged-recipes/build_artifacts/ cuda-nvcc cuda-cudart ) in a clean env.

This creates a broken compiler, due to ${CXX} having no value. We need to either require a CXX compiler or only optionally add this env var when CXX exists.

(ctk) rmaynard@rmaynard-dt:~/Work/cmake/conda_test_build$ conda list
# packages in environment at /home/rmaynard/miniconda3/envs/ctk:
#
# Name                    Version                   Build  Channel
_libgcc_mutex             0.1                 conda_forge    conda-forge
_openmp_mutex             4.5                       2_gnu    conda-forge
cuda-cccl_linux-64        12.0.90              ha770c72_1    conda-forge
cuda-cudart               12.0.107             hcb278e6_0    file:///home/rmaynard/Work/conda-forge/staged-recipes/build_artifacts
cuda-cudart-dev           12.0.107             hcb278e6_0    file:///home/rmaynard/Work/conda-forge/staged-recipes/build_artifacts
cuda-cudart-dev_linux-64  12.0.107             hcb278e6_0    file:///home/rmaynard/Work/conda-forge/staged-recipes/build_artifacts
cuda-cudart-static        12.0.107             hcb278e6_0    file:///home/rmaynard/Work/conda-forge/staged-recipes/build_artifacts
cuda-cudart-static_linux-64 12.0.107             hcb278e6_0    file:///home/rmaynard/Work/conda-forge/staged-recipes/build_artifacts
cuda-nvcc                 12.0.76              h59595ed_0    file:///home/rmaynard/Work/conda-forge/staged-recipes/build_artifacts
cuda-version              12.0                 hffde075_0    conda-forge
libgcc-ng                 12.2.0              h65d4601_19    conda-forge
libgomp                   12.2.0              h65d4601_19    conda-forge
libstdcxx-ng              12.2.0              h46fd767_19    conda-forge

@jakirkham
Copy link
Member

Alex added a clearer error message above. If we want to do more here, let's raise an issue in the feedstock and discuss

@jakirkham jakirkham merged commit 1bb91e4 into conda-forge:main Apr 18, 2023
@jakirkham
Copy link
Member

jakirkham commented Apr 18, 2023

Thanks all! 🙏

Let's follow up on anything else in the feedstock 🙂

Will also include a PR link related to follow ups pertaining to comment ( #21350 (comment) ) once available. ( edit: filed issue conda-forge/cuda-nvcc-feedstock#1 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.