TST: Re-enable test_all for devdeps but only when noscipy is not specified (try 3) by pllim · Pull Request #17900 · astropy/astropy

pllim · 2025-03-17T19:16:43Z

Description

This pull request is an alternative to #17812 and also to #17837 to fix #15242 . This works around tox-dev/tox#3433 .

Also removes unused recdeps.

xref skyfielders/python-skyfield#898 (comment)

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 · 2025-03-17T19:17:11Z

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.

pllim · 2025-03-17T19:26:34Z

I don't think oldestdeps failure is related. It does ask the question on whether we have to revert #17057 .

Linkcheck failures are also unrelated.

neutrinoceros

The oldestdeps run has numpy 2.2.4, so clearly env resolution is taking a wrong turn at some point, but I don't understand where yet.

neutrinoceros · 2025-03-18T14:37:32Z

I initially suspected that some additional indirect dependency was forcing numpy to be upgraded on oldestdeps, but it doesn't appear to be the case: here's a valid solution to requiring exactly all 4 packages you added and numpy==1.23.2 as a dependency tree (using uv tree --resolution lowest-direct)

test-deps v0.1.0
├── array-api-strict v1.0
│   └── numpy v1.23.2
├── numpy v1.23.2
├── objgraph v1.6.0
├── sgp4 v2.3
└── skyfield v1.20
    ├── certifi v2025.1.31
    ├── jplephem v2.22
    │   └── numpy v1.23.2
    ├── numpy v1.23.2
    └── sgp4 v2.3

neutrinoceros · 2025-03-18T14:49:14Z

Can you try running CI again with RUST_LOG='uv=debug' ? it should help understanding what's going wrong in the env resolution

pllim · 2025-03-18T18:19:17Z

@neutrinoceros , what does this mean?

error: Invalid RUST_LOG directives
  Caused by: invalid filter directive

neutrinoceros · 2025-03-19T16:54:44Z

I don't know. I assume that it's either a different rust program crashing before uv installs gets a chance, or tox being weird in how it reads setenv. Can you try RUST_LOG = 'uv=debug' # debug or RUST_LOG='uv=debug' ? (I noticed that everywhere else wa have comments, we used spaces around the = sign)

pllim · 2025-03-19T18:27:57Z

@neutrinoceros that did not seem to help. Any other ideas?

neutrinoceros · 2025-03-20T15:18:40Z

not yet, sorry. What's very frustrating is that I can very easily enable debug outputs by running uv directly with the same command that tox appears to be running:

❯ RUST_LOG=uv=debug uv pip install --resolution lowest-direct 'PyYAML>=6.0.0' 'asdf-astropy>=0.3' 'asdf>=2.8.3' 'astropy-iers-data>=0.2025.2.24.0.34.4' 'beautifulsoup4>=4.9.3' 'bleach>=3.2.1' 'bottleneck>=1.3.3' 'certifi>=2022.6.15.1' 'coverage>=6.4.4' 'dask[array]>=2022.5.1' 'fsspec[http]>=2023.4.0' 'h5py>=3.8.0' 'html5lib>=1.1' 'ipydatagrid>=1.1.13' 'ipykernel>=6.16.0' 'ipython>=8.0.0' 'ipywidgets>=7.7.3' 'jplephem>=2.6' 'jupyter-core>=4.11.2' 'matplotlib>=3.6.0' 'mpmath>=1.2.1' 'numpy>=1.23.2' 'packaging>=22.0.0' 'pandas>=1.5.0' 'pandas>=2.0' 'pre-commit>=2.9.3' 'pyarrow>=10.0.1' 'pyerfa>=2.0.1.1' 'pytest-astropy-header>=0.2.1' 'pytest-astropy>=0.10.0' 'pytest-doctestplus>=1.0.0' 'pytest-xdist>=2.5.0' 'pytest>=8.0.0' 'pytz>=2016.10' 's3fs>=2023.4.0' 'scipy>=1.9.2' 'sortedcontainers>=1.5.7' 'threadpoolctl>=3.0.0'

But then I don't reproduce the problem, and get the expected numpy version

❯ uv pip show numpy
Name: numpy
Version: 1.23.2
Location: /Users/clm/dev/astropy-project/coordinated/astropy/.venv/lib/python3.11/site-packages
Requires:
Required-by: asdf, asdf-astropy, astropy, bottleneck, bqplot, contourpy, h5py, matplotlib, pandas, pyarrow, pyerfa, pytest-arraydiff, scipy

I'll add this to my list of situations I've seen tox creating more problems than solutions...

pllim · 2025-03-20T20:02:52Z

Very frustrating indeed. Your PR did not seem to help in this case. Maybe for now, I should change alldeps directive in tox.ini to devdeps only. That would satisfy my immediate goal without having to deal with tox/uv weirdness.

pllim · 2025-03-20T20:03:56Z

But maybe I should also remove test_all from tox.ini because it is clearly broken and will introduce surprising behavior to oldest-deps when fixed.

pllim · 2025-03-20T21:20:39Z

    test: test
-    recdeps: recommended
-    recdeps: test_all
+    recdeps: all


I don't know what recdeps is for. It tries to install test_all anyway, so might as well be alldeps because test_all already includes recommended. But maybe somewhere downstream is using it, so I'll keep it for now.

Wait, are you saying that our tox envs are considered public API somehow ? This feels daunting.

Not public per se, but it is not in scope in this PR to break a possible usage somewhere.

I'm puzzled. How would it be used anywhere else ?

Looks like @mhvk added it back in #10464 and I really don't remember why I added test_all to it in #13651

We don't use it in docs either. So I think it is okay to remove but let's do that in a different PR.

Actually I change my mind. It is cleaner to remove it here, rather than changing the specs. Thanks for bringing this up!

My logic was that since we have a set of recommended dependencies, we probably wanted some tests to just run with those. And at least at the time, I thought that sometimes I just want the "standard" stuff like scipy to be tested, but not have to wait to pull everything else in. But in practice, I certainly hadn't remembered it and so haven't used it - probably enough reason to remove it... If kept, though, just use recdeps: recommended.

I should have made a note on why I added test_all to it because I have zero recollection. I am neutral on keeping it but reverting back to recommended, or just removing it. I don't use tox locally.

mhvk

I guess my mild preference would be to use this PR to restore recdeps to how it was defined originally (i.e., just remove recdeps: test_all, which brings in everything). But I'm also fine with just removing it.

but only when noscipy is not specified. We have to copy over some stuff from test_all in pyproject.toml to tox.ini because tox unable to grab them automatically when there are multiple extras. Because of the tox bug, test_all does nothing for recdeps and alldeps in extras in tox.ini so we remove the misleading directive. In fact, if you install the rest of the stuff in test_all into alldeps via the deps directive, oldestdeps start to fail because it silently upgrades to latest numpy instead of using the minversion of numpy.

pllim · 2025-03-21T18:38:26Z

@mhvk , ok, I restored recdeps.

pllim · 2025-03-21T18:53:07Z

Linkcheck failure unrelated and I cancelled the rest of the weekly cron jobs because they are irrelevant here.

pllim · 2025-03-21T19:12:30Z

Other 2 failures are all remote data, also unrelated.

mhvk · 2025-03-21T21:00:01Z

OK, thanks! Let's just get this in!

…ly when noscipy is not specified (try 3)

…900-on-v7.0.x Backport PR #17900 on branch v7.0.x (TST: Re-enable test_all for devdeps but only when noscipy is not specified (try 3))

pllim added no-changelog-entry-needed Extra CI Run cron CI as part of PR backport-v7.0.x labels Mar 17, 2025

pllim added this to the v7.0.2 milestone Mar 17, 2025

github-actions bot added the testing label Mar 17, 2025

This comment was marked as resolved.

Sign in to view

pllim marked this pull request as ready for review March 17, 2025 19:35

pllim requested a review from neutrinoceros March 17, 2025 19:36

neutrinoceros mentioned this pull request Mar 18, 2025

DEP: add missing lower bound to array-api-strict test requirement #17903

Merged

1 task

neutrinoceros reviewed Mar 18, 2025

View reviewed changes

Comment thread tox.ini Outdated

pllim force-pushed the devdeps-test-all-try3 branch 2 times, most recently from b60ca48 to fcce310 Compare March 18, 2025 17:13

pllim marked this pull request as draft March 18, 2025 17:47

pllim force-pushed the devdeps-test-all-try3 branch from 540f54e to 5bcd976 Compare March 20, 2025 21:19

pllim commented Mar 20, 2025

View reviewed changes

pllim force-pushed the devdeps-test-all-try3 branch from 5bcd976 to 3c40d45 Compare March 21, 2025 17:30

pllim marked this pull request as ready for review March 21, 2025 17:31

pllim requested a review from mhvk March 21, 2025 17:31

mhvk approved these changes Mar 21, 2025

View reviewed changes

TST: recdeps should only install recommended

c0ca8ae

pllim force-pushed the devdeps-test-all-try3 branch from 3c40d45 to c0ca8ae Compare March 21, 2025 18:37

mhvk merged commit dc9760b into astropy:main Mar 21, 2025
27 of 34 checks passed

meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request Mar 21, 2025

Backport PR astropy#17900: TST: Re-enable test_all for devdeps but on…

804605c

…ly when noscipy is not specified (try 3)

meeseeksmachine mentioned this pull request Mar 21, 2025

Backport PR #17900 on branch v7.0.x (TST: Re-enable test_all for devdeps but only when noscipy is not specified (try 3)) #17923

Merged

pllim deleted the devdeps-test-all-try3 branch March 21, 2025 21:36

pllim added a commit that referenced this pull request Mar 21, 2025

Merge pull request #17923 from meeseeksmachine/auto-backport-of-pr-17…

c62fded

…900-on-v7.0.x Backport PR #17900 on branch v7.0.x (TST: Re-enable test_all for devdeps but only when noscipy is not specified (try 3))

This was referenced Feb 25, 2026

TST: require tox>=4.37 and clean up moot workaround #19325

Closed

CLN: clean up remnants of unused and partially undefined tox env (recdeps) #19326

Merged

TST/DEP: configure devdeps-!noscipy to depend on test_all again #19336

Open

Uh oh!

Conversation

pllim commented Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

github-actions bot commented Mar 17, 2025

Uh oh!

This comment was marked as resolved.

pllim commented Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neutrinoceros left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

neutrinoceros commented Mar 18, 2025

Uh oh!

neutrinoceros commented Mar 18, 2025

Uh oh!

pllim commented Mar 18, 2025

Uh oh!

neutrinoceros commented Mar 19, 2025

Uh oh!

pllim commented Mar 19, 2025

Uh oh!

neutrinoceros commented Mar 20, 2025

Uh oh!

pllim commented Mar 20, 2025

Uh oh!

pllim commented Mar 20, 2025

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!

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!

pllim commented Mar 21, 2025

Uh oh!

pllim commented Mar 21, 2025

Uh oh!

pllim commented Mar 21, 2025

Uh oh!

mhvk commented Mar 21, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pllim commented Mar 17, 2025 •

edited

Loading

pllim commented Mar 17, 2025 •

edited

Loading

neutrinoceros left a comment •

edited

Loading