-
Notifications
You must be signed in to change notification settings - Fork 277
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
Technical debt with deps specifications #3074
Comments
Thanks so much for this! I do agree we have too many ways to define our dependencies and I cannot see one that you would have missed. I'd be happy to see everything be moved to one single file (e.g. One thing I am wondering about is how to handle compile-time dependencies? Should this stay in |
atm I don't think we can avoid this yeah, but at least the separation is quite clean and documenting this state would already reduce the amount of headache it's currently generating, I think. |
Another note is that I've been experimenting with this pre-commit hook (https://github.com/asottile/setup-cfg-fmt) on small projects and it is a very nice helper to maintain consistency in setup.cfg (in particular, it can auto-discover and update |
and as pointed out by @cphyc , there's a tool to automate the migration from setup.py to setup.cfg https://github.com/asottile/setup-py-upgrade edit: it seems this can't be done because some functionalities described in our edit again: doable after all #3078 |
This is mostly solved with #3236 merged. There is still room for improvement but nothing really obvious atm. |
Bug report
Bug summary
Currently, there are several files specifying constraints for our deps. I'll try to list them all here with short descriptions
setup.py
: defines mandatory runtime dependencies (update: migrated to setup.cfg in Migrate metadata from setup.py to setup.cfg #3078)pyproject.toml
: defines only Cython and numpy as compile-time depstests/test_prerequirements.txt
: again, constrains for Cython and numpy, but this time it's needed because Cartopy (an optional dep to us) doesn't define its own compile time deps correctly (?)tests/test_requirements.txt
: contains all optional deps of yt and (mostly) up-to-date versions of mandatory depstests/ci_install.sh
: contains "minimal" requirements for Cython and numpy (for Cartopy again)tests/test_minimal_requirements.txt
: defines minimal mandatory runtime depsEvidently this is getting pretty hard to maintain, especially in PRs like #2917 (ok maybe the plural isn't appropriate)... and quite confusing for everyone.
It seems to me that, for the most part (i.e. ignoring the Cartopy Problem™️), the problem can be addressed by using _only_™️
setup.cfg
(orsetup.py
) to specify minimal runtime depsand a "full" env could be specified as
though I admit it is not clear to me if this would still work with the most recent versions of pip.
Note that the identifier
"full"
is purely arbitrary.The Cartopy Problem ™️
It seems a lot of the technical debt described above is due to the fact Cartopy isn't trivial to install for non-conda environments. However our own tests are run with conda so the setup could still be simplified (I think).
Moreover, we have this helpful comment
and I'm glad to announce that this commit SciTools/cartopy@f6aadf8 (⚠️ not released yet) apparently solved the issue upstream, so there's room for experimentation and simplification on our side !
Issue with PEP 517 (pyproject.toml)
Our own pyproject.toml is causing some undesirable side effects lately because it specifies compile-time dependencies that are installed in isolation, so they actually ignore the runtime environment. This is problematic because compiling yt with e.g. numpy 1.20.0 while 1.19.0 is installed will create a faulty install where
import yt
raises an error.I honestly don't know how to resolve this properly, the only workaround I know atm is to advise users to upgrade numpy before compiling yt (which is done here #3073).
Useful References
The text was updated successfully, but these errors were encountered: