Skip to content

Honor the download-flag contract across all datasets and harden the NEMAR sourcedata path - #1146

Open
bruAristimunha wants to merge 2 commits into
developfrom
fix/download-flags-nemar
Open

Honor the download-flag contract across all datasets and harden the NEMAR sourcedata path#1146
bruAristimunha wants to merge 2 commits into
developfrom
fix/download-flags-nemar

Conversation

@bruAristimunha

Copy link
Copy Markdown
Collaborator

What

Two related fix-sets, landed together because they share the same download layer (datasets/base.py, datasets/download.py):

1. The download-flag contract — enforced for every dataset

An AST audit of every data_path implementation found flags accepted but silently ignored across the catalogue:

  • CacheConfig.overwrite_* was a no-op with the default use=Falseget_data(cache_config=dict(overwrite_raw=True)) erased nothing. The docstring also claimed save_* was ignored in that case while the code honored it. Overwrite now always erases; docs match behavior.
  • CompoundDataset.data_path dropped all four flags when delegating to the wrapped dataset.
  • 22 dataset data_path implementations ignored path and/or force_update (full list in the commit message) — including every recent dataset that resolved its storage dir via get_dataset_path(SIGN, None) with the user's path hardcoded away.
  • Kojima2024a/b narrowed the data_path signature to (subject, path), so BaseDataset.download() raised TypeError: unexpected keyword argument 'force_update'.
  • force_update now also re-extracts stale archives; download_if_missing gains a force_update parameter as the choke point for the manifest-based datasets (dreyer2023, erpcore2021, kojima2024a/b, mainsah2025).
  • update_path is left as documented-deprecated (download.py: "Unused, kept for compatibility purpose").

Guard rails: a new AST regression test fails CI if any dataset's data_path ever ignores path or force_update again (exempt: the abstract base and FakeDataset), and a new cache test proves overwrite_raw=True, use=False actually erases.

2. NEMAR sourcedata hardening (follow-ups to #1139)

  • Per-subject upstream fallback: one failing subject no longer discards the whole NEMAR batch — only the failed subjects fall back to data_path, keeping what NEMAR already served. The now-unused batch helper _download_nemar_sourcedata is removed.
  • sourcedata_path resolves subjects through nemar_subject_template, matching the sibling BIDS path (_download_nemar). Datasets like ErpCore2021 ({subject:03d}) previously sent the raw int to the provenance matcher.
  • Honest error taxonomy: only SelectionError maps to "deposit publishes no sourcedata"; transport/verification/S3 failures now report as download failures with the original error text.
  • Glob safety: manifest filenames are glob.escaped before being used as nemar include patterns and local verification globs, so upstream names containing [ ] * ? still match.
  • Docs: the changelog's nemar_sourcedata_include attribute (never implemented) is replaced with the real provenance mechanism; nemar_dl/nemar_sourcedata_dl are registered in api.rst; docstrings no longer promise a NemarDownloadError for a missing nemar-py (that raises ImportError at import time).
  • Direct tests for set_download_provider/get_download_provider (previously only exercised through monkeypatched seams): validation, case normalization, None reset, env-var precedence, unknown-value fallback — using the isolated MNE config fixture so nothing touches the real ~/.mne config.

Not in scope (deliberate, candidates for follow-ups)

  • A per-call provider= override / context manager instead of the persisted global (set_download_provider still writes the MNE config).
  • Lazy import nemar (today import moabb requires nemar-py even with provider="upstream").
  • Wiring the NEMAR sourcedata cache into data_path/get_data so a prefetch warms the evaluation path — needs a design decision on cache-location mapping.
  • verbose in the pooch-based downloaders (progress bars are hardcoded; no slot to honor it).

Test plan

  • pytest moabb/tests/test_download.py moabb/tests/test_datasets.py -k 'not doi' → 1464 passed
  • pytest moabb/tests/test_paradigms.py → 102 passed; pytest moabb/tests/test_evaluations.py → 93 passed
  • AST audit sweep → every dataset honors path + force_update
  • pre-commit run --all-files → clean

…urcedata path

Flag contract:
- CacheConfig.overwrite_* now erases the cache even when use=False
  (previously a silent no-op with the default config); docstring and
  make() examples corrected to match behavior.
- CompoundDataset.data_path forwards path/force_update/update_path/verbose
  to the wrapped dataset instead of dropping all four.
- data_path implementations that ignored path or force_update now honor
  them: Weibo2014, alphawaves, bbci_eeg_fnirs (Shin2017 helpers), beetl,
  chailloux2020, dreyer2023, epfl, erpcore2021, hinss2021, huebner_llp,
  kaneshiro2015, lee2024, liu2024, mainsah2025, phmd_ml, physionet_mi,
  sosulski2019, speier2017, ssvep_mamem, stieger2021, zhang2025, zheng2020.
- Kojima2024a/b data_path regains the standard signature; its narrowed
  form made BaseDataset.download() raise TypeError.
- force_update also re-extracts stale zip folders; download_if_missing
  gains a force_update parameter as the choke point for manifest-based
  datasets.
- New AST regression test enforces that every dataset's data_path uses
  path and force_update; new test proves overwrite works with use=False.

NEMAR sourcedata:
- download() falls back to the upstream host per failing subject instead
  of discarding the whole NEMAR batch; the unused batch helper is removed.
- sourcedata_path resolves subjects through nemar_subject_template,
  matching the sibling BIDS path.
- Transport/verification failures are no longer misreported as 'deposit
  publishes no sourcedata'; SelectionError keeps that message.
- Manifest filenames are glob-escaped before being used as include
  patterns and local verification globs.
- Stale nemar_sourcedata_include changelog claim replaced with the real
  provenance mechanism; nemar_dl/nemar_sourcedata_dl added to api.rst;
  docstrings no longer promise a NemarDownloadError for a missing
  nemar-py (that raises ImportError at import time).
- Direct round-trip tests for set/get_download_provider (validation,
  normalization, None reset, env-var precedence, unknown-value fallback)
  and a per-subject fallback test.
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

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.

1 participant