-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Bump PyTensor dependency and support Python 3.12 #7203
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
256c392
to
05df0a5
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7203 +/- ##
==========================================
- Coverage 92.29% 91.86% -0.44%
==========================================
Files 100 100
Lines 16875 16875
==========================================
- Hits 15575 15502 -73
- Misses 1300 1373 +73
|
c51b32a
to
68c2944
Compare
Temporarily running the whole suite on 3.12 only |
e2a5202
to
ffdd78a
Compare
All tests passed on 3.12: https://github.com/pymc-devs/pymc/actions/runs/8379574806/job/22948777361?pr=7203 Going to revert so we still run some of the tests on 3.9 |
ffdd78a
to
64e99fe
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.
I didn't realize that we only test Python 3.9 with Windows. That should be fine 99% of the time, but for those crazy edge cases maybe we want some sort of pre-release workflow that runs the whole matrix?
pytestmark = pytest.mark.filterwarnings("error", "ignore: NumPy will stop allowing conversion") | ||
pytestmark = pytest.mark.filterwarnings( | ||
"error", | ||
"ignore: NumPy will stop allowing conversion", |
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.
Probably out-of-scope for this PR, but is this ignore still necessary? The corresponding issue is closed.
Also minor nit, it'd be nice to copy the style for the last comment/argument and inline those two preceding comments.
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.
Removed, thanks for pointing it out
I'm inclined not to :) |
@@ -32,7 +32,11 @@ | |||
from pymc.exceptions import ImputationWarning | |||
|
|||
# Turn all warnings into errors for this module |
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.
My suggestion was to also inline this. (Turn all warnings into errors for this module)
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 now I'm getting to the nittiest of nits. :)
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 that's why the filterwarnings is there in the first place? Without the temporary filters it looks like
# Turn all warnings ...
pytest.mark.filterwarnings("error")
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.
Do I misunderstand how filterwarnings
works? The arguments get stacked, right?
pytestmark = pytest.mark.filterwarnings(
# Turn all warnings into errors for this module
"error",
# except those related to https://github.com/arviz-devs/arviz/issues/2327
"ignore:datetime.datetime.utcnow():DeprecationWarning",
)
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.
My point is that, without the temporary ignores, it makes more sense to have the comment above the call. That also explains why the call is there in the first place.
# Turn all warnings ...
pytest.mark.filterwarnings("error")
Makes more sense than
pytest.mark.filterwarnings(
# Turn all warnings ...
"error",
)
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'm gonna take the nitpick hint and merge as is
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.
Description
Related Issue
Checklist
Type of change
📚 Documentation preview 📚: https://pymc--7203.org.readthedocs.build/en/7203/