Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated setup.py to use extra_requires for tests #289

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ccoulombe
Copy link

Move test requirements to extra_requires.
These are only needed for testing, not install.

@MilesCranmer
Copy link

@ccoulombe would it be okay for you to rebase this on #290? (Since it refactors the setup)

@kburns
Copy link
Member

kburns commented Apr 29, 2024

Thanks for the suggestions but I think we are unlikely to merge this, as we want the tests to be available in a standard pip install.

@ccoulombe
Copy link
Author

ccoulombe commented Apr 29, 2024

A standard pip install that is commonly use to test : pip install .[test] or pip install dedalus[test]
The extra is common and standard : https://pip.pypa.io/en/stable/cli/pip_install/

Installing all packages needed for testing, when you are just installing/running is cumbersome, and adds more constraints when distributing the package.

One very concrete example (which is not the case here), but depending on cmake to build, but still including it in the runtime dependencies. This potentially creates conflict and forces to build a cmake wheel in order to install the said package.

@MilesCranmer yes I can rebase it, but note that if you are moving to pyproject, it could be used to specify the extras https://peps.python.org/pep-0631/#optional-dependencies

@kburns
Copy link
Member

kburns commented Apr 29, 2024

I'm not sure it's really all that standard for numerical packages, e.g. numpy and scipy ship with built-in tests. Also here the test requirements are actually the lightest -- they are pure python packages, whereas the standard build requires c-extensions, etc.

@MilesCranmer
Copy link

I'm not sure it's really all that standard for numerical packages, e.g. numpy and scipy ship with built-in tests. Also here the test requirements are actually the lightest -- they are pure python packages, whereas the standard build requires c-extensions, etc.

@kburns I think they ship with tests but not the test framework. The difference here is that dedalus is shipping pytest itself which is not required for usage, only development.

@kburns
Copy link
Member

kburns commented Apr 29, 2024

Pytest is required to run the test suite which tests the local c-extensions builds, including linking to FFTW and MPI, which happen during the pip installation (the c-extensions are not built and pre-packaged, as this prevents linking to custom MPI and FFTW). Since building against custom FFTW and MPI is pretty key to good performance on clusters, the standard install procedure here definitely encourages all users to run these tests after the build to check for issues.

@MilesCranmer
Copy link

MilesCranmer commented Apr 29, 2024

Oh I see. In that case I guess it makes sense to leave them (maybe?)

@ccoulombe
Copy link
Author

ccoulombe commented Apr 29, 2024

I'm not sure it's really all that standard for numerical packages, e.g. numpy and scipy ship with built-in tests

@kburns scipy and numpy are great example, where pytest is not required for runtime and where they split the requirements :
https://github.com/scipy/scipy/blob/main/requirements/test.txt
https://github.com/scipy/scipy/blob/4ee0de31b758f81be85435a0ad72d315a840441d/pyproject.toml#L71

In other words, and in practice, when you install scipy or numpy, you do not install pytest. Again, this is very common practices to split the requirements.

On HPC systems, it does not encourage the user to run the tests, it actually hinder its installation (generally speaking). It is more up to distributors (like me) to run the tests on the systems before deploying the wheels.

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.

None yet

3 participants