TST: Clean up minversion checks by pllim · Pull Request #16820 · astropy/astropy

pllim · 2024-08-13T12:38:48Z

Description

This pull request is to wrap up #16023 but given all the conflicts over there, I don't feel like clobbering Mridul's work so I open a new PR with my vision of how to resolve it based on past conversations.

Looks like a lot of the minversion was bumped somewhere else. They more or less match SPEC 0 anyway now, except IPython but Eero was hesistant to bump IPython just for the sake of bumping it and Astropy is not Scientific Python "core package" so we are not obligated to follow SPEC 0 to the letter, so I let that one go.
In checking our minversion settings, I did find some dead code, outdated pins, and wrong pins, so I fixed those here.

By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

github-actions · 2024-08-13T12:39:15Z

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

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 instructions for rebase and squash.
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?
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.

github-actions · 2024-08-13T12:39:18Z

👋 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?

pllim · 2024-08-13T12:52:35Z

Just making sure this is not going to break all the optional jobs... 😬

pllim · 2024-08-13T13:32:56Z

Benchmark and linkcheck failures are unrelated. They ran, so that's what matters.

larrybradley

LGTM for visualization.

eerovaher

This is another example of how maintaining separate lists of oldest supported dependencies both in pyproject.toml and in tox.ini is highly error-prone and causes our tests meant to use the oldest versions to often use latest versions instead. Updating tox.ini manually is better than doing nothing, but ideally we should come up with a better solution that would not require such manual updates. The uv --resolution=lowest strategy looks like just what we need for the oldest dependencies tests.

eerovaher · 2024-08-13T20:07:51Z

astropy/units/quantity_helper/scipy_special.py

    for name in dimensionless_to_dimensionless_sps_ufuncs:
-        # In SCIPY_LT_1_5, erfinv and erfcinv are not ufuncs.
        ufunc = getattr(sps, name, None)
        if isinstance(ufunc, np.ufunc):


@mhvk, do you know if we still need the isinstance(ufunc, np.ufunc) check? And related to that, is the following still needed?

astropy/astropy/units/tests/test_quantity_ufuncs.py

Lines 1527 to 1530 in e65f975

if isinstance(sps.erfinv, np.ufunc):

assert sps.erfinv in qh.UFUNC_HELPERS

else:

assert sps.erfinv not in qh.UFUNC_HELPERS

No, we don't -- thanks for catching that. More generally, I really should not have introduced those kinds of checks... Much better just to use version numbers as those can be found much more easily.

I pushed another commit with what I think you wanted. Please re-review. Thanks!

eerovaher · 2024-08-13T20:10:34Z

tox.ini

    # The oldestdeps factor is intended to be used to install the oldest versions of all
    # dependencies that have a minimum version.
-    oldestdeps: pyerfa==2.0.*
+    oldestdeps: pyerfa==2.0.1.1


Why is the oldest version specified in such detail for erfa, but not for the other dependencies?

pyerfa doesn't really follow semver.

eerovaher · 2024-08-13T20:15:36Z

tox.ini

+    oldestdeps: pyerfa==2.0.1.1
    oldestdeps: numpy==1.23.*
-    oldestdeps: matplotlib==3.3.*
+    oldestdeps: matplotlib==3.6.*


I just checked the logs and saw that our oldest dependencies tests have indeed been using the latest matplotlib, but this change fixes it.

pllim · 2024-08-13T20:35:24Z

This is another example of how maintaining separate lists of oldest supported dependencies both in pyproject.toml and in tox.ini is highly error-prone

I do not disagree but refactoring that infrastructure is out of scope here.

mhvk

This looks good, thanks @pllim! Would be great to also make the fixes suggested by @eerovaher in units, but also fine to do that after.

mhvk · 2024-08-14T00:32:23Z

astropy/visualization/wcsaxes/utils.py

 ]

-MATPLOTLIB_LT_3_8 = not minversion(mpl, "3.8.dev")
+MATPLOTLIB_LT_3_8 = not minversion(mpl, "3.8")


Fine not to do it in this PR, but I wonder if it would be better to centralize these kinds of limits - that way one can grep and replace whenever a minimum version is increased.

Kinda tricky for optional dependencies because if we put it too high up, we risk import error on minimal dependencies.

mhvk

Looks good!

nden

Modeling looks good.

pllim · 2024-08-14T12:29:38Z

Thanks for the reviews. Failures still unrelated. Merging!

mhvk · 2024-08-17T21:22:36Z

astropy/utils/masked/tests/test_masked.py

 from astropy.utils.compat import NUMPY_LT_2_0
 from astropy.utils.compat.optional_deps import HAS_PLT
 from astropy.utils.masked import Masked, MaskedNDArray
+from astropy.visualization.wcsaxes.utils import MATPLOTLIB_LT_3_8


Oops, this caused a problem: now the astropy.utils.masked tests get skipped if one does not have matplotlib installed... I'll make a fix...

fix in #16843

Ugh. Sorry for the oversight. I merged your fix. Thanks!

pllim added no-changelog-entry-needed Extra CI Run cron CI as part of PR Build all wheels Run all the wheel builds rather than just a selection labels Aug 13, 2024

pllim added this to the v7.0.0 milestone Aug 13, 2024

github-actions bot added testing units modeling utils visualization.wcsaxes utils.masked labels Aug 13, 2024

pllim mentioned this pull request Aug 13, 2024

MAINT: bump min versions to match SPEC 0 #16023

Closed

pllim added the benchmark Run benchmarks for a PR label Aug 13, 2024

pllim marked this pull request as ready for review August 13, 2024 14:36

pllim requested review from a team, astrofrog and larrybradley as code owners August 13, 2024 14:36

pllim requested review from MridulS and eerovaher August 13, 2024 14:36

larrybradley approved these changes Aug 13, 2024

View reviewed changes

eerovaher reviewed Aug 13, 2024

View reviewed changes

mhvk approved these changes Aug 14, 2024

View reviewed changes

pllim added 2 commits August 13, 2024 20:41

TST: Clean up minversion checks

2daa6c3

Address comments from eerovaher and mhvk

a68c02d

pllim force-pushed the spec0-deps branch from da7e970 to a68c02d Compare August 14, 2024 00:47

mhvk approved these changes Aug 14, 2024

View reviewed changes

nden approved these changes Aug 14, 2024

View reviewed changes

pllim merged commit 7e0c200 into astropy:main Aug 14, 2024

pllim deleted the spec0-deps branch August 14, 2024 12:29

mhvk reviewed Aug 17, 2024

View reviewed changes

mhvk mentioned this pull request Aug 17, 2024

TEST: ensure Masked tests run also without matplotlib installed #16843

Merged

pllim mentioned this pull request Aug 19, 2024

BUG: Fix test_plt_scatter_masked matplotlib check that broke pyinstaller job #16854

Merged

1 task

	if isinstance(sps.erfinv, np.ufunc):
	assert sps.erfinv in qh.UFUNC_HELPERS
	else:
	assert sps.erfinv not in qh.UFUNC_HELPERS

Uh oh!

Conversation

pllim commented Aug 13, 2024

Description

Uh oh!

github-actions bot commented Aug 13, 2024

Uh oh!

github-actions bot commented Aug 13, 2024

Uh oh!

pllim commented Aug 13, 2024

Uh oh!

pllim commented Aug 13, 2024

Uh oh!

larrybradley left a comment

Choose a reason for hiding this comment

Uh oh!

eerovaher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim commented Aug 13, 2024

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

nden left a comment

Choose a reason for hiding this comment

Uh oh!

pllim commented Aug 14, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants