-
Notifications
You must be signed in to change notification settings - Fork 145
Drop Python 3.10 and NumPy < 2 #1253
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1253 +/- ##
==========================================
- Coverage 81.67% 81.65% -0.03%
==========================================
Files 232 232
Lines 53132 53078 -54
Branches 9410 9405 -5
==========================================
- Hits 43396 43340 -56
- Misses 7283 7285 +2
Partials 2453 2453
🚀 New features to boost your workflow:
|
Does this mean we can also drop numpy<2 and related CI matrix? |
Yeah I think that the plan was to drop numpy < 2.0 and python 3.10 together if I remember correctly |
Let's do it. There is stuff in |
There is stuff remaining but I want to check this for now. |
pytensor/tensor/math.py
Outdated
else: | ||
# The value here doesn't matter, it won't be used | ||
c_axis = numpy_axis_is_none_flag | ||
c_axis = np.iinfo(np.int32).min # the value of NPY_RAVEL_AXIS |
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.
Seems better to have this computed in one of the util files with the more readable name?
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.
But in this case, if the value actually doesn't matter as the comment says, perhaps we can even use 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.
Yeah that seems right
I was wondering if I need to increment the c code version after removing the compatibility headers? |
I don't think so, the generated C code didn't change if you are using numpy>2.0, and you can't use the old one anymore now. Could be wrong ofc |
Even with this commit? a0ad246 |
Can you check if we bumped the C versions when we added that header thing to those Ops? If we did, then maybe let's be safe and bump again |
.github/workflows/test.yml
Outdated
- fast-compile: 1 | ||
- float32: 1 | ||
- python-version: "3.13" |
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 couldn't comment with suggestion on the phone, but these are excluding all fast-compile/float32/python 3.13 runs. They only made sense in combination with numpy version (we were trying not to blow the CI when we added the numpy variants)
- fast-compile: 1 | |
- float32: 1 | |
- python-version: "3.13" |
This is the commit that introduced that header, it changed the C code in other ways too but it did bump the C versions: 0aa10c0 |
Right, let's rebump then. Thanks for checking it |
463e252
to
5a8976b
Compare
pytensor/link/c/basic.py
Outdated
|
||
# We must always add the numpy ABI version here as | ||
# DynamicModule always add the include <numpy/arrayobject.h> | ||
ndarray_c_version = _get_ndarray_c_version() |
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.
How slow is this? Do we want to execute ot only once perhaps?
looks good just a small question |
8b375f4
to
e28ba52
Compare
Thanks a ton @Armavica Anything else missing? |
I can't think of anything else |
The PyPI CI seems to be failing |
Not sure what this is about, I will debug in a few days |
I think it's trying to build something with python 3.10. Also are you rebased from main? |
Specifically this job may need the python version specified (the one above pytensor/.github/workflows/pypi.yml Line 186 in 4312d8c
|
Thank you, it looks like that did the trick |
LGTM. I asked @lucianopaz and @maresb to also take a look in case they spot something else problematic. Specifically I suspect @maresb needs to be in the loop for the conda release anyway |
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 @Armavica! I'll be ready for this on the conda-forge side.
Thanks again @Armavica |
Description
Related Issue
Checklist
Type of change
📚 Documentation preview 📚: https://pytensor--1253.org.readthedocs.build/en/1253/