diff --git a/dandi/tests/test_upload.py b/dandi/tests/test_upload.py index ccf6371e6..12e533202 100644 --- a/dandi/tests/test_upload.py +++ b/dandi/tests/test_upload.py @@ -314,6 +314,85 @@ def test_upload_bids_non_nwb_file(bids_dandiset: SampleDandiset) -> None: assert [asset.path for asset in bids_dandiset.dandiset.get_assets()] == ["README"] +@pytest.mark.ai_generated +def test_upload_warns_for_unrecognized_paths( + caplog: pytest.LogCaptureFixture, + new_dandiset: SampleDandiset, + simple2_nwb: Path, +) -> None: + subject_dir = new_dandiset.dspath / "sub-01" + subject_dir.mkdir() + copyfile(simple2_nwb, subject_dir / "sub-01.nwb") + (new_dandiset.dspath / "sidecar.json").write_text("{}") + (new_dandiset.dspath / "notes").mkdir() + (new_dandiset.dspath / "notes" / "readme.txt").write_text("notes") + + with caplog.at_level("WARNING", logger="dandi"): + new_dandiset.upload() + + (record,) = [ + r for r in caplog.records if "not recognized as DANDI assets" in r.message + ] + assert isinstance(record.args, tuple) + assert record.args[-1] == "notes, sidecar.json" + + +@pytest.mark.ai_generated +def test_upload_partial_does_not_warn_for_unrequested_paths( + caplog: pytest.LogCaptureFixture, + new_dandiset: SampleDandiset, + simple2_nwb: Path, +) -> None: + subject_dir = new_dandiset.dspath / "sub-01" + subject_dir.mkdir() + nwb_path = subject_dir / "sub-01.nwb" + copyfile(simple2_nwb, nwb_path) + (new_dandiset.dspath / "sidecar.json").write_text("{}") + + with caplog.at_level("WARNING", logger="dandi"): + new_dandiset.upload(paths=[nwb_path]) + + assert "not recognized as DANDI assets" not in caplog.text + + +@pytest.mark.ai_generated +def test_upload_allow_any_path_suppresses_omission_warning( + caplog: pytest.LogCaptureFixture, text_dandiset: SampleDandiset +) -> None: + with caplog.at_level("WARNING", logger="dandi"): + text_dandiset.upload() + + assert "not recognized as DANDI assets" not in caplog.text + + +@pytest.mark.ai_generated +def test_upload_omission_warning_survives_upload_error( + caplog: pytest.LogCaptureFixture, + mocker: MockerFixture, + new_dandiset: SampleDandiset, + simple2_nwb: Path, +) -> None: + subject_dir = new_dandiset.dspath / "sub-01" + subject_dir.mkdir() + copyfile(simple2_nwb, subject_dir / "sub-01.nwb") + (new_dandiset.dspath / "sidecar.json").write_text("{}") + mocker.patch.object( + LocalFileAsset, "iter_upload", side_effect=UploadError("upload failed") + ) + + with ( + caplog.at_level("WARNING", logger="dandi"), + pytest.raises(UploadError, match="upload failed"), + ): + new_dandiset.upload() + + (record,) = [ + r for r in caplog.records if "not recognized as DANDI assets" in r.message + ] + assert isinstance(record.args, tuple) + assert record.args[-1] == "sidecar.json" + + @sweep_embargo def test_upload_sync_zarr( mocker: MockerFixture, zarr_dandiset: SampleDandiset, embargo: bool diff --git a/dandi/tests/test_upload_discovery.py b/dandi/tests/test_upload_discovery.py new file mode 100644 index 000000000..46d6678ad --- /dev/null +++ b/dandi/tests/test_upload_discovery.py @@ -0,0 +1,108 @@ +from pathlib import Path, PurePosixPath + +import pytest +from pytest_mock import MockerFixture + +from .test_files import mkpaths +from ..dandiset import Dandiset +from ..files import find_dandi_files +from ..upload import _partition_upload_assets, upload + + +@pytest.mark.ai_generated +@pytest.mark.parametrize("allow_any_path", [False, True]) +@pytest.mark.parametrize( + "roots, expected", + [ + (["."], ["Thumbs.db", "mixed/sidecar.json", "notes", "unknown.txt"]), + (["mixed"], ["mixed/sidecar.json"]), + (["notes"], ["notes/nested"]), + (["notes/nested/readme.txt"], ["notes/nested/readme.txt"]), + (["mixed", "mixed/known.nwb"], ["mixed/sidecar.json"]), + (["sample.zarr"], []), + (["empty", ".hidden"], []), + ], +) +def test_partition_matches_discovery( + tmp_path: Path, roots: list[str], expected: list[str], allow_any_path: bool +) -> None: + mkpaths( + tmp_path, + "dandiset.yaml", + "known.nwb", + "unknown.txt", + "Thumbs.db", + "notes/nested/readme.txt", + "mixed/known.nwb", + "mixed/sidecar.json", + "sample.zarr/chunk", + "empty/", + ".hidden/secret.nwb", + "__MACOSX/._known.nwb", + ) + ds = Dandiset(tmp_path) + assets = ds.assets(allow_all=True) + selected, omitted = _partition_upload_assets( + assets, [PurePosixPath(p) for p in roots], allow_any_path + ) + # Compare against the real discovery pipeline, including request scoping. + baseline = ds.assets(allow_all=allow_any_path) + assert sorted(a.path for a in selected) == sorted( + {a.path for root in roots for a in baseline.under_paths([root])} + ) + assert [str(p) for p in omitted] == ([] if allow_any_path else expected) + + +@pytest.mark.ai_generated +def test_wholly_unrecognized_dandiset(tmp_path: Path) -> None: + mkpaths(tmp_path, "dandiset.yaml", "a.txt", "notes/readme.txt") + selected, omitted = _partition_upload_assets( + Dandiset(tmp_path).assets(allow_all=True), [PurePosixPath(".")], False + ) + assert selected == [] + assert omitted == [PurePosixPath("a.txt"), PurePosixPath("notes")] + + +@pytest.mark.ai_generated +def test_bids_assets_stay_recognized(tmp_path: Path) -> None: + mkpaths(tmp_path, "dandiset.yaml", "dataset_description.json", "sidecar.json") + assets = Dandiset(tmp_path).assets(allow_all=True) + selected, omitted = _partition_upload_assets(assets, [PurePosixPath(".")], False) + assert sorted(a.filepath for a in selected) == sorted( + a.filepath for a in find_dandi_files(tmp_path, dandiset_path=tmp_path) + ) + assert omitted == [] + + +@pytest.mark.ai_generated +def test_symlinked_directory_is_not_an_omitted_asset(tmp_path: Path) -> None: + mkpaths(tmp_path, "dandiset.yaml", "target/notes.txt") + try: + (tmp_path / "linked").symlink_to(tmp_path / "target", target_is_directory=True) + except OSError as exc: + pytest.skip(f"Cannot create directory symlink: {exc}") + selected, omitted = _partition_upload_assets( + Dandiset(tmp_path).assets(allow_all=True), [PurePosixPath("linked")], False + ) + assert selected == [] + assert omitted == [] + + +@pytest.mark.ai_generated +@pytest.mark.parametrize("count", [1, 12]) +def test_upload_unknown_only_reports_paths( + tmp_path: Path, mocker: MockerFixture, caplog: pytest.LogCaptureFixture, count: int +) -> None: + mkpaths(tmp_path, "dandiset.yaml", *(f"note-{i:02}.txt" for i in range(count))) + (tmp_path / "dandiset.yaml").write_text("identifier: '000001'\n") + mocker.patch("dandi.upload.DandiAPIClient.for_dandi_instance") + upload([tmp_path]) + (record,) = [ + r for r in caplog.records if "not recognized as DANDI assets" in r.message + ] + expected = ", ".join(f"note-{i:02}.txt" for i in range(min(count, 10))) + if count > 10: + expected += ", ..." + assert isinstance(record.args, tuple) + assert record.args[-1] == expected + assert "note-11.txt" in caplog.text if count > 10 else "note-00.txt" in caplog.text diff --git a/dandi/upload.py b/dandi/upload.py index 6c13aca73..911bfe732 100644 --- a/dandi/upload.py +++ b/dandi/upload.py @@ -17,7 +17,7 @@ from enum import StrEnum import io import os.path -from pathlib import Path +from pathlib import Path, PurePosixPath import re import time from time import sleep @@ -37,11 +37,12 @@ dandiset_metadata_file, ) from .dandiapi import DandiAPIClient, RemoteAsset -from .dandiset import Dandiset +from .dandiset import AssetView, Dandiset from .exceptions import NotFoundError, UploadError, UploadValidationError from .files import ( DandiFile, DandisetMetadataFile, + GenericAsset, LocalAsset, LocalDirectoryAsset, ZarrAsset, @@ -54,6 +55,39 @@ from .validate._types import Severity +def _partition_upload_assets( + assets: AssetView, roots: Sequence[PurePosixPath], allow_any_path: bool +) -> tuple[list[LocalAsset], list[PurePosixPath]]: + """Select uploads and collapse omitted paths using the existing discovery.""" + root_set = set(roots) + roots = [p for p in root_set if not any(a in root_set for a in p.parents)] + selected = [] + omitted = [] + for asset in assets.under_paths(roots): + if type(asset) is GenericAsset and not allow_any_path: + omitted.append(PurePosixPath(asset.path)) + else: + selected.append(asset) + keep = { + ancestor + for asset in selected + for ancestor in (PurePosixPath(asset.path), *PurePosixPath(asset.path).parents) + } + boundaries = set(roots) | {PurePosixPath(".")} + collapsed = set() + for path in omitted: + candidate = path + if path in boundaries: + collapsed.add(path) + continue + for ancestor in path.parents: + if ancestor in boundaries or ancestor in keep: + break + candidate = ancestor + collapsed.add(candidate) + return selected, sorted(collapsed) + + def _check_dandidownload_paths(dfile: DandiFile) -> None: """ Check if an asset contains .dandidownload paths and raise UploadError if found. @@ -235,14 +269,17 @@ def new_super_len(o: Any) -> int: # DO NOT FACTOR OUT THIS VARIABLE! It stores any # BIDSDatasetDescriptionAsset instances for the Dandiset, which need to # remain alive until we're done working with all BIDS assets. - assets = dandiset.assets(allow_all=allow_any_path) + assets = dandiset.assets(allow_all=True) + selected, omitted_paths = _partition_upload_assets( + assets, + [PurePosixPath(Path(p).relative_to(dandiset.path)) for p in paths], + allow_any_path, + ) dandi_files: list[DandiFile] = [] # Build the list step by step so as not to confuse mypy dandi_files.append(dandiset.metadata_file()) - dandi_files.extend( - assets.under_paths(Path(p).relative_to(dandiset.path) for p in paths) - ) + dandi_files.extend(selected) lgr.info(f"Found {len(dandi_files)} files to consider") # We will keep a shared set of "being processed" paths so @@ -461,8 +498,22 @@ def report_validation_failure() -> None: ) lgr.warning(msg) + def report_omitted_paths() -> None: + if not omitted_paths: + return + + relpaths = [path.as_posix() for path in omitted_paths] + lgr.warning( + "%s not uploaded (not recognized as DANDI assets): %s. " + "Review the paths or use --allow-any-path if intentional.", + pluralize(len(relpaths), "path"), + ", ".join(relpaths[:10]) + (", ..." if len(relpaths) > 10 else ""), + ) + lgr.debug("Complete list of paths not uploaded: %s", ", ".join(relpaths)) + with ExitStack() as warning_stack, out: warning_stack.callback(report_validation_failure) + warning_stack.callback(report_omitted_paths) for dfile in dandi_files: while len(process_paths) >= 10: lgr.log(2, "Sleep waiting for some paths to finish processing")