DedalusProject / dedalus

A flexible framework for solving PDEs with modern spectral methods.
http://dedalus-project.org/
GNU General Public License v3.0
489 stars 115 forks source link

Updated setup.py to use extra_requires for tests #289

Open ccoulombe opened 5 months ago

ccoulombe commented 5 months ago

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

MilesCranmer commented 4 months ago

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

kburns commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

ccoulombe commented 4 months ago

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.