diff --git a/doc/changes/devel/12451.bugfix.rst b/doc/changes/devel/12451.bugfix.rst new file mode 100644 index 00000000000..2aca44529f1 --- /dev/null +++ b/doc/changes/devel/12451.bugfix.rst @@ -0,0 +1 @@ +Fix errant redundant use of ``BIDSPath.split`` when writing split raw and epochs data, by `Eric Larson`_. diff --git a/doc/changes/devel/12451.dependency.rst b/doc/changes/devel/12451.dependency.rst new file mode 100644 index 00000000000..8227dd779ad --- /dev/null +++ b/doc/changes/devel/12451.dependency.rst @@ -0,0 +1 @@ +``pytest-harvest`` is no longer used as a test dependency, by `Eric Larson`_. diff --git a/mne/conftest.py b/mne/conftest.py index 17ff64b32f7..fd7d1946843 100644 --- a/mne/conftest.py +++ b/mne/conftest.py @@ -10,6 +10,7 @@ import shutil import sys import warnings +from collections import defaultdict from contextlib import contextmanager from pathlib import Path from textwrap import dedent @@ -900,11 +901,8 @@ def protect_config(): def _test_passed(request): - try: - outcome = request.node.harvest_rep_call - except Exception: - outcome = "passed" - return outcome == "passed" + report = request.node.stash[_phase_report_key] + return "call" in report and report["call"].outcome == "passed" @pytest.fixture() @@ -931,7 +929,6 @@ def brain_gc(request): ignore = set(id(o) for o in gc.get_objects()) yield close_func() - # no need to warn if the test itself failed, pytest-harvest helps us here if not _test_passed(request): return _assert_no_instances(Brain, "after") @@ -960,16 +957,12 @@ def pytest_sessionfinish(session, exitstatus): if n is None: return print("\n") - try: - import pytest_harvest - except ImportError: - print("Module-level timings require pytest-harvest") - return # get the number to print - res = pytest_harvest.get_session_synthesis_dct(session) - files = dict() - for key, val in res.items(): - parts = Path(key.split(":")[0]).parts + files = defaultdict(lambda: 0.0) + for item in session.items: + report = item.stash[_phase_report_key] + dur = sum(x.duration for x in report.values()) + parts = Path(item.nodeid.split(":")[0]).parts # split mne/tests/test_whatever.py into separate categories since these # are essentially submodule-level tests. Keeping just [:3] works, # except for mne/viz where we want level-4 granulatity @@ -978,7 +971,7 @@ def pytest_sessionfinish(session, exitstatus): if not parts[-1].endswith(".py"): parts = parts + ("",) file_key = "/".join(parts) - files[file_key] = files.get(file_key, 0) + val["pytest_duration_s"] + files[file_key] += dur files = sorted(list(files.items()), key=lambda x: x[1])[::-1] # print _files[:] = files[:n] @@ -999,7 +992,7 @@ def pytest_terminal_summary(terminalreporter, exitstatus, config): writer.line(f"{timing.ljust(15)}{name}") -def pytest_report_header(config, startdir): +def pytest_report_header(config, startdir=None): """Add information to the pytest run header.""" return f"MNE {mne.__version__} -- {str(Path(mne.__file__).parent)}" @@ -1122,7 +1115,6 @@ def run(nbexec=nbexec, code=code): return -@pytest.mark.filterwarnings("ignore:.*Extraction of measurement.*:") @pytest.fixture( params=( [nirsport2, nirsport2_snirf, testing._pytest_param()], @@ -1160,8 +1152,7 @@ def qt_windows_closed(request): if "allow_unclosed_pyside2" in marks and API_NAME.lower() == "pyside2": return # Don't check when the test fails - report = request.node.stash[_phase_report_key] - if ("call" not in report) or report["call"].failed: + if not _test_passed(request): return widgets = app.topLevelWidgets() n_after = len(widgets) diff --git a/mne/datasets/__init__.pyi b/mne/datasets/__init__.pyi index 22cb6acce7b..44cee84fe7f 100644 --- a/mne/datasets/__init__.pyi +++ b/mne/datasets/__init__.pyi @@ -66,7 +66,7 @@ from . import ( ) from ._fetch import fetch_dataset from ._fsaverage.base import fetch_fsaverage -from ._infant.base import fetch_infant_template +from ._infant import fetch_infant_template from ._phantom.base import fetch_phantom from .utils import ( _download_all_example_data, diff --git a/mne/datasets/_infant/__init__.py b/mne/datasets/_infant/__init__.py new file mode 100644 index 00000000000..7347d36fcd0 --- /dev/null +++ b/mne/datasets/_infant/__init__.py @@ -0,0 +1 @@ +from .base import fetch_infant_template diff --git a/mne/epochs.py b/mne/epochs.py index 7da05dbd045..7b87dee5179 100644 --- a/mne/epochs.py +++ b/mne/epochs.py @@ -2212,7 +2212,14 @@ def save( ) # check for file existence and expand `~` if present - fname = str(_check_fname(fname=fname, overwrite=overwrite)) + fname = str( + _check_fname( + fname=fname, + overwrite=overwrite, + check_bids_split=True, + name="fname", + ) + ) split_size_bytes = _get_split_size(split_size) diff --git a/mne/fixes.py b/mne/fixes.py index 4759366f386..55e56261866 100644 --- a/mne/fixes.py +++ b/mne/fixes.py @@ -31,9 +31,8 @@ ############################################################################### # distutils -# distutils has been deprecated since Python 3.10 and is scheduled for removal -# from the standard library with the release of Python 3.12. For version -# comparisons, we use setuptools's `parse_version` if available. +# distutils has been deprecated since Python 3.10 and was removed +# from the standard library with the release of Python 3.12. def _compare_version(version_a, operator, version_b): diff --git a/mne/io/base.py b/mne/io/base.py index 4fe7975e1cd..99a8e658fc4 100644 --- a/mne/io/base.py +++ b/mne/io/base.py @@ -1694,7 +1694,13 @@ def save( endings_err = (".fif", ".fif.gz") # convert to str, check for overwrite a few lines later - fname = _check_fname(fname, overwrite=True, verbose="error") + fname = _check_fname( + fname, + overwrite=True, + verbose="error", + check_bids_split=True, + name="fname", + ) check_fname(fname, "raw", endings, endings_err=endings_err) split_size = _get_split_size(split_size) diff --git a/mne/io/fiff/tests/test_raw_fiff.py b/mne/io/fiff/tests/test_raw_fiff.py index cb1626a4ef7..a28844eb5f5 100644 --- a/mne/io/fiff/tests/test_raw_fiff.py +++ b/mne/io/fiff/tests/test_raw_fiff.py @@ -675,6 +675,34 @@ def test_split_files(tmp_path, mod, monkeypatch): assert not fname_3.is_file() +def test_bids_split_files(tmp_path): + """Test that BIDS split files are written safely.""" + mne_bids = pytest.importorskip("mne_bids") + bids_path = mne_bids.BIDSPath( + root=tmp_path, + subject="01", + datatype="meg", + split="01", + suffix="raw", + extension=".fif", + check=False, + ) + (tmp_path / "sub-01" / "meg").mkdir(parents=True) + raw = read_raw_fif(test_fif_fname) + save_kwargs = dict( + buffer_size_sec=1.0, split_size="10MB", split_naming="bids", verbose=True + ) + with pytest.raises(ValueError, match="Passing a BIDSPath"): + raw.save(bids_path, **save_kwargs) + bids_path.split = None + want_paths = [Path(bids_path.copy().update(split=ii).fpath) for ii in range(1, 3)] + for want_path in want_paths: + assert not want_path.is_file() + raw.save(bids_path, **save_kwargs) + for want_path in want_paths: + assert want_path.is_file() + + def _err(*args, **kwargs): raise RuntimeError("Killed mid-write") diff --git a/mne/tests/test_epochs.py b/mne/tests/test_epochs.py index 015974e89cc..fdd91fd96a0 100644 --- a/mne/tests/test_epochs.py +++ b/mne/tests/test_epochs.py @@ -1666,43 +1666,79 @@ def test_split_saving_and_loading_back(tmp_path, epochs_to_split, preload): @pytest.mark.parametrize( - "split_naming, dst_fname, split_fname_fn", + "split_naming, dst_fname, split_fname_fn, check_bids", [ ( "neuromag", "test_epo.fif", lambda i: f"test_epo-{i}.fif" if i else "test_epo.fif", + False, ), ( "bids", - "test_epo.fif", - lambda i: f"test_split-{i + 1:02d}_epo.fif", + Path("sub-01") / "meg" / "sub-01_epo.fif", + lambda i: Path("sub-01") / "meg" / f"sub-01_split-{i + 1:02d}_epo.fif", + True, ), ( "bids", "a_b-epo.fif", # Merely stating the fact: lambda i: f"a_split-{i + 1:02d}_b-epo.fif", + False, ), ], ids=["neuromag", "bids", "mix"], ) def test_split_naming( - tmp_path, epochs_to_split, split_naming, dst_fname, split_fname_fn + tmp_path, epochs_to_split, split_naming, dst_fname, split_fname_fn, check_bids ): """Test naming of the split files.""" epochs, split_size, n_files = epochs_to_split dst_fpath = tmp_path / dst_fname save_kwargs = {"split_size": split_size, "split_naming": split_naming} # we don't test for reserved files as it's not implemented here + if dst_fpath.parent != tmp_path: + dst_fpath.parent.mkdir(parents=True) epochs.save(dst_fpath, verbose=True, **save_kwargs) # check that the filenames match the intended pattern - assert len(list(tmp_path.iterdir())) == n_files - for i in range(n_files): - assert (tmp_path / split_fname_fn(i)).is_file() + assert len(list(dst_fpath.parent.iterdir())) == n_files assert not (tmp_path / split_fname_fn(n_files)).is_file() + want_paths = [tmp_path / split_fname_fn(i) for i in range(n_files)] + for want_path in want_paths: + assert want_path.is_file() + + if not check_bids: + return + # gh-12451 + # If we load sub-01_split-01_epo.fif we should then we shouldn't + # write sub-01_split-01_split-01_epo.fif + mne_bids = pytest.importorskip("mne_bids") + # Let's try to prevent people from making a mistake + bids_path = mne_bids.BIDSPath( + root=tmp_path, + subject="01", + datatype="meg", + split="01", + suffix="epo", + extension=".fif", + check=False, + ) + assert bids_path.fpath.is_file(), bids_path.fpath + for want_path in want_paths: + want_path.unlink() + assert not bids_path.fpath.is_file() + with pytest.raises(ValueError, match="Passing a BIDSPath"): + epochs.save(bids_path, verbose=True, **save_kwargs) + bad_path = bids_path.fpath.parent / (bids_path.fpath.stem[:-3] + "split-01_epo.fif") + assert str(bad_path).count("_split-01") == 2 + assert not bad_path.is_file(), bad_path + bids_path.split = None + epochs.save(bids_path, verbose=True, **save_kwargs) + for want_path in want_paths: + assert want_path.is_file() @pytest.mark.parametrize( diff --git a/mne/utils/check.py b/mne/utils/check.py index 13eca1e0ba0..e73faf0a2e3 100644 --- a/mne/utils/check.py +++ b/mne/utils/check.py @@ -230,11 +230,29 @@ def _check_fname( name="File", need_dir=False, *, + check_bids_split=False, verbose=None, ): """Check for file existence, and return its absolute path.""" _validate_type(fname, "path-like", name) - fname = Path(fname).expanduser().absolute() + # special case for MNE-BIDS, check split + fname_path = Path(fname) + if check_bids_split: + try: + from mne_bids import BIDSPath + except Exception: + pass + else: + if isinstance(fname, BIDSPath) and fname.split is not None: + raise ValueError( + f"Passing a BIDSPath {name} with `{fname.split=}` is unsafe as it " + "can unexpectedly lead to invalid BIDS split naming. Explicitly " + f"set `{name}.split = None` to avoid ambiguity. If you want the " + f"old misleading split naming, you can pass `str({name})`." + ) + + fname = fname_path.expanduser().absolute() + del fname_path if fname.exists(): if not overwrite: diff --git a/pyproject.toml b/pyproject.toml index 39f2d2ee32b..d82518aa70e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -113,7 +113,6 @@ test = [ "pytest>=8.0.0rc2", "pytest-cov", "pytest-timeout", - "pytest-harvest", "pytest-qt", "ruff", "numpydoc", @@ -138,6 +137,7 @@ test_extra = [ "imageio-ffmpeg>=0.4.1", "snirf", "neo", + "mne-bids", ] # Dependencies for building the docuemntation