-
-
Notifications
You must be signed in to change notification settings - Fork 149
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
constrain all numpy-dependent packages to numpy <2 #712
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 ( |
Not sure how well we can express that in the new yaml format, but we could restrict the patching to python >=3.9 builds, because anything below will never get a numpy 2.0 release. |
OK, I did that now, gist is updated; now "only" 100k artefacts are affected, rather than 200k. |
Thanks Axel! 🙏 Interesting observation. Yeah we could limit to Python 3.9+. Timestamp might be a good proxy as we can look at when Python 3.9.0 was packaged. How many would that affect? If we limit to Python 3.9 or newer, we might want to handle Am guessing most of these are packages that simply depend on NumPy at runtime. Is that true? Would think the ones with NumPy as a build dependency are already constrained, but it would be interesting to know if there are any exceptions |
I've sorted the various sections in the output of |
This is all already solved in 9305679
Anything built before the numpy builds using the run-export (which was actually backported until 1.19; conda-forge/numpy-feedstock@b84c9f4) would just be relying on |
if: | ||
has_depends: numpy?( *) | ||
subdir_in: noarch | ||
timestamp_lt: 1713569710960 | ||
then: | ||
- tighten_depends: | ||
name: numpy | ||
upper_bound: "2.0" |
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.
This patch doesn't make any sense to me. A noarch python package that just calls np.sum
on some arrays doesn't need this constraint.
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.
That's fair, though I intentionally included noarch packages here, because it's easier IMO to draw a clear boundary between 1.x & 2.x, rather than hope that these packages are not affected in any way, shape or form by the many changes in 2.0 (which aren't just restricted to ABI-changes).
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.
The same comment applies to any non-noarch package too.
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.
We cannot make the 1.x vs 2.x decision for maintainers. This is too intrusive. They can add their own patches later if they feel they need it.
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.
We cannot make the 1.x vs 2.x decision for maintainers.
I disagree. Perhaps @rgommers can weigh in here.
FWIW, here are all the 43571 packages where at least one artefact gets patched:
Footnotes
|
The patches here need to test if the artifact has an existing constraint with an upper bound, and if it does, it should tighten them |
The issue is exactly unbounded requirements. No-one has prepared for a numpy 2.0 world (except the last couple of months) - a few years ago this was considered a creature of mythology only. I disagree that we should just publish numpy 2.0 and let existing packages with unbounded numpy requirements crash and burn. Every package needs to validate that it's 2.0-ready. |
Sorry Axel was posting on my phone with poor network. So I didn't see your comment. Thanks for following up! IIUC Matt is just saying that if a package already has a constraint (or perhaps a tighter constraint) that already excludes NumPy 2, then we can skip those. Matt do you have a suggestion on syntax? |
The historical approach of conda-forge has been to take a minimal approach to these issues. We manage and run ABI migrations primarily out of necessity. In the vast majority of cases, we leave API breaks to maintainers except for a few special cases (e.g., noarch python 2x to 3x).
We're not letting packages crash and burn. We're giving people choices about how they want to handle a partial API break. There are a lot of APIs in numpy that will stay the same.
This is potentially a ton of work we are foisting on maintainers and only one approach. My personal approach is to wait and see for feedstocks I maintain. If things break, I'll fix them. |
No, this is not what I am saying. The repodata patching code already knows how to do this. What I am saying is that we should only be patching packages that used numpy at build time, as opposed to a pure runtime requirement. |
Except NumPy has made breaking changes in the pure Python layer |
Yes, but as I have said above, we have always left the vast majority of API breaks to maintainers to handle. |
Ok well I disagree and would agree with Axel here The issue is insufficiently constraining packages will negatively impact users installing the packages (maybe with no idea what is going on) and ourselves as we will have to scramble to do what we are already proposing to do now In terms of maintainers we can extend the NumPy migration to pure Python packages. That would build confidence that packages are working properly by testing them. Though ultimately it is a core package making a major version bump, it will be a lot of work no matter what |
Yeah I disagree. Sorry for being late to the party here. I had to miss the meeting this week. FWIW, I 100% agree we should be doing the ABI-related ones. IIUIC, the ABI related ones are already handled if people have been using pin_compatible properly and not putting numpy versions in host. We should take a look at similar situations in the past. As far as I know, we did not make any global effort on patching when pandas had major version bumps. For sure pandas isn't as core to the ecosystem as numpy, but it is big. We did see quite a few breaks and they were fixed.
We can leave this PR open and merge it later if the sky falls down or w/e. No scramble is needed.
That sounds nice but I think the marginal value is small. We don't typically do a ton of detailed testing in a build feedstock. OFC the build itself will test build issues very well!
Agreed it will be a lot of work. I am advocating that we let people choose how they want to do that work instead of forcing them to come through with a new release of their package and new build of the associated feedstock. If this PR is merged, and I want to remove the patch from a feedstock I maintain, it is going to be a pain and also very confusing. Why not make an opt-in system for the API-related patches here and let folks submit PRs to add their packages if they feel they need to? |
Numpy has too big a blast radius IMO to just leave this to shake out "naturally". It's IMO a substantial risk that regular environments start breaking when they get upgraded to numpy 2.0 due to the packages not having foreseen numpy 2.0 and lacking that constraint - that's IMO not worth the hassle of breaking users, lots of confused bug reports, stressed feedstock maintainers, and ultimately reputational damage. I'd much prefer to take our time to roll out 2.0 more judiciously than just lobbing it into the room and seeing if things go boom. |
Agreed! Let's do that in a way that gives people choices instead of patching 100k artifacts at once. |
Sorry, our answers crossed wires.
I don't see how we could reasonably distinguish that (unless you mean leaving out |
The lack of constraints in the metadata forces our hand. Publishing |
Assuming folks have been following our docs (which is all we are responsible for anyways), then their packages that build against numpy use pin_compatible in run and so have a <2 constraint already. See an example here for a package I help maintain ![]() So all of the ABI-related breakages have already been handled. Thus we can reasonably assume that anything left without a <2 constraint is API-related only. |
We could imagine lots of procedures to help folks. Here are some ideas:
|
Not to derail this important discussion, but would it be possible to look at whether there are any packages using NumPy at build, but missing an appropriate constraint at runtime? |
Yes, this is a good question. Two ideas:
We can look but ultimately folks who don't follow our docs are own their own at some level. |
Even without pin_compatible, they'd already be getting the run-export for ~2 years if they host-depend on numpy. :) Though there are still builds from before that time that the resolver would then take into account, e.g.
That is another side of the major risks IMO - the fact that we could be pushing the solver into a corner that hasn't been closed off, because it finds old package versions that are "compatible" while looking for ways to install numpy 2.0.
The various API incompatibilities are what I meant by not just releasing and waiting to see if things go boom.
Agreed that with big hammers like track_features or mutexes we could avoid surprises, though at this point, the discussion should probably be moved to conda-forge/conda-forge.github.io#1997. :) |
Ah sorry @h-vetinari. I was responding to the comment you said about how we would know if the runtime dep was API or ABI. Agreed that the run export helps too. I am all for preemptively patching selected packages (e.g., tensorflow, packages we can figure out build with numpy but are not pinned correctly, other common cases) which we know will have issues. What I do not like is patching 100k artifacts at once. We can and should be using a more delicate approach. Also, just to be clear, as usual since I am speaking up, I am more than happy to help out with the extra work I am creating here. |
Kind reminder @beckermr that we need to do something here. Otherwise a lot of stuff will break (upon the release of 2.0 GA). |
Thanks for the reminder. I think we should go ahead and formulate a list of big packages we know about with issues. IDK precisely which ones, but here is a possible start: non-compiled packages from the gist above:
What other packages do you think we should proactively include? I am happy with this list. Once this PR has been restricted to those packages, we can make an announcement telling folks to add their package to the patch via a PR if they want. Then we should be good to go. |
All packages compiled against numpy must get an upper bound. The only real discussion (to my understanding) was whether to also cap <2 for packages that are just runtime users. |
Yep and those packages already have an upper bound. So anything in the list I made above that is not compiled against numpy can be removed. Thanks for catching that! Are there any big ones that are not compiled that you think we should patch? Edit: I went through the gist and added a few non-compiled ones. |
@jakirkham what about fastparquet? Should we go ahead and patch that to numpy < 2 ahead of time? |
Not all of them. If someone relies on numpy being present as a transitive host-dependence, it won't get the run-export (yes, you can argue that the recipe is wrong, but we don't know how many could be affected). Skimming the list of changes from this PR, I also found cases that are curious. For example py39 builds of astropy 4.1, which correctly did:
(though with a superfluous |
Perhaps @rgommers can provide some perspective here. Personally, I still think patching broadly to add |
Patching 100k artifacts is really not going to work. It also worries me from a technical perspective of the CDN and the workflow on the anaconda backend. I flagged astropy already above in the list. Any others that you feel are needed? As I said before, we cannot be responsible for incorrect recipes. We can provide tools and information for folks to help themselves. |
The streamlit app is wrong here. cc @jaimergp I looked directly on anaconda.org and see the upper bound for numpy. |
I'm not saying to assume responsibility for incorrect recipes. But it is our responsibility if we do something that breaks existing packages/workflows, incorrect or not. Before it was working (even if partly by accident), then it suddenly breaks. Whether we like it or not, this IMO falls under our responsibility as stewards of "keeping the show on the road", and doubly so if we foresee the possibility of a problem beforehand. If this is even moderately widespread, it'll create a lot of negative sentiment on conda & conda-forge. I worry that (through cumulative effects) we'd end up breaking a lot of current deployments/environments, and insisting that the recipes were wrong isn't gonna buy us any favours IMO.
Can we get input on that from the anaconda folks? |
I also assume that Anaconda will have to deal with very similar issues once they introduce numpy 2.0. Perhaps its worth having the discussion how to tackle that in a wider circle? |
Note also that widespread patching also causes issues, especially when the local repodata differs from upstream. Not all of our solvers correctly handle that case. Last I checked they use the local repodata instead of the downloaded repodata for already installed packages. |
Yeah, there's no free lunch... I'd like to understand better what the costs are though. Prioritizing local repodata is always going to be an issue in repodata patching efforts, so it's not especially related to how many packages we patch? Out of curiosity, I removed the noarch-packages from the packages to patch here, which would save ~30k artefacts, leaving ~70k to be patched. |
One reason I think we cannot really make that argument in the case of numpy, is that our own tooling creates false positive warnings like:
That's because numpy's symbols are never linked into the binary and only loaded at runtime (therefore not detected by lief; as an aside, conda-build really should special-case numpy IMO). But if someone takes our own warning and adds it to |
Yeah we're in the weeds here on what we are and are not responsible for. Regardless of that issue or even if ananconda.org can handle the patches, I'm still not in favor because it is very intrusive. Repodata patching is fundamentally an ugly hack and while I am a big proponent of it as necessary evil, I'd like to minimize it. |
When there's usage of the C API, then yes - but I believe that case was in good shape already? Modulo the "transitive host-dependence" corner case that is. For regular usage of NumPy's Python API, this may be overstating the problem. The fixes in many downstream packages were fairly small. So I expect most packages to fail some of their tests, but import fine and have most of their functionality work.
I would add everything from numpy/numpy#26191 - it contains the exact versions that are compatible; older versions should get the constraint for Python >=3.9. That list is broad enough that I expect it to catch a lot of issues (most Python environments that use |
Superseded by #728. |
This addresses #516. It creates a huge diff (
~200k~100k1 artefacts affected 😱), which can be viewed in this gist.AFAICT, this is mainly due to various feedstocks open-coding a pure run-dependency like
- numpy >=1.16
, which unsurprisingly doesn't get affected by the run-export we've had for 2 years (conda-forge/numpy-feedstock@bca0916) when building against numpy.Footnotes
after restricting to py>39 & noarch ↩