BUG: Fix bug with BIDS split saving

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`_.

doc/changes/devel/12451.dependency.rst

@@ -0,0 +1 @@
+``pytest-harvest`` is no longer used as a test dependency, by `Eric Larson`_.

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)

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,

mne/datasets/_infant/__init__.py

@@ -0,0 +1 @@
+from .base import fetch_infant_template

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)
 

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):

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)

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")
 

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(

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:

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