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

MAINT: Simplify requirements structure #10219

Closed
larsoner opened this issue Jan 18, 2022 · 4 comments
Closed

MAINT: Simplify requirements structure #10219

larsoner opened this issue Jan 18, 2022 · 4 comments

Comments

@larsoner
Copy link
Member

  1. the pip requirements_* files seem to be multiplying... is there need/value in specifying them in separate text files rather than directly in setup.py?

  2. for the install shorthands, to me it seems the fewer options the better; something like:

  • pip install mne # base
  • pip install mne[hdf5] # + hdf5 / export deps; other shortands could be "io" or "export"
  • pip install mne[dev] # everything above + doc and testing deps; could also be "all"

so if we find ourselves wanting to add more and more install shorthands as our dependencies evolve, I'd rather think about how to shoe-horn new deps into one of those 3 levels.

Originally posted by @drammock in #10199 (review)

@larsoner
Copy link
Member Author

larsoner commented Jan 18, 2022

The need/value arises because our CIs make use of them in different ways depending on the different run type (conda, pip, minimal, old, doc-build, notebook, ultraslow, etc.):

$ git grep requirements_
azure-pipelines.yml:        python -m pip install --progress-bar off -r requirements_base.txt -r requirements_hdf5.txt -r requirements_testing.txt
azure-pipelines.yml:    - script: pip install --progress-bar off -r requirements_doc.txt
...
tools/circleci_dependencies.sh: python -m pip install --progress-bar off pillow pytest -r requirements_base.txt -r requirements_doc.txt
tools/circleci_dependencies.sh: python -m pip install --upgrade --progress-bar off --only-binary matplotlib -r requirements.txt -r requirements_testing.txt -r requirements_doc.txt
tools/github_actions_dependencies.sh:   pip install -r requirements_base.txt -r requirements_testing.txt
tools/github_actions_dependencies.sh:   pip install $STD_ARGS $EXTRA_ARGS -r requirements_base.txt -r requirements_testing.txt -r requirements_hdf5.txt
tools/github_actions_dependencies.sh:   pip install $STD_ARGS $EXTRA_ARGS -r requirements_testing_extra.txt

Just one counterexample to your suggested hierarchy no give some sense of the problem, requirements_testing.txt is used by all CIs that run pytest, but it can't simply be rolled into something that already includes hdf5 (e.g., dev) because the minimal run should not include h5py (or any doc deps) but does need pytest-related dependencies.

Maybe a good first step would to add comments to the developer docs to lay out more explicitly what each CI job does. This structure would make it clear what each run uses, and why. @GuillaumeFavelier had to suffer quite a bit with this a while back when transitioning to GitHub Actions (from Travis I think?) so this is already a bit overdue. Once that's done, if someone is motivated to propose a structure that would simplify things, having those docs as a reference will make it clearer how it can be done.

@larsoner
Copy link
Member Author

larsoner commented Oct 1, 2023

I'm going to close this as won'tfix I think, and we can adjust going forward as needed (or if someone comes up with a clear plan)

@larsoner larsoner closed this as not planned Won't fix, can't repro, duplicate, stale Oct 1, 2023
@larsoner larsoner reopened this Nov 13, 2023
@larsoner
Copy link
Member Author

From @drammock in #12178 (comment) :

[I think we should suggest that devs install test_extra instead of test...] what motivates me is making it so that new contributors have less to do / think about when setting up environments. IMO they have enough cognitive load learning about how to use pre-commit, pytest, rST, git, etc, so if we can simplify the environment-wrangling / reduce the odds of them hitting missing-dependency-related errors midway through their first contribution experience, then we absolutely should do that if the cost is a few extra MB / tens of seconds during initial environment creation.

I think switching from test to test_extra won't gain contributors much in practice --it will un-skip a small set of tests that are not very super widely developed or changed -- mainly our internal scraper tests, multitaper tests, notebook GUI abstraction tests. The one exception is maybe the extended import/export libs (pybv, EDFLIB-Python, snirf) but these are kind of advanced use cases that necessarily will have those packages installed for anyone working on them, and are generally pretty well isolated from other tests. I can't think of a case in the last year or two where test or test_extra would have helped them at least.

The big win -- and what I think would reduce contributor pain -- would be to give people a way to make sure they have stuff like pandas, scikit-learn, or dipy that are currently only part of full and are used a lot of places -- those can lead to a lot of test skips. These are not part of test_extra.

So instead of modifying test to test_extra (incomplete) or creating dev off of full (Qt problems), we could build dev off of test_extra (even if it's overkill for most devs) and add everything we pytest.importorskip at the moment as well? It will have a lot of redundancies with full but won't be opinionated about Qt. (It's very likely that most devs will have a working Qt binding anyway, so we can just omit it from dev.)

Incidentally, even this dev won't prevent all tests from being skipped because you need MNE-C installed for some of them. But it will at least help the problem!

@drammock
Copy link
Member

The big win -- and what I think would reduce contributor pain -- would be to give people a way to make sure they have stuff like pandas, scikit-learn, or dipy that are currently only part of full and are used a lot of places

sounds good so far...

So instead of modifying test to test_extra (incomplete) or creating dev off of full (Qt problems), we could build dev off of test_extra (even if it's overkill for most devs) and add everything we pytest.importorskip at the moment as well

still with you...

It will have a lot of redundancies with full but won't be opinionated about Qt. (It's very likely that most devs will have a working Qt binding anyway, so we can just omit it from dev.)

so this would be better for users who are e.g. missing dipy because they never ran pip install mne[full] --- either because they use conda, or because they never needed mne[full] for their own work (e.g., EEG sensor-space analyses only), or some other reason I haven't thought of --- and who now want to contribute to MNE. Right?

Incidentally, even this dev won't prevent all tests from being skipped because you need MNE-C installed for some of them. But it will at least help the problem!

argh. I think I'm ready to give up this fight. Our dev env is so complicated.

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

No branches or pull requests

2 participants