MAINT: No longer bundle packages #10199

larsoner · 2022-01-13T15:58:25Z

Implements the plan from #10198 (comment).

Will iterate until green, then do a [circle full] build.

Close #10198

Todo:

Update README.rst with jinja2 and the other hard deps here
Make nested imports for h5io and pymatreader so that h5py is not a hard dependency.
Add jinja2 files to setup.py data list

environment.yml

larsoner · 2022-01-14T18:05:17Z

Okay we're all green, so this is ready for review/merge from my end.

The only thing I realized that I missed in #10198 (comment) is that we now implicitly also have a hard dependency on h5py, which is not pure Python. If this is problematic, we could move back to having optional dependencies on pymatreader and h5io (both of which depend on h5py) -- this would affect read_raw_nirx, read_raw_eeglab, and any I/O that uses HDF5 under the hood (morphing, TFR, some STCs).

I think my vote is to try the hard dependency on h5py and if we get reports of problems from people, make them optional. Plus I think with the mne-installers coming around that should hopefully serve 99% of our users (at least the ones who couldn't figure out how to get h5py working on their own) hopefully we're in safe territory anyway.

cbrnr · 2022-01-14T18:43:46Z

+1 for including h5py and monitoring it closely. Also, this means that we could replace edflib-python with pyedflib (which is much faster). BTW, this package is not in any requirements file (it should not be in base I think).

Did you decide what is a base dependency by what we previously vendored?

larsoner · 2022-01-14T18:47:37Z

Did you decide what is a base dependency by what we previously vendored?

That plus adding pooch (to remove the need for mne[data]) and packaging for #10197, i.e., just following my previous suggestion/idea as much as possible (#10198 (comment))

larsoner · 2022-01-14T18:48:21Z

BTW, this package is not in any requirements file (it should not be in base I think).

Yes we do not currently have a requirements_export.txt or anything like that, though we could add it.

agramfort · 2022-01-15T13:43:58Z

my only big concerns is about h5io and pymatreader. For me it's not a core requirement of the package and I have terribly painful experience with hdf5 dependencies in the past. It's not a core package from the scipy stack and the risk of technical dept is for me very real. I would vote to keep h5io and pymatreader optional dependencies.

my 2c

cbrnr · 2022-01-15T19:03:23Z

Also, we never vendored these packages before, so this seems like a different issue than just replacing the vendored packages. What was the situation before this PR? They were optional dependencies, right?

larsoner · 2022-01-16T15:17:48Z

Before, we vendored the code and nested the import of the libraries that need h5py. We could continue to nest them to keep them as soft rather than hard dependencies. I'll make that change

agramfort · 2022-01-16T15:53:10Z

Before, we vendored the code and nested the import of the libraries that need h5py. We could continue to nest them to keep them as soft rather than hard dependencies. I'll make that change

👍

hoechenberger · 2022-01-17T10:40:43Z

We could continue to nest them to keep them as soft rather than hard dependencies. I'll make that change

I like this idea.

larsoner · 2022-01-18T13:13:47Z

Okay I ended up adding an optional pip install mne[hdf5] for people who want HDF5 support. This is ready for review/merge from my end.

setup.py

Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>

hoechenberger

There's actually two subdirs in html_templates, albeit with very similar names 🙈

hoechenberger · 2022-01-18T13:41:22Z

Oh you already fixed it while I was typing my comment! 🚀

cbrnr

Looks good, just a few comments/questions.

doc/install/index.rst

README.rst

mne/io/nirx/nirx.py

mne/tests/test_import_nesting.py

requirements_doc.txt

drammock

LGTM. A couple non-blocker subjective comments

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?
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.

drammock · 2022-01-18T17:31:09Z

mne/fixes.py

+    with warnings.catch_warnings(record=True):
+        warnings.simplefilter('ignore')
+        return eval(f'parse("{version_a}") {operator} parse("{version_b}")')


can you explain why this change was needed?

parse emitted a warning because some package had a non-PEP440-compliant version. We should hide these from the user, they shouldn't hear about this issue. (It didn't break the comparison in this case anyway -- I think it was a .develop or something at the end.)

larsoner · 2022-01-18T18:57:57Z

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?

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.). I'll open a separate issue for this since I think this problem exists to some extent in main so I think we should tackle it separately (even though this PR exacerbates the problem).

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

In main we currently have [data]. This PR just adds [hdf5]. Your proposal also includes [hdf5], and adds [dev], but omits [data]. Regarding [data], I think we should keep it -- multiple downstream packages recently had to start doing pip install mne[data] because we broke dataset downloading for them. If we remove it, we're making another breaking change for them. Leaving [data] in as a no-op is a low-cost (to us) way to not break their workflows.

If we keep [data], your proposed set of options is a superset of the ones here, so again I think we can do it in another PR.

I'd rather think about how to shoe-horn new deps into one of those 3 levels.

Agreed this would be cleaner if we can do it. I'll open a separate issue for this.

drammock · 2022-01-18T19:47:16Z

Regarding [data], I think we should keep it -- multiple downstream packages recently had to start doing pip install mne[data] because we broke dataset downloading for them. If we remove it, we're making another breaking change for them. Leaving [data] in as a no-op is a low-cost (to us) way to not break their workflows.

+1, I have no objection keeping legacy shorthands as synonyms for ones that we consider "current" and advise people to use.

agramfort

merge if happy !

thx heaps @larsoner 👏 🙏

requirements_doc.txt

hoechenberger · 2022-01-19T06:20:21Z

Thanks @larsoner!

larsoner commented Jan 13, 2022

View reviewed changes

environment.yml Outdated Show resolved Hide resolved

larsoner marked this pull request as ready for review January 14, 2022 18:02

larsoner requested a review from britta-wstnr as a code owner January 14, 2022 18:02

larsoner added this to the 1.0 milestone Jan 14, 2022

larsoner mentioned this pull request Jan 14, 2022

MAINT: Remove tempita #10204

Closed

larsoner mentioned this pull request Jan 16, 2022

MRG: Fix verbose of mne.datasets fetching #10210

Merged

sappelhoff mentioned this pull request Jan 16, 2022

Improve documentation for PyPREP/MatPREP highpass differences sappelhoff/pyprep#110

Merged

4 tasks

larsoner added 2 commits January 17, 2022 13:23

MAINT: No longer bundle packages

cafed5d

FIX: Try nesting

5efb5b3

larsoner force-pushed the externals branch from fdaea04 to 5efb5b3 Compare January 17, 2022 19:21

larsoner added 4 commits January 17, 2022 15:39

FIX: Better try

91d4552

FIX: Move

c0e6f6e

FIX: Manifest

5514466

FIX: Try again

2613446

larsoner requested a review from sappelhoff as a code owner January 17, 2022 23:03

larsoner added 5 commits January 17, 2022 21:21

FIX: Default

107cdd3

MAINT: Simplify

ff99d38

FIX: Revert

83aaf9f

FIX: Cleaner

08230db

FIX: Better

5458b59

hoechenberger reviewed Jan 18, 2022

View reviewed changes

setup.py Show resolved Hide resolved

Update setup.py

ceaa2da

Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>

hoechenberger reviewed Jan 18, 2022

View reviewed changes

hoechenberger approved these changes Jan 18, 2022

View reviewed changes

cbrnr reviewed Jan 18, 2022

View reviewed changes

doc/install/index.rst Show resolved Hide resolved

README.rst Show resolved Hide resolved

mne/io/nirx/nirx.py Show resolved Hide resolved

mne/tests/test_import_nesting.py Show resolved Hide resolved

cbrnr approved these changes Jan 18, 2022

View reviewed changes

FIX: Circle [skip azp] [skip actions]

344774b

larsoner commented Jan 18, 2022

View reviewed changes

requirements_doc.txt Outdated Show resolved Hide resolved

larsoner added 2 commits January 18, 2022 11:23

FIX: Try again [skip azp] [skip actions]

a44bbcb

FIX: Commit [ci skip]

3a68ca1

drammock approved these changes Jan 18, 2022

View reviewed changes

larsoner mentioned this pull request Jan 18, 2022

MAINT: Simplify requirements structure #10219

Closed

agramfort approved these changes Jan 18, 2022

View reviewed changes

requirements_doc.txt Outdated Show resolved Hide resolved

larsoner commented Jan 19, 2022

View reviewed changes

requirements_doc.txt Outdated Show resolved Hide resolved

Update requirements_doc.txt

1e28fea

hoechenberger merged commit 6e312e1 into mne-tools:main Jan 19, 2022

larsoner deleted the externals branch January 19, 2022 12:06

larsoner mentioned this pull request Jan 19, 2022

Use packaging.version in _compare_version #10223

Merged

rob-luke mentioned this pull request Jan 20, 2022

Fix SNIRF data shapes mne-tools/mne-nirs#429

Merged

larsoner mentioned this pull request Nov 7, 2023

Move requirements to pyproject.toml #12178

Merged

Uh oh!

MAINT: No longer bundle packages #10199

MAINT: No longer bundle packages #10199

Uh oh!

Conversation

larsoner commented Jan 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

larsoner commented Jan 14, 2022

Uh oh!

cbrnr commented Jan 14, 2022

Uh oh!

larsoner commented Jan 14, 2022

Uh oh!

larsoner commented Jan 14, 2022

Uh oh!

agramfort commented Jan 15, 2022

Uh oh!

cbrnr commented Jan 15, 2022

Uh oh!

larsoner commented Jan 16, 2022

Uh oh!

agramfort commented Jan 16, 2022 via email

Uh oh!

hoechenberger commented Jan 17, 2022

Uh oh!

larsoner commented Jan 18, 2022

Uh oh!

Uh oh!

hoechenberger left a comment

Choose a reason for hiding this comment

Uh oh!

hoechenberger commented Jan 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cbrnr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

drammock left a comment

Choose a reason for hiding this comment

Uh oh!

drammock Jan 18, 2022

Choose a reason for hiding this comment

Uh oh!

larsoner Jan 18, 2022

Choose a reason for hiding this comment

Uh oh!

larsoner commented Jan 18, 2022

Uh oh!

drammock commented Jan 18, 2022

Uh oh!

agramfort left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hoechenberger commented Jan 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

larsoner commented Jan 13, 2022 •

edited

Loading

hoechenberger commented Jan 18, 2022 •

edited

Loading