Skip to content

Separate tests into their own package#184

Merged
h-vetinari merged 9 commits into
conda-forge:mainfrom
h-vetinari:separate-tests
May 12, 2023
Merged

Separate tests into their own package#184
h-vetinari merged 9 commits into
conda-forge:mainfrom
h-vetinari:separate-tests

Conversation

@h-vetinari

@h-vetinari h-vetinari commented Aug 6, 2021

Copy link
Copy Markdown
Member

Continuing @xhochy's work from #175, lightly edited.

Fixes #160
Closes #175

@conda-forge-linter

Copy link
Copy Markdown

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@h-vetinari

Copy link
Copy Markdown
Member Author

@xhochy, is there a way to avoid the full rebuild for the tests? Could we not make a top-level script that runs only once and copies then copy the right files from build folder as needed?

With the current approach, we're doubling CI time, which is a no-go on drone, and potentially critical on stuff like cross-compiling PPC.

@h-vetinari h-vetinari force-pushed the separate-tests branch 5 times, most recently from f8fcd26 to b89991e Compare October 13, 2021 08:45
@h-vetinari

Copy link
Copy Markdown
Member Author

@xhochy
So I revived this old PR of yours, and tried to avoid doubling the build time by having one top-level build.sh / bld.bat. Other than small things that are not build-relevant (like adding an explicit list of directories to remove for windows and validating it on the linux side), I don't see what fundamental differences there are between your PR and this one, yet mine fails and yours passes.

For the scipy output, everything that's happening boils down to find ${SP_DIR}/scipy -name tests -type d | xargs rm -r, yet now import scipy fails. Any ideas?

@conda-forge-webservices

Copy link
Copy Markdown

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@conda-forge conda-forge deleted a comment from conda-forge-webservices Bot May 6, 2023
@h-vetinari h-vetinari force-pushed the separate-tests branch 4 times, most recently from 3a0e39e to 95def9c Compare May 6, 2023 16:41
@h-vetinari

Copy link
Copy Markdown
Member Author

@xhochy @rgommers
Made some good progress finally on this old PR. It builds on unix now, where the new meson build gives us a nice separation into the global build.sh phase and then installing the correct files for the respective outputs.

Windows is harder to do with the distutils build (no good separation into build & installation steps, so have to do a full recompilation twice; this then fails for reasons that elude me currently, perhaps due to not cleaning up some build artefacts from the previous run correctly; any insights there would be appreciated).

In any case, it should drastically reduce the default download size, so should be a nice win and is probably worth doing even if it's unix-only for now.

Comment thread recipe/build-output.sh Outdated
Comment thread recipe/meta.yaml Outdated
{% set tests_to_skip = tests_to_skip + " or test_minimize_callback_copies_array[fmin]" %} # [pypy_emu]
{% set tests_to_skip = tests_to_skip + " or test_mip1 or test_mixed_threads_processes" %} # [pypy_emu]
{% set tests_to_skip = tests_to_skip + " or test_modules_importable or test_neldermead_limit" %} # [pypy_emu]
{% set tests_to_skip = tests_to_skip + " or test_random_exact or test_random_complex_exact" %} # [pypy_emu]

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.

Some of these shouldn't be running I think because of the set label = "'fast'" logic a few lines below. For the ones that are actually needed, how about we upstream the skips?

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.

All these are current (as in: running even with label='fast'). I updated them recently in #227

@rgommers rgommers 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.

These test files aren't being deleted yet with the pattern being */tests/*, that'd be another 800 kb or so.

_lib/_test_ccallback.cpython-39-darwin.so
_lib/_test_deprecation_call.cpython-39-darwin.so
_lib/_test_deprecation_def.cpython-39-darwin.so
integrate/_test_multivariate.cpython-39-darwin.so
integrate/_test_odeint_banded.cpython-39-darwin.so
io/_test_fortran.cpython-39-darwin.so
ndimage/_ctest.cpython-39-darwin.so
ndimage/_cytest.cpython-39-darwin.so
special/_test_internal.cpython-39-darwin.so

It'd be great to summarize the final gain here (total size with/without); it'll justify doing this and maybe in the future doing this for SciPy itself or rearranging the folder layout so it's much easier to do.

+1 for not doing this on Windows right now, doubling the build time isn't worth it and once we get rid of distutils it'll be easy to do.

This does still require a patch to ensure that scipy.test() or pytest --pyargs scipy clearly tells the user to install scipy-tests.

@h-vetinari

Copy link
Copy Markdown
Member Author

It'd be great to summarize the final gain here (total size with/without); it'll justify doing this and maybe in the future doing this for SciPy itself or rearranging the folder layout so it's much easier to do.

I don't see the current footprint but it'd show up once it's on https://anaconda.org/conda-forge/scipy/files. I'll update with a summary afterwards.

These test files aren't being deleted yet with the pattern being /tests/, that'd be another 800 kb or so. [...]

For now, I think I'll take the win of removing the test folders with a simple procedure. We can fine-tune afterwards IMO.

+1 for not doing this on Windows right now, doubling the build time isn't worth it and once we get rid of distutils it'll be easy to do.

On windows I actually don't mind the doubled built time. We have long-running jobs for the emulated test suites on aarch/ppc anyway that run way longer than a twice compiled build on windows. But I don't know why it's failing to compile the second time (with identical build script & host deps), and if it becomes too vexing I might give up. 🤷

This does still require a patch to ensure that scipy.test() or pytest --pyargs scipy clearly tells the user to install scipy-tests.

Fair enough.

@h-vetinari

Copy link
Copy Markdown
Member Author

This does still require a patch to ensure that scipy.test() or pytest --pyargs scipy clearly tells the user to install scipy-tests.

Fair enough.

While I have an idea of how to patch scipy.test, I'm not actually sure how to do that for pytest --pyargs foo. What I can tell from the pytest docs is that it will auto-discover the tests based on the filesystem location of foo.

Pytest has quite an array of hooks, but I'm not even sure if any of scipy's code will run if invoked with pytest --pyargs scipy? Do you have an idea/intuition how to achieve that?

@rgommers

rgommers commented May 7, 2023

Copy link
Copy Markdown
Contributor

I'm not even sure if any of scipy's code will run if invoked with pytest --pyargs scipy? Do you have an idea/intuition how to achieve that?

How about simply writing a file scipy/_lib/test_condaforge.py with a single test function that raises an exception saying to conda install scipy-tests?

@h-vetinari

Copy link
Copy Markdown
Member Author

How about simply writing a file scipy/_lib/test_condaforge.py with a single test function that raises an exception saying to conda install scipy-tests?

I didn't think of that because I was aiming to not get to the test collection stage, but I doubt that's possible. So I think that's a pretty good idea because, not least because it solves both cases at once, and because we don't even have to patch it in, we can just copy it from $RECIPE_DIR to $SP_DIR.

@h-vetinari h-vetinari force-pushed the separate-tests branch 2 times, most recently from a7a8a67 to b5bb2ce Compare May 8, 2023 02:42
@h-vetinari h-vetinari force-pushed the separate-tests branch 3 times, most recently from 122c4b7 to 4f82945 Compare May 9, 2023 08:01
@h-vetinari

Copy link
Copy Markdown
Member Author

So this comes with a few extra bells & whistles now 🙃

These test files aren't being deleted yet with the pattern being */tests/*, that'd be another 800 kb or so.

This does still require a patch to ensure that scipy.test() or pytest --pyargs scipy clearly tells the user to install scipy-tests.

Both of these are done and tested now. Also I've figured out windows (recompilation is not elegant, but better than not having the separation; I'll switch away from that as soon as we have meson builds on windows).

We can still upstream part of the pypy-in-emulation skips, but it's quite specific to that combination. For example, the slowest pypy tests on native hardware take ~120sec, the slowest cpython test in emulation take ~360sec, but for pypy-in-emulation, many tests will blow through our 1200sec timeout. Accurately representing that sounds like a hassle, but if we don't mind marking a couple tests as non-'fast', then all the better of course.

In any case, this PR should be on home stretch now.

@rgommers

rgommers commented May 9, 2023

Copy link
Copy Markdown
Contributor

Great! Happy to take a final look when it's ready.

Accurately representing that sounds like a hassle, but if we don't mind marking a couple tests as non-'fast', then all the better of course.

Any of those timings are super slow; marking things as xslow for PyPy only would be fully justified if they take 120 seconds without emulation.

@h-vetinari

Copy link
Copy Markdown
Member Author

Happy to take a final look when it's ready.

It is, that's what I meant by home stretch. :)

marking things as xslow for PyPy only would be fully justified if they take 120 seconds without emulation.

I'll have a look at adding some markers. For context, I gave the numbers for the very longest running tests (according to the logs), not all of them take ~120sec without emulation, just 1-2.

yes, we're talking about windows
@h-vetinari

Copy link
Copy Markdown
Member Author

Happy to take a final look when it's ready.

It is, that's what I meant by home stretch. :)

OK, I rebased this after #234 and managed to (hopefully finally) beat those intermittent errors on windows.

@rgommers rgommers 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.

Two timeouts for PyPy on aarch64 it looks like.

This LGTM in principle. It'd be great if someone else could review too though, I can't really judge the .bat file changes or whether this is all according to conda-forge best practices. From the "how the scipy package behaves" point of view it seems good to go.

@h-vetinari

Copy link
Copy Markdown
Member Author

Two timeouts for PyPy on aarch64 it looks like.

Not even a normal 6h timeout, but two workers that stopped responding. Haven't had that one in a long time, just my luck that they appear when I declare things ready. 😅

In any case, I restarted and it shouldn't be an issue

@xhochy xhochy 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.

Looking good! Thanks @h-vetinari for bringing this here.

@h-vetinari h-vetinari merged commit 7b43d92 into conda-forge:main May 12, 2023
@h-vetinari h-vetinari deleted the separate-tests branch May 12, 2023 14:00
@h-vetinari

Copy link
Copy Markdown
Member Author

Seems we managed to drop about 6MB out of 19-20MB previously. Slightly less than hoped for, but better than nothing I'd say!

@rgommers

Copy link
Copy Markdown
Contributor

Great, thanks. I sent an FYI to the scipy-dev mailing list

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Separate tests into a scipy-tests package

4 participants