Move requirements to `pyproject.toml` #12178

hoechenberger · 2023-11-06T19:59:20Z

This is based on the discussion in #12173

This PR removes all requirements*.txt files and moves the dependencies to pyproject.toml.

requirements_base.txt is now declared under project.dependencies (no change )
requirements_hdf5.txt is now declared under the mne[hdf5] variant (no change)
🆕 requirements.txt is now declared under the mne[full] variant (new)
🆕 requirements_testing.txt is now declared under the mne[test_base] variant (new)
requirements_testing_extra.txt is now declared under the mne[test] variant (existed before, but was undocumented)
🆕 requirement_doc.txt is now declared under the mne[doc] variant (new)
🆕 New mne[dev] variant, which resolve to mne[test,doc]

❓I wanted to ask about the naming of those variants. I think I would prefer tests or testing over test; and docs over doc. But no super strong feelings here. Just wanted to ask what you think.

I also realized that when building the sdist via python -m build -s, I get some warnings related to (outdated) definitions in MANIFEST.in, which will warrant an investigation sometime when we have time.

warning: no files found matching 'mne/data/dataset_checksums.txt'
warning: no files found matching '*.json' under directory 'mne/gui/help'
warning: no files found matching '*.css' under directory 'mne/html'
warning: no files found matching 'mne/datasets' under directory 'mne'
warning: no files found matching 'mne/io/edf/gdf_encodes.txt'
warning: no previously-included files matching '*' found under directory 'examples/MNE-sample-data'
warning: no previously-included files matching '*' found under directory 'examples/MNE-testing-data'
warning: no previously-included files matching '*' found under directory 'examples/MNE-spm-face'
warning: no previously-included files matching '*' found under directory 'examples/MNE-somato-data'
warning: no previously-included files found matching 'tools'
warning: no previously-included files found matching 'CODE_OF_CONDUCT.md'
warning: no previously-included files found matching '.github'
warning: no previously-included files found matching '.github/ISSUE_TEMPLATE'
warning: no previously-included files found matching '.github/ISSUE_TEMPLATE/blank.md'
warning: no previously-included files found matching '.github/ISSUE_TEMPLATE/bug_report.md'
warning: no previously-included files found matching '.github/ISSUE_TEMPLATE/feature_request.md'

hoechenberger · 2023-11-06T20:00:29Z

doc/development/contributing.rst

@@ -304,11 +304,11 @@ be reflected the next time you open a Python interpreter and ``import mne``
 Finally, we'll add a few dependencies that are not needed for running
 MNE-Python, but are needed for locally running our test suite::

-    $ pip install -r requirements_testing.txt
+    $ pip install -e .[test]


behavior change: now install full install + full set up test dependencies

I don't think we should advertise this variant actually because it will force people to install PyQt6. Conda users -- including those of our installers -- will have PyQt5

@drammock this is also an issue with the monolithic dev -- we can't directly recommend using it because it uses full which in turn installs PyQt6

Can you point me at a discussion of why this fractionation exists? My naive question is why can't we harmonize what gets installed by the standalone installer, conda-forge recipe and pip (i.e., can't we always install PyQt5 regardless of install route)

PyQt6 isn't available on on conda forge yet. PySide6 doesn't support interactive updates well. We could use PyQt5 everywhere but PyQt6 should be better as it's newer and has more features etc (so why not use it and promote it where we can).

PyQt5 doesn't receive any updates or bug fixes. I've encountered text rendering bugs that will never be fixed, so I decided to support only PySide6 in MNELAB. PyQt6 should also work, but I'd really stay away from Qt5 stuff.

👍 for recommending pip over conda.

I'm still not seeing the compelling argument to require pyqt6 when possible and pyqt5 otherwise

Yeah we don't explicitly use any Qt6 features yet but it's more about general GUI library usability, features, etc. Qt6 came out almost 3 years ago so it's disappointing to recommend that people use Qt5. We have no choice for our installers at the moment, though, since PyQt6 isn't available there yet 😞 If we change full to install PyQt5 then people who use pip/venv (and previously used requirements.txt) will experience a bit of a step back.

Stepping back a bit, would it work to change the contribution guidelines to say ...

We could recommend using a venv but I think that's a harder step for most people in terms of remembering to activate, relationship to conda envs, etc. So it depends a bit on how advanced/experienced our audience is for the dev part.

To keep things simple and easy for us (in terms of maintenance) and the less experienced devs (in terms of what they need to do), we could focus on supporting what's required for development assuming you're using our installers. In 1.6 at least these will fortunately come with a lot of what's needed:

https://github.com/mne-tools/mne-installers/blob/46a2a7b205cfdfbc1b6969f6436ee3229fedb9f7/recipes/mne-python/construct.yaml#L157

If we consider others with different sys configs (like those using pip venvs) as "advanced" devs, maybe it's okay to expect them to use pytest SKIP statements to see what they need when they need it. If this is acceptable, then recommending in our docs that people do pip install -e .[test] or pip install -e .[test,docs] might be sufficient and easier than trying to roll an all-encompassing dev option.

Do we even still need conda? I mean, for testing and in our install instructions.

I believe @cbrnr only uses pip to install everything these days, right?

I personally use conda every day still, but as I'm moving all my development into dev containers, I started using pipx and pip more, for easier setup.

I believe @cbrnr only uses pip to install everything these days, right?

Yes, I've been using pip (and venv) exclusively for a long time now, and I haven't had any major problems. Note that I haven't really tested PyVista though, since I don't need it in my daily workflow. If we recommend VS Code, by default venvs are auto-activated, so I don't think this will be a huge issue.

We could recommend using a venv but I think that's a harder step for most people in terms of remembering to activate, relationship to conda envs, etc. So it depends a bit on how advanced/experienced our audience is for the dev part.

what I meant by the "however you like" in "please create an empty venv (however you like)" was that e.g., conda users could do

$ conda create -n mnedev pip $ conda activate mnedev $ pip install -e .[dev]

...so no need to require new knowledge of venv if they understand at least one way of creating environments. Or stated differently: "recommend that contributors install via pip in their dev environment even if they use conda to manage environments".

To keep things simple and easy for us (in terms of maintenance) and the less experienced devs (in terms of what they need to do), we could focus on supporting what's required for development assuming you're using our installers.

-0.5 on this; the installers contain a LOT of stuff that is not needed for MNE dev work (tensorflow, plotly, etc).

pyproject.toml

tools/generate_codemeta.py

…ed thanks [build-system] in pyproject.toml

hoechenberger · 2023-11-06T21:08:07Z

I will only have time to continue work on this on Wednesday.

drammock · 2023-11-06T21:29:57Z

I wanted to ask about the naming of those variants. I think I would prefer tests or testing over test; and docs over doc. But no super strong feelings here. Just wanted to ask what you think.

No strong preference here. Channeling @larsoner I'd say "see what numpy/scipy/pandas/matplotlib call them" and follow suit if there's a pattern.

One opinion I do have is that we should reconsider test_extra (really, all the variants). I'm guessing (?) that the main reason test_extra is separate is to save CI install time for CIs that don't need the "extra" ones? If that's right then maybe having a special variant called mne[ci] or mne[citest] makes sense. The same logic could extend to doc as well: if we need separate varaints for CI efficiency, let's name them accordingly, and have fewer variants that are intended for user-usage. In other words, we could assume that anyone who wants to be able to run our tests might also want to build our docs locally (and vice versa) and just have one variant for both cases (maybe mne[contrib]?). The (more comprehensive) user-facing variants could then inherit from the (more granular) CI variants, e.g., contrib = mne[citest], mne[citestextra], mne[cidoc].

If CI efficiency is not the reason for the granularity, well then I still think we should consider merging some of the variants. I want our (advanced) install instructions as simple as possible, and we already have 4 variants at least: mne, mne[full], mne[doc] and mne[test] to describe to people (not to mention hdf5...). That feels like too much.

cbrnr · 2023-11-06T21:42:13Z

That's exactly what I mentioned previously. Let's try to make do with the smallest set of variants. For example, let's get rid of hdf5 and incorporate it in either the base requirements or in full.

drammock · 2023-11-06T21:44:08Z

That's exactly what I mentioned previously. Let's try to make do with the smallest set of variants. For example, let's get rid of hdf5 and incorporate it in either the base requirements or in full.

yes, I was echoing your prior comment, but forgot to give you credit, sorry @cbrnr.

hoechenberger · 2023-11-06T21:48:19Z

That's exactly what I mentioned previously. Let's try to make do with the smallest set of variants. For example, let's get rid of hdf5 and incorporate it in either the base requirements or in full.

I think it makes sense to have it in "base", considering that we're now offering to save to HDF5 in several places. We will keep the then empty variant around for compatibility, like we do with "data".

Any vetos?

@agramfort Would that work for you? Or ... not so happy? 😅🫣

cbrnr · 2023-11-07T05:48:54Z

We will keep the then empty variant around for compatibility, like we do with "data".

I would not keep them around forever though. Let's deprecate them.

hoechenberger · 2023-11-07T06:20:52Z

How are you going to let users know that they're deprecated, though?

I think I read in the PyPA docs that it's recommended to just keep them around, empty ...

cbrnr · 2023-11-07T06:25:45Z

Except for people using pip install in a script, this is not really part of their code, so I assume it won't really break a lot of people's workflows. I think it's fine to keep them around for two more releases, but then remove them. Surely there has to be a way of getting pip to print some warning?

larsoner · 2023-11-07T13:49:55Z

For example, let's get rid of hdf5 and incorporate it in either the base requirements or in full.

I think it makes sense to have it in "base", considering that we're now offering to save to HDF5 in several places.

We had some extended discussion a while ago about what's required to add something to the base / required dependencies. I was trying to make the case for nibabel. It was rejected because:

Primarily, it causes legal headaches for downstream users (e.g. in managed/corporate envs). This issue still applies.
Secondarily, there was sentiment that something that is necessary for only some users/use cases might not meet the bar (e.g., EEG ML pipelines that don't do source imaging). nibabel is also an excellently maintained, pure Python library. h5py is a compiled library with more complicated deployment/installation/compat (this is my sense historically and @agramfort made the same observation when we originally considered the hdf5 option two years ago). So that adds a lot of hesitation at my end. My sense is also that nibabel is used more often than h5py by our code and possibly end users (git grep suggests this internally, but who knows once you start thinking about users and read_raw_eeglab etc.).

So to me if we reject nibabel as a core dep we should reject h5py.

I would not keep [no-op optional specifications] around forever though. Let's deprecate them.

Previously we decided to keep the legacy no-op options. There is close to zero maintenance burden for this so I wouldn't bother deprecating it.

I'm guessing (?) that the main reason test_extra is separate is to save CI install time for CIs that don't need the "extra" ones? If that's right then maybe having a special variant called mne[ci] or mne[citest] makes sense

Historically the motivation for the two files are:

_testing.txt : pytest etc. that you'd want for a fully functional EDIT: testing infrastructure
_testing_extra.txt : extra deps that are not needed for full MNE functionality but that are needed for fewer tests to be skipped. A prime example is nitime which is just used for validating some multitaper implementations.

So I like the idea of citest or probably better fulltest -- I think actually 95%+ of devs and PRs do not need the stuff in requirements_testing_extra.txt in order for their tests to run. People who do advanced stuff like developing a scraper for a new GUI can be expected to do pip install sphinx_gallery or whatever to get their scraping test to run. And we should not group this all together because we want to be able to install [test] in the minimal CI build, which needs pytest infra but should not pull in stuff like SG and nitime.

cbrnr · 2023-11-07T15:15:06Z

I'm fine with moving h5py into [full], I agree that it probably doesn't qualify as a base dependency.

Regarding keeping the legacy variants around as no-ops, this means that they suddenly and silently stop doing what they used to do. I would print a warning for a couple of releases and eventually remove these options.

larsoner · 2023-11-07T15:33:37Z

Regarding keeping the legacy variants around as no-ops, this means that they suddenly and silently stop doing what they used to do.

I don't think we should make any existing option stop doing what it used to do. IIRC [data] stuck around because we rolled pooch into base deps. I don't think we should change the behavior of [hdf5] because that would be backward incompatible. I think we should keep [hdf5] as it is.

cbrnr · 2023-11-07T15:43:56Z

OK. But then they are not no-ops, right?

larsoner · 2023-11-07T15:56:00Z

[data] still will be a no-op, and I don't think we should bother deprecating it. [hdf5] would not be a no-op (it would still install whatever it installs now).

hoechenberger · 2023-11-07T15:58:14Z

Yep, unless we add [hdf5] dependencies to project.dependencies, [hfd5] won't be a no-op!

drammock · 2023-11-07T16:01:26Z

here's what I think is a concrete and complete proposal. we have:

variant data (already legacy, synonymous with passing no variant)
variant hdf5: make it legacy (meaning, we stop mentioning hdf5 in our docs, but keep it around for backcompat for downstreams)
variant test (which currently installs testing and testing_extra). To me this is another candidate for becoming legacy; I still think that many folks who want to run our tests might also want to build our docs, and it's not so onerous to bundle the doc and test dependencies together, so why not just say "contributors should install mne[dev]" (or whatever we call it) and keep mne[test] around as a legacy variant for CIs and/or downstream compatibility?
new variant full or all that installs full usage functionality but nothing that is only needed for doc builds or tests. No preference between "full" and "all"
new variant dev or contrib that installs everything. I prefer "dev"

So then publicly we advertise the options mne and mne[full] (or all) and mne[dev]. Privately we also have data, hdf5, test, and whatever else we need to make our various CI jobs only install what they need (so, e.g., if some CIs need testing but not testing_extra then fine, we make a variant for that CI. Same goes for doc: we can have it for easy use in CircleCI but let's not advertise/recommend it to humans)

…o-pyproject.toml * upstream/main: Fix copy-view issue in epochs (mne-tools#12121)

larsoner · 2023-11-13T17:30:30Z

Okay I'm going to get rid of the notebook job and adjust CI requirements. I don't think it's worth us testing the osmesa branch for this much hassle

…o-pyproject.toml * upstream/main: MRG: Add ICA's `fit_params` to HTML representation (mne-tools#12194)

larsoner · 2023-11-13T19:29:59Z

Green! Fixes failures currently in main as well.

mne/viz/_brain/tests/test_brain.py

pyproject.toml

tools/generate_codemeta.py

pyproject.toml

drammock · 2023-11-14T15:41:38Z

thanks @larsoner and @hoechenberger !!

hoechenberger · 2023-11-14T15:44:07Z

Thanks @larsoner, massive effort! I hope I will find some time to test this before the weekend

Thanks everybody for your input, esp @drammock, @cbrnr and @agramfort

larsoner · 2023-11-14T15:46:26Z

Good team effort!

@hoechenberger feel free to come back to #12173 when you want. I don't think we should merge it this week as we push out 1.6 to be safe -- it's better as an early PR in the 1.7 cycle

Co-authored-by: Eric Larson <[email protected]>

hoechenberger added 4 commits November 6, 2023 19:16

Run linter & autoformatter

a6b737f

Move requirements*.txt dependencies to pyproject.toml

e7a74e1

Adjust all references to requirements*.txt files

f8a1a47

Add [dev] variant

56ddbeb

hoechenberger requested review from cbrnr and sappelhoff November 6, 2023 19:59

hoechenberger commented Nov 6, 2023

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

hoechenberger commented Nov 6, 2023

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

hoechenberger commented Nov 6, 2023

View reviewed changes

tools/generate_codemeta.py Outdated Show resolved Hide resolved

hoechenberger added 5 commits November 6, 2023 21:02

Merge branch 'main' into move-requirements-to-pyproject.toml

80a4faa

Don't install build dependencies that should be automatically install…

ae54537

…ed thanks [build-system] in pyproject.toml

Use "build" to build sdist and wheel

934f949

Add Dan as maintainer

ce62c00

Restore accidentally deleted "version" entry

58128f1

Merge remote-tracking branch 'upstream/main' into move-requirements-t…

2c50bfe

…o-pyproject.toml * upstream/main: Fix copy-view issue in epochs (mne-tools#12121)

larsoner marked this pull request as ready for review November 13, 2023 17:10

larsoner requested review from agramfort, dengemann and wmvanvliet as code owners November 13, 2023 17:10

MAINT: Bye bye notebook

8dfe427

larsoner removed request for agramfort, wmvanvliet, dengemann, cbrnr and sappelhoff November 13, 2023 17:34

larsoner added 2 commits November 13, 2023 12:34

Merge remote-tracking branch 'upstream/main' into move-requirements-t…

17494d6

…o-pyproject.toml * upstream/main: MRG: Add ICA's `fit_params` to HTML representation (mne-tools#12194)

FIX: Which

ec47ba5

drammock reviewed Nov 13, 2023

View reviewed changes

mne/viz/_brain/tests/test_brain.py Show resolved Hide resolved

pyproject.toml Show resolved Hide resolved

tools/generate_codemeta.py Show resolved Hide resolved

tools/generate_codemeta.py Show resolved Hide resolved

pyproject.toml Outdated Show resolved Hide resolved

larsoner mentioned this pull request Nov 13, 2023

MAINT: Simplify requirements structure #10219

Closed

FIX: Alphabetize

770e225

larsoner enabled auto-merge (squash) November 14, 2023 14:28

This was referenced Nov 14, 2023

[WIP] [BUG] Display title in plot_compare_evokeds when axes='topo' #12192

Merged

Fix copy-view issue in epochs #12121

Merged

FIX: Versionadded

6cb5a06

larsoner requested a review from mscheltienne as a code owner November 14, 2023 14:33

larsoner merged commit 22a6dc6 into mne-tools:main Nov 14, 2023
27 checks passed

hoechenberger deleted the move-requirements-to-pyproject.toml branch November 14, 2023 15:44

drammock mentioned this pull request Dec 11, 2023

non-interactive brain figure #12276

Closed

snwnde pushed a commit to snwnde/mne-python that referenced this pull request Mar 20, 2024

Move requirements to pyproject.toml (mne-tools#12178)

44da13a

Co-authored-by: Eric Larson <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move requirements to `pyproject.toml` #12178

Move requirements to `pyproject.toml` #12178

hoechenberger commented Nov 6, 2023 •

edited

Loading

hoechenberger Nov 6, 2023

larsoner Nov 8, 2023

larsoner Nov 8, 2023

drammock Nov 8, 2023 •

edited

Loading

larsoner Nov 8, 2023

cbrnr Nov 8, 2023

larsoner Nov 8, 2023

hoechenberger Nov 8, 2023 •

edited

Loading

cbrnr Nov 9, 2023

drammock Nov 9, 2023

hoechenberger commented Nov 6, 2023

drammock commented Nov 6, 2023

cbrnr commented Nov 6, 2023

drammock commented Nov 6, 2023

hoechenberger commented Nov 6, 2023 •

edited

Loading

cbrnr commented Nov 7, 2023

hoechenberger commented Nov 7, 2023

cbrnr commented Nov 7, 2023

larsoner commented Nov 7, 2023 •

edited

Loading

cbrnr commented Nov 7, 2023

larsoner commented Nov 7, 2023

cbrnr commented Nov 7, 2023

larsoner commented Nov 7, 2023

hoechenberger commented Nov 7, 2023 •

edited

Loading

drammock commented Nov 7, 2023 •

edited

Loading

larsoner commented Nov 13, 2023

larsoner commented Nov 13, 2023

drammock commented Nov 14, 2023

hoechenberger commented Nov 14, 2023

larsoner commented Nov 14, 2023

Move requirements to pyproject.toml #12178

Move requirements to pyproject.toml #12178

Conversation

hoechenberger commented Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drammock Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoechenberger Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoechenberger commented Nov 6, 2023

drammock commented Nov 6, 2023

cbrnr commented Nov 6, 2023

drammock commented Nov 6, 2023

hoechenberger commented Nov 6, 2023 • edited Loading

cbrnr commented Nov 7, 2023

hoechenberger commented Nov 7, 2023

cbrnr commented Nov 7, 2023

larsoner commented Nov 7, 2023 • edited Loading

cbrnr commented Nov 7, 2023

larsoner commented Nov 7, 2023

cbrnr commented Nov 7, 2023

larsoner commented Nov 7, 2023

hoechenberger commented Nov 7, 2023 • edited Loading

drammock commented Nov 7, 2023 • edited Loading

larsoner commented Nov 13, 2023

larsoner commented Nov 13, 2023

drammock commented Nov 14, 2023

hoechenberger commented Nov 14, 2023

larsoner commented Nov 14, 2023

Move requirements to `pyproject.toml` #12178

Move requirements to `pyproject.toml` #12178

hoechenberger commented Nov 6, 2023 •

edited

Loading

drammock Nov 8, 2023 •

edited

Loading

hoechenberger Nov 8, 2023 •

edited

Loading

hoechenberger commented Nov 6, 2023 •

edited

Loading

larsoner commented Nov 7, 2023 •

edited

Loading

hoechenberger commented Nov 7, 2023 •

edited

Loading

drammock commented Nov 7, 2023 •

edited

Loading