Skip to content

drop CUDA 11.8#7431

Merged
hmaarrfk merged 4 commits into
conda-forge:mainfrom
h-vetinari:cuda118
Jun 10, 2025
Merged

drop CUDA 11.8#7431
hmaarrfk merged 4 commits into
conda-forge:mainfrom
h-vetinari:cuda118

Conversation

@h-vetinari

Copy link
Copy Markdown
Member

For #7404; needs to wait for announcement conda-forge/conda-forge.github.io#2532 to be merged and the announced time to pass.

@conda-forge-admin

Copy link
Copy Markdown
Contributor

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/meta.yaml) and found it was in an excellent condition.

Comment thread recipe/conda_build_config.yaml Outdated
Co-authored-by: jakirkham <jakirkham@gmail.com>

@jakirkham jakirkham left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks Axel! 🙏

Think there are a couple more things we can drop

Added suggestions where I could below and left comments for the remainder

Comment thread recipe/conda_build_config.yaml Outdated
Comment thread recipe/conda_build_config.yaml Outdated
Comment thread recipe/conda_build_config.yaml Outdated
Comment thread recipe/conda_build_config.yaml Outdated

@jakirkham jakirkham left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry think I was mistaken, we still need None for cuda_compiler as recipes do check for it

For example in openmp. Also here are some more

So maybe we keep it zipped and just drop nvcc

Have added some suggestions below to update this accordingly

- cxx_compiler_version # [unix]
- fortran_compiler_version # [unix]
- cuda_compiler # [linux and os.environ.get("CF_CUDA_ENABLED", "False") == "True"]
- cuda_compiler_version # [linux and os.environ.get("CF_CUDA_ENABLED", "False") == "True"]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
- cuda_compiler_version # [linux and os.environ.get("CF_CUDA_ENABLED", "False") == "True"]
- cuda_compiler # [linux and os.environ.get("CF_CUDA_ENABLED", "False") == "True"]
- cuda_compiler_version # [linux and os.environ.get("CF_CUDA_ENABLED", "False") == "True"]

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No

- # [win64 and os.environ.get("CF_CUDA_ENABLED", "False") == "True"]
- cuda_compiler # [win64 and os.environ.get("CF_CUDA_ENABLED", "False") == "True"]
- cuda_compiler_version # [win64 and os.environ.get("CF_CUDA_ENABLED", "False") == "True"]
-

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
-
- # [win64 and os.environ.get("CF_CUDA_ENABLED", "False") == "True"]
- cuda_compiler # [win64 and os.environ.get("CF_CUDA_ENABLED", "False") == "True"]
- cuda_compiler_version # [win64 and os.environ.get("CF_CUDA_ENABLED", "False") == "True"]
-

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No

- None
- nvcc # [(linux or win64) and os.environ.get("CF_CUDA_ENABLED", "False") == "True"]
- cuda-nvcc # [(linux or win64) and os.environ.get("CF_CUDA_ENABLED", "False") == "True"]
- cuda-nvcc

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
- cuda-nvcc
- None
- cuda-nvcc # [(linux or win64) and os.environ.get("CF_CUDA_ENABLED", "False") == "True"]

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No

@h-vetinari

Copy link
Copy Markdown
Member Author

Sorry think I was mistaken, we still need None for cuda_compiler as recipes do check for it

How about making it official that they should check cuda_compiler_version != "None"? A more precise search shows that only ~14 feedstocks are affected.

@jakirkham

Copy link
Copy Markdown
Member

Would prefer to keep this focused on dropping CUDA 11.8

@h-vetinari

Copy link
Copy Markdown
Member Author

Would prefer to keep this focused on dropping CUDA 11.8

you asked for cleaning up cuda_compiler in the zip. And we have a week at least anyway until we can merge this. Plenty of time to fix a handful of feedstocks.

@jakirkham

Copy link
Copy Markdown
Member

And as already noted, I've changed my mind

Let's remember why we are looking at dropping CUDA 11.8. Namely it is holding up other work we would like to do and it is becoming a burden to maintain

If those are true, let's make sure the CUDA 11.8 drop goes through cleanly

There will be plenty of cleanup work to do on the other side of this

@h-vetinari

h-vetinari commented May 30, 2025

Copy link
Copy Markdown
Member Author

Let's remember why we are looking at dropping CUDA 11.8. Namely it is holding up other work we would like to do and it is becoming a burden to maintain

I don't know what to call this if not tonedeaf. I am the one picking up the mantle of pushing this forward, after #7005 hasn't progressed in month. And it's not like this needs a unique skillset - it could have been done by anyone halfway familiar with conda-forge. Turning this around and saying "the way you're doing the work is holding up an important change" is more than a bit rich, even before we get to asking for something and then its opposite.

@h-vetinari h-vetinari marked this pull request as ready for review June 2, 2025 22:55
@h-vetinari h-vetinari requested a review from a team as a code owner June 2, 2025 22:55
@h-vetinari

Copy link
Copy Markdown
Member Author

@conda-forge/core, this is ready for review. We've announced the removal on June 5th, which is in ~2 days. PTAL :)

@jakirkham jakirkham left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not entirely sure I follow the response above

However it does look like my requested changes were marked resolved, but don't appear to be

Have marked them unresolved and requesting changes for clarity

@h-vetinari

Copy link
Copy Markdown
Member Author

Have marked them unresolved and requesting changes for clarity

This is all addressed by #7438 and the PRs to the handful of feedstocks that used cuda_compiler instead of cuda_compiler_version.

@hmaarrfk

hmaarrfk commented Jun 3, 2025

Copy link
Copy Markdown
Contributor

Let's remember why we are looking at dropping CUDA 11.8. Namely it is holding up other work we would like to do and it is becoming a burden to maintain

I don't know what to call this if not tonedeaf. I am the one picking up the mantle of pushing this forward,

Reading this from a 3rd party. I think it read more like "note to self".

Thank you both for thoroughly reviewing and acting on this!

@h-vetinari h-vetinari requested a review from jakirkham June 4, 2025 23:07
@h-vetinari

Copy link
Copy Markdown
Member Author

We're dropping CUDA 11.8 explicitly due to the complexity of the infrastructure setup that it forces upon us (e.g. not being able to migrate for 12.9). So your initial reaction was right here @jakirkham - cuda_compiler should be removed from the zip because we don't need it anymore (as CUDA/non-CUDA can be distinguished easily using cuda_compiler_version).

I did the work to fix the handful of cases where this was still being used, so the argument to keep cuda_version in the zip here has lost its footing both conceptually and practically. As such, this PR is and remains ready (and why I had originally resolved the review requests).

PS. I had wanted to set a looser date ("next week") in conda-forge/conda-forge.github.io#2532, you requested to make the date specific. Now that the announcement went out with a precise date, let's get it done.

@jakirkham

Copy link
Copy Markdown
Member

Axel would appreciate breaking out the cuda_compiler change as its own change

It can be done independently of this and is a trivial follow up

We have a number of cleanup items (for example CUDA 11/12 selectors in receipes). This is just one of many

Would rather we figure them out after

@jakirkham

Copy link
Copy Markdown
Member

Should add we already have some folks asking why trivial changes like this cause issues: conda-forge/openmpi-feedstock#196 (comment)

Whether or not they correctly understand the causes is in my view irrelevant, I would like to avoid even the perception of disruption with this removal, which we can accomplish with the changes noted above

We can combine any cleanup changes (like those I described) as a migrator afterwards

@h-vetinari

Copy link
Copy Markdown
Member Author

Axel would appreciate breaking out the cuda_compiler change as its own change

And I'm rejecting that request. If you insist on it, I'll bow out and let you handle things.

Whether or not they correctly understand the causes is in my view irrelevant, I would like to avoid even the perception of disruption with this removal, which we can accomplish with the changes noted above

It's absurd to attribute a failed build to a trivial selector change. If someone makes that false conclusion, I'm happy to correct it, but I don't care.

@hmaarrfk hmaarrfk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think whatever we can do remove keys from the zip is fine with me.

This is a long time coming for conda-forge.

often "volunteers" bear the brunt of companies trying to maintain support.

I think we should be quite agressive in pruning features and let people build off their own "maintaince forks" for conda-forge for special use cases.

@hmaarrfk

hmaarrfk commented Jun 5, 2025

Copy link
Copy Markdown
Contributor

This PR looks fine as is to me. No need to be super dogmatic about how we make cleanups. @h-vetinari broke out their keyboard and typed and made changes.

has has taken owernship for many "maintenance PRs" such as this one.

I don't think we need to scrutinize it too much.

@jakirkham

Copy link
Copy Markdown
Member

For clarity am supportive of removing CUDA 11.8

We also put in a ton of work to provide the updated packages, fix the various issues that came in, etc.

Am just trying to separate concerns in the following way:

  1. Remove CUDA 11.8 from the matrix
  2. Cleanup leftover bits from CUDA 11.8

Would like to keep this PR to 1 to minimize disruption. Provided the exact changes above to disaggregate: #7431 (review)

Would like to pursue the cleanup of CUDA 11.8 in follow up work. Ideally with a migrator to help automate this across conda-forge

Am not totally following why that wouldn't be amenable to our goals, but would be happy to listen

@hmaarrfk

hmaarrfk commented Jun 5, 2025

Copy link
Copy Markdown
Contributor

Am not totally following why that wouldn't be amenable to our goals, but would be happy to listen

Its honestly just about protecting core maintainer time.

I really don't want "perfect" to be a blocker for "good enough".

CUDA recipe are fragile, but h-vetinari has been following through with many PRs.

I want to give him the ability to take these shortcuts.

This is important for core-member sanity.

@jakirkham

Copy link
Copy Markdown
Member

If it is about time, then we could have committed the reversions with a few clicks (the code is above) and moved on

Instead we now have an issue, several PRs, folks discussing this is too much work and its impacts

The goal of descoping was to cut out this additional work at the start

@h-vetinari

Copy link
Copy Markdown
Member Author

Would like to pursue the cleanup of CUDA 11.8 in follow up work. Ideally with a migrator to help automate this across conda-forge

Am not totally following why that wouldn't be amenable to our goals, but would be happy to listen

Beyond hypotheticals, what actual use does cuda_compiler serve in the pinning or extant feedstocks after CUDA 11.8 is removed? What *concretely* would be there to migrate about it?

You want to separate changes (something I'm normally quite amenable to, where they are unrelated), but the only reason cuda_compiler is a variable is because the name changed between 11.x and 12.x. It is deeply related to the change here, and splitting it up "just because" is just extra work that you're asking be done, for no tangible benefit.

The goal of descoping was to cut out this additional work at the start

And yet I have already done that work. It's a completely moot point by now.

@h-vetinari

Copy link
Copy Markdown
Member Author

What's it going to be? Do I need to revert the announcement, or will you lift your hold?

@hmaarrfk hmaarrfk mentioned this pull request Jun 8, 2025
@hmaarrfk

hmaarrfk commented Jun 8, 2025

Copy link
Copy Markdown
Contributor

@jakirkham don't let my merge requests be an indicator that "i think this is the right way to go".

I feel strongly that we should strive to minimize "churn", whether that messages that we have to type, or "tiny PRs".

For what its worth, the "commit" history from h-vetinari is clean:
7e46ffe

if you merge this without squashing, the "cleanup" is very distinct from the "dropping".

@h-vetinari

Copy link
Copy Markdown
Member Author

I appreciate your support @hmaarrfk, and thanks for the PRs to split up this process. However, I consider this a frivolous waste of everyone's time on the part of @jakirkham, so I'm not in favour of going along with that.

John

Now that I took the time to find a way forward, make the case, get consensus in core, implement it, I get

Let's remember why we are looking at dropping CUDA 11.8. Namely it is holding up other work we would like to do and it is becoming a burden to maintain

If it's such a burden to maintain, why haven't you been pursuing this topic yourself? I find this behaviour absurd, especially when it's coming from someone is in a highly paid position to maintain these bits, throwing up blockers for the unpaid volunteer actually doing the work.

PS. If anyone is wondering about why I don't just go along to move forward: John has, by his own actions on an unrelated topic, entirely lost my trust in his capability to do what's right for the community, so while I will of course take into account valid technical issues, I have very little appetite to jump through hoops for him.

@hmaarrfk

Copy link
Copy Markdown
Contributor

I missed the meeting, but it seems that there is general agreement in dropping 11.8.

I know that no PR can be perfect.

IMO the comments are pretty "minor" and more "stylistic", the commits from h-vetirnari are well organized and for "posterity" there is little advantange in having "separate PRs" vs "separate commits".

Lets merge and let people on different timezones help deal with the fallout.

I understand that this might not be popular, but we decided to drop this 2 weeks ago, we shouldn't be so strict on "spelling"

#6980 (comment)

@hmaarrfk hmaarrfk merged commit 07e16d1 into conda-forge:main Jun 10, 2025
3 checks passed
@hmaarrfk

Copy link
Copy Markdown
Contributor

Thank you all for your work and reviews.

@h-vetinari h-vetinari deleted the cuda118 branch June 11, 2025 01:57
@h-vetinari

Copy link
Copy Markdown
Member Author

FTR, I don't condone merging over an outstanding change request. Neither do I condone leaving a blocking review and then disappearing. Both are a pity.

Ultimately, it's good to keep the ball rolling though, so thanks for (one way or the other) cutting the Gordian knot here, @hmaarrfk.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants