-
-
Notifications
You must be signed in to change notification settings - Fork 523
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
Add message to maintainers about CUDA 12 transition #4527
Conversation
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 ( |
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 Daniel! 🙏
Think we need to tweak the formatting for YAML to handle this correctly
Also changed libblas
to libcublas
and spaced out the link (to avoid rendering issues)
I feel like there may be other useful information to add to this text. For example, adding something about the headers moving around? It seems that the headers files for |
With previous CUDA versions, the headers came from the Docker image (so were not in In CUDA 12, the headers are supplied. However they are supplied in architecture specific directories to enable cross-compilation (amongst other things) Some context in issue ( conda-forge/cuda-nvcc-feedstock#12 ). Though there were other discussions prior |
If you are ok with the suggestion above, perhaps we can commit that and then discuss changes on top of the new PR diff? Would be good to address the YAML formatting issues first (since those unfortunately affect the diff of the whole message 😞) |
Co-authored-by: jakirkham <[email protected]>
I don't really know what message to write about how the headers are supplied because after reading through some Issues/PRs, it's still not clear to me whether downstream maintainers are expected to add the new include path or if changes to CUDA packages are forthcoming. A method for adding |
Yeah it might be better to have a CUDA 12 bringup issue. Can work on that a bit later (need to get some food) Not expecting lots more changes to the tooling, but it kind of depends on the issues encountered in practice We had reasonable success when building ~5 things, but now we are building ~50+ things. So it is not too surprising when increasing the number of the things we are building that we may encounter new issues that will need to be fixed (either in feedstocks or in tooling) Edit: Probably the thing to do is open up a CUDA 12 bringup issue (on me to do) and then update the commit here to point to that (or just link it from existing PRs) |
Sounds good to me, thanks for following the migration so closely! |
Ok have filed issue ( conda-forge/conda-forge.github.io#1963 ) Please look it over and let me know if you have questions. Happy to iterate on the content there and clarify anything else as needed 🙂 |
Also thanks for diving in here and providing useful feedback! 🙏 |
I think the bringup issue is very helpful and is the best place to point maintainers. |
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 Daniel! 🙏
This looks good. Had some minor tweaks.
Also thanks for reading through the CUDA 12 bringup issue ❤️
Co-authored-by: jakirkham <[email protected]>
Thanks Daniel! 🙏 |
Looks like the whole commit message gets added to the PR title Here's an example PR ( conda-forge/timemory-feedstock#15 ). Also added a screenshot below ![]() Submitting PR ( regro/cf-scripts#1699 ) upstream to fix this |
Think we can improve the visibility of commit messages (like this one) by adding it to the PR body. Have tried to do this with PR ( regro/cf-scripts#1713 ) |
Here's an example ( conda-forge/rmm-feedstock#24 ) with the aforementioned update: ![]() |
Think we can tweak the formatting slightly to make the message stick out a bit more ( regro/cf-scripts#1715) |
After those last changes, here's a refreshed example ( conda-forge/rmm-feedstock#25 ): ![]() |
Have also edited the OPs of the still open PRs that preceded this change to include the updated messaging. Hopefully that provides more context for those maintainers when they are able to look |
The purpose of this PR is to add more context to automigrations for CUDA 12 because the transition is more involved than just swapping out the compiler version.
Ideally, these notes would be added to the PR header, but I didn't see a key for that in the migration YAML. Maybe there is one that I don't know about?
Checklist