-
-
Notifications
You must be signed in to change notification settings - Fork 15
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
Rebuild for CUDA 12 #148
base: main
Are you sure you want to change the base?
Rebuild for CUDA 12 #148
Conversation
The transition to CUDA 12 SDK includes new packages for all CUDA libraries and build tools. Notably, the cudatoolkit package no longer exists, and packages should depend directly on the specific CUDA libraries (libcublas, libcusolver, etc) as needed. For an in-depth overview of the changes and to report problems [see this issue]( conda-forge/conda-forge.github.io#1963 ). Please feel free to raise any issues encountered there. Thank you! 🙏
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 ( |
@conda-forge-admin, please rerender |
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 ( |
@conda-forge-admin, please rerender |
…nda-forge-pinning 2024.08.09.05.46.50
cd7f4fc
to
46c3957
Compare
Haven't seen such a failure before:
Looks like this is a known issue and we need to point to |
I gather from the diff that the changes here cover Windows but Windows support isn't added? |
The migrator would be adding CUDA 12.0 builds on windows, if windows weren't skipped completely here. That's okay though, it's just the default title of PRs opened by this migrator. Actual windows enablement should be done independently from this PR. |
ad43279
to
7f37f2f
Compare
OK, moved past the
I cannot tell from the recipe where things would refer to |
Sorry for the slow reply here Axel Discussed this with my colleagues today When we have seen similar issues before, they have tended to trace back to using the legacy CMake Am not entirely sure the right place to look at the source code for ambertools, but was able to find the Amber-MD GitHub org, which references the webpage ( https://ambermd.org/ ) used in downloads here ambertools-feedstock/recipe/meta.yaml Line 15 in 725d666
Looking in that org do see usage of As an interesting note did see this comment in that ambertools code: # With CMake 3.7, FindCUDA.cmake crashes when crosscompiling.
if(CROSSCOMPILE)
message(STATUS "CUDA disabled when crosscompiling.")
set(CUDA FALSE)
else() One of the things the CMake team solved by adding the CUDA language and Think to move this forward, would recommend working with upstream to adopt these changes. Possibly the build here can be patched to use those upstream changes (though it may be simpler to update to a new release with the build fixes) cc @robertmaynard @bdice (for awareness & in case revisions to the above are needed) |
As far as I recall, the canonical source code is non-public and on GitLab. But it's several projects stapled together, including cc: @dacase who is likely the best person to coordinate making any needed changes away from deprecated calls |
Thanks for the analysis John! |
So I downloaded the tarball (man there's a lot of stuff in there; a cool 3GB when unpacked, and a mass of vendored bits), and searched for the occurrences of
Given that there's only 4, this sounds quite patchable. |
6f070d5
to
0f5e3cb
Compare
The "canonical source code" is actually public, and available here: [It has an odd file name just because we encourage most folks to download via a web link, so that we can monitor total usage. But it's all open source, and the link above was actually made to be used by conda builds, among other purposes.] I'm not sure how @jakirkham (above) found the corresponding link to AmberTools23, but that is outdated. If it is indeed the case that |
Thanks David! 🙏 Does this live in a source controlled repo somewhere (GitHub, GitLab, Bitbucket, etc.)? The version of the file is just coming from the recipe. It looks like Matt started a PR to update ( #141 ). Though appears that ran into build issues AIUI this feedstock doesn't build Windows ( #148 (comment) ). So that is not relevant In any event, yes, migrating to It would also help when building via cross-compilation for other Linux architectures (like ARM) |
0f5e3cb
to
504b0a8
Compare
Am renaming the PR to avoid further confusion. Hope that is ok |
95560e0
to
3b4686d
Compare
1e0c112
to
c01fe85
Compare
Well, I'm several patches deep into trying to make this work, and I think I'm hitting a CMake bug. Surely it would be better to do less hacky changes in AmberTools upstream; I was mainly trying to see what would be necessary to unblock the build and tried to keep patching ~minimal, at least conceptually (feel free to pick up anything, though these were not really written with being upstreamed in mind - not least because there's no public repo to contribute to - but rather as the most immediately necessary fixes to overcome the failures here). |
Just to avoid any confusion - this PR would have to be for AmberTools 23 until #141 or a similar build is complete, so using the |
Am deeply impressed by the amount of effort you spent patching here Axel! 🙏 Subscribed to that issue. Though it looks like my colleague Rob already replied to you over there. Agree with him we likely need That all being said, agree this is work probably best taken on upstream. Think the other pieces you included here are a good starting point for anyone wanting to push this forward Agreed Matt. Was trying to capture that in my comments above. Apologies if that was too muddled with other details |
This is what I've been doing, the sources are unchanged in this PR. |
👍 yep just wanted to be sure we were all on the same page, that comment was mostly to explain to David why this is being applied to 23, not 24 |
while languages usually get defined around where the (sub)project has its own CMakeLists.txt, this still doesn't work, so move it to the very top
Well, I got things to build, but then run into:
|
Does the header in question have That seems like the kind of thing we would need. It is also covered in this blogpost Also worth noting this header lives in |
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.
Does the header in question have
#include <cooperative_groups.h>
?
Actually, looking at the source code, it does something like this:
#ifdef USE_LEGACY_ATOMICS
QUICKULL val1 = (QUICKULL) (fabs( _tmp * OSCALE) + (QUICKDouble)0.5);
if ( _tmp * weight < (QUICKDouble)0.0)
val1 = 0ull - val1;
QUICKADD(devSim_dft.DFT_calculated[0].Eelxc, val1);
#else
atomicAdd(&devSim_dft.DFT_calculated[0].Eelxc, _tmp);
#endif
The header is missing though, so realistically only the USE_LEGACY_ATOMICS
branch has a chance of working.
@conda-forge-admin, please rerender |
…nda-forge-pinning 2024.10.10.05.31.44
looks like there is an extra or missing |
This PR has been triggered in an effort to update cuda120.
Notes and instructions for merging this PR:
Please note that if you close this PR we presume that the feedstock has been rebuilt, so if you are going to perform the rebuild yourself don't close this PR until the your rebuild has been merged.
Here are some more details about this specific migrator:
If this PR was opened in error or needs to be updated please add the
bot-rerun
label to this PR. The bot will close this PR and schedule another one. If you do not have permissions to add this label, you can use the phrase@conda-forge-admin, please rerun bot
in a PR comment to have theconda-forge-admin
add it for you.This PR was created by the regro-cf-autotick-bot. The regro-cf-autotick-bot is a service to automatically track the dependency graph, migrate packages, and propose package version updates for conda-forge. Feel free to drop us a line if there are any issues! This PR was generated by - please use this URL for debugging.