[MAINT, MRG] Enable pooch to perform fetching of datasets #9742

adam2392 · 2021-09-15T17:22:09Z

Reference issue

First part of #8679 #9736

What does this implement/fix?

Builds on top of @drammock work in #8679 to:

move all definition of a dataset to a config.py + dataset_checksums.txt file. These define urls, archive names, configuration keys, folder names, and md5 hashes
use pooch to do the downloading, unzip/untar and QA checks
replace all of utils/fetching.py with pooch usage in all datasets that used _fetch_file().

~~I'm not sure how to handle bst, eegbci, hf_sef, limo and other complicated files yet. I think maybe I can copy @drammock's work? I'll have to investigate.~~

Note that the checksums file for eegbci is 3058 lines long, so in reality this code diff is:

~800 green lines
784 red lines
as of 09/15/21

Additional information

The next PR should handle:

passing in an optional authorization token to _data_path which would then address Adding an optional token to the dataset fetcher code to allow optional fetching from private repositories #9736
generalize _data_path to a public function, perhaps... mne_dataset_path(), which then can be used by 3rd parties who don't want to store data in the MNE datasets section.

drammock

a few prelim comments

environment.yml

mne/datasets/config.py

adam2392 · 2021-09-15T21:05:55Z

So majority of the work was done by @drammock originally in his PR. Really great push! I've added some additional changes to basically remove as much redundant code as possible.

Here, I have basically removed almost the entire utils/fetching.py file because we frankly don't need it if we allow pooch to be an optional requirement for fetching datasets.

This PR does not change the user interface, and so far works pretty great for me to pull datasets. If this PR is approved, then I can easily add in a public interface (turning _data_path to a public function) that assumes some structure on top of pooch to basically enable any downstream MNE package to define their own data_paths, leveraging the _data_path() function.

adam2392 · 2021-09-16T01:55:00Z

Fixing the docs error.

Everything else actually LGTM. There's many LOC reduced (~800). The only other error is:

________________________________ test_downloads ________________________________
3404
mne/datasets/tests/test_datasets.py:79: in test_downloads
3405
    new_path = datasets._fake.data_path(update_path=True, **kwargs)
3406
E   Failed: DID NOT WARN. No warnings of type (<class 'RuntimeWarning'>,) was emitted. The list of emitted warnings is: [].

However, I'm not entirely sure why that line is producing an error... Any ideas?

adam2392 · 2021-09-16T13:12:49Z

The error has to do w/ capsys, or calling data_path twice? I think?

This test succeeds:

def test_download(tmp_path, monkeypatch, capsys):
    kwargs = dict(path=str(tmp_path), verbose=True)
    # XXX we shouldn't need to disable capsys here, but there's a pytest bug
    # that we're hitting (https://github.com/pytest-dev/pytest/issues/5997)
    # now that we use pooch
    # with capsys.disabled(): 
    # path = datasets._fake.data_path(update_path=False, **kwargs)

    monkeypatch.setenv('_MNE_FAKE_HOME_DIR', str(tmp_path))
    with pytest.warns(RuntimeWarning, match='non-standard config'):
        new_path = datasets._fake.data_path(update_path=True, **kwargs)

    # assert path == new_path
    out, _ = capsys.readouterr()
    assert 'Downloading' not in out

But this one fails

def test_download(tmp_path, monkeypatch, capsys):
    kwargs = dict(path=str(tmp_path), verbose=True)
    # XXX we shouldn't need to disable capsys here, but there's a pytest bug
    # that we're hitting (https://github.com/pytest-dev/pytest/issues/5997)
    # now that we use pooch
    with capsys.disabled(): 
        path = datasets._fake.data_path(update_path=False, **kwargs)

    monkeypatch.setenv('_MNE_FAKE_HOME_DIR', str(tmp_path))
    with pytest.warns(RuntimeWarning, match='non-standard config'):
        new_path = datasets._fake.data_path(update_path=True, **kwargs)

    assert path == new_path
    out, _ = capsys.readouterr()
    assert 'Downloading' not in out

mne/datasets/utils.py

setup.cfg

drammock

just one last bit of cruft in requirements_testing.txt. I think this is good to go after that.

requirements_testing.txt

adam2392 · 2021-09-17T14:41:04Z

Now there is an error with GH actions though:

    pooch = _soft_import('pooch', 'dataset downloading', strict=True)
61
  File "/usr/share/miniconda/envs/test/lib/python3.7/site-packages/mne-0.24.dev0-py3.7.egg/mne/utils/check.py", line 273, in _soft_import
62
    raise RuntimeError(f'For {purpose} to work, the {name} module is '
63
RuntimeError: For dataset downloading to work, the pooch module is needed, but it could not be imported.

Do we need pooch somewhere besides where we already added?

drammock · 2021-09-17T14:48:56Z

Now there is an error with GH actions though:

dang, OK, I'll fix it

drammock · 2021-09-17T16:07:15Z

ci failure is unrelated (Windows 3.7 pip timeout)

adam2392 · 2021-09-17T16:32:48Z

mne/data/dataset_checksums.txt

@@ -0,0 +1,46 @@
+# include here the name of the zipped file and the md5 hash


later PR: Put into python code and create temp file for internal

drammock · 2021-09-17T16:36:33Z

TODO list (from live chat with @agramfort and @adam2392):

make sure pip install mne and then import mne will still work without pooch (i.e., make sure pooch imports are properly nested)
add pooch to test_import_nesting
restructure dataset info as nested dict of dicts; incorporate MD5s into python and change pooch to temporarily write the registry file (separate PR) and whatever else is necessary to make it easier to add new built-in datasets
add generic dataset downloader function, that allows e.g., API keys (separate PR). This should probably look like a data_path() function that takes in URL, archive name, hash, etc, and does everything that one of our built-in data_path() functions does (except for checking/setting config keys)

adam2392 · 2021-09-17T20:13:01Z

Unrelated CI failure.

drammock · 2021-09-17T20:33:59Z

Unrelated CI failure.

indeed they look unrelated to this PR, though did you see the ResourceWarning: unclosed file related to EDF export?
https://dev.azure.com/mne-tools/mne-python/_build/results?buildId=15442&view=logs&j=fa98c7b3-575e-55b6-6946-22dea6f6daf8&t=c0866e65-eed5-5461-eadc-f0ff150eefd8&l=3120

adam2392 · 2021-09-17T21:11:55Z

Ah fixed the resource warning. It was an error with not closing the files when a runtime error is raised.

@drammock

drammock · 2021-09-17T22:20:25Z

all green! pooch PR is ready for final review

agramfort

@larsoner merge if happy

larsoner · 2021-09-19T21:55:11Z

mne/export/_edf.py

@@ -261,6 +266,7 @@ def _export_raw(fname, raw, physical_range, add_ch_type):
            onset = onset * 10000
            duration = duration * 10000
            if hdl.writeAnnotation(onset, duration, desc) != 0:
+                hdl.close()


This solution is less clean / DRY than a solution that uses a context manager like with hdl: or, if that's not an option:

@contextmanager def _close_file(fid): try: yield finally: fid.close()

But we can clean this up in a follow-up PR

larsoner · 2021-09-19T22:01:00Z

tools/azure_dependencies.sh

@@ -17,6 +17,7 @@ elif [ "${TEST_MODE}" == "pip-pre" ]; then
 	python -m pip install --progress-bar off --upgrade --only-binary ":all" "vtk<=9.0.1"
 	python -m pip install --progress-bar off https://github.com/pyvista/pyvista/zipball/main
 	python -m pip install --progress-bar off https://github.com/pyvista/pyvistaqt/zipball/main
+	python -m pip install --progress-bar off pooch


This is not needed because the last line includes pip install -r requirements_testing.txt and it's in there

larsoner · 2021-09-19T22:01:15Z

tools/github_actions_dependencies.sh

+	echo "pooch"
+	pip install --progress-bar off pooch


larsoner · 2021-09-19T22:02:10Z

Thanks @adam2392 @drammock ! I'll open a quick PR for my tiny comments

adam2392 · 2021-09-22T21:45:11Z

mne/datasets/utils.py

+        hf_sef_raw=pooch.Untar(
+            extract_dir=path, members=[f'hf_sef/{subdir}' for subdir in
+                                       ('MEG', 'SSS', 'subjects')]),
+        hf_sef_evoked=pooch.Untar(
+            extract_dir=path, members=[f'hf_sef/{subdir}' for subdir in
+                                       ('MEG', 'SSS', 'subjects')]),


@drammock do you know why we needed hf_Sef to be like this? These LOC I copied seems to break circle CI in : #9774

It seems to work perfectly fine w/ processor = 'untar' untaring to the path. I confirm that the error was not raised when I changed the processor to just regular pooch.Untar(extract_dir=path

To clarify, I copied these from your original PR aimed to tackle #8674

…e-tools#9742)" This reverts commit c42a7b7.

drammock reviewed Sep 15, 2021

View reviewed changes

environment.yml Outdated Show resolved Hide resolved

mne/datasets/config.py Outdated Show resolved Hide resolved

mne/datasets/config.py Show resolved Hide resolved

adam2392 mentioned this pull request Sep 15, 2021

WIP: use pooch for dataset downloading #8679

Closed

adam2392 requested a review from drammock September 16, 2021 13:29

adam2392 changed the title ~~[WIP] Enable pooch to perform fetching of datasets~~ [MAINT, WIP] Enable pooch to perform fetching of datasets Sep 16, 2021

drammock reviewed Sep 16, 2021

View reviewed changes

mne/datasets/utils.py Outdated Show resolved Hide resolved

drammock reviewed Sep 16, 2021

View reviewed changes

setup.cfg Outdated Show resolved Hide resolved

adam2392 and others added 21 commits September 16, 2021 14:52

Adding updated checksum

a8c12a2

Initial try

ceb1447

Fix import

bbfae63

Fix check

7591137

Fix GH Actions

a536d26

Try again to fix CI

0a52585

make brainstorm work

0c8fba2

Address comments initially

b3f4583

Removed _fetch_file dependency

47dab05

Remove fetch file entirely

1c3c290

Lets go again with updated deletion

8229145

Try again with testing reqs

7b052da

Try again

789c677

Fix test constants

93207ed

Fix docs

843427e

fix unclosed resource warning (checksum registry)

0dced07

make compatible with py3.7

7b0c924

fix CIs?

7893c36

Fixed all unit tests

79a77b8

Fix spell check

b3fb7b5

Fix sys

736bdc0

drammock approved these changes Sep 17, 2021

View reviewed changes

requirements_testing.txt Outdated Show resolved Hide resolved

Update requirements_testing.txt

dfaa1fc

restore pooch to testing reqs

c8bcfdf

adam2392 commented Sep 17, 2021

View reviewed changes

adam2392 added 3 commits September 17, 2021 13:42

Merging

2835feb

Nested import for pooch

c1ac41d

Add back in

7b134cc

drammock mentioned this pull request Sep 17, 2021

CI, MAINT: Unclosed file ResourceWarning in EDF export #9756

Closed

Fix resoure warning

204c146

agramfort approved these changes Sep 18, 2021

View reviewed changes

larsoner reviewed Sep 19, 2021

View reviewed changes

tools/github_actions_dependencies.sh

Comment on lines +34 to +35

echo "pooch"

pip install --progress-bar off pooch

Copy link

Member

larsoner Sep 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

larsoner merged commit c42a7b7 into mne-tools:main Sep 19, 2021

This was referenced Sep 20, 2021

RFC, MAINT: use pooch for download management? #8674

Closed

FTP protocol download support #9226

Closed

larsoner mentioned this pull request Sep 22, 2021

BUG: Pooch fails to download on CircleCI #9774

Closed

adam2392 commented Sep 22, 2021

View reviewed changes

adam2392 mentioned this pull request Sep 23, 2021

Untarring members that are sub-directories themselves results in an error fatiando/pooch#264

Closed

larsoner added a commit to larsoner/mne-python that referenced this pull request Oct 7, 2021

Revert "[MAINT, MRG] Enable pooch to perform fetching of datasets (mn…

061a1ba

…e-tools#9742)" This reverts commit c42a7b7.

larsoner mentioned this pull request Oct 7, 2021

WIP, MAINT: Fix CircleCI #9813

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MAINT, MRG] Enable pooch to perform fetching of datasets #9742

[MAINT, MRG] Enable pooch to perform fetching of datasets #9742

adam2392 commented Sep 15, 2021 •

edited by drammock

drammock left a comment

adam2392 commented Sep 15, 2021 •

edited

adam2392 commented Sep 16, 2021 •

edited

adam2392 commented Sep 16, 2021

drammock left a comment

adam2392 commented Sep 17, 2021

drammock commented Sep 17, 2021

drammock commented Sep 17, 2021

adam2392 Sep 17, 2021

drammock commented Sep 17, 2021 •

edited

adam2392 commented Sep 17, 2021

drammock commented Sep 17, 2021

adam2392 commented Sep 17, 2021

drammock commented Sep 17, 2021

agramfort left a comment

larsoner Sep 19, 2021

larsoner Sep 19, 2021

larsoner Sep 19, 2021

larsoner commented Sep 19, 2021

adam2392 Sep 22, 2021

adam2392 Sep 22, 2021

		@@ -0,0 +1,46 @@
		# include here the name of the zipped file and the md5 hash

[MAINT, MRG] Enable pooch to perform fetching of datasets #9742

[MAINT, MRG] Enable pooch to perform fetching of datasets #9742

Conversation

adam2392 commented Sep 15, 2021 • edited by drammock

Reference issue

What does this implement/fix?

Additional information

drammock left a comment

Choose a reason for hiding this comment

adam2392 commented Sep 15, 2021 • edited

adam2392 commented Sep 16, 2021 • edited

adam2392 commented Sep 16, 2021

drammock left a comment

Choose a reason for hiding this comment

adam2392 commented Sep 17, 2021

drammock commented Sep 17, 2021

drammock commented Sep 17, 2021

adam2392 Sep 17, 2021

Choose a reason for hiding this comment

drammock commented Sep 17, 2021 • edited

adam2392 commented Sep 17, 2021

drammock commented Sep 17, 2021

adam2392 commented Sep 17, 2021

drammock commented Sep 17, 2021

agramfort left a comment

Choose a reason for hiding this comment

larsoner Sep 19, 2021

Choose a reason for hiding this comment

larsoner Sep 19, 2021

Choose a reason for hiding this comment

larsoner Sep 19, 2021

Choose a reason for hiding this comment

larsoner commented Sep 19, 2021

adam2392 Sep 22, 2021

Choose a reason for hiding this comment

adam2392 Sep 22, 2021

Choose a reason for hiding this comment

adam2392 commented Sep 15, 2021 •

edited by drammock

adam2392 commented Sep 15, 2021 •

edited

adam2392 commented Sep 16, 2021 •

edited

drammock commented Sep 17, 2021 •

edited