Pin numpy for old numba versions #376
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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 ( |
beckermr
requested changes
Dec 20, 2022
recipe/gen_patch_json.py
Outdated
| pkg_resources.parse_version("0.53.1") | ||
| ) | ||
| ): | ||
| _replace_pin("numpy >=1.19.5,<2.0a0", "numpy >=1.19.5,<1.24", record["depends"], record) |
There was a problem hiding this comment.
This won't catch all old versions. Don't we need to adjust all pins that go up to 2.a0?
There was a problem hiding this comment.
Yes, I can patch more versions. But for most of them the upper bound of 1.24 will not be strict enough.
|
When I patch all versions with a |
|
Ah great. I just remembered that we need to follow the published table here. conda-forge/numba-feedstock#90 |
|
Okay, this patch is getting bigger than I had anticipated. In eb2c378, I'm using the compatibility table from the numba docs to pin the upper bound of the numpy dependency. I'm leaving the lower bound alone. Does that make sense? I'm not touching the other dependencies (for now). |
|
We'll need to make sure all numba packages have an upper bound, or the solver will attempt to install the oldest ones. I think anything older than what is in their published table should have an upper bound of say 1.18 or something. cc @conda-forge/core @conda-forge/numba for guidance |
beckermr
reviewed
Dec 20, 2022
Allow for approximate comparisons of package versions.
|
I added a new upper bound of 1.18 for all numba versions before 0.47 and made sure that comparisons between, say, New diff: |
|
@stuartarchibald, could you please take a look at this and let us know what you think? |
|
Are the older versions entirely broken unless the upper bounds declared here are met? What I'm getting at is: Do we really want to put in the tightest known bounds for old versions just because of correctness? Adding (upper) bounds is certainly helpful and needed now; no dispute on that. But I'd rather have us use more lenient bounds, e.g., |
|
Right @mbargull. I agree with this. Do you know how we might decide what the looser bounds should be? |
I wouldn't go lower than The lower bounds I wouldn't touch at all since I believe we don't get any benefit from that:
|
|
Alright. I'd be ok with |
|
Okay, thanks for the comments! I made a new proposal: For
That still fixes my original issue (I was pulling in numba 0.53.1 together with numpy 1.24). I'm happy to adjust the exact pin. Also happy to not patch ancient versions that we're unlikely or unable to pull in because of python/numpy versions. Full diff: |
|
thank you! Let's wait for @stuartarchibald |
|
Thought the point of this was to pin old Numba versions that lacked the NumPy constraint. There already was a NumPy constraint for Numba 0.54.0. Not sure why we need to change that. Would prefer we stick to fixing the old versions of Numba that lacked the pin in this PR unless there is a compelling reason not to (like the pin used was wrong). |
I'm only adding NumPy constraints before Numba 0.54.0. So I'm not changing the constraint you're linking to. |
|
@mbargull thanks for your thoughts on this...
It's hard to give a concrete answer to this. Most of the time Numba will "work" and produce results similar to NumPy across NumPy versions. Occasionally there are things that change in new NumPy versions that result in significant numerical differences against Numba and more rarely there are things that cause Numba to just e.g. segmentation fault (IIRC NumPy
I'm inclined to agree with this on the basis that users could well have created workflows that "mostly work" based on packages that are declared incompatible due to lack of specific constraints at the time and it would be good to avoid breaking these. @jtilly @beckermr RE the specific version...
I think this is a reasonable version given the above arguments. As this is patching meta-data, it can be patched again if more information becomes available. Therefore I'm in favour of adding the lenient bound and assuming it's "ok" until proven otherwise as that covers what is potentially happening in practice! |
|
Thanks @stuartarchibald! LGTM! |
beckermr
approved these changes
Jan 3, 2023
Great to see this resolved, thanks all! |
|
@jakirkham Any further comments or shall we merge? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With the latest version of numpy (1.24.0), we're pulling in old, incompatible versions of numba (0.53.1).
For current numba versions, the numba feedstock pins numpy more strictly (see here).
Checklist
0(if the version changed)conda-smithy(Use the phrase@conda-forge-admin, please rerenderin a comment in this PR for automated rerendering)This is the diff:
Fixes conda-forge/numba-feedstock#90