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

Update CI to use Python 3.12 and add Python=3.12 to package description in setup.py #111

Merged

Conversation

valeriupredoi
Copy link
Contributor

Hi @robertjwilson, I thought that, rather than me opening an issue, and let you do the work, I can do this myself and just open a PR, hope you don't mind it 🍺 I noticed that the PyPI package actually builds with Python=3.12, and it'd be good to have the CI run tests with it too, plus, add it as a pkg descriptor, so the PyPI page lists it correctly; then we can go ask conda-forge to rerender with Python 3.12 so we can have it deployed there too (would you reckon noarch package would be better though?). I actually tested that cartopy, cdo and nco all sit in a 3.12 environment before I created this PR 😁 Cheers!

@codecov-commenter
Copy link

codecov-commenter commented Dec 4, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9663108) 83.79% compared to head (5b1b5ac) 83.79%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #111   +/-   ##
=======================================
  Coverage   83.79%   83.79%           
=======================================
  Files          69       69           
  Lines        7295     7295           
=======================================
  Hits         6113     6113           
  Misses       1182     1182           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@robertjwilson robertjwilson merged commit df5fe71 into pmlmodelling:master Dec 4, 2023
2 checks passed
@valeriupredoi valeriupredoi deleted the update_CI_to_Python312 branch December 4, 2023 17:58
@robertjwilson
Copy link
Member

I have just merged this. Thanks for drawing this to my attention @valeriupredoi. The tests were in fact failing locally on python 3.12, and I couldn't figure out why. Essentially the tests check to make sure temporary files are being cleaned up, but they weren't in one of the tests. Initially I thought it was some multiprocessing update. But after looking into it just now, it's an update to the garbage collector. Collection is now triggered differently in Python 3.12, and temporary files are being cleaned up effectively but the triggering event is slightly different. In this case it was a couple of lines of code later that was triggering.

I still don't quite understand the change in behaviour, but it looks like there is no need to update nctoolkit for it. All I've done in one of the tests is force collection using gc.collect(), which fixes the test.

I have just released a new version of nctoolkit's plotting dependency ncplot, which should be available on Python 3.12 once their bot picks it up in a couple of hours. So a 3.12 re-render should be possible tomorrow, unless something else isn't available on 3.12 on conda.

I will think about noarch. I'm not entirely sure if it's wise as multiprocess is currently used for macOS, but not Linux. I haven't dropped that dependency on Linux (too lazy :)), but ideally I want to be able to. I guess with noarch there is no way to remove it.

@valeriupredoi
Copy link
Contributor Author

valeriupredoi commented Dec 5, 2023

great stuff @robertjwilson, many thanks 🍺 I am looking at the conda feedstock and see that you are the only maintainer there - I can sign up as a helper there, and give you a hand with package maintenance, if that's OK with you, I'm a maintainer for a bunch of Python packages anyway, plus we use nctoolkit in BGCVal2 (with @ledm, am sure you know Lee, since both you guys are from PML 😁 ) 👍

@robertjwilson
Copy link
Member

I could definitely do with help on the conda feedstock, as I barely understand how to maintain it @valeriupredoi.

Oddly the new release of ncplot was build by conda forge earlier. But it's not trying to build for Python 3.12. It must be the case that a dependency of it isn't available for 3.12. I changed the ncplot feedstock so that it's noarch, but that maybe doesn't affect things until I create a new release. I guess there's some way to force it to build, but that's beyond me. ;) https://github.com/conda-forge/ncplot-feedstock/blob/main/recipe/meta.yaml

@valeriupredoi
Copy link
Contributor Author

@robertjwilson the migration for new Pythons is done automatically, have a look at this doc https://conda-forge.org/docs/maintainer/updating_pkgs.html under "Updating for newly released Python version" - you can check the status of your packages at https://conda-forge.org/status/#big_migrations - if you look at the Python312 migration, specifically the not solvable ones, I don't see nctoolkit as an Immediate Child, so I believe all's good to have it migrated to 312, when that'll happen, I am not sure - it takes a bit of a while, I can ask a friend who's involved with conda-forge much more than I am; for the noarch packages that's easy, one can just plug a new build, and as long as Python's not pinned in the conda recipe, the latest one will get picked up 🍺

@valeriupredoi
Copy link
Contributor Author

but that maybe doesn't affect things until I create a new release

Indeed, or - I think you can just pop a new build and when you ask the bot to rerender, then it'll figure out it's noarch, try that

@robertjwilson
Copy link
Member

Thansk @valeriupredoi. I forgot about that page. It actually looks like nctoolkit can't be updated to 3.12 because numba isn't available for it. I checked under awaiting-parents.

The underlying cause is that geoviews relies on numba, so you cannot install geoviews in 3.12. I believe this can be resolved temporarily by using geoviews for python < 3.12 and geoviews-core for 3.12. This will result in plots not looking quite as good in Python 3.12, but that's preferable to not having a package at all. I've just changed the ncplot feedstock, so hopefully it will be re-rendered shortly https://github.com/conda-forge/ncplot-feedstock/blob/main/recipe/meta.yaml

@valeriupredoi
Copy link
Contributor Author

@robertjwilson happy new year, mate! My apologies I left this unattended, pre-holidays mayhem at fault 😁 Numba has been a PITA for us for a while - have you had a chance to see if it finally got updated to allow 3.12 support? 🍺

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants