Serve dataset downloads from NEMAR's sourcedata/, and add a provider switch - #1139
Open
bruAristimunha wants to merge 9 commits into
Open
Serve dataset downloads from NEMAR's sourcedata/, and add a provider switch#1139bruAristimunha wants to merge 9 commits into
bruAristimunha wants to merge 9 commits into
Conversation
…switch
NEMAR deposits now republish the original, pre-BIDS distribution under
`sourcedata/` -- byte-identical to what the upstream host serves, stored
under the same upstream filenames, with a `sourcedata_provenance.json`
recording each file's name, size and SHA-256. That makes NEMAR usable as
a mirror of the original source, which matters because those upstream
hosts are variously slow, rate-limited, bot-gated or retired.
`download()` now fetches that tree instead of the deposit's BIDS copy.
Why sourcedata and not the BIDS copy
------------------------------------
The BIDS copy is a re-encoding, and substituting it would change results
rather than only change where bytes come from:
* Session/run labels are derived from BIDS entities, so they do not match
what a dataset's own loader produces. Weibo2014 returns
{"0": {"0": raw}} from one concatenated RawArray; Lee2019 uses
semantic "1train"/"4test" labels that have no BIDS equivalent.
* Events differ. ErpCore2021 applies a 26 ms LCD-delay correction and
remaps raw integer codes to "Target"/"NonTarget" via
handle_events_reading; a BIDS-loaded raw carries the unmapped
annotations, `events_from_annotations` then finds nothing, and
RawToEvents swallows that as zero events -- so the run is silently
dropped as empty with no exception raised.
`sourcedata/` avoids all of it: each dataset's own parser runs on the
same files it always ran on. Loading is untouched -- `get_data` still
goes through `_get_single_subject_data`.
What's here
-----------
* `nemar_sourcedata_dl` fetches a deposit's sourcedata/ and fails loudly
when a deposit publishes none, rather than leaving a confusing empty
cache (nemar-py "succeeds" with zero matching files).
* `BaseDataset.sourcedata_path` is the public accessor, mirroring
data_path's contract. Optional `nemar_sourcedata_include` addresses one
subject; without it the tree is fetched once, because sourcedata keeps
the upstream layout and there is no general subject->file rule to guess.
* `set_download_provider` / `get_download_provider`: "auto" (default --
NEMAR first, upstream fallback), "nemar" (no fallback), "upstream"
(never NEMAR). Also settable per run via MOABB_DOWNLOAD_PROVIDER.
* Falls back to the dataset's own downloader, with a warning, when a
deposit has no sourcedata/ -- so the deposits that were never enriched
keep working.
Verified against live NEMAR: Cattan2019_PHMD().download() populates
.../nm000341/sourcedata with 18 files and fetches no BIDS eeg files.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a2dd6cb6c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| force_update=force_update, | ||
| verbose=verbose, | ||
| ) | ||
| return |
There was a problem hiding this comment.
When download() succeeds for a mirrored dataset, this return skips its existing data_path, but nothing redirects the loader to the new directory. For example, Cattan2019_PHMD._get_single_subject_data() still calls data_path(), which resolves under MNE-headmounted-data, while nemar_sourcedata_dl() writes under MNE-cattan2019-phmd-data/nm000341/sourcedata. Consequently, a subsequent get_data() still contacts Zenodo—even when the provider is pinned to nemar—so the downloaded mirror is unusable through the normal loading path.
Useful? React with 👍 / 👎.
| # A deposit without sourcedata/ downloads "successfully" with zero matching | ||
| # files, which would otherwise surface much later as a confusing empty | ||
| # cache. Fail here instead, where the cause is still obvious. | ||
| if not sourcedata_dir.is_dir() or not any(sourcedata_dir.rglob("*")): |
There was a problem hiding this comment.
When calls reuse a target directory and include selects one subject, this condition accepts any file left in sourcedata/ by an earlier call. If subject 1 downloaded successfully and the subject 2 filter matches zero files, NEMAR can return successfully with no matches, but rglob finds subject 1's file and suppresses the intended error; the batch then completes with subject 2 absent. Validate files matching the current include value, or use the downloader's result, rather than checking the entire tree.
Useful? React with 👍 / 👎.
test_download_prefers_nemar was failing
---------------------------------------
Both pre-existing NEMAR tests in test_datasets.py monkeypatch
`nemar_dl`, which download() no longer calls, so one asserted on an
empty call list and failed. The other passed only incidentally -- its
monkeypatch was dead code, so it made a *live* call to data.nemar.org
from a unit test and matched the warning that real failure produced.
Both now patch `nemar_sourcedata_dl`. CI was red on every job for this.
One deposit, one copy
---------------------
The cache path embedded the MOABB code, but a deposit backs several
classes: nm000132 is declared by all seven ErpCore2021 subclasses and
nm000250 by four Dreyer2023 ones, so `_download_all()` wrote seven
complete copies of the same tree. Keyed on the NEMAR id now.
"No sourcedata" was decided against the wrong thing
--------------------------------------------------
The guard tested whether the shared directory was non-empty, but
download() calls this once per subject into that same tree, so from the
second subject on it was true regardless -- a subject whose include glob
matched nothing was reported as downloaded and then fetched from neither
NEMAR nor upstream. It now compares the tree before and after the call.
Relatedly, the error branch was unreachable: nemar-py raises
SelectionError (a NemarError) for a zero-match scope query *before*
returning, so the caller only ever saw the generic "could not download"
message. The except branch now produces the specific one and
distinguishes "this deposit has no sourcedata/" from "this subject is
not in it", since the remedies differ.
Template errors defeated the fallback
-------------------------------------
`nemar_sourcedata_include.format(subject=...)` was unguarded, unlike the
sibling _nemar_subject. A glob template is a natural place to write
braces ("*.{mat,fdt}"), which str.format reads as a field and raises
KeyError -- escaping download()'s `except NemarDownloadError` and killing
the call with no upstream fallback. Converted, with a message that says
to escape the braces.
Also
----
- nemar_sourcedata_dl accepted and documented `verbose` but had no
@verbose decorator, so it was inert on the path download() now takes
by default. Added, matching nemar_dl.
- download()'s docstring still described the pre-NEMAR behaviour;
update_path and accept do not apply on the NEMAR path, and
subject_list is only honoured when nemar_sourcedata_include is set.
… fetch
Tests
-----
Four groups of near-identical tests collapse into parametrized ones:
argument forwarding (scope / include / force_update), download selection
(whole-tree vs per-subject), the two sourcedata_path refusals, and the
two "nothing was fetched" cases. Two fixtures replace the repeated
monkeypatch preamble; `nemar_calls` patches BOTH NEMAR entry points so
every case can assert the BIDS copy was never fetched, which is the
whole point of preferring sourcedata/.
_FakeNemar now honours `include`, so the per-subject miss is actually
exercised rather than passing vacuously.
Emptiness check
---------------
The guard snapshotted `set(sourcedata_dir.rglob("*"))` before and after
the call. download() runs it once per subject into a shared tree, so a
100k-file deposit with 50 subjects meant 100 full recursive walks and
~10M Path allocations -- and the `before` walk ran even on the failure
path. It now asks about the subset the call requested
(`next(target_dir.glob(include), None)`), which short-circuits and costs
nothing when the download raised.
Also
----
- Deduped the MNE warning-suppression block into `_set_moabb_config`; it
was a verbatim copy of one 40 lines above in the same file.
- Dropped `os.environ.get(...) or get_config(...)`: verified against
mne's source that get_config already reads os.environ first, so the
env-wins-over-config behaviour is unchanged.
- Removed unreachable branches in _download_nemar_sourcedata (its only
caller always passes a subject list) and folded the whole-tree case
into the same loop.
- Hoisted the error message so the sentence is written once instead of
three times, replacing a multi-line conditional inside a raise.
`nemar_sourcedata_include` asked each dataset to describe its subjects
with a format template. That cannot work: sourcedata/ preserves whatever
layout the authors published, and across the catalogue that is genuinely
arbitrary. Surveying the live deposits:
nm000119 27901941 no subject in the path
nm000193 sub-A/eeg/... letters, subject 1 -> A
nm000273 session1/s1/sess01_subj01_... split across components
nm000236 subject_01_PC.mat, subject_01_VR.mat two files per subject
nm000311 sub1/... nm000339 S10_Session_1.mat
nm000348 "2C dataset"/sub-001/... space in the directory
No `{subject}` template spans that, and for nm000119 none could exist.
Worse, the template put the burden on 141 hand-written globs, each one a
chance to silently fetch the wrong subset -- the same silent-wrongness
that made partial deposits so expensive to find earlier in this project.
Selection is now data-driven. `sourcedata_path(subject=...)` fetches the
deposit's sourcedata_provenance.json (a few KB), reads which files that
subject owns, and passes those exact paths to nemar-py. Nothing is
inferred from the filename, so every layout works, and an unknown
subject is reported with the list of subjects the deposit does carry.
`nemar_sourcedata_include` is removed; `include` stays as an escape
hatch for callers that do know the layout.
Compatibility: no deposit records subjects yet -- the manifests carry
only file/bytes/sha256 -- so the live path is the fallback, which fetches
the whole tree and warns. That is correct today and becomes precise once
the manifests are regenerated (metadata-only, no data re-upload).
`enrich_sourcedata.py` now threads subject ownership through the
unwrap/expand pipeline and records it, so newly enriched deposits carry
it from the start.
Tests cover the three real shapes (letters, nested session dir, two
files per subject), the unknown-subject error, and the no-subject-records
fallback. Two bugs in the first cut were caught by them: `include` became
a list while the emptiness check still passed it to `glob()` as a string,
and that check counted the manifest itself as data, so a deposit
publishing only its provenance would have looked successful.
test_datasets.py::test_download_prefers_nemar still asserted the old kwargs (include=None) after the call changed to subject=1. I had run only test_download.py locally, so CI caught it instead of me. test_sourcedata_falls_back_to_the_whole_tree_without_subject_records passed alone but failed alongside test_datasets.py: the fallback used mne.utils.warn, which emits a given warning once per session, so whichever test ran second saw nothing. Switched to warnings.warn, which is also what base.py already uses for the sibling upstream fallback -- an assertion on a warning should not depend on test order.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
NEMAR deposits now republish the original, pre-BIDS distribution under
sourcedata/— byte-identical to what the upstream host serves, under the same upstream filenames, with asourcedata_provenance.jsonrecording each file's name, size and SHA-256. That makes NEMAR usable as a mirror of the original source, which matters because those upstream hosts are variously slow, rate-limited, bot-gated, or retired.download()now fetches that tree instead of the deposit's BIDS copy.Why
sourcedata/and not the BIDS copyThis is the crux, so it is worth being explicit. The BIDS copy is a re-encoding, and substituting it would change results, not just change where the bytes come from:
Weibo2014returns{"0": {"0": raw}}from one concatenatedRawArray;Lee2019uses semantic"1train"/"4test"labels that have no BIDS equivalent at all.ErpCore2021applies a 26 ms LCD-delay correction and remaps raw integer codes to"Target"/"NonTarget"viahandle_events_reading. A BIDS-loaded raw carries the unmapped annotations;events_from_annotationsthen finds nothing, andRawToEvents._find_eventsswallows exactly that as zero events, so the run is dropped as empty — no exception, so no fallback can catch it. A user would get benchmark numbers computed on an empty or partial dataset.sourcedata/sidesteps all of it: each dataset's own parser runs on the same files it always ran on. Loading is untouched —get_datastill goes through_get_single_subject_data.What is here
nemar_sourcedata_dlfetches a deposit'ssourcedata/, and fails loudly when a deposit publishes none rather than leaving a confusing empty cache (nemar-py "succeeds" with zero matching files).BaseDataset.sourcedata_path— public accessor mirroringdata_path's contract. Optionalnemar_sourcedata_include(e.g."sourcedata/subject_{subject:02d}.*") addresses one subject; without it the tree is fetched once, becausesourcedata/keeps the upstream layout and there is no general subject→file rule to guess.set_download_provider/get_download_provider—auto(default: NEMAR first, upstream fallback),nemar(no fallback: a failure is raised rather than silently reaching for a host the caller opted out of),upstream(never NEMAR). Also settable per run viaMOABB_DOWNLOAD_PROVIDER.sourcedata/, so deposits that were never enriched keep working.Verification
Against live NEMAR:
Cattan2019_PHMD().download()populates.../nm000341/sourcedatawith 18 files and fetches 0 BIDS eeg files. Per-subject filtering with a template fetches exactlysubject_03.mat.moabb/tests/test_download.pypasses, with new tests covering the scope requested,includeforwarding, the no-sourcedata failure, provider precedence, per-subject vs whole-tree fetching, and the upstream fallback.Requires the matching
nemar-pychange (nemarOrg/nemar-py#3) only for the--sourcedataCLI flag; thescope="sourcedata"API this uses already exists in the released version.