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

[v2.11.1] Remove support for Python 3.9 #2447

Merged
merged 18 commits into from
Jul 8, 2024
Merged

Conversation

valeriupredoi
Copy link
Contributor

@valeriupredoi valeriupredoi commented Jun 4, 2024

Description

Closes #2406

AFAIK I removed all references for Python 3.9 - if you know of any others please holler back so I can replace them 🍺


Before you get started

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the πŸ›  Technical or πŸ§ͺ Scientific review.


To help with the number pull requests:

@valeriupredoi valeriupredoi added the installation Installation problem label Jun 4, 2024
@valeriupredoi valeriupredoi added this to the v2.12.0 milestone Jun 4, 2024
@valeriupredoi valeriupredoi requested a review from bouweandela June 4, 2024 12:44
Copy link

codecov bot commented Jun 4, 2024

Codecov Report

All modified and coverable lines are covered by tests βœ…

Project coverage is 94.62%. Comparing base (f9e6f46) to head (e04d666).
Report is 34 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2447   +/-   ##
=======================================
  Coverage   94.62%   94.62%           
=======================================
  Files         247      247           
  Lines       14057    14057           
=======================================
  Hits        13301    13301           
  Misses        756      756           

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

@valeriupredoi valeriupredoi changed the title Remove support python==3.9 Remove support for Python 3.9 Jun 4, 2024
@bouweandela
Copy link
Member

I think we do not need from __future__ import annotations anymore with recent Python versions, would you like to give that a try? i.e. remove it from everywhere and then run the GitHub Actions matrix tests to see if it works?

@bouweandela
Copy link
Member

This can also be removed:

@pytest.mark.skipif(sys.version_info.major == 3
and sys.version_info.minor == 9,
reason="bug in mock.py for Python 3.9.0 and 3.9.1")

@bouweandela
Copy link
Member

This needs an update:

ESMValCore/setup.cfg

Lines 37 to 39 in af5490f

[mypy]
# see mypy.readthedocs.io/en/stable/command_line.html
python_version = 3.9

@valeriupredoi
Copy link
Contributor Author

from future import annotations

you're right! It seems it's not needed anymore (after 3.10+), though I couldn't find an official source (yet my reasearch took no longer than 5 whole minutes 🀣 ), will do it here, and test with 3.10

@valeriupredoi
Copy link
Contributor Author

tests/unit/preprocessor/_regrid_esmpy/test_regrid_esmpy.py

done in df2aa31

@valeriupredoi
Copy link
Contributor Author

This needs an update:

ESMValCore/setup.cfg

Lines 37 to 39 in af5490f

[mypy]
# see mypy.readthedocs.io/en/stable/command_line.html
python_version = 3.9

done in fb2411b

@valeriupredoi
Copy link
Contributor Author

valeriupredoi commented Jun 5, 2024

@bouweandela have a look at the flake8 barf now that we've removed from future... - F821 is indeed an undefined name in flake8 error parlance, but it's typing assignment, not variable assignment - any suggestions (before I peasantly put a # noqa there 😁 )? Related issue PyCQA/pyflakes#528 - but it doesn't seem to have a proper resolution

@bouweandela
Copy link
Member

bouweandela commented Jun 5, 2024

I tried to fix the flake8 error (it happens because the class used as a type hint is not yet defined at the point where it is used as a type hint, so in some cases you can just re-order the code and then it works), but then I tried to run the tests and found that we still need the from __future__ import annotations also where we use the bare class name (i.e. without quotes) when the used classes are imported only for type checking. In short, I think we cannot yet get rid of the from __future__ import annotations. Thanks for trying though!

@valeriupredoi
Copy link
Contributor Author

I tried to fix the flake8 error (it happens because the class used as a type hint is not yet defined at the point where it is used as a type hint, so in some cases you can just re-order the code and then it works), but then I tried to run the tests and found that we still need the from __future__ import annotations also where we use the bare class name (i.e. without quotes) when the used classes are imported only for type checking. In short, I think we cannot yet get rid of the from __future__ import annotations. Thanks for trying though!

cheers for testing, bud! Lemme revert the commit then - easy-peasy πŸ‘

@valeriupredoi
Copy link
Contributor Author

OK buds, reverted those two last commites (running GA and removing from future) 🍺

Copy link
Member

@bouweandela bouweandela left a comment

Choose a reason for hiding this comment

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

Thanks! Maybe we should wait with merging until the v2.11 release is done?

@valeriupredoi
Copy link
Contributor Author

valeriupredoi commented Jun 6, 2024

Yep let's do that, maybe we do a rebuild when ESMValTool has the same package specs? We seeing esmpy>=8.6.0 causing some issues with resolving the env with python 3.12 at the mo at Tool, so we can wait a bit

@valeriupredoi
Copy link
Contributor Author

I'd argue we should add a label a la "warrants/motivates a rebuild" just as we have "needs new ESMValCore release" in Tool - what do you think, bud?

@valeriupredoi
Copy link
Contributor Author

also, excuse my verbosity today, what do you think I just remove 3.9 from the GA tests ie https://github.com/ESMValGroup/ESMValCore/actions/runs/9393366881 in a separate PR just so that release folks don't get annoyed when it comes to checking tests for the release?

@bouweandela
Copy link
Member

They may want to run the GitHub actions on the v2.11.x branch instead of on main, as it is getting rather behind main and I would not expect the failing Python 3.9 tests on the v2.11.x branch as they were introduced in #2445 (comment), which is not included in v2.11.x.

@valeriupredoi
Copy link
Contributor Author

sounds like what I'd do too 😁

@valeriupredoi valeriupredoi changed the title Remove support for Python 3.9 [Merge after v2.11.0 Release] Remove support for Python 3.9 Jun 11, 2024
@valeriupredoi
Copy link
Contributor Author

@bouweandela now that @ehogan has released 2.11, would you mind if I merged this, so we get us rid of that dinosaur once and for all? πŸ¦•

@valeriupredoi
Copy link
Contributor Author

πŸ¦• just about to be extinct 😁 Many thanks for the review @bouweandela 🍺

@valeriupredoi valeriupredoi merged commit 495176a into main Jul 8, 2024
7 checks passed
@valeriupredoi valeriupredoi deleted the remove_support_python39 branch July 8, 2024 12:55
schlunma added a commit that referenced this pull request Nov 25, 2024
schlunma pushed a commit that referenced this pull request Nov 25, 2024
@schlunma schlunma changed the title [Merge after v2.11.0 Release] Remove support for Python 3.9 [v2.11.1] Remove support for Python 3.9 Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
installation Installation problem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drop support for Python 3.9
2 participants