Skip to content

Drop Python 3.8 for next release - #14286

Merged
pllim merged 7 commits into
astropy:mainfrom
nstarman:drop-py38
Apr 18, 2023
Merged

Drop Python 3.8 for next release#14286
pllim merged 7 commits into
astropy:mainfrom
nstarman:drop-py38

Conversation

@nstarman

@nstarman nstarman commented Jan 18, 2023 •
edited by pllim
Loading

Copy link
Copy Markdown
Member

Fixes #14284

After merge

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see "When to rebase and squash commits".
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the Extra CI label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the no-changelog-entry-needed label. If this is a manual backport, use the skip-changelog-checks label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • Is a milestone set? Milestone must be set but astropy-bot check might be missing; do not let the green checkmark fool you.
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate backport-X.Y.x label(s) before merge.

@nstarman nstarman added this to the v5.3 milestone Jan 18, 2023
@nstarman nstarman changed the title Drop py38 Drop Python 3.8 for next release Jan 18, 2023
@github-actions

github-actions Bot commented Jan 18, 2023

Copy link
Copy Markdown
Contributor

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

@nstarman nstarman mentioned this pull request Jan 18, 2023
Merged
13 tasks done
@nstarman

nstarman commented Jan 18, 2023

Copy link
Copy Markdown
Member Author

Hm. Since I bumped all the python version in all the tests, GitHubs (Required) branch protections need to be updated.

mhvk
mhvk reviewed Jan 18, 2023
View reviewed changes

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Have only glanced over this, but it looks good - nice to get rid of those typing work-arounds!
(Aside, I mentioned numpy 1.20 in #14284, but good to do that in a separate PR.)

@nstarman

nstarman commented Apr 13, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

Trying to resolve #14286 (comment). The Cron don't seem to install tzdata ? https://github.com/astropy/astropy/actions/runs/4685565473/jobs/8302783584?pr=14286#step:3:2296

@pllim

pllim commented Apr 14, 2023

Copy link
Copy Markdown
Member

We should really wrap this up and merge early next week if we still want to drop Python 3.8 for v5.3. It won't do to merge this one last-last minute. If a change broke CI and is unrelated to dropping Python 3.8, open a follow-up issue for it and revert that change. Thanks!

@nstarman

nstarman commented Apr 15, 2023

Copy link
Copy Markdown
Member Author

Done?! 🤞

@pllim

pllim commented Apr 15, 2023

Copy link
Copy Markdown
Member

Still not sure about the mpl bump, so I'll re-review in a couple of days. Thanks for your patience!

eerovaher
eerovaher requested changes Apr 17, 2023
View reviewed changes

@eerovaher eerovaher left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree that the doctest involving timezones doesn't have to be sorted out in this pull request, but it looks like something's gone wrong during merge conflict resolution and the changes to astropy/coordinates/tests should be undone.

Comment thread astropy/coordinates/tests/test_velocity_corrs.py

@eerovaher eerovaher Apr 17, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This pull request shouldn't touch this file at all.

Comment thread docs/coordinates/index.rst
Comment on lines +430 to 434
.. doctest-remote-data::

>>> from zoneinfo import ZoneInfo # requires Python 3.9 or greater
>>> import datetime
>>> from zoneinfo import ZoneInfo
>>> tz = ZoneInfo(tz_name)

@eerovaher eerovaher Apr 17, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd prefer this example to be included in the non-remote tests, but with the feature freeze approaching it is indeed best to leave that for a followup. Having it included in the remote tests is an improvement already.

@nstarman
nstarman force-pushed the drop-py38 branch 2 times, most recently from 1f0ee10 to c18adc1 Compare April 17, 2023 22:20
@nstarman
nstarman requested a review from eerovaher April 17, 2023 22:21
pllim
pllim reviewed Apr 18, 2023
View reviewed changes
Comment thread setup.cfg Outdated
typing_extensions>=3.10.0.1
fsspec[http]>=2022.8.2
s3fs>=2022.8.2
timezonefinder

@pllim pllim Apr 18, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Where did this come from?

@pllim pllim Apr 18, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, so this is the cost of enabling that coordinates test above. Hmm... I guess it is ok for now. But if it causes too much maintenance headache in the future, we can revisit this.

@pllim pllim Apr 18, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is only used for that one doctest, so I propose we remove from all=.

@nstarman nstarman Apr 18, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Will do!

@pllim pllim Apr 18, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

BTW, in case you missed the Slack msg, I will fold this into my edit and I plan to push to this branch directly, so pls don't touch it or you might lose your changes. FYI. Thanks!

nstarman and others added 6 commits April 18, 2023 13:27
@nstarman @pllim
drop py3.9 and bump py versions
480bb19 
Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>
@nstarman @pllim
update docs tests
a5c1e17 
Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>
@nstarman @pllim
Apply pyupgrade and py3.9 updates
a1bc8b3 
Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>
@nstarman @pllim
changelog
424d9cb 
linkcheck fixes

Co-authored-by: P. L. Lim <[email protected]>

Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>
@nstarman @pllim
bump min matplotlib to 3.3
3b8b44b 
Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>
@pllim
Fix matplotlib minversion bump
fba070c 
and some minor clean-ups.

Create hash file for new mpl334
and delete old ones (mpl311 and mpl322).
@pllim
pllim dismissed their stale review April 18, 2023 17:46

I am coauthor now.

@pllim

pllim commented Apr 18, 2023

Copy link
Copy Markdown
Member

linkcheck failed at https://ieeexplore.ieee.org/document/5254933 but it is not a broken link, so maybe the server temporary blacklisted our CI. We can ignore the failure for now.

A bunch of the "required" tests that won't run will block normal merge, so I will use my admin power to merge when CI passes. I need to update the branch protection rules with new CI names but only after merging this. 🐔 🥚

Thanks everyone, especially to @nstarman for your patience!

@pllim
pllim merged commit 01c96c8 into astropy:main Apr 18, 2023
@pllim

pllim commented Apr 18, 2023

Copy link
Copy Markdown
Member

Alas, the protection rules will change and impact all the other PRs wanting to make it to feature freeze. I will update the rules and send out a memo to astropy-dev.

@pllim pllim mentioned this pull request Apr 18, 2023
Open
@nstarman
nstarman deleted the drop-py38 branch April 18, 2023 21:24
@eerovaher eerovaher mentioned this pull request May 4, 2023
Merged
This was referenced Jun 1, 2023
Merged
Closed
@eerovaher eerovaher mentioned this pull request Oct 24, 2023
Merged
1 task done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pllim pllim pllim left review comments
@mhvk mhvk mhvk left review comments
@WilliamJamieson WilliamJamieson WilliamJamieson approved these changes
@eerovaher eerovaher eerovaher approved these changes
@astrofrog astrofrog Awaiting requested review from astrofrog astrofrog is a code owner
@Cadair Cadair Awaiting requested review from Cadair
@larrybradley larrybradley Awaiting requested review from larrybradley larrybradley is a code owner

Assignees

No one assigned

Labels

cosmology dev-automation Extra CI Run cron CI as part of PR installation modeling testing units zzz 💤 Python3.9 archived: Python 3.9 is no longer supported

Projects

Astropy Infrastructure
Status: Done

Milestone

v5.3

Development

Successfully merging this pull request may close these issues.

Drop py3.8 support tracker

5 participants

@nstarman @pllim @mhvk @eerovaher @WilliamJamieson