Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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-cudart #21723
Adding cuda-cudart #21723
Changes from 40 commits
5e7ab63
54e9ad1
77d5387
cb4ddf1
aeabc3a
753fdda
2a95315
cb06971
e47fc74
47c0e12
1a3b333
f8e274b
c0cd174
3ab1ad9
dc4a28d
dada5b9
ec2a827
3d09008
7fdd23e
d966b9a
855014c
c098e5d
039461a
a3c7bae
fae8026
7fd615f
cbce911
d0847bb
9278294
a974fbb
f4c6f2d
7b0d9a7
5eb9324
f239ddc
5106c80
26d7a52
97d62e3
097daa9
e48d9b1
01063be
52bd85d
20212b9
622cf4d
251fa03
2af2140
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Currently the top-level package and one of the outputs have the same name. We should change these to different names to avoid issues. We could rename the top-level name to something like (
cuda-cudart-split
) or we could rename the library tocuda-cudart-shared
(or similar)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.
cuda-cudart-split
is betterThere 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.
ping
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.
Can I get some context on the issue here please? Ideally we would have consistent naming for all component feedstocks without needing to know if that component has multiple outputs.
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 think the issue is that the top level package and the output defined on line 40 have the same name which makes things ambiguous at best and likely breaks things.
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.
That sounds right. It use to be the case different names were required just to get through
staged-recipes
or runconda-build
.Though it is possible this is less of an issue than has been in the past. For example
libnuma
was added in PR ( #21176 ) and use the same top-level package name and output name. The feedstock still follows this pattern. AFAICT it is working ok.So maybe this isn't actually an issue anymore
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.
Windows CI appears to be failing because of overlinking.
Seems related to the problems mentioned in previous PRs here:
libcusolver
,libcusparse
, andlibnvjitlink
recipes #21924 (comment)libcusolver
,libcusparse
, andlibnvjitlink
recipes #21924 (comment)windows-terminal
#21913The solution in #21924 was disabling the overlinking check. Is this what we should do, or is there a different approach?
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.
At one point the recipes in the aforementioned PR had
error_overlinking
( 3ed95cf ), but it seems to be reverted later ( 96f6ad0 ). Am not seeingerror_overlinking
in the final changes in that PR or in the recipes. So think Leo came up with another solution, but it is not obvious what it was based on the commit history of the PR what it was. Might be worth studying thelibnvjitlink
recipe more closely (since this is where it was needed and later dropped)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.
Hmm. I looked over the diff between those commits and compared the requirements of those packages to
cuda-cudart
but I'm not seeing much that points to the original cause or solution.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.
Yeah it wasn't obvious to me how this was fixed after a cursory look. Though assumed I had just missed something
Let's try merging in
main
(as we discussed offline) and see if the issue persists. Did this below ( 01063be )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.
Still seeing the same issue after updating the branch. This Microsoft support thread looks relevant
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.
So I do see
api-ms-win-core-libraryloader-*.dll
in thevc
package, but the SONAME(?) appears different. Maybe we are getting the wrong version?With
api-ms-win-security-systemfunctions-*.dll
, it doesn't not appear to be in that list. Unclear as to whether it should be (possibly in a newer version?)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.
My impression was that after I followed the libcublas approach and added compiler to build, I no longer saw warnings on the dlls, so I assumed it's safe to undo the WAR and gave it a shot.
Another issue was that I noticed the internal recipe had a wrong cusparse/nvjitlink version, mismatching the CUDA 12.0 one, so I fixed it and that change might also explain. Not sure which is the real key.
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.
Thanks Leo! 🙏 Really appreciate you taking time to chime in here 🙂
Yeah the
compiler
s part seems like the key. Did look at that. Though this recipe already seems to include these for multiple packages.That said, I do see
noarch: generic
in a few places where we are addingcompiler
s and doing Linux/Windows specific things. Maybe these are holdovers from before we started doing architecture specific things. Not sure if they are causing this problem, but would expect them to cause some issues. So we may want to drop them. Have outlined them in review comments belowThere 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.
Discussed this overlinking issue with @jakirkham / @adibbley. We agreed to skip Windows for now, and then work on fixing this problem in the feedstock. That way Linux packages can be unblocked and we can continue on
cuda-nvcc
while we investigate the Windows overlinking further.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.
Raised feedstock issue ( conda-forge/cuda-cudart-feedstock#1 ) for further discussion/investigation.