Skip to content
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

Bump PyTensor dependency and support Python 3.12 #7203

Merged
merged 4 commits into from
Mar 22, 2024

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented Mar 20, 2024

Description

Related Issue

Checklist

Type of change

  • New feature / enhancement
  • Bug fix
  • Documentation
  • Maintenance
  • Other (please specify):

📚 Documentation preview 📚: https://pymc--7203.org.readthedocs.build/en/7203/

@ricardoV94 ricardoV94 added maintenance pytensor major Include in major changes release notes section release labels Mar 20, 2024
Copy link

codecov bot commented Mar 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.86%. Comparing base (aa679f3) to head (fc98122).
Report is 6 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            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     
Files Coverage Δ
pymc/sampling/forward.py 95.90% <ø> (ø)

... and 4 files with indirect coverage changes

@ricardoV94
Copy link
Member Author

Temporarily running the whole suite on 3.12 only

@ricardoV94
Copy link
Member Author

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

@ricardoV94 ricardoV94 marked this pull request as ready for review March 22, 2024 08:32
@ricardoV94 ricardoV94 requested a review from maresb March 22, 2024 08:32
Copy link
Contributor

@maresb maresb left a 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",
Copy link
Contributor

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.

Copy link
Member Author

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

@ricardoV94
Copy link
Member Author

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?

I'm inclined not to :)

@@ -32,7 +32,11 @@
from pymc.exceptions import ImputationWarning

# Turn all warnings into errors for this module
Copy link
Contributor

@maresb maresb Mar 22, 2024

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)

Copy link
Contributor

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. :)

Copy link
Member Author

@ricardoV94 ricardoV94 Mar 22, 2024

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")

Copy link
Contributor

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",
)

Copy link
Member Author

@ricardoV94 ricardoV94 Mar 22, 2024

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",
)

Copy link
Member Author

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ricardoV94 ricardoV94 merged commit 61ce412 into pymc-devs:main Mar 22, 2024
23 checks passed
@ricardoV94 ricardoV94 deleted the bump_pytensor_dep branch April 5, 2024 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance major Include in major changes release notes section pytensor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend support to Python 3.12
2 participants