Skip to content
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

unpause nogil migration #6673

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

h-vetinari
Copy link
Member

Now that we have a tagged cython release that's compatible with nogil python (even if it's just an alpha...), I think it'd be OK to start the nogil migration. @isuruf has been doing this manually across a bunch of feedstocks already, but that doesn't show up in the status page anywhere.

Even if the migration will move only very slowly, I think it would be good to start it, if only for the increased visibility.

@h-vetinari h-vetinari requested a review from a team as a code owner November 9, 2024 11:40
@conda-forge-admin
Copy link
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.

Copy link
Member

@isuruf isuruf left a comment

Choose a reason for hiding this comment

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

Let's not do this right now. I talked with @rgommers, and we thought it'd be best to wait until there's general availability. The PRs I sent are for packages that upstream already supports. I intentionally didn't make PRs for packages without freethreading support. The bot can't make those distinguishments yet

@rgommers
Copy link

rgommers commented Nov 9, 2024

Indeed, we happened to discuss this just yesterday. Q1 2025 seems like a good time to do this migration. Right now there are only a few packages which have a first release that supports free-threading up on PyPI, and there will be too many reliability issues when building the previous release. For one, large parts of the numerical/scientific stack will need SciPy, and while 1.14.0 builds right now it is far from safe - 1.15.0 in Jan'25 is the first version that should be tested by a broader audience. The same is true right now for anything that relies on PyO3 (first release expected any day now), cffi, Mypyc, etc.

@h-vetinari
Copy link
Member Author

I intentionally didn't make PRs for packages without freethreading support. The bot can't make those distinguishments yet

Sure. My point is that such PR can stay open (same situation like we have for numpy2 in many cases). It says "we're waiting for upstream support to be ready".

I think that would be preferable from a visibility POV, because people are asking (example), and there's no good way for people to see what we're blocked on. conda-forge is ready, but many packages aren't yet -- having open PRs (with many "children" on the migrator status page) would make that clear, and also where interested people can start helping out with testing/debugging etc.

@rgommers
Copy link

rgommers commented Nov 9, 2024

My point is that such PR can stay open (same situation like we have for numpy2 in many cases).

The problem is that in many cases the test suite will just pass, because test suites typically don't contain tests for concurrency. So it's very different from numpy 2.0 in that respect - there if tests pass, things were safe to merge. I'm worried that green PRs on feedstocks will be misleading and someone will just merge them.

@h-vetinari
Copy link
Member Author

That's a fair point of course. While we could add some text explaining this to the opening comment (& commit message), I agree that people don't necessarily read those. However, we could also put a "[DO NOT MERGE]" in the title, and then explain what's needs to be done to remove that (i.e. check that upstream is compatible)

@rgommers
Copy link

However, we could also put a "[DO NOT MERGE]" in the title, and then explain what's needs to be done to remove that (i.e. check that upstream is compatible)

That would address my concern. Whether it's desirable to open PRs with such a title I have no strong opinion on (I can see some pros as well as cons).

@hmaarrfk
Copy link
Contributor

However, we could also put a "[DO NOT MERGE]"

do you really want to be trailblazing on this front? It just seems like adding extra work for maintainers at this point.

@h-vetinari
Copy link
Member Author

Well, the point is exactly to avoid feedstock maintainers from blindly pressing merge due to green CI - and I think that's a relevant concern for the nogil migration regardless of whether it happens in 2025Q1 or sooner. We've seen that many people do not read instructions that we put in the opening comment of the bot PR, so then the only thing left would be changing the title.

That, and/or I guess we could also teach regro/cf-scripts to open PRs as drafts (from the POV of the GH UI), which makes them non-unmergeable by accident.

@hmaarrfk
Copy link
Contributor

I just don't see the point in rushing this. Numpy has a (critical IMO) bug that does not seem resolved
numpy/numpy#27199

glib also seems like "next in the PR" according to conda-forge.org/status
but has an open issue where it seems that upstream developers need to spend time on it more carefully.
https://gitlab.gnome.org/GNOME/pygobject/-/issues/646

Starting in Feb/March 2025 just seems like the better use of your time IMO.

@rgommers
Copy link

I just don't see the point in rushing this. Numpy has a (critical IMO) bug that does not seem resolved
numpy/numpy#27199

I agree with the "no rushing" sentiment, but note that the NumPy bug you're linking is not blocking, so that's not the best example. NumPy itself has already released cp313t wheels, documented thread safety guarantees (mentioning mutating object arrays) at https://numpy.org/devdocs/reference/thread_safety.html, and there are also conda-forge free-threaded numpy packages up already. Free-threading is still experimental in 3.13, so there will be rough edges and known limitations. But we're not going to wait until every last hole is plugged before releasing conda-forge packages.

The glib issue is indeed a good example of what should be blocking in my opinion. That's a "we haven't looked at this at all yet, and when thinking for a minute we already realize internal caches are unsafe". So there are likely fairly basic correctness issues.

@h-vetinari
Copy link
Member Author

Starting in Feb/March 2025 just seems like the better use of your time IMO.

I don't think the nogil migration can be accelerated substantially (beyond what the various upstream maintainers are able to do to achieve compatibility) - or in other words, the effort doesn't change from our POV. The one benefit I see from starting the migration is that we can point to existing bottlenecks, rather than effectively saying "conda-forge is not ready yet". The interest in nogil might even be big enough that some non-zero percentage of users starts helping out in the respective upstream to unblock things (that's only a side benefit though, not the main reason).

@hmaarrfk
Copy link
Contributor

given how touch each recipe is, can you start by issuing manual PRs?

If users are interested in 2024, I would be interested in hearing them them chime in here before we go out of our way squashing bugs upstream.

@h-vetinari
Copy link
Member Author

given how touch[?] each recipe is, can you start by issuing manual PRs?

This is what Isuru has been doing. My point is that all this is completely invisible except to people who're actively scouring conda-forge for that information.

If users are interested in 2024, I would be interested in hearing them them chime in here before we go out of our way squashing bugs upstream.

I seem to be failing to communicate. We don't need to go our way to squash upstream bugs. Those can be fixed at the regular pace. But having a publicly tracked (on the status page) PR pointing to "this is the missing dependency for the package you want, and here's the upstream repo if you want to help fix it" would be very beneficial IMO.

PS. I also already gave an example further up for pandas. Many end users don't understand the distinction between an upstream project and the distribution through conda-forge, so often these issues just get raised upstream. It would likewise help those maintainers to be able point to the status page of the nogil-migrator for such questions.

@isuruf
Copy link
Member

isuruf commented Nov 11, 2024

I completely agree with Mark's sentiment.

PS. I also already gave an example further up for pandas-dev/pandas#60016.

That's just one user. One user does not warrant a huge effort from all of our maintainers.

But having a publicly tracked (on the status page) PR pointing to "this is the missing dependency for the package you want, and here's the upstream repo if you want to help fix it" would be very beneficial IMO.

We can have an issue about ongoing freethreading support in conda-forge and users can chime in there. There's no need to burden our maintainers with this right now.

@h-vetinari
Copy link
Member Author

One user does not warrant a huge effort from all of our maintainers.

This was just an indicative example. Interest in nogil is high, so there's far more than one user involved.

More importantly, I don't understand what "huge effort" you're talking about. How does an open bot PR (which can just sit there until upstream is compatible) cause extra effort, much less a huge one?

@dopplershift
Copy link
Member

Well it burdens the conda-forge package-level maintainers with yet another PR that requires research to understand whether merging it is a good idea, even if it does pass.

I'd personally much rather this process do something opt-in, like our osx_arm64 support (which 4 years on is not the default), rather than one that is opt-out after deciding to ignore a PR (which also can't be closed).

@h-vetinari h-vetinari marked this pull request as draft November 12, 2024 01:48
@h-vetinari
Copy link
Member Author

Well it burdens the conda-forge package-level maintainers with yet another PR that requires research to understand whether merging it is a good idea, even if it does pass.

Understood (though I fail to see the "huge" effort in that).

I'd personally much rather this process do something opt-in, like our osx_arm64 support (which 4 years on is not the default), rather than one that is opt-out after deciding to ignore a PR (which also can't be closed).

Interesting idea. This would also allow to keep it tightly scoped (e.g. to known compatible packages) in the beginning. OTOH, we should probably consider making osx-arm64 a default. We roughly have a 50:50 split between x64 and arm64 for osx downloads already, and it'll only get more skewed towards arm64 over time.

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.

6 participants