Refresh CUDA 12 migrator #34

conda-forge-admin · 2024-01-12T06:30:51Z

Recently the CUDA 11.8 migrator was closed out ( conda-forge/conda-forge-pinning-feedstock#5340 ). As part of that it was determined a small updated was needed to the CUDA 12 migrator ( conda-forge/conda-forge-pinning-feedstock#5340 (comment) ). While this largely works for most feedstocks without changes, those that have use_local: true will have issues with re-rendering until the CUDA 12 migrator they have is updated. This PR pulls over those updates to fix re-rendering on the feedstock. Please let us know if you have any other questions.

Hi! This is the friendly automated conda-forge-webservice.

I've started rerendering the recipe as instructed in #33.

If I find any needed changes to the recipe, I'll push them to this PR shortly. Thank you for waiting!

Here's a checklist to do before merging.

Bump the build number if needed.

Fixes #33

conda-forge-webservices · 2024-01-12T06:31:01Z

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.

github-actions · 2024-01-12T06:32:22Z

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you but ran into some issues. Please check the output logs of the latest rerendering GitHub actions workflow run for errors. You can also ping conda-forge/core for further assistance or try re-rendering locally.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/nnpops-feedstock/actions/runs/7498754771.

Pull in latest changes from `conda-forge-pinning`. https://github.com/conda-forge/conda-forge-pinning-feedstock/blob/cbccb41e313c017c1ad15db6c287e87aa57d54dd/recipe/migrations/cuda120.yaml

jakirkham · 2024-01-12T06:42:20Z

@conda-forge-admin , please re-render

…nda-forge-pinning 2024.01.11.21.29.17

jakirkham · 2024-01-12T19:35:40Z

This is passing! 🎉

@conda-forge/nnpops this is ready for review 🙂

h-vetinari · 2024-01-12T20:11:40Z

What's the purpose of this PR? There are no additional CI jobs because (checking Azure...) we already have CUDA 12 builds here.

Also, turning a rerender PR into a migration PR is confusing at best - the migration machinery needs to mark things as completed as it marches through the DAG, but not even that is necessary here because it was started by hand?

jakirkham · 2024-01-12T20:16:28Z

There's a description in the OP

h-vetinari · 2024-01-12T20:25:14Z

There's a description in the OP

Which was edited in, so not exactly visible... But anyway, there's no use_local: true here, so this PR is pointless.

h-vetinari · 2024-01-12T20:26:21Z

But anyway, there's no use_local: true here, so this PR is pointless.

Sorry, I was just looking at the diff, I see it's there but not being removed

h-vetinari

This PR should be a single-line change to remove use_local: true, not manually update the migrator file and still leave it out of sync with eventual updates in the global pinning

jakirkham · 2024-01-12T20:28:49Z

Yep it's here

nnpops-feedstock/.ci_support/migrations/cuda120.yaml

Lines 5 to 8 in b37e978

    
           # Vendor CUDA 12 migrator to use GCC 11. 
        
           # This is needed to workaround a pybind + GCC 12 + nvcc 12 bug 
        
           # xref: https://github.com/pybind/pybind11/issues/4606 
        
           use_local: true

Also would appreciate moderating the tone here. It is coming off a bit harsh

h-vetinari · 2024-01-12T20:40:26Z

Yep it's here

See follow-up comments.

Also would appreciate moderating the tone here. It is coming off a bit harsh

Sorry for that. My point is: use_local: true should only ever be used as a stopgap and then removed asap. Working around its presence rather than removing it is just wrong.

jakirkham · 2024-01-12T20:50:57Z

Apology accepted

Above the use_local: true line there is a comment with more context on why it was added. Please let me know if you have more questions after reading that

stale

h-vetinari · 2024-01-12T21:03:53Z

OK, sorry for missing that (I was on a phone, which didn't help...).

IMO we're just layering bandaids on top of each other here, but I just tried

commit 48e30057a1c52821ba3e313ea7ceb83d1eeb7bfb
Author: H. Vetinari <[email protected]>
Date:   Sat Jan 13 07:57:39 2024 +1100

    replace use_local in CUDA 12 migrator with local compiler override

diff --git a/.ci_support/migrations/cuda120.yaml b/.ci_support/migrations/cuda120.yaml
index aa2b2a2..ab54105 100644
--- a/.ci_support/migrations/cuda120.yaml
+++ b/.ci_support/migrations/cuda120.yaml
@@ -2,10 +2,6 @@ migrator_ts: 1682985063
 __migrator:
   kind:
     version
-  # Vendor CUDA 12 migrator to use GCC 11.
-  # This is needed to workaround a pybind + GCC 12 + nvcc 12 bug
-  # xref: https://github.com/pybind/pybind11/issues/4606
-  use_local: true
   migration_number:
     3
   build_number:
diff --git a/recipe/conda_build_config.yaml b/recipe/conda_build_config.yaml
new file mode 100644
index 0000000..ff0c248
--- /dev/null
+++ b/recipe/conda_build_config.yaml
@@ -0,0 +1,6 @@
+# Work around a bug in CUDA 12.0, see
+# https://github.com/pybind/pybind11/issues/4606
+c_compiler_version:     # [linux]
+  - 11                  # [linux]
+cxx_compiler_version:   # [linux]
+  - 11                  # [linux]

and to my surprise, this does not work. Local conda_build_config.yaml should always "win", but it doesn't in this case. IMO this is broken, but there doesn't seem to be another good work around for now.

h-vetinari · 2024-01-12T21:12:18Z

This is passing! 🎉

TBH, this is what threw me, because there are no relevant changes in this PR (no builds added; all were passing before).

It's proactively ensuring things keep working after the closing of the CUDA 11.8 migration (which is laudable!), but celebrating that this clerical PR passes made me expect an entirely different caliber of changes.

jakirkham · 2024-01-12T21:39:19Z

No worries

Recall trying something similar when the bug surfaced, but wasn't able to get it to work either. Maybe we are missing something about how to get that to work. Or maybe there is a build tooling bug

Yeah the pybind11 + CUDA 12.0 + GCC 12 bug is an unfortunate cross-section of things

AIUI there is a fix in pybind11 ( pybind/pybind11#4893 ), but that is unreleased. Not sure what their release plan is. Even with a release, there are some libraries that vendor pybind11 (like PyTorch). So we would still need libraries to update their vendored copy (though that at least could happen without a pybind11 release)

There was also a fix on the NVCC side as part of CUDA 12.2 ( pybind/pybind11#4606 (comment) ), but currently the migrator is pinned to CUDA 12.0. Until very recently CUDA 12.0 was the only version in conda-forge, but we do now have 12.2 ( conda-forge/cuda-feedstock#13 ). We could revisit how we are building for CUDA 12.x in conda-forge (and we briefly discussed this at the recent core meeting), but it probably deserves an issue and some more discussion (just to make sure we are considering everything we should be). Am planning to do that next week

Given the options at the time, GCC 11 was the only realistic and least disruptive solution. So that's what we went with

This is passing! 🎉

TBH, this is what threw me, because there are no relevant changes in this PR (no builds added; all were passing before).

It's proactively ensuring things keep working after the closing of the CUDA 11.8 migration (which is laudable!), but celebrating that this clerical PR passes made me expect an entirely different caliber of changes.

Sorry this comment didn't show up until I posted

Noted thanks for the feedback. Will keep this in mind for the future

h-vetinari · 2024-01-12T21:54:04Z

Given the options at the time, GCC 11 was the only realistic and least disruptive solution. So that's what we went with

Yeah, I get that that's the only good choice right now, but even then, there should be a solution that doesn't require use_local: true (with CBC). Very unfortunate that this doesn't work as it should.

In any case, sorry for the friction here and thanks for taking care of this. Looking forward to unblocking CUDA 12.x builds. :)

jakirkham · 2024-01-12T23:41:57Z

Gotcha yeah agreed

No worries. Understand this is a confusing issue. Sorry for not communicating it clearer. Happy to help. Likewise 🙂

RaulPPelaez · 2024-01-15T08:21:49Z

Thanks for the good work guys, always refreshing to see you around.
The dreaded GCC12+CUDA12+pybind11 pops up in my radar quite often, it is a fact of life at this point :P. I am guilty of abusing the use_local: true thingy to work around it. Will go around replacing it when I learn of a better solution.
I am going to merge this in then.

dummy commit for rerendering

7896a3b

conda-forge-admin requested review from RaulPPelaez, h-vetinari, mikemhenry and raimis as code owners January 12, 2024 06:30

conda-forge-webservices bot mentioned this pull request Jan 12, 2024

@conda-forge-admin , please re-render #33

Closed

Refresh CUDA 12 migrator

e9dfd41

Pull in latest changes from `conda-forge-pinning`. https://github.com/conda-forge/conda-forge-pinning-feedstock/blob/cbccb41e313c017c1ad15db6c287e87aa57d54dd/recipe/migrations/cuda120.yaml

MNT: Re-rendered with conda-build 3.28.3, conda-smithy 3.30.4, and co…

220e56a

…nda-forge-pinning 2024.01.11.21.29.17

jakirkham changed the title ~~MNT: rerender~~ Refresh CUDA 12 migrator Jan 12, 2024

h-vetinari previously requested changes Jan 12, 2024

View reviewed changes

jakirkham mentioned this pull request Jan 12, 2024

Refresh CUDA 12 migrator conda-forge/pytorch_sparse-feedstock#64

Closed

1 task

RaulPPelaez merged commit a466cc1 into conda-forge:main Jan 15, 2024
22 checks passed

wshanks mentioned this pull request Apr 5, 2024

qiskit-aer v0.14 conda-forge/qiskit-aer-feedstock#34

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refresh CUDA 12 migrator #34

Refresh CUDA 12 migrator #34

conda-forge-admin commented Jan 12, 2024 •

edited by jakirkham

Loading

conda-forge-webservices bot commented Jan 12, 2024

github-actions bot commented Jan 12, 2024

jakirkham commented Jan 12, 2024

jakirkham commented Jan 12, 2024

h-vetinari commented Jan 12, 2024

jakirkham commented Jan 12, 2024

h-vetinari commented Jan 12, 2024

h-vetinari commented Jan 12, 2024

h-vetinari left a comment

jakirkham commented Jan 12, 2024

h-vetinari commented Jan 12, 2024

jakirkham commented Jan 12, 2024

h-vetinari commented Jan 12, 2024

h-vetinari commented Jan 12, 2024

jakirkham commented Jan 12, 2024 •

edited

Loading

h-vetinari commented Jan 12, 2024

jakirkham commented Jan 12, 2024

RaulPPelaez commented Jan 15, 2024

Refresh CUDA 12 migrator #34

Refresh CUDA 12 migrator #34

Conversation

conda-forge-admin commented Jan 12, 2024 • edited by jakirkham Loading

conda-forge-webservices bot commented Jan 12, 2024

github-actions bot commented Jan 12, 2024

jakirkham commented Jan 12, 2024

jakirkham commented Jan 12, 2024

h-vetinari commented Jan 12, 2024

jakirkham commented Jan 12, 2024

h-vetinari commented Jan 12, 2024

h-vetinari commented Jan 12, 2024

h-vetinari left a comment

Choose a reason for hiding this comment

jakirkham commented Jan 12, 2024

h-vetinari commented Jan 12, 2024

jakirkham commented Jan 12, 2024

h-vetinari commented Jan 12, 2024

h-vetinari commented Jan 12, 2024

jakirkham commented Jan 12, 2024 • edited Loading

h-vetinari commented Jan 12, 2024

jakirkham commented Jan 12, 2024

RaulPPelaez commented Jan 15, 2024

conda-forge-admin commented Jan 12, 2024 •

edited by jakirkham

Loading

jakirkham commented Jan 12, 2024 •

edited

Loading