ESMValGroup / ESMValCore

ESMValCore: A community tool for pre-processing data from Earth system models in CMIP and running analysis scripts.
https://www.esmvaltool.org
Apache License 2.0
40 stars 36 forks source link

[Merge after v2.11.0 Release] Remove support for Python 3.9 #2447

Closed valeriupredoi closed 1 week ago

valeriupredoi commented 1 month ago

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 :beer:


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:

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

bouweandela commented 1 month ago

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 commented 1 month ago

This can also be removed: https://github.com/ESMValGroup/ESMValCore/blob/07ba25c425724e61be1ce109ea98a2d791abb3fa/tests/unit/preprocessor/_regrid_esmpy/test_regrid_esmpy.py#L384-L386

bouweandela commented 1 month ago

This needs an update: https://github.com/ESMValGroup/ESMValCore/blob/af5490f9c0e285f65ab8a213629ada7e903c1e36/setup.cfg#L37-L39

valeriupredoi commented 1 month ago

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 :rofl: ), will do it here, and test with 3.10

valeriupredoi commented 1 month ago

tests/unit/preprocessor/_regrid_esmpy/test_regrid_esmpy.py

done in https://github.com/ESMValGroup/ESMValCore/pull/2447/commits/df2aa31b94ce10c0f0adc62d9036f2a567067b67

valeriupredoi commented 1 month ago

This needs an update:

https://github.com/ESMValGroup/ESMValCore/blob/af5490f9c0e285f65ab8a213629ada7e903c1e36/setup.cfg#L37-L39

done in https://github.com/ESMValGroup/ESMValCore/pull/2447/commits/fb2411b50f1b4a28110251db11c97de1cc58391d

valeriupredoi commented 1 month ago

@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 :grin: )? Related issue https://github.com/PyCQA/pyflakes/issues/528 - but it doesn't seem to have a proper resolution

bouweandela commented 1 month ago

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 commented 1 month ago

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 :+1:

valeriupredoi commented 1 month ago

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

valeriupredoi commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 https://github.com/ESMValGroup/ESMValCore/pull/2445#issuecomment-2145281138, which is not included in v2.11.x.

valeriupredoi commented 1 month ago

sounds like what I'd do too :grin:

valeriupredoi commented 1 week ago

@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? :sauropod:

valeriupredoi commented 1 week ago

:sauropod: just about to be extinct :grin: Many thanks for the review @bouweandela :beer: