Skip to content

Conversation

@larsoner
Copy link
Member

@larsoner larsoner commented Jan 4, 2021

  1. Move the variables we change most of the times in utils.py to the top. In theory we could move all hashes and ?version=s up there, but I think it actually becomes less readable to people who might need to change the values. 90% of the time we're just updating testing or misc so this seems like a nice compromise.
  2. Detect if a dataset in _RELEASES releases (currently only testing and misc) is out of date based on version.txt and download it if necessary.
  3. Remove a crufty warning from when we tied dataset versions to MNE versions, we don't do this anymore AFAIK

This is meant to work nicely with #8695 so that when a PR updates the testing version etc. it should automatically re-download the testing dataset, overwriting the previously cached version. So @GuillaumeFavelier as long as the cache is restored at the beginning, the download called, than cache saved, then this should auto-update things I think.

This will make it so that the cache gets redownloaded a lot while there is a PR in progress to update the testing dataset. One way around this would be to parse mne/datasets/utils.py for the testing= line, and name the cache based on the testing dataset version. @GuillaumeFavelier this might be the way to go for #8695. Then hopefully the oldest caches will just be booted by GitHub, which should be the case assuming they default to a MRU cache.

@GuillaumeFavelier can you look?

@drammock
Copy link
Member

drammock commented Jan 4, 2021

this PR will necessitate a mildly painful rebase on #8679 if this one is merged first, because the pooch PR touches datasets/utils.py with a heavy hand. So bearing in mind that getting pooch to work is still uncertain, I would say keep this open but don't merge yet, and if pooch ends up working we can cherry-pick any changes from here that are still useful / not superseded? WDYT?

@larsoner
Copy link
Member Author

larsoner commented Jan 4, 2021

So bearing in mind that getting pooch to work is still uncertain, I would say keep this open but don't merge yet, and if pooch ends up working we can cherry-pick any changes from here that are still useful / not superseded? WDYT?

This would not be good because this PR is useful for #8695 which both speeds up and improves the reliability of our CI testing.

The only really necessary change to utils.py for this PR is really this addition:

    # additional condition: check for version.txt and parse it
    want_version = _RELEASES.get(name, None)
    if not need_download and want_version is not None:
        data_version = _dataset_version(folder_path[0], name)
        need_download = data_version != want_version
        if need_download:
            logger.info(f'Dataset {name} version {data_version} out of date, '
                        f'latest version is {want_version}')

The other changes to utils.py are just cleanups more or less. Would it work for you if I changed this PR just to add these lines?

I see your PR does not address dataset testing yet, which will require quite a bit of refactoring I think since our tests (on master and this PR) monkeypatch utils.py and _fetch_file etc. So I think if this PR ()with just the lines above changed in utils.py) is merged before yours it actually won't create too much additional work for you. WDYT?

@larsoner
Copy link
Member Author

larsoner commented Jan 4, 2021

Would it work for you if I changed this PR just to add these lines?

I've updated the PR to do this, so feel free to look at the diff inline to see if it looks acceptable...

@drammock
Copy link
Member

drammock commented Jan 4, 2021

diff looks fine now, thanks for accommodating. Cleanup of datasets/utils.py will happen anyway in #8679 eventually.

@larsoner larsoner merged commit 1386e14 into mne-tools:master Jan 4, 2021
@larsoner larsoner deleted the version branch January 4, 2021 20:26
cbrnr pushed a commit to cbrnr/mne-python that referenced this pull request Jan 15, 2021
* MAINT: Better downloading for testing and misc

* STY: Missed [circle front]

* FIX: Misc [circle front]

* FIX: Version [circle full]

* FIX: Version [circle full]
@larsoner larsoner mentioned this pull request Jun 3, 2022
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

Successfully merging this pull request may close these issues.

2 participants