-
-
Notifications
You must be signed in to change notification settings - Fork 33
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
Drop EOL Python, simplify PyPy situation, improve testing #74
Conversation
f9f361c
to
e204ff0
Compare
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.
Some notes for ease of reviewing (though I recommend to go through it per commit)
tests/test_dependencies.py
Outdated
# since we cannot run the installation tests on all platforms, | ||
# we formulate the conditions when we expect a pin | ||
expect_pin = False | ||
match (platform_system, platform_machine): | ||
case [("Linux" | "Darwin" | "Windows"), "x86_64"]: | ||
expect_pin = True # baseline | ||
case [("Linux" | "Windows"), "x86"]: | ||
# 32 bit wheels on Linux only until numpy 1.21, but should still be compatible | ||
expect_pin = True | ||
case ["Linux", "aarch64"]: | ||
expect_pin = True # as of 1.19.2 | ||
case ["Darwin", "arm64"]: | ||
expect_pin = True # as of 1.21 | ||
# no official wheels, but register minimum compatibility | ||
case ["Linux", "ppc64le"]: | ||
expect_pin = True # support +/- equivalent to Linux/aarch64, but no wheels | ||
case ["Linux", "s390x"]: | ||
expect_pin = True # as of 1.17.5 | ||
case ["Linux", "loongarch64"]: | ||
expect_pin = True # as of 1.22 |
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.
I find this a very compact and readable crystallization of what we're trying to achieve in the setup.cfg, in addition to the distinction of whether wheels are available or not (we could add more testing for this if we whip up some emulation, see #73).
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.
Thanks @h-vetinari. This looks quite good overall. The testing changes seem helpful; I didn't review them in a lot of detail, but they're quite understandable and I'll do a bit more checking before merging.
76eccc3
to
adf7141
Compare
I switched this now to use the default pins for linux-ppc & win-arm64. However, I'm still proposing to not distinguish PyPy from CPython for language level >=3.9. Otherwise I think I've responded to all review feedback now. PS. I rebased & refactored the commits to have a proper history (for archeology, potential reverts, etc.), IMO this should not be squash-merged. |
numpy has been passing the test suite on PyPy 3.8 & 3.9 since at least 1.19. There are no wheels for PyPy 3.9 until numpy 1.25, but neither are there official wheels for loongarch64 etc., so we should reflect that PyPy is supported. Future PyPy versions will only be more compatible with the respective CPython version, so it's not a stretch to mirror the constraints we have for (say) CPython 3.11 to those for a future PyPy 3.11.
Co-Authored-By: Randy Döring <[email protected]>
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.
Thanks @h-vetinari.
However, I'm still proposing to not distinguish PyPy from CPython for language level >=3.9. Otherwise I think I've responded to all review feedback now.
Changing this kind of thing without a compelling reason seems potentially risky to me, without much upside. However, it's likely we'll have to change it again either way when a next PyPy version is released. So let's go with this and if anyone complains, we can revert.
In it goes!
Addressing a couple of inconsistencies & clean-ups:
What I'm doing:
What should still be improved:
"x86", "x86_64", "s390x"
, which is broadly equivalent to what it was set to before (though I removedarm64
from that list, because it's +/- equivalent toaarch64
that was already marked as invalid)Closes #70
Closes #71
Closes #72