Add downstream-pyvo and downstream-astroquery tox jobs - #20604

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.

xref astropy/astropy-integration-testing#25

Eventually we will also need to update the developer doc to go with this, but we can hold off on that for now until we figure out the gritty details. Thanks!

Doesn't this duplicate the integration dashboard?

Does the integration dashboard allow testing from unmerged astropy PR branch on demand?

No but I guess I'm worried about a slippery slope here - reproject for instance integrates tightly with astropy.wcs etc so we could end up testing a lot here. Actually the integration dashboard can be called via a reusable workflow so maybe we could call it here? I'll have a think!

Anyway I guess it would be good to have a way to share infrastructure between the integration dashboard and this.

Maybe it can look like this but over at integration-testing. A maintainer would have to run it and manually. So basically it is a workflow_dispatch with user inputs.

So far this is a proof-of-concept. Developing locally, I wanted a one-liner to run downstream dependency unit tests. And having it as something that can be called via CI with say a label (maybe needs-downstream-testing) or something similar on a PR would be useful. In the same way we currently do with some category of tests for certain labels.

asdf has a downstream set of jobs defined and run via nox on over a dozen downstream dependencies, and it makes finding regressions really easy, especially if you are spelunking around in the bowels of a library.

Clearly this would need to be opt-in if available for CI.

Developing locally, I wanted a one-liner to run downstream dependency unit tests:

https://github.com/astropy/astropy-integration-testing

gives a CLI such as:

integration-dashboard run --variant stable --python 3.12

Maybe we could extend it to be able to test PRs?

Philosophically I also worry about creating an undesirable precedent here. If we start upstreaming integration testing, where does it end ? and what would remain the responsibility of downstream testing ? The integration testing dashboard is precisely designed to fit that space so it would make much more sense to develop a solution there.

So I don't think this is a slippery slope. The goal here isn't to replace integration testing. It's just a proposal for something that is useful here, now, for testing PRs. It's just one tool.

I think this is quite a different use case than integration testing for releases, which we already have, or for backports as #17861 was concerned with. Though clearly the tox infrastructure does not need to be unique. I did read the comments on #17861. The differences here in this draft PR:

• astropy-integration-testing tests both pyvo and astroquery, weekly, against astropy stable, pre-release and dev builds. But it puts every package in one shared environment with pytest --pyargs, as TST: Add downstream testing #17861 did. And it doesn't appear to be able to test an unmerged astropy PR, which is the whole point of this job. Catching the pyvo jobInfo break in Reduce memory needed to read VOTable BINARY and BINARY2 streams #20509 is the use case for this tox job.
• Because each downstream package gets its own tox job here, if one fails, it does not affect the others. The generated downstream-pyvo and downstream-astroquery env names ensure this. And running the tox jobs that are defined already in those repos makes sure this job is a 1-to-1 match to what the downstream repo CI will see once a PR in astropy is merged.
• This job runs CI on downstream main. For a PR CI, that is the check that you want, since a fix affects downstream main first. That doesn't preclude or get rid of the need for integration testing at release time. It just means you might catch regressions sooner and have less to do at release time.
• With it being opt-in via dispatch or via the needs-downstream-testing label it could be made unobtrusive. Also, it is handy to have locally as a one-liner, regardless if a github actions CI job is defined for it.

So I've added a small CI workflow to this as well, that runs each of the 2 downstream-pyvo and downstream-astroquery jobs as separate jobs in github actions. Independent run, independent possible failures. It runs them under python 3.14, which is set by the CI workflow itself, so the tox job is now python version agnostic.

So the CI workflow can run on a PR with the needs-downstream-testing label, or from the "Run workflow" button.

Here it is running on my fork

https://github.com/jdavies-st/astropy/actions/runs/38701185565

Just to clarify, I think it would be useful to have a way to run downstream tests here - what I am concerned about is duplicating the infrastructure and config to do so. Another concern is that it's not unusual for the main branch of coordinated packages to be failing for other reasons, so what we really need to see here is the before/after of the tests based on this PR. I'm pretty sure we could adjust the integration dashboard so that we could easily just run it here on-demand and make a before/after dashboard for a set of downstream packages (and I can adjust the integration dashboard to support testing packages individually rather than all together). So basically I'm conceptually 👍 on having downstream testing, but I'd like to take a bit of time today or tomorrow to investigate if we can integrate it with the integration dashboard 😆

Ok so I've been prototyping this a bit, and I think what would be valuable is a way to compare the results for tests on main vs the PR to know what new tests are failing or are fixed downstream, and ignore pre-existing failures. The prototype I have would run (when opted-in) the tests for all coordinated packages based on the config/list in the integration testing repo, and would post a comment with results in the PR:

and would also have one PR status for each coordinated package so that one could go and look in more detail at failures. Green would not mean all tests passed but rather no new tests failed, which I think will be more useful to us here. We could then also add a tox env as done in this PR but which runs the same testing as run in CI locally. If this sounds good, I could open PRs to the integration repo and also here to try this out for real?

That looks really cool @astrofrog ! The before/after is the right approach. I have hit two bugs already existing in astropy and numpy this week that are unrelated to the PRs at hand that were flagged in the downstream tox jobs. Both were pre-existing.

So compared to the integration testing approach, this PR:

1. Here, each downstream test is built in a separate tox env. That minimizes clashes or dependency oddities. It also means that the downstream tox env can be run directly, replacing dev astropy with the PR astropy. The cost is that you have to build astropy again and again for each downstream test. The benefit is isolation and exact reproducibility.
2. But here you can parallelize with the CI runner and you get them all completing together, and if one fails, the others keep running. So again, isolation gives you reproducibility. So while it wastes electrons, it works. The CI workflow is pretty simple, and you can see it works via the above run.

@astrofrog it looks like you can do that on your end too, so I am on board. 👍

One thing I'll note is that running pytest --pyargs pyvo as is the pattern in the integration testing would not have caught the regression that the tox job here caught. For pyvo, the docs are not installed, so doctests don't run. And the test that failed was a public-facing documentation example, showing users how to use job.jobinfo which my astropy bugfix broke downstream. So while testing can't be perfect, there is some value in running the defined tox jobs in the downstream repos as they are.

I also notice that pyvo is failing on the integration dashbaord. The cause is the same reason - astropy runs the tests in a different way from pyvo. 😅 (Gory detail interlude - it has a top-level conftest.py that sets some variables, and testing it with --pyargs doesn't install that file so you get failure. Classic non-src layout problem.)

So perhaps you could add a per-package option to test the way the package's own CI does: from a clone of its repo, using its own test command, as here with astropy replaced with PR astropy. That's how pyvo's doctest break would be caught.

Also pyvo is an affiliated package, not a coordinated package. So if we only added the per-PR run capability to coordinated packages, it would miss pyvo, my use case here. Can we add affiliated as well? And make them selectable in which run, so as not to waste electrons?

Anyway, I can close this PR, or shrink it down just to the tox wrapper for local runs if that still has some value.

Gory detail interlude - it has a top-level conftest.py that sets some variables, and testing it with --pyargs doesn't install that file so you get failure. Classic non-src layout problem.

If you're brave enough, you have my support to re-open the gates of Hell this discussion at #13678
disclaimer: to me, the blocking factor seems to be that some maintainers do not see the problems caused by a flat layout and are tired of discussing it. In my personal opinion this line of reasoning doesn't constitute an effective argument against src layout, but the status quo has inertia for itself...

Nooo! 😅 I don't have much to add to those discussions except for opinion and lots of time fixing test/build systems, including half my commit history in this repo. Only thing I will say is that once you've decided on src/ layout you have then decided not to bundle your tests or test data in the wheel, and you definitely don't have tests in subdirs in your production code. All benefits from my view, but given the history of this repo, controversial.

Two things not mentioned there:

• it is a 1-liner mostly git mv ... with some conftest.py and pyproject.toml cleanup (famous last words)
• and .git-blame-ignore-revs can mask the blame for whatever commit(s) did the deed, so complete git traceability as if it never happened. I see black-ifying and clang-format-ing the repo is already blame ignored.

In order for a src/-layout to work for the test scenarios discussed here, all the under-test packages would have to switch to src/, which is probably less likely than this repo. 😅

Hence my suggestion to just run the tox envs already in those repos. It's the minimum churn, maximum "catch all the bugs" workflow.