diff --git a/docs/source/whats_new.rst b/docs/source/whats_new.rst index 08418d41e..420a733a1 100644 --- a/docs/source/whats_new.rst +++ b/docs/source/whats_new.rst @@ -51,6 +51,7 @@ Requirements Bugs ~~~~ +- Fix datasets ignoring a change of download directory: :func:`moabb.utils.set_download_dir` now realigns every per-dataset ``MNE_DATASETS__PATH`` config entry that still mirrors the previous ``MNE_DATA`` value, so changing the location in the MNE configuration is honoured by all datasets instead of reverting to the old folder (leaving keys the user configured elsewhere untouched). Adds an integration test that iterates over every dataset with mocked path resolution to verify the recovery (:gh:`1115` by `Bruno Aristimunha`_). - Add a ``__repr__`` to :class:`moabb.datasets.base.BaseDataset` so datasets display by their code (e.g. ``BNCI2014-001``) when printed, instead of the verbose default ``<...object at 0x...>``. This declutters the output of ``print(paradigm.datasets)`` in the tutorials and of the paradigm and evaluation compatibility warnings (by `Danae`_) - Add ``age_median`` field to :class:`moabb.datasets.metadata.schema.ParticipantMetadata` and populate ``age_std`` / ``age_median`` / ``n_blocks`` metadata for :class:`moabb.datasets.Rodrigues2017` (Alphawaves), fixing a ``TypeError`` at import time (by `Grace Xu`_) - Fix :class:`moabb.datasets.BNCI2014_001` descriptive ``METADATA``, which had many fields copied from BCI Competition IV Data set 1. Correct ``n_subjects`` (4 → 9), ``n_classes`` (2 → 4), ``class_labels`` / ``events`` / ``imagery_tasks`` (now ``left_hand`` / ``right_hand`` / ``feet`` / ``tongue``), ``synchronicity`` (asynchronous → synchronous), ``sessions_per_subject`` (1 → 2), the per-session trial structure (288 trials, 6 runs of 48), the acquisition ``reference`` / ``ground`` / ``filters`` and the ``preprocessing`` band (0.5–100 Hz with a 50 Hz notch, no 100 Hz downsampling), and the dataset ``description`` so they all match the dataset's own constructor and docstring (by `YG-paaleee`_) diff --git a/moabb/tests/test_download.py b/moabb/tests/test_download.py index ef69223c2..89ed16c38 100644 --- a/moabb/tests/test_download.py +++ b/moabb/tests/test_download.py @@ -1,12 +1,26 @@ """Tests to ensure that datasets download correctly using pytest.""" +import importlib import inspect +import sys import mne import pytest +from mne import get_config, set_config +import moabb.datasets.download as dl from moabb.datasets.bbci_eeg_fnirs import BaseShin2017 from moabb.datasets.utils import dataset_list +from moabb.utils import set_download_dir + + +# Datasets that legitimately do not resolve their storage location through the +# shared ``get_dataset_path``/``MNE_DATA`` mechanism, so the config-change +# recovery guarantee does not apply to them: +# * the fake datasets generate synthetic data and never download anything; +# * ``RomaniBF2025ERP`` manages its own ``data_folder`` (temporary directory +# or user-provided path) instead of the MNE config path system. +_PATH_RECOVERY_EXEMPT = {"FakeDataset", "FakeVirtualRealityDataset", "RomaniBF2025ERP"} def _get_events(raw): @@ -114,3 +128,137 @@ def test_dataset_download(dl_data, dataset): assert len(events) != 0, ( f"No events found in run {run} of session {session}." ) + + +class _ProbeStop(Exception): + """Sentinel raised to abort a download once the storage path is resolved.""" + + def __init__(self, sign, resolved): + super().__init__(sign) + self.sign = sign + self.resolved = resolved + + +@pytest.fixture +def _restore_mne_config(): + """Isolate the MNE config touched by the path tests. + + Snapshot the current config, clear any pre-existing + ``MNE_DATASETS_*_PATH`` entries so each dataset starts from a clean + "first access" state, then restore everything afterwards. + """ + before = dict(get_config() or {}) + for key in list(before): + if key.startswith("MNE_DATASETS_") and key.endswith("_PATH"): + set_config(key, None) + try: + yield + finally: + after = dict(get_config() or {}) + # Reset keys that were added and restore keys that were modified. + for key in set(after) | set(before): + if before.get(key) != after.get(key): + set_config(key, before.get(key)) + + +def _install_path_probe(monkeypatch, dataset_class): + """Replace ``get_dataset_path`` so it records the sign and aborts. + + The real ``get_dataset_path`` is still invoked (so the per-dataset config + key is persisted exactly as in production) but a :class:`_ProbeStop` is + raised immediately afterwards to avoid any network access. + """ + real = dl.get_dataset_path + + def probe(sign, path): + resolved = real(sign, path) + raise _ProbeStop(sign, str(resolved)) + + monkeypatch.setattr(dl, "get_dataset_path", probe) + # Datasets that used ``from .download import get_dataset_path`` hold their + # own reference to the original function, so patch those bindings too. + for cls in dataset_list: + module = sys.modules.get(cls.__module__) or importlib.import_module( + cls.__module__ + ) + if getattr(module, "get_dataset_path", None) is real: + monkeypatch.setattr(module, "get_dataset_path", probe, raising=False) + + +def _resolve_dataset_path(dataset_class, download_dir): + """Resolve a dataset's storage location for the given download directory.""" + set_download_dir(str(download_dir)) + kwargs = {} + if "accept" in inspect.signature(dataset_class).parameters: + kwargs["accept"] = True + dataset = dataset_class(**kwargs) + subject = dataset.subject_list[0] + + # Fill any required positional parameters that come before ``path`` in the + # ``data_path`` signature (e.g. ``session`` or ``paradigm_type``) so that we + # reach the path-resolution call for datasets with custom signatures. + extra_args = [] + for name in inspect.signature(dataset.data_path).parameters: + if name in ("self", "subject"): + continue + if name == "path": + break + # ``session``-like parameters need an integer (they are often used in + # f-strings before the path is resolved); anything else only appears + # after the probe fires, so an empty string is a safe placeholder. + extra_args.append(1 if "session" in name else "") + + try: + dataset.data_path(subject, *extra_args) + except _ProbeStop as stop: + return stop.resolved + return None + + +@pytest.mark.parametrize( + "dataset_class", + [pytest.param(dataset, id=dataset.__name__) for dataset in dataset_list], +) +def test_download_dir_change_is_respected( + dataset_class, tmp_path, monkeypatch, _restore_mne_config +): + """A change of download directory must be honoured by every dataset. + + ``get_dataset_path`` persists a ``MNE_DATASETS__PATH`` config entry the + first time a dataset is accessed. If that entry is not refreshed when the + download directory changes, the dataset keeps pointing at the old location + (see issue #1115). This integration test iterates over all datasets, mocks + the download mechanism, and asserts that switching the download directory + moves the resolved storage location instead of reverting to the old one. + """ + if dataset_class.__name__ in _PATH_RECOVERY_EXEMPT: + pytest.skip( + f"{dataset_class.__name__} does not use the shared MNE_DATA path mechanism." + ) + + _install_path_probe(monkeypatch, dataset_class) + + old_dir = tmp_path / "old" + new_dir = tmp_path / "new" + old_dir.mkdir() + new_dir.mkdir() + + resolved_old = _resolve_dataset_path(dataset_class, old_dir) + if resolved_old is None: + pytest.skip( + f"{dataset_class.__name__} does not resolve its path through " + "get_dataset_path with a subject-only call." + ) + resolved_new = _resolve_dataset_path(dataset_class, new_dir) + + assert resolved_old.startswith(str(old_dir)), ( + f"{dataset_class.__name__} did not resolve under the configured directory." + ) + assert resolved_new is not None + assert resolved_new.startswith(str(new_dir)), ( + f"{dataset_class.__name__} did not follow the new download directory; " + f"resolved {resolved_new!r} instead of a path under {new_dir}." + ) + assert not resolved_new.startswith(str(old_dir)), ( + f"{dataset_class.__name__} reverted to the old download directory." + ) diff --git a/moabb/tests/util_tests.py b/moabb/tests/util_tests.py index 9b66c1aa9..147cd11f9 100644 --- a/moabb/tests/util_tests.py +++ b/moabb/tests/util_tests.py @@ -270,5 +270,45 @@ def worker(p): assert mne_data_value == str(path) +def test_set_download_dir_propagates_to_dataset_keys(tmp_path): + """Changing the download dir must update per-dataset MNE config keys. + + ``get_dataset_path`` persists a ``MNE_DATASETS__PATH`` entry mirroring + ``MNE_DATA`` the first time a dataset is accessed. Changing the download + directory afterwards must realign those entries, otherwise datasets keep + pointing at the old location (see issue #1115). + """ + keys = ( + "MNE_DATA", + "MNE_DATASETS_WEIBO_PATH", + "MNE_DATASETS_BBCIFNIRS_PATH", + "MNE_DATASETS_CUSTOM_PATH", + ) + original = {key: get_config(key) for key in keys} + try: + old_dir = tmp_path / "old" + new_dir = tmp_path / "new" + old_dir.mkdir() + + set_download_dir(str(old_dir)) + # Simulate two datasets that mirrored MNE_DATA on first access ... + set_config("MNE_DATASETS_WEIBO_PATH", str(old_dir)) + set_config("MNE_DATASETS_BBCIFNIRS_PATH", str(old_dir)) + # ... and a key that the user deliberately configured elsewhere. + custom = str(tmp_path / "custom") + set_config("MNE_DATASETS_CUSTOM_PATH", custom) + + set_download_dir(str(new_dir)) + + assert get_config("MNE_DATA") == str(new_dir) + assert get_config("MNE_DATASETS_WEIBO_PATH") == str(new_dir) + assert get_config("MNE_DATASETS_BBCIFNIRS_PATH") == str(new_dir) + # Independently-configured keys must be left untouched. + assert get_config("MNE_DATASETS_CUSTOM_PATH") == custom + finally: + for key, value in original.items(): + set_config(key, value) + + if __name__ == "__main__": unittest.main() diff --git a/moabb/utils.py b/moabb/utils.py index 218d98f7c..00de533e0 100644 --- a/moabb/utils.py +++ b/moabb/utils.py @@ -183,6 +183,29 @@ def wrapper(*args, **kwargs): return wrapper +def _propagate_download_dir(old_path, new_path): + """Update per-dataset MNE config keys after changing ``MNE_DATA``. + + ``moabb.datasets.download.get_dataset_path`` persists a per-dataset + ``MNE_DATASETS__PATH`` config entry (mirroring ``MNE_DATA``) the first + time a dataset is accessed. These entries are otherwise never refreshed, so + changing the download directory would leave datasets pointing at the old + location. Here we realign every per-dataset key that still mirrors the old + ``MNE_DATA`` value so that a configuration change is honoured by all + datasets, while leaving keys the user configured to a different location + untouched. + """ + if new_path is None or old_path == new_path: + return + for key, value in (get_config() or {}).items(): + if ( + key.startswith("MNE_DATASETS_") + and key.endswith("_PATH") + and value == old_path + ): + set_config(key, new_path) + + def set_download_dir(path): """Set the download directory if required to change from default mne path. @@ -194,6 +217,7 @@ def set_download_dir(path): a warning is raised and the storage location is set to the MNE default directory. """ + old_path = get_config("MNE_DATA") if path is None: if get_config("MNE_DATA") is None: log.info( @@ -204,13 +228,16 @@ def set_download_dir(path): "already downloaded, please move manually to this location" ) - set_config("MNE_DATA", osp.join(osp.expanduser("~"), "mne_data")) + new_path = osp.join(osp.expanduser("~"), "mne_data") + set_config("MNE_DATA", new_path) + _propagate_download_dir(old_path, new_path) else: # Check if the path exists, if not, create it if not osp.isdir(path): log.info("The path given does not exist, creating it..") - os.makedirs(path) + os.makedirs(path, exist_ok=True) set_config("MNE_DATA", path) + _propagate_download_dir(old_path, path) def make_process_pipelines(