Do not include tests in Astropy wheels (reduce wheel size for AWS Lambda)

[lpsinger]

What is the problem this feature will solve?

Astropy is a very heavyweight dependency and is difficult to use in a serverless, space-constrained environment like AWS Lambda. When installed from wheels for Linux aarch64, the astropy directory in site-packages takes up 58 MB. Of that, the astropy/**/tests directories take up 20 MB.

Describe the desired outcome

Move the test directories out of the installed Python package, or exclude them from packaging.

Additional context

No response

[neutrinoceros]

It would be useful to compare these numbers against the size of astropy's dependencies in the same context.

[lpsinger]

By far the largest dependency of Astropy is Numpy, which on Linux aarch64 takes up 63 MB. Of that, 15 MB is tests and test data.

[pllim]

Packaging for AWS is painful. When I had to do this once for a temporary project, I had to strip docs, tests, and some unused sub-packages out to get it to stay in budget.

But by doing this for AWS, the cost is that user will no longer be able to test their installation. That cost is quite high.

Maybe we can discuss this at the Coordination Meeting? 🤔

Or at least bring it up at the next dev telecon? 🤔

[lpsinger]

Sure... I have been out of the loop of dev telecons. When are they?

[hamogu]

Next dev-telecon is this Thursday: https://docs.google.com/document/d/15JSFh3OMF9Iz6ov3q_xxGO_BL8hRnuse4IMUrqEIvcg/edit#heading=h.ntv4vdb5izj7

[lpsinger]

I can't make it to this Thursday, but I can try for the next one.

[astrofrog]

user will no longer be able to test their installation

I'd be curious how often this is used. At the start of the project, before we had any proper or limited CI, it was one of the main ways we found out about platform-specific issues, but with such extensive CI coverage now, do any users installing wheels for example ever run astropy.test()?

[WilliamJamieson]

If users really do want the ability to test their installation we could look into packaging the tests into a separate package, say astropy-tests on PyPi. Then let users know that if they want to test their installation from a PyPi wheel that they just need to install that package in addition to astropy. This being said we should consider if testing an installation like this is even necessary before pursuing this further.

[MridulS]

+1 with @astrofrog

(Just thinking out loud here) I think many packages in the scientific python ecosystem had this in built testing due to limited CI + a need to build packages during pip install in the good ol days. With wheels today I wonder how many users would need to worry about installing a wheel and then the wheel failing the tests (we should be able to catch that quickly in our CI!). And for the user group who want to build and test I think it's fair to ask them to clone the repo/tag directly from GitHub.

[pllim]

Now it is starting to sound like y'all wanna kill the test runner altogether!

astropy/astropy/__init__.py

Line 137 in d32a06a

test = TestRunner.make_test_runner_in(__path__[0])

Perhaps it is time (see #16165) but this is a long standing public API. Additionally we also encouraged downstream packages to ship their own package.test() for a while, so there are some active usage downstream. So, this really needs to be thought out and executed carefully.

[namurphy]

My inclination would be to include tests, etc. in the source distribution (sdist) but not include tests in wheels. I looked into this a little bit and I think we could do this by including tests in MANIFEST.in (to get them in the sdist) and excluding tests in setup.py (so they're not in the wheel). Something like...

from setuptools import setup, find_packages

setup(..., packages=find_packages(exclude=["tests"]))

[pllim]

That or revive the controversial discussion to move tests outside of astropy package dir (can't seem to find that issue now but we definitely debated on it a long time ago). 😬

[namurphy]

Good point, that would also work! And that seems to be the packaging trend these years, with src layouts too. 🙃

Turns out there was a lot of discussion about dropping tests from wheels in pandas in the last few years, again motivated by AWS: pandas-dev/pandas#30741

[pllim]

Ah, right, the src layout. I found the issue opened by @nstarman here:

• Move astropy to src directory #13678

Well, if pandas cannot get it done in the past 3 years, what chance do we have? 😆

[nstarman]

Good point, that would also work! And that seems to be the packaging trend these years, with src layouts too. 🙃

That's what I've been doing for a while and it's great. The only inconvenience is you can't run the tests from the REPL, but I haven't actually done that even with Astropy for years, so it was no great loss.

Well, if pandas cannot get it done in the past 3 years, what chance do we have? 😆

I think we can do it!

[eteq]

As I recall, the primary motivation for having the tests in the wheels/installed version is that a user who wants to do release candidate testing is often not going to bother grabbing the source, but they will run the tests if its packaged with the code. This certainly happens in release candidates, whenever anyone tests it locally. I don't see that that has changed. So it's now a question if that's more important than the cloud packaging issues (which I relate to, as I've had exactly the same lambda pain @lpsinger pointed out!)

That said, I think there's a couple of alternatives to consider:

1. Have non-release builds (including RCs, which might be risky...) include the tests, but just not release builds
2. Build a second wheel/etc without the tests
3. Have a docker container (which is what nearly all of these cloud tools use) that explicitly strips the test in the build process.

I think 3 is the easiest and cleanest, personally, if our motivation is basically "for the cloud" since cloud is container-dominated anyway.

[pllim]

@saimn pointed out there is related discussions about wheel size at https://astropy.slack.com/archives/C7D5BPFJR/p1680076057936899

[eteq]

Another idea that came up in the dev telecon today: do whatever @olebole does for debian testing as the recommended way of testing which then makes it less worrisome to remove (not pain-free but not as painful). We weren't sure what that was though, so maybe @olebole can offer more insight?

[pllim]

Ole replied here: #16165 (comment) (sorry I didn't get a chance to cross-post before the dev telecon)

In Debian, we use pytest only (specifically python3 -m pytest --astropy-header -m "not hypothesis" --pyargs astropy for the CI test on the installed package), so astropy.test() can be removed IMO.

[lpsinger]

Putting on my Debian hat (which I imagine is a liberty cap), @olebole may have been referring to autopkgtest. Autopkgtests are run against installed packages to check that they have not regressed (e.g. due to changes in other packages). It is certainly very simple to write autopkgtests when the tests are actually installed.

However, "The cwd of each test is guaranteed to be the root of the source package, which will have been unpacked but not built." (See autopkgtest spec.) So including the tests in the source distribution (or even just in the git repository, as many Debian source packages these days are derived from upstream's git repo) but not the installation would also be fine for autopkgtest.

[olebole]

In Debian, we have two types of tests: First during the build (which is often a bit fiddly because the test runs on the built, but not installed, package). And we have "CI tests", which run on the installed package. The command for Astropy is

python3 -m pytest --astropy-header -m "not hypothesis" --pyargs astropy

For the installed package however, we still have the sources so that we can access test data etc. if required.
So, if the package doesn't have the tests installed, we can still run them. Usually then the tests are copied to a temporary directiry (to not interfere with the source package) and then run. It is however easier (and IMO more robust) if the tests are included in the package.

[namurphy]

This is reminding me of pytest-picked, which runs "the tests related to the unstaged files or the current branch (according to Git)".

[pllim]

Update: Killing the test runner is now its own separate issue.

• Deprecate astropy test runner in core and downstream #16177

[jdavies-st]

But by doing this for AWS, the cost is that user will no longer be able to test their installation. That cost is quite high.

But nobody actually does this. If people do pip install astropy and don't get an error, they use the installation. This is literally not a use case for any end users.

The only people who run tests are developers, who already have the git repo with the tests and test data, and maybe a first time user who is reading through the docs of installing astropy for the first time, and uses the test runner after pip installing because the docs tell them to do so. How about we stop telling people to do that, and like every other well-tested package out there, just ship it and rely on CI to make sure it works on supported platforms. Users should not run the test suite as a matter of course unless there's a serious problem that needs debugging, and at that point they file an issue or have the git repo to poke around in themselves.

And of course we should also move the tests outside of a src/astropy layout, as that would accomplish the goal of this issue and make packaging much easier, i.e. doing it the way python has recommended new projects do packaging for the past 6+ years.

And deprecate the test runner.

🔥

[pllim]

But nobody actually does this

Strong argument there. And concise. I have no objection, your honor. 😆

[pllim]

So, to recap what I think came out of the dev telecon on 2024-03-07, there are several options:

1. Minimal impact: Provide AWS people a helper script to manually strip out the tests from wheels, or "a Docker image" (ask @eteq what that means exactly). No change to code base whatsoever. Tech debt stays. Fastest turnaround time to close this issue.
2. Some impact: Strip tests but only during wheel building process. Renders astropy.test() useless (so maybe we could deprecate/remove it too) but the test runner stays. Some change to code base. Tech debt mostly stays.

2.5. (See astrofrog comment below.) Less of Most Impact: We go all in with src layout (#13678) and getting rid of astropy.test() but not the test runner. Major change to code base (plus all the open PRs will be affected, doc overhaul, etc). Tech debt exorcised here but not downstream. Wheels naturally will no longer have tests. This will take weeks/months.

3. Most impact: We go all in with src layout (Move astropy to src directory #13678) and getting rid of the test runner (Deprecate astropy test runner in core and downstream #16177). Major change to code base (plus all the open PRs will be affected, doc overhaul, etc). Tech debt exorcised. Wheels naturally will no longer have tests. This will take months.

Or some combo of the above, like we give you a stop gap script from Step 1 until we can execute Step 3. Though if you already have a way to strip out tests yourselves, Step 1 is probably unnecessary.

[pllim]

@lpsinger , given you were not able to attend the dev telecon on 2024-03-07, we have encouraged people to comment on the issue(s) and we will discuss again when you are able to attend next time. Thanks for your patience!

[astrofrog]

Just a quick note on option 3 - the test runner class that downstream packages use to expose package.test() could technically be kept for a long time if we wanted to keep supporting downstream packages, long after we stop using it in astropy core itself. We don't even have to deprecate it yet.

Removing astropy.test() in astropy itself is not really a significant API change in that it is unlikely many people (besides a few developers in the project) use it as part of scripts/programs.

[pllim]

So is the following a good interpretation of your suggestion in #16152 (comment) ?

2.5. Less of Most Impact: We go all in with src layout (#13678) and getting rid of astropy.test() but not the test runner. Major change to code base (plus all the open PRs will be affected, doc overhaul, etc). Tech debt exorcised here but not downstream. Wheels naturally will no longer have tests. This will take weeks/months.

[astrofrog]

Yep!

[mhvk]

I'd vote for option (2), as I think it makes sense not to include tests, and think it is OK to remove use of astropy.test (and deprecate the TestRunner, but best done separately).

I don't see the connection with src layout and indeed do not understand what "technical debt" we are talking about, since I don't see the src layout as giving much of an advantage (if any).

I feel a bit like a broken record, but really wish we could focus more on improving astropy in ways that a user might actually notice; there is very little joy in reviewing yet another PR that enforces some other ruff rule, especially if done by a first-time contributor who doesn't have the knowledge to see the pitfalls.