diff --git a/.github/workflows/duplication.yml b/.github/workflows/duplication.yml new file mode 100644 index 000000000..ea79a8e79 --- /dev/null +++ b/.github/workflows/duplication.yml @@ -0,0 +1,32 @@ +name: Duplication check + +on: + - push + - pull_request + +jobs: + duplication: + runs-on: ubuntu-latest + steps: + - name: Check out repository + uses: actions/checkout@v7 + with: + fetch-depth: 0 + + - name: Set up Python + uses: actions/setup-python@v6 + with: + python-version: '3.11' + + - name: Set up Node.js + uses: actions/setup-node@v5 + with: + node-version: '24' + + - name: Install dependencies + run: | + python -m pip install --upgrade pip + python -m pip install --upgrade tox + + - name: Run duplication check + run: tox -e duplication diff --git a/.gitignore b/.gitignore index e2d2dab22..9d282c3ce 100644 --- a/.gitignore +++ b/.gitignore @@ -9,6 +9,7 @@ .mypy_cache/ .pytest_cache/ .ruff_cache/ +.tmp/ .tox/ .venv*/ __pycache__/ diff --git a/.jscpd.json b/.jscpd.json new file mode 100644 index 000000000..0a74fb2cf --- /dev/null +++ b/.jscpd.json @@ -0,0 +1,24 @@ +{ + "threshold": 2, + "reporters": ["console"], + "ignore": [ + "**/.git/**", + "**/.tox/**", + "**/.venv*/**", + "**/venv*/**", + "**/node_modules/**", + "**/__pycache__/**", + "**/.hypothesis/**", + "**/*.egg-info/**", + "**/.eggs/**", + "**/.npm/**", + "**/.tmp/**", + "**/dist/**", + "**/build/**", + "**/tests/data/**", + "**/versioneer.py", + "**/dandi/_version.py" + ], + "minTokens": 50, + "minLines": 5 +} diff --git a/dandi/bids_validator_deno/_validator.py b/dandi/bids_validator_deno/_validator.py index 8c55cb488..a946e6e9f 100644 --- a/dandi/bids_validator_deno/_validator.py +++ b/dandi/bids_validator_deno/_validator.py @@ -206,22 +206,8 @@ def _bids_validate( recursive: bool = False, ) -> BidsValidationResult: """ - Validate a file directory as a BIDS dataset with the deno-compiled BIDS validator - - Parameters - ---------- - dir_ : DirectoryPath - The path to the directory to validate - config : Optional[dict] - The configuration to use in the validation. This specifies a JSON configuration - file to be provided through the `--config` option when invoking the underlying - deno-compiled BIDS validator. If `None`, the deno-compiled BIDS validator will - be invoked without the `--config` option. - ignore_nifti_headers : bool - If `True`, disregard NIfTI header content during validation - recursive : bool - If `True`, validate datasets found in derivatives directories in addition to - root dataset + Implementation of `bids_validate()`. See that function for a description of the + parameters. Returns ------- diff --git a/dandi/cli/cmd_ls.py b/dandi/cli/cmd_ls.py index 1969236ad..8b4b49e1a 100644 --- a/dandi/cli/cmd_ls.py +++ b/dandi/cli/cmd_ls.py @@ -286,17 +286,9 @@ def flatten_v(v): def flatten_meta_to_pyout_v1(meta): - """Given a meta record, possibly flatten record since no nested records - supported yet - - lists become joined using ', ', dicts get individual key: values. - lists of dict - doing nothing magical. - - Empty values are not considered. - - Parameters - ---------- - meta: dict + """Like `flatten_meta_to_pyout`, but nested dicts get flattened + recursively into individual "key: value" entries instead of being + joined into a single string. """ out = {} diff --git a/dandi/cli/tests/test_cmd_validate.py b/dandi/cli/tests/test_cmd_validate.py index 1da5c16ca..178e78f94 100644 --- a/dandi/cli/tests/test_cmd_validate.py +++ b/dandi/cli/tests/test_cmd_validate.py @@ -345,13 +345,7 @@ def test_validate_load_mutual_exclusivity(simple2_nwb: Path, tmp_path: Path) -> assert "mutually exclusive" in r.output -@pytest.mark.ai_generated -@pytest.mark.parametrize( - "grouping", - ["severity", "id", "validator", "standard", "dandiset"], -) -def test_render_text_grouping(grouping: str, capsys: pytest.CaptureFixture) -> None: - """Test extended grouping renders section headers with counts.""" +def _sample_validation_issues() -> tuple[Origin, list[ValidationResult]]: origin = Origin( type=OriginType.VALIDATION, validator=Validator.nwbinspector, @@ -377,6 +371,17 @@ def test_render_text_grouping(grouping: str, capsys: pytest.CaptureFixture) -> N dandiset_path=Path("/data/ds001"), ), ] + return origin, issues + + +@pytest.mark.ai_generated +@pytest.mark.parametrize( + "grouping", + ["severity", "id", "validator", "standard", "dandiset"], +) +def test_render_text_grouping(grouping: str, capsys: pytest.CaptureFixture) -> None: + """Test extended grouping renders section headers with counts.""" + _, issues = _sample_validation_issues() _render_text(issues, grouping=(grouping,)) captured = capsys.readouterr().out @@ -435,30 +440,8 @@ def test_validate_grouping_text_cli( @pytest.mark.ai_generated def test_render_text_multilevel_grouping(capsys: pytest.CaptureFixture) -> None: """Test multi-level grouping renders nested section headers.""" - origin = Origin( - type=OriginType.VALIDATION, - validator=Validator.nwbinspector, - validator_version="", - ) - issues = [ - ValidationResult( - id="NWBI.check_data_orientation", - origin=origin, - scope=Scope.FILE, - message="Data may be in the wrong orientation.", - path=Path("sub-01/sub-01.nwb"), - severity=Severity.WARNING, - dandiset_path=Path("/data/ds001"), - ), - ValidationResult( - id="NWBI.check_missing_unit", - origin=origin, - scope=Scope.FILE, - message="Missing text for attribute 'unit'.", - path=Path("sub-02/sub-02.nwb"), - severity=Severity.WARNING, - dandiset_path=Path("/data/ds001"), - ), + origin, issues = _sample_validation_issues() + issues.append( ValidationResult( id="NWBI.check_data_orientation", origin=origin, @@ -467,8 +450,8 @@ def test_render_text_multilevel_grouping(capsys: pytest.CaptureFixture) -> None: path=Path("sub-03/sub-03.nwb"), severity=Severity.ERROR, dandiset_path=Path("/data/ds001"), - ), - ] + ) + ) _render_text(issues, grouping=("severity", "id")) captured = capsys.readouterr().out diff --git a/dandi/cli/tests/test_digest.py b/dandi/cli/tests/test_digest.py index 9c8fcccbf..00fd01ad0 100644 --- a/dandi/cli/tests/test_digest.py +++ b/dandi/cli/tests/test_digest.py @@ -57,9 +57,9 @@ def test_digest( assert r.output == f"file.txt: {filehash}\n" -def test_digest_zarr(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: - # Expected digest is selected by the Zarr serialisation format that - # ``zarr.save`` actually produced (V2 vs V3 layouts have different digests). +def _make_sample_zarr( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> tuple[CliRunner, str]: runner = CliRunner() monkeypatch.chdir(tmp_path) dt = np.dtype(" None: expected = _EXPECTED_SAMPLE_ZARR_DIGEST_BY_FORMAT[ zarr_format_of(Path("sample.zarr")) ] + return runner, expected + + +def test_digest_zarr(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + # Expected digest is selected by the Zarr serialisation format that + # ``zarr.save`` actually produced (V2 vs V3 layouts have different digests). + runner, expected = _make_sample_zarr(tmp_path, monkeypatch) r = runner.invoke(digest, ["--digest", "zarr-checksum", "sample.zarr"]) assert r.exit_code == 0 assert r.output == f"sample.zarr: {expected}\n" @@ -87,15 +94,7 @@ def test_digest_zarr_with_excluded_dotfiles( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: # See comment in `test_digest_zarr` regarding V2 vs V3 serialisation. - runner = CliRunner() - monkeypatch.chdir(tmp_path) - dt = np.dtype(" RemoteDandiset: f"No published versions found for Dandiset {self.identifier}" ) + def _iter_version_assets(self, params: dict) -> Iterator[RemoteAsset]: + try: + for a in self.client.paginate( + f"{self.version_api_path}assets/", params=params + ): + yield RemoteAsset.from_data(self, a) + except HTTP404Error: + raise NotFoundError( + f"No such version: {self.version_id!r} of Dandiset {self.identifier}" + ) + def get_assets(self, order: str | None = None) -> Iterator[RemoteAsset]: """ Returns an iterator of all assets in this version of the Dandiset. @@ -1416,15 +1435,7 @@ def get_assets(self, order: str | None = None) -> Iterator[RemoteAsset]: ``"created"``, ``"modified"``, and ``"path"``. Prepend a hyphen to the field name to reverse the sort order. """ - try: - for a in self.client.paginate( - f"{self.version_api_path}assets/", params={"order": order} - ): - yield RemoteAsset.from_data(self, a) - except HTTP404Error: - raise NotFoundError( - f"No such version: {self.version_id!r} of Dandiset {self.identifier}" - ) + return self._iter_version_assets({"order": order}) def get_asset(self, asset_id: str) -> RemoteAsset: """ @@ -1453,16 +1464,9 @@ def get_assets_with_path_prefix( ``"created"``, ``"modified"``, and ``"path"``. Prepend a hyphen to the field name to reverse the sort order. """ - try: - for a in self.client.paginate( - f"{self.version_api_path}assets/", - params={"path": self._normalize_path(path), "order": order}, - ): - yield RemoteAsset.from_data(self, a) - except HTTP404Error: - raise NotFoundError( - f"No such version: {self.version_id!r} of Dandiset {self.identifier}" - ) + return self._iter_version_assets( + {"path": self._normalize_path(path), "order": order} + ) def get_assets_by_glob( self, pattern: str, order: str | None = None @@ -1478,16 +1482,7 @@ def get_assets_by_glob( ``"created"``, ``"modified"``, and ``"path"``. Prepend a hyphen to the field name to reverse the sort order. """ - try: - for a in self.client.paginate( - f"{self.version_api_path}assets/", - params={"glob": pattern, "order": order}, - ): - yield RemoteAsset.from_data(self, a) - except HTTP404Error: - raise NotFoundError( - f"No such version: {self.version_id!r} of Dandiset {self.identifier}" - ) + return self._iter_version_assets({"glob": pattern, "order": order}) def get_asset_by_path(self, path: str) -> RemoteAsset: """ @@ -1564,15 +1559,19 @@ def upload_raw_asset( :param RemoteAsset replace_asset: If set, replace the given asset, which must have the same path as the new asset """ + df = self._local_asset_file(filepath) + return df.upload( + self, metadata=asset_metadata, jobs=jobs, replacing=replace_asset + ) + + def _local_asset_file(self, filepath: str | Path) -> "LocalAsset": # Avoid circular import by importing within function: from .files import LocalAsset, dandi_file df = dandi_file(filepath) if not isinstance(df, LocalAsset): raise ValueError(f"{filepath}: not an asset file") - return df.upload( - self, metadata=asset_metadata, jobs=jobs, replacing=replace_asset - ) + return df def iter_upload_raw_asset( self, @@ -1601,22 +1600,33 @@ def iter_upload_raw_asset( :param RemoteAsset replace_asset: If set, replace the given asset, which must have the same path as the new asset :returns: - A generator of `dict`\\s containing at least a ``"status"`` key. - Upon successful upload, the last `dict` will have a status of - ``"done"`` and an ``"asset"`` key containing the resulting - `RemoteAsset`. + A generator of `dict`\\s; see `~dandi.files.LocalAsset.iter_upload` + for the shape of the status dicts """ - # Avoid circular import by importing within function: - from .files import LocalAsset, dandi_file - - df = dandi_file(filepath) - if not isinstance(df, LocalAsset): - raise ValueError(f"{filepath}: not an asset file") + df = self._local_asset_file(filepath) return df.iter_upload( self, metadata=asset_metadata, jobs=jobs, replacing=replace_asset ) +def _get_download_response( + session: requests.Session, url: str, start_at: int = 0 +) -> requests.Response: + """ + Issue the (optionally range-restricted) GET request for streaming a + download from ``url``, raising for any HTTP error status. + """ + lgr.debug("Starting download from %s", url) + headers = None + if start_at > 0: + headers = {"Range": f"bytes={start_at}-"} + result = session.get(url, stream=True, headers=headers, timeout=DOWNLOAD_TIMEOUT) + # TODO: apparently we might need retries here as well etc + # if result.status_code not in (200, 201): + result.raise_for_status() + return result + + class BaseRemoteAsset(ABC, APIBase): """ Representation of an asset retrieved from the API without associated @@ -1862,16 +1872,7 @@ def get_download_file_iter( url = self.base_download_url def downloader(start_at: int = 0) -> Iterator[bytes]: - lgr.debug("Starting download from %s", url) - headers = None - if start_at > 0: - headers = {"Range": f"bytes={start_at}-"} - result = self.client.session.get( - url, stream=True, headers=headers, timeout=DOWNLOAD_TIMEOUT - ) - # TODO: apparently we might need retries here as well etc - # if result.status_code not in (200, 201): - result.raise_for_status() + result = _get_download_response(self.client.session, url, start_at) nbytes, nchunks = 0, 0 for chunk in result.iter_content(chunk_size=chunk_size): nchunks += 1 @@ -2143,6 +2144,20 @@ def set_raw_metadata(self, metadata: dict[str, Any]) -> None: """ ... + def _put_raw_metadata( + self, metadata: dict[str, Any], id_field: str, id_value: str + ) -> None: + set_asset_schema_key(metadata) + data = self.client.put( + self.api_path, json={"metadata": metadata, id_field: id_value} + ) + self.identifier = data["asset_id"] + self.path = data["path"] + self.size = int(data["size"]) + self.created = ensure_datetime(data["created"]) + self.modified = ensure_datetime(data["modified"]) + self._metadata = data["metadata"] + def rename(self, dest: str) -> None: """ .. versionadded:: 0.41.0 @@ -2172,16 +2187,7 @@ def set_raw_metadata(self, metadata: dict[str, Any]) -> None: Set the metadata for the asset on the server to the given value and update the `RemoteBlobAsset` in place. """ - set_asset_schema_key(metadata) - data = self.client.put( - self.api_path, json={"metadata": metadata, "blob_id": self.blob} - ) - self.identifier = data["asset_id"] - self.path = data["path"] - self.size = int(data["size"]) - self.created = ensure_datetime(data["created"]) - self.modified = ensure_datetime(data["modified"]) - self._metadata = data["metadata"] + self._put_raw_metadata(metadata, "blob_id", self.blob) class RemoteZarrAsset(RemoteAsset, BaseRemoteZarrAsset): @@ -2196,16 +2202,7 @@ def set_raw_metadata(self, metadata: dict[str, Any]) -> None: Set the metadata for the asset on the server to the given value and update the `RemoteZarrAsset` in place. """ - set_asset_schema_key(metadata) - data = self.client.put( - self.api_path, json={"metadata": metadata, "zarr_id": self.zarr} - ) - self.identifier = data["asset_id"] - self.path = data["path"] - self.size = int(data["size"]) - self.created = ensure_datetime(data["created"]) - self.modified = ensure_datetime(data["modified"]) - self._metadata = data["metadata"] + self._put_raw_metadata(metadata, "zarr_id", self.zarr) @dataclass @@ -2260,42 +2257,21 @@ def name(self) -> str: @property def suffix(self) -> str: """The final file extension of the basename, if any""" - i = self.name.rfind(".") - if 0 < i < len(self.name) - 1: - return self.name[i:] - else: - return "" + return _path_suffix(self.name) @property def suffixes(self) -> list[str]: """A list of the basename's file extensions""" - if self.name.endswith("."): - return [] - name = self.name.lstrip(".") - return ["." + suffix for suffix in name.split(".")[1:]] + return _path_suffixes(self.name) @property def stem(self) -> str: """The basename without its final file extension, if any""" - i = self.name.rfind(".") - if 0 < i < len(self.name) - 1: - return self.name[:i] - else: - return self.name + return _path_stem(self.name) def match(self, pattern: str) -> bool: """Tests whether the path matches the given glob pattern""" - if pattern.startswith("/"): - raise ValueError(f"Absolute paths not allowed: {pattern!r}") - patparts = tuple(q for q in pattern.split("/") if q) - if not patparts: - raise ValueError("Empty pattern") - if len(patparts) > len(self.parts): - return False - for part, pat in zip(reversed(self.parts), reversed(patparts)): - if not fnmatchcase(part, pat): - return False - return True + return _match_parts(self.parts, pattern) @property def download_url(self) -> str: @@ -2320,16 +2296,7 @@ def get_download_file_iter( url = self.download_url def downloader(start_at: int = 0) -> Iterator[bytes]: - lgr.debug("Starting download from %s", url) - headers = None - if start_at > 0: - headers = {"Range": f"bytes={start_at}-"} - result = self.client.session.get( - url, stream=True, headers=headers, timeout=DOWNLOAD_TIMEOUT - ) - # TODO: apparently we might need retries here as well etc - # if result.status_code not in (200, 201): - result.raise_for_status() + result = _get_download_response(self.client.session, url, start_at) for chunk in result.iter_content(chunk_size=chunk_size): if chunk: # could be some "keep alive"? yield chunk diff --git a/dandi/dandiarchive.py b/dandi/dandiarchive.py index 262d162d8..37c5b10a8 100644 --- a/dandi/dandiarchive.py +++ b/dandi/dandiarchive.py @@ -28,7 +28,7 @@ from __future__ import annotations from abc import ABC, abstractmethod -from collections.abc import Iterable, Iterator +from collections.abc import Callable, Iterable, Iterator from contextlib import contextmanager from dataclasses import dataclass, field import posixpath @@ -387,15 +387,13 @@ def get_assets( `NotFoundError` will now be raised if `strict` is true and there are no such assets. """ - any_assets = False - with _maybe_strict(strict): - d = self.get_dandiset(client, lazy=not strict) - assert d is not None - for a in d.get_assets_with_path_prefix(self.path, order=order): - any_assets = True - yield a - if strict and not any_assets: - raise NotFoundError(f"No assets found with path prefix {self.path!r}") + return _iter_multi_assets( + self, + client, + strict, + lambda d: d.get_assets_with_path_prefix(self.path, order=order), + f"No assets found with path prefix {self.path!r}", + ) @dataclass @@ -535,15 +533,13 @@ def get_assets( path = self.path if not path.endswith("/"): path += "/" - any_assets = False - with _maybe_strict(strict): - d = self.get_dandiset(client, lazy=not strict) - assert d is not None - for a in d.get_assets_with_path_prefix(path, order=order): - any_assets = True - yield a - if strict and not any_assets: - raise NotFoundError(f"No assets found under folder {path!r}") + return _iter_multi_assets( + self, + client, + strict, + lambda d: d.get_assets_with_path_prefix(path, order=order), + f"No assets found under folder {path!r}", + ) @dataclass @@ -566,15 +562,13 @@ def get_assets( `NotFoundError` will now be raised if `strict` is true and there are no such assets. """ - any_assets = False - with _maybe_strict(strict): - d = self.get_dandiset(client, lazy=not strict) - assert d is not None - for a in d.get_assets_by_glob(self.path, order=order): - any_assets = True - yield a - if strict and not any_assets: - raise NotFoundError(f"No assets found matching glob {self.path!r}") + return _iter_multi_assets( + self, + client, + strict, + lambda d: d.get_assets_by_glob(self.path, order=order), + f"No assets found matching glob {self.path!r}", + ) def get_asset_download_path( self, asset: BaseRemoteAsset, preserve_tree: bool @@ -597,6 +591,24 @@ def _maybe_strict(strict: bool) -> Iterator[None]: raise +def _iter_multi_assets( + url: MultiAssetURL, + client: DandiAPIClient, + strict: bool, + get_assets: Callable[[RemoteDandiset], Iterator[BaseRemoteAsset]], + not_found_msg: str, +) -> Iterator[BaseRemoteAsset]: + any_assets = False + with _maybe_strict(strict): + d = url.get_dandiset(client, lazy=not strict) + assert d is not None + for a in get_assets(d): + any_assets = True + yield a + if strict and not any_assets: + raise NotFoundError(not_found_msg) + + @contextmanager def navigate_url( url: str, *, strict: bool = False, authenticate: bool | None = None diff --git a/dandi/files/bases.py b/dandi/files/bases.py index db502dcc3..97a8a3c38 100644 --- a/dandi/files/bases.py +++ b/dandi/files/bases.py @@ -292,19 +292,10 @@ def iter_upload( replacing: RemoteAsset | None = None, ) -> Iterator[dict]: """ - Upload the asset with the given metadata to the given Dandiset, - returning a generator of status `dict`\\s. + Like `upload()`, but returns a generator of status `dict`\\s instead + of blocking until the upload is complete. See `upload()` for a + description of the parameters. - :param RemoteDandiset dandiset: - the Dandiset to which the asset will be uploaded - :param dict metadata: - Metadata for the uploaded asset. The "path" field will be set to - the value of the instance's ``path`` attribute if no such field is - already present. - :param int jobs: Number of threads to use for uploading; defaults to 5 - :param RemoteAsset replacing: - If set, replace the given asset, which must have the same path as - the new asset :returns: A generator of `dict`\\s containing at least a ``"status"`` key. Upon successful upload, the last `dict` will have a status of @@ -344,26 +335,7 @@ def iter_upload( jobs: int | None = None, replacing: RemoteAsset | None = None, ) -> Iterator[dict]: - """ - Upload the file as an asset with the given metadata to the given - Dandiset, returning a generator of status `dict`\\s. - - :param RemoteDandiset dandiset: - the Dandiset to which the file will be uploaded - :param dict metadata: - Metadata for the uploaded asset. The "path" field will be set to - the value of the instance's ``path`` attribute if no such field is - already present. - :param int jobs: Number of threads to use for uploading; defaults to 5 - :param RemoteAsset replacing: - If set, replace the given asset, which must have the same path as - the new asset - :returns: - A generator of `dict`\\s containing at least a ``"status"`` key. - Upon successful upload, the last `dict` will have a status of - ``"done"`` and an ``"asset"`` key containing the resulting - `RemoteAsset`. - """ + """See `LocalAsset.iter_upload`.""" # Avoid heavy import by importing within function: from dandi.support.digests import get_dandietag diff --git a/dandi/files/bids.py b/dandi/files/bids.py index 008b085dd..d44dc7b32 100644 --- a/dandi/files/bids.py +++ b/dandi/files/bids.py @@ -11,7 +11,7 @@ from dandi.bids_validator_deno import bids_validate -from .bases import GenericAsset, LocalFileAsset, NWBAsset +from .bases import DandiFile, GenericAsset, LocalFileAsset, NWBAsset from .zarr import ZarrAsset from ..consts import ZARR_MIME_TYPE, dandiset_metadata_file from ..metadata.core import add_common_metadata, prepare_metadata @@ -197,6 +197,21 @@ def get_validation_errors( # get_metadata(): inherit use of default metadata from LocalFileAsset +def _bids_combined_validation_errors( + self: "BIDSAsset", + format_cls: type[DandiFile], + schema_version: str | None, + devel_debug: bool, + missing_file_content: MissingFileContent | None, +) -> list[ValidationResult]: + return format_cls.get_validation_errors( + self, + schema_version, + devel_debug, + missing_file_content=missing_file_content, + ) + BIDSAsset.get_validation_errors(self) + + @dataclass class BIDSAsset(LocalFileAsset): """ @@ -271,12 +286,9 @@ def get_validation_errors( devel_debug: bool = False, missing_file_content: MissingFileContent | None = None, ) -> list[ValidationResult]: - return NWBAsset.get_validation_errors( - self, - schema_version, - devel_debug, - missing_file_content=missing_file_content, - ) + BIDSAsset.get_validation_errors(self) + return _bids_combined_validation_errors( + self, NWBAsset, schema_version, devel_debug, missing_file_content + ) def get_metadata( self, @@ -306,22 +318,16 @@ def get_validation_errors( devel_debug: bool = False, missing_file_content: MissingFileContent | None = None, ) -> list[ValidationResult]: - return ZarrAsset.get_validation_errors( - self, - schema_version, - devel_debug, - missing_file_content=missing_file_content, - ) + BIDSAsset.get_validation_errors(self) + return _bids_combined_validation_errors( + self, ZarrAsset, schema_version, devel_debug, missing_file_content + ) def get_metadata( self, digest: Digest | None = None, ignore_errors: bool = True, ) -> BareAsset: - metadata = self.bids_dataset_description.get_asset_metadata(self) - start_time = end_time = datetime.now().astimezone() - add_common_metadata(metadata, self.filepath, start_time, end_time, digest) - metadata.path = self.path + metadata = BIDSAsset.get_metadata(self, digest, ignore_errors) metadata.encodingFormat = ZARR_MIME_TYPE return metadata diff --git a/dandi/files/zarr.py b/dandi/files/zarr.py index 25394769e..e3454b7a6 100644 --- a/dandi/files/zarr.py +++ b/dandi/files/zarr.py @@ -565,28 +565,18 @@ def iter_upload( zarr_mode: ZarrMode = "full", # type: ignore[assignment] ) -> Iterator[dict]: """ - Upload the Zarr directory as an asset with the given metadata to the - given Dandiset, returning a generator of status `dict`\\s. + Like `LocalAsset.iter_upload`, with two differences: ``replacing``, + if set and the old asset is a Zarr, has that Zarr updated & reused + for the new asset; and the resulting status generator can terminate + as ``"skipped"`` (see below). :param RemoteDandiset dandiset: the Dandiset to which the Zarr will be uploaded - :param dict metadata: - Metadata for the uploaded asset. The "path" field will be set to - the value of the instance's ``path`` attribute if no such field is - already present. - :param int jobs: Number of threads to use for uploading; defaults to 5 - :param RemoteAsset replacing: - If set, replace the given asset, which must have the same path as - the new asset; if the old asset is a Zarr, the Zarr will be updated - & reused for the new asset :returns: - A generator of `dict`\\s containing at least a ``"status"`` key. - Upon successful upload, the last `dict` will have a status of - ``"done"`` and an ``"asset"`` key containing the resulting - `RemoteAsset`. If the local Zarr is bit-identical to the remote - (nothing to upload or delete), the terminal status is instead - ``"skipped"`` with a ``"message"`` of ``"identical"`` and the same - ``"asset"`` key. + See `LocalAsset.iter_upload`. Additionally, if the local Zarr is + bit-identical to the remote (nothing to upload or delete), the + terminal status is instead ``"skipped"`` with a ``"message"`` of + ``"identical"`` and the same ``"asset"`` key. """ asset_path = metadata.setdefault("path", self.path) set_asset_schema_key(metadata) diff --git a/dandi/misctypes.py b/dandi/misctypes.py index b50cffc42..bbe4220b5 100644 --- a/dandi/misctypes.py +++ b/dandi/misctypes.py @@ -60,6 +60,53 @@ def asdict(self) -> dict[DigestType, str]: value=32 * "d" + "-1--1", ) + +def _split_path(path: str) -> tuple[str, ...]: + """Split a path into its path components""" + if path.startswith("/"): + raise ValueError(f"Absolute paths not allowed: {path!r}") + return tuple(q for q in path.split("/") if q) + + +def _match_parts(parts: tuple[str, ...], pattern: str) -> bool: + """Tests whether ``parts`` matches the glob pattern ``pattern``""" + patparts = _split_path(pattern) + if not patparts: + raise ValueError("Empty pattern") + if len(patparts) > len(parts): + return False + for part, pat in zip(reversed(parts), reversed(patparts)): + if not fnmatchcase(part, pat): + return False + return True + + +def _path_suffix(name: str) -> str: + """The final file extension of ``name``, if any""" + i = name.rfind(".") + if 0 < i < len(name) - 1: + return name[i:] + else: + return "" + + +def _path_suffixes(name: str) -> list[str]: + """A list of ``name``'s file extensions""" + if name.endswith("."): + return [] + name = name.lstrip(".") + return ["." + suffix for suffix in name.split(".")[1:]] + + +def _path_stem(name: str) -> str: + """``name`` without its final file extension, if any""" + i = name.rfind(".") + if 0 < i < len(name) - 1: + return name[:i] + else: + return name + + P = TypeVar("P", bound="BasePath") @@ -103,7 +150,7 @@ def _get_subpath(self: P, name: str) -> P: def __truediv__(self: P, path: str) -> P: p = self - for q in self._split_path(path): + for q in _split_path(path): p = p._get_subpath(q) return p @@ -117,13 +164,6 @@ def joinpath(self: P, *paths: str) -> P: p /= q return p - @staticmethod - def _split_path(path: str) -> tuple[str, ...]: - """Split a path into its path components""" - if path.startswith("/"): - raise ValueError(f"Absolute paths not allowed: {path!r}") - return tuple(q for q in path.split("/") if q) - def is_root(self) -> bool: """ Returns true if this path object represents the root of its hierarchy @@ -168,28 +208,17 @@ def with_name(self: P, name: str) -> P: @property def suffix(self) -> str: """The final file extension of the basename, if any""" - i = self.name.rfind(".") - if 0 < i < len(self.name) - 1: - return self.name[i:] - else: - return "" + return _path_suffix(self.name) @property def suffixes(self) -> list[str]: """A list of the basename's file extensions""" - if self.name.endswith("."): - return [] - name = self.name.lstrip(".") - return ["." + suffix for suffix in name.split(".")[1:]] + return _path_suffixes(self.name) @property def stem(self) -> str: """The basename without its final file extension, if any""" - i = self.name.rfind(".") - if 0 < i < len(self.name) - 1: - return self.name[:i] - else: - return self.name + return _path_stem(self.name) def with_stem(self: P, stem: str) -> P: """Returns a new path with the stem changed""" @@ -209,15 +238,7 @@ def with_suffix(self: P, suffix: str) -> P: def match(self, pattern: str) -> bool: """Tests whether the path matches the given glob pattern""" - patparts = self._split_path(pattern) - if not patparts: - raise ValueError("Empty pattern") - if len(patparts) > len(self.parts): - return False - for part, pat in zip(reversed(self.parts), reversed(patparts)): - if not fnmatchcase(part, pat): - return False - return True + return _match_parts(self.parts, pattern) @abstractmethod def exists(self) -> bool: @@ -343,9 +364,8 @@ class RemoteReadableAsset(Readable): def open(self) -> IO[bytes]: # Optional dependency: - import fsspec - from aiohttp import ClientTimeout + import fsspec # We need to call open() on the return value of fsspec.open() because # otherwise the filehandle will only be opened when used to enter a diff --git a/dandi/move.py b/dandi/move.py index 887c7dc1f..95570de16 100644 --- a/dandi/move.py +++ b/dandi/move.py @@ -248,10 +248,7 @@ def resolve(self, path: str) -> tuple[AssetPath, bool]: def calculate_moves( self, *srcs: str, dest: str, existing: MoveExisting ) -> list[Movement]: - """ - Given a sequence of input source paths and a destination path, return a - sorted list of all assets that will be moved/renamed - """ + """See `Mover.calculate_moves`.""" destpath, dest_is_dir = self.resolve(dest) destobj: File | Folder | None try: @@ -322,10 +319,7 @@ def calculate_moves( def calculate_moves_by_regex( self, find: str, replace: str, existing: MoveExisting ) -> list[Movement]: - """ - Given a regular expression and a replacement string, return a sorted - list of all assets that will be moved/renamed - """ + """See `Mover.calculate_moves_by_regex`.""" rgx = re.compile(find) moves: dict[AssetPath, AssetPath] = {} rev: dict[AssetPath, AssetPath] = {} @@ -447,14 +441,7 @@ def placename(self) -> str: return "local" def get_assets(self, subpath_only: bool = False) -> Iterator[tuple[AssetPath, str]]: - """ - Yield all available assets as ``(asset_path, relpath)`` pairs, where - ``asset_path`` is a ``/``-separated path relative to the root of the - Dandiset and ``relpath`` is a ``/``-separated path to that asset, - relative to `subpath` (For assets outside of `subpath`, ``relpath`` - starts with ``"../"``). If ``subpath_only`` is true, only assets - underneath `subpath` are returned. - """ + """See `LocalizedMover.get_assets`.""" root = self.dandiset_path if subpath_only: root /= self.subpath @@ -470,14 +457,7 @@ def get_assets(self, subpath_only: bool = False) -> Iterator[tuple[AssetPath, st yield (AssetPath(df.path), relpath) def get_path(self, path: str, is_src: bool = True) -> File | Folder: - """ - Return the asset or folder of assets at ``path`` (relative to - `subpath`) as a `File` or `Folder` instance. If there is nothing at - the given path, raises `NotFoundError`. - - If the path points to a folder, its `~Folder.relcontents` attribute - will be populated iff ``is_src`` is given. - """ + """See `LocalizedMover.get_path`.""" rpath, needs_dir = self.resolve(path) p = self.dandiset_path / rpath if not os.path.lexists(p): @@ -525,10 +505,7 @@ def is_file(self, path: AssetPath) -> bool: ) def move(self, src: AssetPath, dest: AssetPath) -> None: - """ - Move the asset at path ``src`` to path ``dest`` (which can be assumed - to not exist) - """ + """See `LocalizedMover.move`.""" lgr.debug("Moving local file %r to %r", src, dest) target = self.dandiset_path / dest try: @@ -602,14 +579,7 @@ def placename(self) -> str: return "remote" def get_assets(self, subpath_only: bool = False) -> Iterator[tuple[AssetPath, str]]: - """ - Yield all available assets as ``(asset_path, relpath)`` pairs, where - ``asset_path`` is a ``/``-separated path relative to the root of the - Dandiset and ``relpath`` is a ``/``-separated path to that asset, - relative to `subpath` (For assets outside of `subpath`, ``relpath`` - starts with ``"../"``). If ``subpath_only`` is true, only assets - underneath `subpath` are returned. - """ + """See `LocalizedMover.get_assets`.""" for path in self.assets.keys(): relpath = posixpath.relpath(path, self.subpath.as_posix()) if subpath_only and relpath.startswith("../"): @@ -617,14 +587,7 @@ def get_assets(self, subpath_only: bool = False) -> Iterator[tuple[AssetPath, st yield (path, relpath) def get_path(self, path: str, is_src: bool = True) -> File | Folder: - """ - Return the asset or folder of assets at ``path`` (relative to - `subpath`) as a `File` or `Folder` instance. If there is nothing at - the given path, raises `NotFoundError`. - - If the path points to a folder, its `~Folder.relcontents` attribute - will be populated iff ``is_src`` is given. - """ + """See `LocalizedMover.get_path`.""" rpath, needs_dir = self.resolve(path) relcontents: list[str] = [] file_found = False @@ -680,10 +643,7 @@ def is_file(self, path: AssetPath) -> bool: return path in self.assets def move(self, src: AssetPath, dest: AssetPath) -> None: - """ - Move the asset at path ``src`` to path ``dest`` (which can be assumed - to not exist) - """ + """See `LocalizedMover.move`.""" lgr.debug("Moving remote asset %r to %r", src, dest) assert src in self.assets try: @@ -742,10 +702,7 @@ def status_field(self) -> str: def calculate_moves( self, *srcs: str, dest: str, existing: MoveExisting ) -> list[Movement]: - """ - Given a sequence of input source paths and a destination path, return a - sorted list of all assets that will be moved/renamed - """ + """See `Mover.calculate_moves`.""" local_moves = self.local.calculate_moves(*srcs, dest=dest, existing=existing) remote_moves = self.remote.calculate_moves(*srcs, dest=dest, existing=existing) self.compare_moves(local_moves, remote_moves) @@ -754,10 +711,7 @@ def calculate_moves( def calculate_moves_by_regex( self, find: str, replace: str, existing: MoveExisting ) -> list[Movement]: - """ - Given a regular expression and a replacement string, return a sorted - list of all assets that will be moved/renamed - """ + """See `Mover.calculate_moves_by_regex`.""" local_moves = self.local.calculate_moves_by_regex(find, replace, existing) remote_moves = self.remote.calculate_moves_by_regex(find, replace, existing) self.compare_moves(local_moves, remote_moves) diff --git a/dandi/tests/fixtures.py b/dandi/tests/fixtures.py index 9b128a23d..bc56edbf3 100644 --- a/dandi/tests/fixtures.py +++ b/dandi/tests/fixtures.py @@ -138,14 +138,13 @@ def simple3_nwb( ) -@pytest.fixture(scope="session") -def simple4_nwb(tmp_path_factory: pytest.TempPathFactory) -> Path: - """With subject, subject_id, species, but including data orientation ambiguity.""" - +def _make_ambiguous_orientation_nwb( + tmp_path_factory: pytest.TempPathFactory, *, unit: str, subject_id: str +) -> Path: start_time = datetime(2017, 4, 3, 11, tzinfo=timezone.utc) time_series = pynwb.TimeSeries( name="test_time_series", - unit="test_units", + unit=unit, data=np.zeros(shape=(2, 100)), rate=1.0, ) @@ -155,7 +154,7 @@ def simple4_nwb(tmp_path_factory: pytest.TempPathFactory) -> Path: identifier="NWBE4", session_start_time=start_time, subject=Subject( - subject_id="mouse004", + subject_id=subject_id, age="P1D/", sex="O", species="Mus musculus", @@ -168,6 +167,14 @@ def simple4_nwb(tmp_path_factory: pytest.TempPathFactory) -> Path: return filename +@pytest.fixture(scope="session") +def simple4_nwb(tmp_path_factory: pytest.TempPathFactory) -> Path: + """With subject, subject_id, species, but including data orientation ambiguity.""" + return _make_ambiguous_orientation_nwb( + tmp_path_factory, unit="test_units", subject_id="mouse004" + ) + + @pytest.fixture(scope="session") def simple5_nwb(tmp_path_factory: pytest.TempPathFactory) -> Path: """ @@ -180,46 +187,28 @@ def simple5_nwb(tmp_path_factory: pytest.TempPathFactory) -> Path: https://github.com/NeurodataWithoutBorders/nwbinspector/issues/345#issuecomment-1459232718 """ - - start_time = datetime(2017, 4, 3, 11, tzinfo=timezone.utc) - time_series = pynwb.TimeSeries( - name="test_time_series", - unit="", - data=np.zeros(shape=(2, 100)), - rate=1.0, + return _make_ambiguous_orientation_nwb( + tmp_path_factory, unit="", subject_id="mouse001" ) - nwbfile = NWBFile( - session_description="some session", - identifier="NWBE4", - session_start_time=start_time, - subject=Subject( - subject_id="mouse001", - age="P1D/", - sex="O", - species="Mus musculus", - ), - ) - nwbfile.add_acquisition(time_series) - filename = tmp_path_factory.mktemp("simple4") / "simple4.nwb" - with pynwb.NWBHDF5IO(filename, "w") as io: - io.write(nwbfile, cache_spec=False) - return filename - -@pytest.fixture(scope="session") -def organized_nwb_dir( - simple2_nwb: Path, tmp_path_factory: pytest.TempPathFactory -) -> Path: +def _organize_copy(nwb_path: Path, tmp_path_factory: pytest.TempPathFactory) -> Path: tmp_path = tmp_path_factory.mktemp("organized_nwb_dir") (tmp_path / dandiset_metadata_file).write_text("{}\n") r = CliRunner().invoke( - organize, ["-f", "copy", "--dandiset-path", str(tmp_path), str(simple2_nwb)] + organize, ["-f", "copy", "--dandiset-path", str(tmp_path), str(nwb_path)] ) assert r.exit_code == 0, r.stdout return tmp_path +@pytest.fixture(scope="session") +def organized_nwb_dir( + simple2_nwb: Path, tmp_path_factory: pytest.TempPathFactory +) -> Path: + return _organize_copy(simple2_nwb, tmp_path_factory) + + @pytest.fixture(scope="session") def organized_nwb_dir2( simple1_nwb_metadata: dict[str, Any], @@ -252,13 +241,7 @@ def organized_nwb_dir2( def organized_nwb_dir3( simple4_nwb: Path, tmp_path_factory: pytest.TempPathFactory ) -> Path: - tmp_path = tmp_path_factory.mktemp("organized_nwb_dir") - (tmp_path / dandiset_metadata_file).write_text("{}\n") - r = CliRunner().invoke( - organize, ["-f", "copy", "--dandiset-path", str(tmp_path), str(simple4_nwb)] - ) - assert r.exit_code == 0, r.stdout - return tmp_path + return _organize_copy(simple4_nwb, tmp_path_factory) @pytest.fixture(scope="session") diff --git a/dandi/tests/test_dandiapi.py b/dandi/tests/test_dandiapi.py index 8da28852e..9f0152b14 100644 --- a/dandi/tests/test_dandiapi.py +++ b/dandi/tests/test_dandiapi.py @@ -230,6 +230,13 @@ def test_authenticate_bad_key_keyring_good_key_input( confirm_mock.assert_called_once_with("API key is invalid; enter another?") +def _fetch_content(client: DandiAPIClient, url: str, tmp_path: Path) -> None: + r = client.get(url, stream=True, json_resp=False) + with open(tmp_path / "asset.nwb", "wb") as fp: + for chunk in r.iter_content(chunk_size=8192): + fp.write(chunk) + + @mark.skipif_no_network def test_get_content_url(tmp_path: Path) -> None: with DandiAPIClient.for_dandi_instance("dandi") as client: @@ -243,10 +250,7 @@ def test_get_content_url(tmp_path: Path) -> None: + UUID_PATTERN.rstrip("$") + "/download/?$", url, ) - r = client.get(url, stream=True, json_resp=False) - with open(tmp_path / "asset.nwb", "wb") as fp: - for chunk in r.iter_content(chunk_size=8192): - fp.write(chunk) + _fetch_content(client, url, tmp_path) @mark.skipif_no_network @@ -256,10 +260,7 @@ def test_get_content_url_regex(tmp_path: Path) -> None: "sub-RAT123/sub-RAT123.nwb" ) url = asset.get_content_url(r"amazonaws.com/.*blobs/") - r = client.get(url, stream=True, json_resp=False) - with open(tmp_path / "asset.nwb", "wb") as fp: - for chunk in r.iter_content(chunk_size=8192): - fp.write(chunk) + _fetch_content(client, url, tmp_path) @mark.skipif_no_network @@ -645,6 +646,25 @@ def test_search_get_dandisets( assert ds.dandiset_id not in [d.identifier for d in dandisets] +def _assert_dandiset_fields( + dandiset: RemoteDandiset, version_id: str, most_recent_published: str | None +) -> None: + assert dandiset.version_id == version_id + assert isinstance(dandiset.created, datetime) + assert isinstance(dandiset.modified, datetime) + assert isinstance(dandiset.version, Version) + assert dandiset.version.identifier == version_id + if most_recent_published is None: + assert dandiset.most_recent_published_version is None + else: + assert isinstance(dandiset.most_recent_published_version, Version) + assert ( + dandiset.most_recent_published_version.identifier == most_recent_published + ) + assert isinstance(dandiset.draft_version, Version) + assert isinstance(dandiset.contact_person, str) + + def test_get_dandiset_lazy( mocker: MockerFixture, text_dandiset: SampleDandiset ) -> None: @@ -657,13 +677,7 @@ def test_get_dandiset_lazy( assert isinstance(dandiset.created, datetime) get_spy.assert_called_once() get_spy.reset_mock() - assert isinstance(dandiset.created, datetime) - assert isinstance(dandiset.modified, datetime) - assert isinstance(dandiset.version, Version) - assert dandiset.version.identifier == DRAFT - assert dandiset.most_recent_published_version is None - assert isinstance(dandiset.draft_version, Version) - assert isinstance(dandiset.contact_person, str) + _assert_dandiset_fields(dandiset, DRAFT, None) get_spy.assert_not_called() @@ -677,30 +691,14 @@ def test_get_dandiset_non_lazy( get_spy.reset_mock() assert dandiset.version_id == DRAFT get_spy.assert_not_called() - assert isinstance(dandiset.created, datetime) - get_spy.assert_not_called() - assert isinstance(dandiset.created, datetime) - assert isinstance(dandiset.modified, datetime) - assert isinstance(dandiset.version, Version) - assert dandiset.version.identifier == DRAFT - assert dandiset.most_recent_published_version is None - assert isinstance(dandiset.draft_version, Version) - assert isinstance(dandiset.contact_person, str) + _assert_dandiset_fields(dandiset, DRAFT, None) get_spy.assert_not_called() @pytest.mark.parametrize("lazy", [True, False]) def test_get_dandiset_no_version_id(lazy: bool, text_dandiset: SampleDandiset) -> None: dandiset = text_dandiset.client.get_dandiset(text_dandiset.dandiset_id, lazy=lazy) - assert dandiset.version_id == DRAFT - assert isinstance(dandiset.created, datetime) - assert isinstance(dandiset.created, datetime) - assert isinstance(dandiset.modified, datetime) - assert isinstance(dandiset.version, Version) - assert dandiset.version.identifier == DRAFT - assert dandiset.most_recent_published_version is None - assert isinstance(dandiset.draft_version, Version) - assert isinstance(dandiset.contact_person, str) + _assert_dandiset_fields(dandiset, DRAFT, None) versions = list(dandiset.get_versions()) assert len(versions) == 1 assert versions[0].identifier == DRAFT @@ -712,16 +710,7 @@ def test_get_dandiset_published(lazy: bool, text_dandiset: SampleDandiset) -> No d.wait_until_valid() v = d.publish().version.identifier dandiset = text_dandiset.client.get_dandiset(d.identifier, v, lazy=lazy) - assert dandiset.version_id == v - assert isinstance(dandiset.created, datetime) - assert isinstance(dandiset.created, datetime) - assert isinstance(dandiset.modified, datetime) - assert isinstance(dandiset.version, Version) - assert dandiset.version.identifier == v - assert isinstance(dandiset.most_recent_published_version, Version) - assert dandiset.most_recent_published_version.identifier == v - assert isinstance(dandiset.draft_version, Version) - assert isinstance(dandiset.contact_person, str) + _assert_dandiset_fields(dandiset, v, v) versions = list(dandiset.get_versions()) assert len(versions) == 2 assert sorted(vobj.identifier for vobj in versions) == [v, DRAFT] @@ -735,16 +724,7 @@ def test_get_dandiset_published_no_version_id( d.wait_until_valid() v = d.publish().version.identifier dandiset = text_dandiset.client.get_dandiset(d.identifier, lazy=lazy) - assert dandiset.version_id == v - assert isinstance(dandiset.created, datetime) - assert isinstance(dandiset.created, datetime) - assert isinstance(dandiset.modified, datetime) - assert isinstance(dandiset.version, Version) - assert dandiset.version.identifier == v - assert isinstance(dandiset.most_recent_published_version, Version) - assert dandiset.most_recent_published_version.identifier == v - assert isinstance(dandiset.draft_version, Version) - assert isinstance(dandiset.contact_person, str) + _assert_dandiset_fields(dandiset, v, v) versions = list(dandiset.get_versions()) assert len(versions) == 2 assert sorted(vobj.identifier for vobj in versions) == [v, DRAFT] @@ -758,16 +738,7 @@ def test_get_dandiset_published_draft( d.wait_until_valid() v = d.publish().version.identifier dandiset = text_dandiset.client.get_dandiset(d.identifier, DRAFT, lazy=lazy) - assert dandiset.version_id == DRAFT - assert isinstance(dandiset.created, datetime) - assert isinstance(dandiset.created, datetime) - assert isinstance(dandiset.modified, datetime) - assert isinstance(dandiset.version, Version) - assert dandiset.version.identifier == DRAFT - assert isinstance(dandiset.most_recent_published_version, Version) - assert dandiset.most_recent_published_version.identifier == v - assert isinstance(dandiset.draft_version, Version) - assert isinstance(dandiset.contact_person, str) + _assert_dandiset_fields(dandiset, DRAFT, v) versions = list(dandiset.get_versions()) assert len(versions) == 2 assert sorted(vobj.identifier for vobj in versions) == [v, DRAFT] @@ -788,16 +759,7 @@ def test_get_dandiset_published_other_version( assert v1 != v2 dandiset = text_dandiset.client.get_dandiset(d.identifier, v1, lazy=lazy) - assert dandiset.version_id == v1 - assert isinstance(dandiset.created, datetime) - assert isinstance(dandiset.created, datetime) - assert isinstance(dandiset.modified, datetime) - assert isinstance(dandiset.version, Version) - assert dandiset.version.identifier == v1 - assert isinstance(dandiset.most_recent_published_version, Version) - assert dandiset.most_recent_published_version.identifier == v2 - assert isinstance(dandiset.draft_version, Version) - assert isinstance(dandiset.contact_person, str) + _assert_dandiset_fields(dandiset, v1, v2) versions = list(dandiset.get_versions()) assert len(versions) == 3 diff --git a/dandi/tests/test_dandiarchive.py b/dandi/tests/test_dandiarchive.py index 1154b025b..0754f5323 100644 --- a/dandi/tests/test_dandiarchive.py +++ b/dandi/tests/test_dandiarchive.py @@ -6,6 +6,7 @@ import responses from dandi.consts import DandiInstance, known_instances +from dandi.dandiapi import DandiAPIClient from dandi.dandiarchive import ( AssetFolderURL, AssetGlobURL, @@ -663,6 +664,25 @@ def test_parse_arbitrary_api_url() -> None: ) +def _assert_no_assets( + url: str, client: DandiAPIClient, expected_message: str, use_list: bool = False +) -> None: + parsed_url = parse_dandi_url(url) + assert list(parsed_url.get_assets(client)) == [] + with pytest.raises(NotFoundError) as excinfo: + if use_list: + list(parsed_url.get_assets(client, strict=True)) + else: + next(parsed_url.get_assets(client, strict=True)) + assert str(excinfo.value) == expected_message + + +NO_SUCH_DANDISET_999999 = ( + "No such Dandiset: '999999'. " + "Verify the Dandiset ID is correct and that you have access. " +) + + @pytest.mark.parametrize("version_suffix", ["", "@draft", "@0.999999.9999"]) def test_get_nonexistent_dandiset( local_dandi_api: DandiAPI, version_suffix: str @@ -673,17 +693,8 @@ def test_get_nonexistent_dandiset( parsed_url.get_dandiset(client) # No error with pytest.raises(NotFoundError) as excinfo: parsed_url.get_dandiset(client, lazy=False) - assert str(excinfo.value) == ( - "No such Dandiset: '999999'. " - "Verify the Dandiset ID is correct and that you have access. " - ) - assert list(parsed_url.get_assets(client)) == [] - with pytest.raises(NotFoundError) as excinfo: - next(parsed_url.get_assets(client, strict=True)) - assert str(excinfo.value) == ( - "No such Dandiset: '999999'. " - "Verify the Dandiset ID is correct and that you have access. " - ) + assert str(excinfo.value) == NO_SUCH_DANDISET_999999 + _assert_no_assets(url, client, NO_SUCH_DANDISET_999999) @pytest.mark.parametrize("version", ["draft", "0.999999.9999"]) @@ -694,15 +705,7 @@ def test_get_nonexistent_dandiset_asset_id( f"{local_dandi_api.api_url}/dandisets/999999/versions/{version}" "/assets/00000000-0000-0000-0000-000000000000/" ) - parsed_url = parse_dandi_url(url) - client = local_dandi_api.client - assert list(parsed_url.get_assets(client)) == [] - with pytest.raises(NotFoundError) as excinfo: - next(parsed_url.get_assets(client, strict=True)) - assert str(excinfo.value) == ( - "No such Dandiset: '999999'. " - "Verify the Dandiset ID is correct and that you have access. " - ) + _assert_no_assets(url, local_dandi_api.client, NO_SUCH_DANDISET_999999) def test_get_dandiset_nonexistent_asset_id(text_dandiset: SampleDandiset) -> None: @@ -711,28 +714,22 @@ def test_get_dandiset_nonexistent_asset_id(text_dandiset: SampleDandiset) -> Non f"{text_dandiset.dandiset_id}/versions/draft/assets/" "00000000-0000-0000-0000-000000000000/" ) - parsed_url = parse_dandi_url(url) - client = text_dandiset.client - assert list(parsed_url.get_assets(client)) == [] - with pytest.raises(NotFoundError) as excinfo: - next(parsed_url.get_assets(client, strict=True)) - assert str(excinfo.value) == ( + _assert_no_assets( + url, + text_dandiset.client, "No such asset: '00000000-0000-0000-0000-000000000000' for" - f" DANDI-API-LOCAL-DOCKER-TESTS:{text_dandiset.dandiset_id}/draft" + f" DANDI-API-LOCAL-DOCKER-TESTS:{text_dandiset.dandiset_id}/draft", ) def test_get_nonexistent_asset_id(local_dandi_api: DandiAPI) -> None: url = f"{local_dandi_api.api_url}/assets/00000000-0000-0000-0000-000000000000/" - parsed_url = parse_dandi_url(url) - client = local_dandi_api.client - assert list(parsed_url.get_assets(client)) == [] - with pytest.raises(NotFoundError) as excinfo: - next(parsed_url.get_assets(client, strict=True)) - assert str(excinfo.value) == ( + _assert_no_assets( + url, + local_dandi_api.client, "No such asset: '00000000-0000-0000-0000-000000000000'. " "Verify the asset ID is correct. " - "Use 'dandi ls' to list available assets." + "Use 'dandi ls' to list available assets.", ) @@ -741,15 +738,7 @@ def test_get_nonexistent_dandiset_asset_path( local_dandi_api: DandiAPI, version_suffix: str ) -> None: url = f"dandi://{local_dandi_api.instance_id}/999999{version_suffix}/does/not/exist" - parsed_url = parse_dandi_url(url) - client = local_dandi_api.client - assert list(parsed_url.get_assets(client)) == [] - with pytest.raises(NotFoundError) as excinfo: - next(parsed_url.get_assets(client, strict=True)) - assert str(excinfo.value) == ( - "No such Dandiset: '999999'. " - "Verify the Dandiset ID is correct and that you have access. " - ) + _assert_no_assets(url, local_dandi_api.client, NO_SUCH_DANDISET_999999) def test_get_nonexistent_asset_path(text_dandiset: SampleDandiset) -> None: @@ -757,15 +746,12 @@ def test_get_nonexistent_asset_path(text_dandiset: SampleDandiset) -> None: f"dandi://{text_dandiset.api.instance_id}/" f"{text_dandiset.dandiset_id}/does/not/exist" ) - parsed_url = parse_dandi_url(url) - client = text_dandiset.client - assert list(parsed_url.get_assets(client)) == [] - with pytest.raises(NotFoundError) as excinfo: - next(parsed_url.get_assets(client, strict=True)) - assert str(excinfo.value) == ( + _assert_no_assets( + url, + text_dandiset.client, "No asset at path 'does/not/exist' in version draft. " "Verify the path is correct and the asset exists in this version. " - "Use 'dandi ls' to list available assets." + "Use 'dandi ls' to list available assets.", ) @@ -777,15 +763,7 @@ def test_get_nonexistent_dandiset_asset_folder( f"dandi://{local_dandi_api.instance_id}/999999{version_suffix}" "/does/not/exist/" ) - parsed_url = parse_dandi_url(url) - client = local_dandi_api.client - assert list(parsed_url.get_assets(client)) == [] - with pytest.raises(NotFoundError) as excinfo: - next(parsed_url.get_assets(client, strict=True)) - assert str(excinfo.value) == ( - "No such Dandiset: '999999'. " - "Verify the Dandiset ID is correct and that you have access. " - ) + _assert_no_assets(url, local_dandi_api.client, NO_SUCH_DANDISET_999999) def test_get_nonexistent_asset_folder(text_dandiset: SampleDandiset) -> None: @@ -793,12 +771,12 @@ def test_get_nonexistent_asset_folder(text_dandiset: SampleDandiset) -> None: f"dandi://{text_dandiset.api.instance_id}/" f"{text_dandiset.dandiset_id}/does/not/exist/" ) - parsed_url = parse_dandi_url(url) - client = text_dandiset.client - assert list(parsed_url.get_assets(client)) == [] - with pytest.raises(NotFoundError) as excinfo: - list(parsed_url.get_assets(client, strict=True)) - assert str(excinfo.value) == "No assets found under folder 'does/not/exist/'" + _assert_no_assets( + url, + text_dandiset.client, + "No assets found under folder 'does/not/exist/'", + use_list=True, + ) @pytest.mark.parametrize("version", ["draft", "0.999999.9999"]) @@ -809,15 +787,7 @@ def test_get_nonexistent_dandiset_asset_prefix( f"{local_dandi_api.api_url}/dandisets/999999/versions/{version}" "/assets/?path=does/not/exist" ) - parsed_url = parse_dandi_url(url) - client = local_dandi_api.client - assert list(parsed_url.get_assets(client)) == [] - with pytest.raises(NotFoundError) as excinfo: - next(parsed_url.get_assets(client, strict=True)) - assert str(excinfo.value) == ( - "No such Dandiset: '999999'. " - "Verify the Dandiset ID is correct and that you have access. " - ) + _assert_no_assets(url, local_dandi_api.client, NO_SUCH_DANDISET_999999) def test_get_nonexistent_asset_prefix(text_dandiset: SampleDandiset) -> None: @@ -825,12 +795,12 @@ def test_get_nonexistent_asset_prefix(text_dandiset: SampleDandiset) -> None: f"{text_dandiset.api.api_url}/dandisets/" f"{text_dandiset.dandiset_id}/versions/draft/assets/?path=does/not/exist" ) - parsed_url = parse_dandi_url(url) - client = text_dandiset.client - assert list(parsed_url.get_assets(client)) == [] - with pytest.raises(NotFoundError) as excinfo: - list(parsed_url.get_assets(client, strict=True)) - assert str(excinfo.value) == "No assets found with path prefix 'does/not/exist'" + _assert_no_assets( + url, + text_dandiset.client, + "No assets found with path prefix 'does/not/exist'", + use_list=True, + ) def test_get_nonexistent_asset_glob(text_dandiset: SampleDandiset) -> None: @@ -838,12 +808,12 @@ def test_get_nonexistent_asset_glob(text_dandiset: SampleDandiset) -> None: f"{text_dandiset.api.api_url}/dandisets/" f"{text_dandiset.dandiset_id}/versions/draft/assets/?glob=d*/*/*st" ) - parsed_url = parse_dandi_url(url) - client = text_dandiset.client - assert list(parsed_url.get_assets(client)) == [] - with pytest.raises(NotFoundError) as excinfo: - list(parsed_url.get_assets(client, strict=True)) - assert str(excinfo.value) == "No assets found matching glob 'd*/*/*st'" + _assert_no_assets( + url, + text_dandiset.client, + "No assets found matching glob 'd*/*/*st'", + use_list=True, + ) @pytest.mark.parametrize( diff --git a/dandi/tests/test_delete.py b/dandi/tests/test_delete.py index 9a6ff8dd1..b64483af7 100644 --- a/dandi/tests/test_delete.py +++ b/dandi/tests/test_delete.py @@ -1,6 +1,7 @@ from __future__ import annotations from pathlib import Path +from typing import Any import pytest from pytest_mock import MockerFixture @@ -14,6 +15,24 @@ from ..utils import list_paths +def _setup_delete_spy( + mocker: MockerFixture, monkeypatch: pytest.MonkeyPatch, api: DandiAPI +) -> tuple[str, Any]: + api.monkeypatch_set_api_key_env(monkeypatch) + delete_spy = mocker.spy(RESTFullAPIClient, "delete") + return api.instance_id, delete_spy + + +def _assert_remaining_assets( + text_dandiset: SampleDandiset, tmp_path: Path, remaining: list[Path] +) -> None: + download(text_dandiset.dandiset.version_api_url, tmp_path) + dandiset_id = text_dandiset.dandiset_id + assert list_paths(tmp_path) == [ + tmp_path / dandiset_id / f for f in [Path("dandiset.yaml")] + remaining + ] + + @pytest.mark.parametrize( "paths,remainder", [ @@ -67,10 +86,8 @@ def test_delete_paths( remainder: list[Path], ) -> None: monkeypatch.chdir(text_dandiset.dspath) - text_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) - instance = text_dandiset.api.instance_id + instance, delete_spy = _setup_delete_spy(mocker, monkeypatch, text_dandiset.api) dandiset_id = text_dandiset.dandiset_id - delete_spy = mocker.spy(RESTFullAPIClient, "delete") delete( [p.format(instance=instance, dandiset_id=dandiset_id) for p in paths], dandi_instance=instance, @@ -78,10 +95,7 @@ def test_delete_paths( force=True, ) delete_spy.assert_called() - download(text_dandiset.dandiset.version_api_url, tmp_path) - assert list_paths(tmp_path) == [ - tmp_path / dandiset_id / f for f in [Path("dandiset.yaml")] + remainder - ] + _assert_remaining_assets(text_dandiset, tmp_path, remainder) @pytest.mark.parametrize("confirm", [True, False]) @@ -92,10 +106,8 @@ def test_delete_path_confirm( text_dandiset: SampleDandiset, ) -> None: monkeypatch.chdir(text_dandiset.dspath) - text_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) - instance = text_dandiset.api.instance_id + instance, delete_spy = _setup_delete_spy(mocker, monkeypatch, text_dandiset.api) dandiset_id = text_dandiset.dandiset_id - delete_spy = mocker.spy(RESTFullAPIClient, "delete") confirm_mock = mocker.patch("click.confirm", return_value=confirm) delete(["subdir2/coconut.txt"], dandi_instance=instance, devel_debug=True) confirm_mock.assert_called_with( @@ -113,9 +125,7 @@ def test_delete_path_pyout( text_dandiset: SampleDandiset, ) -> None: monkeypatch.chdir(text_dandiset.dspath) - text_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) - instance = text_dandiset.api.instance_id - delete_spy = mocker.spy(RESTFullAPIClient, "delete") + instance, delete_spy = _setup_delete_spy(mocker, monkeypatch, text_dandiset.api) delete(["subdir2/coconut.txt"], dandi_instance=instance, force=True) delete_spy.assert_called() @@ -143,10 +153,8 @@ def test_delete_dandiset( paths: list[str], ) -> None: monkeypatch.chdir(text_dandiset.dspath) - text_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) - instance = text_dandiset.api.instance_id + instance, delete_spy = _setup_delete_spy(mocker, monkeypatch, text_dandiset.api) dandiset_id = text_dandiset.dandiset_id - delete_spy = mocker.spy(RESTFullAPIClient, "delete") delete( [p.format(instance=instance, dandiset_id=dandiset_id) for p in paths], dandi_instance=instance, @@ -166,10 +174,8 @@ def test_delete_dandiset_confirm( text_dandiset: SampleDandiset, ) -> None: monkeypatch.chdir(text_dandiset.dspath) - text_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) - instance = text_dandiset.api.instance_id + instance, delete_spy = _setup_delete_spy(mocker, monkeypatch, text_dandiset.api) dandiset_id = text_dandiset.dandiset_id - delete_spy = mocker.spy(RESTFullAPIClient, "delete") confirm_mock = mocker.patch("click.confirm", return_value=confirm) delete( [f"dandi://{instance}/{dandiset_id}"], dandi_instance=instance, devel_debug=True @@ -187,11 +193,9 @@ def test_delete_dandiset_mismatch( text_dandiset: SampleDandiset, ) -> None: monkeypatch.chdir(text_dandiset.dspath) - text_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) - instance = text_dandiset.api.instance_id + instance, delete_spy = _setup_delete_spy(mocker, monkeypatch, text_dandiset.api) dandiset_id = text_dandiset.dandiset_id not_dandiset = str(int(dandiset_id) - 1).zfill(6) - delete_spy = mocker.spy(RESTFullAPIClient, "delete") for paths in [ [ "subdir1/apple.txt", @@ -216,10 +220,8 @@ def test_delete_instance_mismatch( text_dandiset: SampleDandiset, ) -> None: monkeypatch.chdir(text_dandiset.dspath) - text_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) - instance = text_dandiset.api.instance_id + instance, delete_spy = _setup_delete_spy(mocker, monkeypatch, text_dandiset.api) dandiset_id = text_dandiset.dandiset_id - delete_spy = mocker.spy(RESTFullAPIClient, "delete") for paths in [ [ "subdir1/apple.txt", @@ -242,9 +244,7 @@ def test_delete_instance_mismatch( def test_delete_nonexistent_dandiset( local_dandi_api: DandiAPI, mocker: MockerFixture, monkeypatch: pytest.MonkeyPatch ) -> None: - local_dandi_api.monkeypatch_set_api_key_env(monkeypatch) - instance = local_dandi_api.instance_id - delete_spy = mocker.spy(RESTFullAPIClient, "delete") + instance, delete_spy = _setup_delete_spy(mocker, monkeypatch, local_dandi_api) with pytest.raises(NotFoundError) as excinfo: delete( [f"dandi://{instance}/999999/subdir1/apple.txt"], @@ -262,9 +262,7 @@ def test_delete_nonexistent_dandiset( def test_delete_nonexistent_dandiset_skip_missing( local_dandi_api: DandiAPI, mocker: MockerFixture, monkeypatch: pytest.MonkeyPatch ) -> None: - local_dandi_api.monkeypatch_set_api_key_env(monkeypatch) - instance = local_dandi_api.instance_id - delete_spy = mocker.spy(RESTFullAPIClient, "delete") + instance, delete_spy = _setup_delete_spy(mocker, monkeypatch, local_dandi_api) delete( [f"dandi://{instance}/999999/subdir1/apple.txt"], dandi_instance=instance, @@ -280,10 +278,8 @@ def test_delete_nonexistent_asset( monkeypatch: pytest.MonkeyPatch, text_dandiset: SampleDandiset, ) -> None: - text_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) - instance = text_dandiset.api.instance_id + instance, delete_spy = _setup_delete_spy(mocker, monkeypatch, text_dandiset.api) dandiset_id = text_dandiset.dandiset_id - delete_spy = mocker.spy(RESTFullAPIClient, "delete") with pytest.raises(NotFoundError) as excinfo: delete( [ @@ -307,10 +303,8 @@ def test_delete_nonexistent_asset_skip_missing( text_dandiset: SampleDandiset, tmp_path: Path, ) -> None: - text_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) - instance = text_dandiset.api.instance_id + instance, delete_spy = _setup_delete_spy(mocker, monkeypatch, text_dandiset.api) dandiset_id = text_dandiset.dandiset_id - delete_spy = mocker.spy(RESTFullAPIClient, "delete") delete( [ f"dandi://{instance}/{dandiset_id}/file.txt", @@ -322,13 +316,15 @@ def test_delete_nonexistent_asset_skip_missing( skip_missing=True, ) delete_spy.assert_called() - download(text_dandiset.dandiset.version_api_url, tmp_path) - assert list_paths(tmp_path) == [ - tmp_path / dandiset_id / "dandiset.yaml", - tmp_path / dandiset_id / "subdir1" / "apple.txt", - tmp_path / dandiset_id / "subdir2" / "banana.txt", - tmp_path / dandiset_id / "subdir2" / "coconut.txt", - ] + _assert_remaining_assets( + text_dandiset, + tmp_path, + [ + Path("subdir1", "apple.txt"), + Path("subdir2", "banana.txt"), + Path("subdir2", "coconut.txt"), + ], + ) def test_delete_nonexistent_asset_folder( @@ -336,10 +332,8 @@ def test_delete_nonexistent_asset_folder( monkeypatch: pytest.MonkeyPatch, text_dandiset: SampleDandiset, ) -> None: - text_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) - instance = text_dandiset.api.instance_id + instance, delete_spy = _setup_delete_spy(mocker, monkeypatch, text_dandiset.api) dandiset_id = text_dandiset.dandiset_id - delete_spy = mocker.spy(RESTFullAPIClient, "delete") with pytest.raises(NotFoundError) as excinfo: delete( [ @@ -363,10 +357,8 @@ def test_delete_nonexistent_asset_folder_skip_missing( text_dandiset: SampleDandiset, tmp_path: Path, ) -> None: - text_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) - instance = text_dandiset.api.instance_id + instance, delete_spy = _setup_delete_spy(mocker, monkeypatch, text_dandiset.api) dandiset_id = text_dandiset.dandiset_id - delete_spy = mocker.spy(RESTFullAPIClient, "delete") delete( [ f"dandi://{instance}/{dandiset_id}/subdir1/", @@ -378,21 +370,21 @@ def test_delete_nonexistent_asset_folder_skip_missing( skip_missing=True, ) delete_spy.assert_called() - download(text_dandiset.dandiset.version_api_url, tmp_path) - assert list_paths(tmp_path) == [ - tmp_path / dandiset_id / "dandiset.yaml", - tmp_path / dandiset_id / "file.txt", - tmp_path / dandiset_id / "subdir2" / "banana.txt", - tmp_path / dandiset_id / "subdir2" / "coconut.txt", - ] + _assert_remaining_assets( + text_dandiset, + tmp_path, + [ + Path("file.txt"), + Path("subdir2", "banana.txt"), + Path("subdir2", "coconut.txt"), + ], + ) def test_delete_version( local_dandi_api: DandiAPI, mocker: MockerFixture, monkeypatch: pytest.MonkeyPatch ) -> None: - local_dandi_api.monkeypatch_set_api_key_env(monkeypatch) - instance = local_dandi_api.instance_id - delete_spy = mocker.spy(RESTFullAPIClient, "delete") + instance, delete_spy = _setup_delete_spy(mocker, monkeypatch, local_dandi_api) with pytest.raises(NotImplementedError) as excinfo: delete( [f"dandi://{instance}/999999@draft"], diff --git a/dandi/tests/test_download.py b/dandi/tests/test_download.py index 43bb9579f..65dfa530f 100644 --- a/dandi/tests/test_download.py +++ b/dandi/tests/test_download.py @@ -443,15 +443,27 @@ def test_download_asset_by_equal_prefix( assert (tmp_path / "apple.txt").read_text() == "Apple\n" -@pytest.mark.parametrize("confirm", [True, False]) -def test_download_sync( - confirm: bool, mocker: MockerFixture, text_dandiset: SampleDandiset, tmp_path: Path -) -> None: +def _prep_sync_download( + mocker: MockerFixture, + text_dandiset: SampleDandiset, + tmp_path: Path, + return_value: str | None, +) -> tuple[Path, mock.MagicMock]: text_dandiset.dandiset.get_asset_by_path("file.txt").delete() dspath = tmp_path / text_dandiset.dandiset_id os.rename(text_dandiset.dspath, dspath) confirm_mock = mocker.patch( - "dandi.download.abbrev_prompt", return_value="yes" if confirm else "no" + "dandi.download.abbrev_prompt", return_value=return_value + ) + return dspath, confirm_mock + + +@pytest.mark.parametrize("confirm", [True, False]) +def test_download_sync( + confirm: bool, mocker: MockerFixture, text_dandiset: SampleDandiset, tmp_path: Path +) -> None: + dspath, confirm_mock = _prep_sync_download( + mocker, text_dandiset, tmp_path, "yes" if confirm else "no" ) download( f"dandi://{text_dandiset.api.instance_id}/{text_dandiset.dandiset_id}", @@ -470,10 +482,7 @@ def test_download_sync( def test_download_sync_do( mocker: MockerFixture, text_dandiset: SampleDandiset, tmp_path: Path ) -> None: - text_dandiset.dandiset.get_asset_by_path("file.txt").delete() - dspath = tmp_path / text_dandiset.dandiset_id - os.rename(text_dandiset.dspath, dspath) - confirm_mock = mocker.patch("dandi.download.abbrev_prompt") + dspath, confirm_mock = _prep_sync_download(mocker, text_dandiset, tmp_path, None) download( f"dandi://{text_dandiset.api.instance_id}/{text_dandiset.dandiset_id}", tmp_path, @@ -484,12 +493,18 @@ def test_download_sync_do( assert not (dspath / "file.txt").exists() -def test_download_sync_folder( +def _prep_sync_delete_two_assets( mocker: MockerFixture, text_dandiset: SampleDandiset -) -> None: +) -> mock.MagicMock: text_dandiset.dandiset.get_asset_by_path("file.txt").delete() text_dandiset.dandiset.get_asset_by_path("subdir2/banana.txt").delete() - confirm_mock = mocker.patch("dandi.download.abbrev_prompt", return_value="yes") + return mocker.patch("dandi.download.abbrev_prompt", return_value="yes") + + +def test_download_sync_folder( + mocker: MockerFixture, text_dandiset: SampleDandiset +) -> None: + confirm_mock = _prep_sync_delete_two_assets(mocker, text_dandiset) download( f"dandi://{text_dandiset.api.instance_id}/{text_dandiset.dandiset_id}/subdir2/", text_dandiset.dspath, @@ -1182,13 +1197,7 @@ def test_download_multiple_urls( ] -def test_download_glob_option(text_dandiset: SampleDandiset, tmp_path: Path) -> None: - dandiset_id = text_dandiset.dandiset_id - download( - f"dandi://{text_dandiset.api.instance_id}/{dandiset_id}/s*.Txt", - tmp_path, - path_type=PathType.GLOB, - ) +def _assert_glob_download(tmp_path: Path) -> None: assert list_paths(tmp_path, dirs=True) == [ tmp_path / "subdir1", tmp_path / "subdir1" / "apple.txt", @@ -1201,26 +1210,25 @@ def test_download_glob_option(text_dandiset: SampleDandiset, tmp_path: Path) -> assert (tmp_path / "subdir2" / "coconut.txt").read_text() == "Coconut\n" +def test_download_glob_option(text_dandiset: SampleDandiset, tmp_path: Path) -> None: + dandiset_id = text_dandiset.dandiset_id + download( + f"dandi://{text_dandiset.api.instance_id}/{dandiset_id}/s*.Txt", + tmp_path, + path_type=PathType.GLOB, + ) + _assert_glob_download(tmp_path) + + def test_download_glob_url(text_dandiset: SampleDandiset, tmp_path: Path) -> None: download(f"{text_dandiset.dandiset.version_api_url}assets/?glob=s*.Txt", tmp_path) - assert list_paths(tmp_path, dirs=True) == [ - tmp_path / "subdir1", - tmp_path / "subdir1" / "apple.txt", - tmp_path / "subdir2", - tmp_path / "subdir2" / "banana.txt", - tmp_path / "subdir2" / "coconut.txt", - ] - assert (tmp_path / "subdir1" / "apple.txt").read_text() == "Apple\n" - assert (tmp_path / "subdir2" / "banana.txt").read_text() == "Banana\n" - assert (tmp_path / "subdir2" / "coconut.txt").read_text() == "Coconut\n" + _assert_glob_download(tmp_path) def test_download_sync_glob( mocker: MockerFixture, text_dandiset: SampleDandiset ) -> None: - text_dandiset.dandiset.get_asset_by_path("file.txt").delete() - text_dandiset.dandiset.get_asset_by_path("subdir2/banana.txt").delete() - confirm_mock = mocker.patch("dandi.download.abbrev_prompt", return_value="yes") + confirm_mock = _prep_sync_delete_two_assets(mocker, text_dandiset) download( f"{text_dandiset.dandiset.version_api_url}assets/?glob=s*.Txt", text_dandiset.dspath, @@ -1502,31 +1510,28 @@ def test__check_attempts_and_sleep_retries(status_code: int) -> None: mock_sleep.assert_called_once() assert mock_sleep.call_args.args[0] > 0 - # shifted by 1 year! (too long) - response.headers["Retry-After"] = "Wed, 21 Oct 2016 07:28:00 GMT" - with mock.patch("time.sleep") as mock_sleep, mock.patch( - "dandi.utils.datetime" - ) as mock_datetime: - mock_datetime.datetime.now.return_value = parsedate_to_datetime( - "Wed, 21 Oct 2015 07:28:00 GMT" - ) - assert f(HTTPError(response=response), attempt=1, attempts_allowed=2) == 2 - mock_sleep.assert_called_once() - # and we do sleep some time - assert mock_sleep.call_args.args[0] > 0 + def check_retry_after(retry_after: str, now: str, sleeps: bool) -> None: + response.headers["Retry-After"] = retry_after + with mock.patch("time.sleep") as mock_sleep, mock.patch( + "dandi.utils.datetime" + ) as mock_datetime: + mock_datetime.datetime.now.return_value = parsedate_to_datetime(now) + assert f(HTTPError(response=response), attempt=1, attempts_allowed=2) == 2 + mock_sleep.assert_called_once() + if sleeps: + assert mock_sleep.call_args.args[0] > 0 + else: + assert not mock_sleep.call_args.args[0] + + # shifted by 1 year! (too long) -- we do sleep some time + check_retry_after( + "Wed, 21 Oct 2016 07:28:00 GMT", "Wed, 21 Oct 2015 07:28:00 GMT", sleeps=True + ) - # in the past second (too quick) - response.headers["Retry-After"] = "Wed, 21 Oct 2015 07:27:59 GMT" - with mock.patch("time.sleep") as mock_sleep, mock.patch( - "dandi.utils.datetime" - ) as mock_datetime: - mock_datetime.datetime.now.return_value = parsedate_to_datetime( - "Wed, 21 Oct 2015 07:28:00 GMT" - ) - assert f(HTTPError(response=response), attempt=1, attempts_allowed=2) == 2 - mock_sleep.assert_called_once() - # and we do not sleep really - assert not mock_sleep.call_args.args[0] + # in the past second (too quick) -- we do not sleep really + check_retry_after( + "Wed, 21 Oct 2015 07:27:59 GMT", "Wed, 21 Oct 2015 07:28:00 GMT", sleeps=False + ) # ---------- Partial Zarr download tests ---------- @@ -1553,9 +1558,9 @@ def test_download_zarr_with_glob_filter( all_files = list_paths(zarr_dir) assert len(all_files) > 0 for f in all_files: - assert f.name.startswith(".z") or f.name == "zarr.json", ( - f"Non-metadata file downloaded: {f}" - ) + assert ( + f.name.startswith(".z") or f.name == "zarr.json" + ), f"Non-metadata file downloaded: {f}" @pytest.mark.ai_generated diff --git a/dandi/tests/test_files.py b/dandi/tests/test_files.py index bc73aece2..00723eed1 100644 --- a/dandi/tests/test_files.py +++ b/dandi/tests/test_files.py @@ -66,37 +66,40 @@ def test_find_dandi_files(tmp_path: Path) -> None: ".ignored.dir/ignored.nwb", ) + video = VideoAsset( + filepath=tmp_path / "glarch.mp4", path="glarch.mp4", dandiset_path=tmp_path + ) + image = ImageAsset( + filepath=tmp_path / "quux.png", path="quux.png", dandiset_path=tmp_path + ) + zarr1 = ZarrAsset( + filepath=tmp_path / "sample01.zarr", + path="sample01.zarr", + dandiset_path=tmp_path, + ) + nwb2 = NWBAsset( + filepath=tmp_path / "sample02.nwb", + path="sample02.nwb", + dandiset_path=tmp_path, + ) + nwb3 = NWBAsset( + filepath=tmp_path / "subdir" / "sample03.nwb", + path="subdir/sample03.nwb", + dandiset_path=tmp_path, + ) + zarr4 = ZarrAsset( + filepath=tmp_path / "subdir" / "sample04.zarr", + path="subdir/sample04.zarr", + dandiset_path=tmp_path, + ) + metadata_file = DandisetMetadataFile( + filepath=tmp_path / dandiset_metadata_file, dandiset_path=tmp_path + ) + files = sorted( find_dandi_files(tmp_path, dandiset_path=tmp_path), key=attrgetter("filepath") ) - assert files == [ - VideoAsset( - filepath=tmp_path / "glarch.mp4", path="glarch.mp4", dandiset_path=tmp_path - ), - ImageAsset( - filepath=tmp_path / "quux.png", path="quux.png", dandiset_path=tmp_path - ), - ZarrAsset( - filepath=tmp_path / "sample01.zarr", - path="sample01.zarr", - dandiset_path=tmp_path, - ), - NWBAsset( - filepath=tmp_path / "sample02.nwb", - path="sample02.nwb", - dandiset_path=tmp_path, - ), - NWBAsset( - filepath=tmp_path / "subdir" / "sample03.nwb", - path="subdir/sample03.nwb", - dandiset_path=tmp_path, - ), - ZarrAsset( - filepath=tmp_path / "subdir" / "sample04.zarr", - path="subdir/sample04.zarr", - dandiset_path=tmp_path, - ), - ] + assert files == [video, image, zarr1, nwb2, nwb3, zarr4] files = sorted( find_dandi_files(tmp_path, dandiset_path=tmp_path, allow_all=True), @@ -106,26 +109,12 @@ def test_find_dandi_files(tmp_path: Path) -> None: GenericAsset( filepath=tmp_path / "bar.txt", path="bar.txt", dandiset_path=tmp_path ), - DandisetMetadataFile( - filepath=tmp_path / dandiset_metadata_file, dandiset_path=tmp_path - ), + metadata_file, GenericAsset(filepath=tmp_path / "foo", path="foo", dandiset_path=tmp_path), - VideoAsset( - filepath=tmp_path / "glarch.mp4", path="glarch.mp4", dandiset_path=tmp_path - ), - ImageAsset( - filepath=tmp_path / "quux.png", path="quux.png", dandiset_path=tmp_path - ), - ZarrAsset( - filepath=tmp_path / "sample01.zarr", - path="sample01.zarr", - dandiset_path=tmp_path, - ), - NWBAsset( - filepath=tmp_path / "sample02.nwb", - path="sample02.nwb", - dandiset_path=tmp_path, - ), + video, + image, + zarr1, + nwb2, GenericAsset( filepath=tmp_path / "subdir" / "cleesh.txt", path="subdir/cleesh.txt", @@ -136,53 +125,15 @@ def test_find_dandi_files(tmp_path: Path) -> None: path="subdir/gnusto", dandiset_path=tmp_path, ), - NWBAsset( - filepath=tmp_path / "subdir" / "sample03.nwb", - path="subdir/sample03.nwb", - dandiset_path=tmp_path, - ), - ZarrAsset( - filepath=tmp_path / "subdir" / "sample04.zarr", - path="subdir/sample04.zarr", - dandiset_path=tmp_path, - ), + nwb3, + zarr4, ] files = sorted( find_dandi_files(tmp_path, dandiset_path=tmp_path, include_metadata=True), key=attrgetter("filepath"), ) - assert files == [ - DandisetMetadataFile( - filepath=tmp_path / dandiset_metadata_file, dandiset_path=tmp_path - ), - VideoAsset( - filepath=tmp_path / "glarch.mp4", path="glarch.mp4", dandiset_path=tmp_path - ), - ImageAsset( - filepath=tmp_path / "quux.png", path="quux.png", dandiset_path=tmp_path - ), - ZarrAsset( - filepath=tmp_path / "sample01.zarr", - path="sample01.zarr", - dandiset_path=tmp_path, - ), - NWBAsset( - filepath=tmp_path / "sample02.nwb", - path="sample02.nwb", - dandiset_path=tmp_path, - ), - NWBAsset( - filepath=tmp_path / "subdir" / "sample03.nwb", - path="subdir/sample03.nwb", - dandiset_path=tmp_path, - ), - ZarrAsset( - filepath=tmp_path / "subdir" / "sample04.zarr", - path="subdir/sample04.zarr", - dandiset_path=tmp_path, - ), - ] + assert files == [metadata_file, video, image, zarr1, nwb2, nwb3, zarr4] def test_find_dandi_files_with_bids(tmp_path: Path) -> None: @@ -202,6 +153,61 @@ def test_find_dandi_files_with_bids(tmp_path: Path) -> None: "bids2/subbids/data.json", ) + bidsignore = GenericBIDSAsset( + filepath=tmp_path / "bids1" / ".bidsignore", + path="bids1/.bidsignore", + dandiset_path=tmp_path, + bids_dataset_description_ref=ANY, # type: ignore[arg-type] + ) + bids1_dd = BIDSDatasetDescriptionAsset( + filepath=tmp_path / "bids1" / "dataset_description.json", + path="bids1/dataset_description.json", + dandiset_path=tmp_path, + dataset_files=ANY, # type: ignore[arg-type] + ) + bids1_file = GenericBIDSAsset( + filepath=tmp_path / "bids1" / "file.txt", + path="bids1/file.txt", + dandiset_path=tmp_path, + bids_dataset_description_ref=ANY, # type: ignore[arg-type] + ) + bids1_zarr = ZarrBIDSAsset( + filepath=tmp_path / "bids1" / "subdir" / "glarch.zarr", + path="bids1/subdir/glarch.zarr", + dandiset_path=tmp_path, + bids_dataset_description_ref=ANY, # type: ignore[arg-type] + ) + bids1_nwb = NWBBIDSAsset( + filepath=tmp_path / "bids1" / "subdir" / "quux.nwb", + path="bids1/subdir/quux.nwb", + dandiset_path=tmp_path, + bids_dataset_description_ref=ANY, # type: ignore[arg-type] + ) + bids2_dd = BIDSDatasetDescriptionAsset( + filepath=tmp_path / "bids2" / "dataset_description.json", + path="bids2/dataset_description.json", + dandiset_path=tmp_path, + dataset_files=ANY, # type: ignore[arg-type] + ) + bids2_movie = GenericBIDSAsset( + filepath=tmp_path / "bids2" / "movie.mp4", + path="bids2/movie.mp4", + dandiset_path=tmp_path, + bids_dataset_description_ref=ANY, # type: ignore[arg-type] + ) + bids2_data = GenericBIDSAsset( + filepath=tmp_path / "bids2" / "subbids" / "data.json", + path="bids2/subbids/data.json", + dandiset_path=tmp_path, + bids_dataset_description_ref=ANY, # type: ignore[arg-type] + ) + bids2_subdd = GenericBIDSAsset( + filepath=tmp_path / "bids2" / "subbids" / "dataset_description.json", + path="bids2/subbids/dataset_description.json", + dandiset_path=tmp_path, + bids_dataset_description_ref=ANY, # type: ignore[arg-type] + ) + files = sorted( find_dandi_files(tmp_path, dandiset_path=tmp_path, allow_all=False), key=attrgetter("filepath"), @@ -209,89 +215,24 @@ def test_find_dandi_files_with_bids(tmp_path: Path) -> None: assert files == [ NWBAsset(filepath=tmp_path / "bar.nwb", path="bar.nwb", dandiset_path=tmp_path), - GenericBIDSAsset( - filepath=tmp_path / "bids1" / ".bidsignore", - path="bids1/.bidsignore", - dandiset_path=tmp_path, - bids_dataset_description_ref=ANY, # type: ignore[arg-type] - ), - BIDSDatasetDescriptionAsset( - filepath=tmp_path / "bids1" / "dataset_description.json", - path="bids1/dataset_description.json", - dandiset_path=tmp_path, - dataset_files=ANY, # type: ignore[arg-type] - ), - GenericBIDSAsset( - filepath=tmp_path / "bids1" / "file.txt", - path="bids1/file.txt", - dandiset_path=tmp_path, - bids_dataset_description_ref=ANY, # type: ignore[arg-type] - ), - ZarrBIDSAsset( - filepath=tmp_path / "bids1" / "subdir" / "glarch.zarr", - path="bids1/subdir/glarch.zarr", - dandiset_path=tmp_path, - bids_dataset_description_ref=ANY, # type: ignore[arg-type] - ), - NWBBIDSAsset( - filepath=tmp_path / "bids1" / "subdir" / "quux.nwb", - path="bids1/subdir/quux.nwb", - dandiset_path=tmp_path, - bids_dataset_description_ref=ANY, # type: ignore[arg-type] - ), - BIDSDatasetDescriptionAsset( - filepath=tmp_path / "bids2" / "dataset_description.json", - path="bids2/dataset_description.json", - dandiset_path=tmp_path, - dataset_files=ANY, # type: ignore[arg-type] - ), - GenericBIDSAsset( - filepath=tmp_path / "bids2" / "movie.mp4", - path="bids2/movie.mp4", - dandiset_path=tmp_path, - bids_dataset_description_ref=ANY, # type: ignore[arg-type] - ), - GenericBIDSAsset( - filepath=tmp_path / "bids2" / "subbids" / "data.json", - path="bids2/subbids/data.json", - dandiset_path=tmp_path, - bids_dataset_description_ref=ANY, # type: ignore[arg-type] - ), - GenericBIDSAsset( - filepath=tmp_path / "bids2" / "subbids" / "dataset_description.json", - path="bids2/subbids/dataset_description.json", - dandiset_path=tmp_path, - bids_dataset_description_ref=ANY, # type: ignore[arg-type] - ), + bidsignore, + bids1_dd, + bids1_file, + bids1_zarr, + bids1_nwb, + bids2_dd, + bids2_movie, + bids2_data, + bids2_subdd, ] bidsdd = files[2] assert isinstance(bidsdd, BIDSDatasetDescriptionAsset) assert sorted(bidsdd.dataset_files, key=attrgetter("filepath")) == [ - GenericBIDSAsset( - filepath=tmp_path / "bids1" / ".bidsignore", - path="bids1/.bidsignore", - dandiset_path=tmp_path, - bids_dataset_description_ref=ANY, # type: ignore[arg-type] - ), - GenericBIDSAsset( - filepath=tmp_path / "bids1" / "file.txt", - path="bids1/file.txt", - dandiset_path=tmp_path, - bids_dataset_description_ref=ANY, # type: ignore[arg-type] - ), - ZarrBIDSAsset( - filepath=tmp_path / "bids1" / "subdir" / "glarch.zarr", - path="bids1/subdir/glarch.zarr", - dandiset_path=tmp_path, - bids_dataset_description_ref=ANY, # type: ignore[arg-type] - ), - NWBBIDSAsset( - filepath=tmp_path / "bids1" / "subdir" / "quux.nwb", - path="bids1/subdir/quux.nwb", - dandiset_path=tmp_path, - bids_dataset_description_ref=ANY, # type: ignore[arg-type] - ), + bidsignore, + bids1_file, + bids1_zarr, + bids1_nwb, ] for asset in bidsdd.dataset_files: assert asset.bids_dataset_description is bidsdd @@ -299,24 +240,9 @@ def test_find_dandi_files_with_bids(tmp_path: Path) -> None: bidsdd = files[6] assert isinstance(bidsdd, BIDSDatasetDescriptionAsset) assert sorted(bidsdd.dataset_files, key=attrgetter("filepath")) == [ - GenericBIDSAsset( - filepath=tmp_path / "bids2" / "movie.mp4", - path="bids2/movie.mp4", - dandiset_path=tmp_path, - bids_dataset_description_ref=ANY, # type: ignore[arg-type] - ), - GenericBIDSAsset( - filepath=tmp_path / "bids2" / "subbids" / "data.json", - path="bids2/subbids/data.json", - dandiset_path=tmp_path, - bids_dataset_description_ref=ANY, # type: ignore[arg-type] - ), - GenericBIDSAsset( - filepath=tmp_path / "bids2" / "subbids" / "dataset_description.json", - path="bids2/subbids/dataset_description.json", - dandiset_path=tmp_path, - bids_dataset_description_ref=ANY, # type: ignore[arg-type] - ), + bids2_movie, + bids2_data, + bids2_subdd, ] for asset in bidsdd.dataset_files: assert asset.bids_dataset_description is bidsdd diff --git a/dandi/tests/test_keyring.py b/dandi/tests/test_keyring.py index 9a7d4dab9..8ae57b0fe 100644 --- a/dandi/tests/test_keyring.py +++ b/dandi/tests/test_keyring.py @@ -2,8 +2,9 @@ from collections.abc import Callable from pathlib import Path +from unittest.mock import MagicMock -from keyring.backend import get_all_keyring +from keyring.backend import KeyringBackend, get_all_keyring from keyring.backends import fail, null from keyring.errors import KeyringError from keyrings.alt import file as keyfile @@ -170,14 +171,18 @@ class EncryptedFailure(fail.Keyring, keyfile.EncryptedKeyring): pass +def _mock_no_keyring( + mocker: MockerFixture, monkeypatch: pytest.MonkeyPatch, keyring: KeyringBackend +) -> MagicMock: + monkeypatch.delenv("PYTHON_KEYRING_BACKEND", raising=False) + return mocker.patch("dandi.keyring_utils.get_keyring", return_value=keyring) + + @pytest.mark.usefixtures("tmp_home") def test_keyring_lookup_fail_default_encrypted( mocker: MockerFixture, monkeypatch: pytest.MonkeyPatch ) -> None: - monkeypatch.delenv("PYTHON_KEYRING_BACKEND", raising=False) - get_keyring = mocker.patch( - "dandi.keyring_utils.get_keyring", return_value=EncryptedFailure() - ) + get_keyring = _mock_no_keyring(mocker, monkeypatch, EncryptedFailure()) with pytest.raises(KeyringError): keyring_lookup("testservice", "testusername") get_keyring.assert_called_once_with() @@ -187,10 +192,7 @@ def test_keyring_lookup_fail_default_encrypted( def test_keyring_lookup_encrypted_fallback_exists_no_password( mocker: MockerFixture, monkeypatch: pytest.MonkeyPatch ) -> None: - monkeypatch.delenv("PYTHON_KEYRING_BACKEND", raising=False) - get_keyring = mocker.patch( - "dandi.keyring_utils.get_keyring", return_value=fail.Keyring() - ) + get_keyring = _mock_no_keyring(mocker, monkeypatch, fail.Keyring()) kf = Path(keyfile.EncryptedKeyring().file_path) kf.parent.mkdir(parents=True, exist_ok=True) kf.touch() @@ -204,10 +206,7 @@ def test_keyring_lookup_encrypted_fallback_exists_no_password( def test_keyring_lookup_encrypted_fallback_exists_password( mocker: MockerFixture, monkeypatch: pytest.MonkeyPatch ) -> None: - monkeypatch.delenv("PYTHON_KEYRING_BACKEND", raising=False) - get_keyring = mocker.patch( - "dandi.keyring_utils.get_keyring", return_value=fail.Keyring() - ) + get_keyring = _mock_no_keyring(mocker, monkeypatch, fail.Keyring()) kb0 = keyfile.EncryptedKeyring() getpass = mocker.patch("getpass.getpass", return_value="file-password") kb0.set_password("testservice", "testusername", "testpassword") @@ -224,10 +223,7 @@ def test_keyring_lookup_encrypted_fallback_exists_password( def test_keyring_lookup_encrypted_fallback_not_exists_no_create( mocker: MockerFixture, monkeypatch: pytest.MonkeyPatch ) -> None: - monkeypatch.delenv("PYTHON_KEYRING_BACKEND", raising=False) - get_keyring = mocker.patch( - "dandi.keyring_utils.get_keyring", return_value=fail.Keyring() - ) + get_keyring = _mock_no_keyring(mocker, monkeypatch, fail.Keyring()) confirm = mocker.patch("click.confirm", return_value=False) with pytest.raises(KeyringError): keyring_lookup("testservice", "testusername") @@ -241,10 +237,7 @@ def test_keyring_lookup_encrypted_fallback_not_exists_no_create( def test_keyring_lookup_encrypted_fallback_not_exists_create_rcconf( mocker: MockerFixture, monkeypatch: pytest.MonkeyPatch ) -> None: - monkeypatch.delenv("PYTHON_KEYRING_BACKEND", raising=False) - get_keyring = mocker.patch( - "dandi.keyring_utils.get_keyring", return_value=fail.Keyring() - ) + get_keyring = _mock_no_keyring(mocker, monkeypatch, fail.Keyring()) confirm = mocker.patch("click.confirm", return_value=True) kb, password = keyring_lookup("testservice", "testusername") assert isinstance(kb, keyfile.EncryptedKeyring) @@ -263,10 +256,7 @@ def test_keyring_lookup_encrypted_fallback_not_exists_create_rcconf( def test_keyring_lookup_encrypted_fallback_not_exists_create_rcconf_exists( mocker: MockerFixture, monkeypatch: pytest.MonkeyPatch ) -> None: - monkeypatch.delenv("PYTHON_KEYRING_BACKEND", raising=False) - get_keyring = mocker.patch( - "dandi.keyring_utils.get_keyring", return_value=fail.Keyring() - ) + get_keyring = _mock_no_keyring(mocker, monkeypatch, fail.Keyring()) confirm = mocker.patch("click.confirm", return_value=True) rc = keyringrc_file() rc.parent.mkdir(parents=True, exist_ok=True) diff --git a/dandi/tests/test_metadata.py b/dandi/tests/test_metadata.py index 26f29acbb..2770bf057 100644 --- a/dandi/tests/test_metadata.py +++ b/dandi/tests/test_metadata.py @@ -489,6 +489,31 @@ def test_time_extract_gest() -> None: ) +def _assert_session_duration(metadata: dict[str, Any], expected_seconds: float) -> None: + assert "session_start_time" in metadata + assert "session_end_time" in metadata + duration = ( + metadata["session_end_time"] - metadata["session_start_time"] + ).total_seconds() + assert abs(duration - expected_seconds) < 1.0 # Allow small floating point errors + + +def _assert_session_activity_dates(nwb_path: Path, metadata: dict[str, Any]) -> None: + # Check that Session activity includes endDate + asset = nwb2asset(nwb_path, digest=DUMMY_DANDI_ETAG) + assert asset.wasGeneratedBy is not None + + # Find Session activities + sessions = [act for act in asset.wasGeneratedBy if act.schemaKey == "Session"] + assert len(sessions) > 0 + + session = sessions[0] + assert session.startDate is not None + assert session.endDate is not None + assert session.startDate == metadata["session_start_time"] + assert session.endDate == metadata["session_end_time"] + + @pytest.mark.ai_generated def test_session_duration_extraction(tmp_path: Path) -> None: """Test that session duration is extracted and included in Session activity""" @@ -522,29 +547,9 @@ def test_session_duration_extraction(tmp_path: Path) -> None: metadata = get_metadata(nwb_path) - # Check that session_end_time was calculated - assert "session_start_time" in metadata - assert "session_end_time" in metadata - # Calculate duration - should be 150 seconds (max) - 0 seconds (min) - duration = ( - metadata["session_end_time"] - metadata["session_start_time"] - ).total_seconds() - assert abs(duration - 150.0) < 1.0 # Allow small floating point errors - - # Check that Session activity includes endDate - asset = nwb2asset(nwb_path, digest=DUMMY_DANDI_ETAG) - assert asset.wasGeneratedBy is not None - - # Find Session activities - sessions = [act for act in asset.wasGeneratedBy if act.schemaKey == "Session"] - assert len(sessions) > 0 - - session = sessions[0] - assert session.startDate is not None - assert session.endDate is not None - assert session.startDate == metadata["session_start_time"] - assert session.endDate == metadata["session_end_time"] + _assert_session_duration(metadata, 150.0) + _assert_session_activity_dates(nwb_path, metadata) @pytest.mark.ai_generated @@ -580,29 +585,9 @@ def test_session_duration_with_trials(tmp_path: Path) -> None: metadata = get_metadata(nwb_path) - # Check that session_end_time was calculated - assert "session_start_time" in metadata - assert "session_end_time" in metadata - # Calculate duration - should be 200 (max from trials) - 5 (min from trials) = 195 seconds - duration = ( - metadata["session_end_time"] - metadata["session_start_time"] - ).total_seconds() - assert abs(duration - 195.0) < 1.0 # Allow small floating point errors - - # Check that Session activity includes endDate - asset = nwb2asset(nwb_path, digest=DUMMY_DANDI_ETAG) - assert asset.wasGeneratedBy is not None - - # Find Session activities - sessions = [act for act in asset.wasGeneratedBy if act.schemaKey == "Session"] - assert len(sessions) > 0 - - session = sessions[0] - assert session.startDate is not None - assert session.endDate is not None - assert session.startDate == metadata["session_start_time"] - assert session.endDate == metadata["session_end_time"] + _assert_session_duration(metadata, 195.0) + _assert_session_activity_dates(nwb_path, metadata) @pytest.mark.ai_generated @@ -636,15 +621,8 @@ def test_session_duration_with_units(tmp_path: Path) -> None: metadata = get_metadata(nwb_path) - # Check that session_end_time was calculated - assert "session_start_time" in metadata - assert "session_end_time" in metadata - # Duration should be 250 (max spike) - 5 (min spike) = 245 seconds - duration = ( - metadata["session_end_time"] - metadata["session_start_time"] - ).total_seconds() - assert abs(duration - 245.0) < 1.0 # Allow small floating point errors + _assert_session_duration(metadata, 245.0) @pytest.mark.ai_generated @@ -670,16 +648,12 @@ def test_session_duration_with_scattered_non_spiking_units(tmp_path: Path) -> No io.write(nwbfile) metadata = get_metadata(nwb_path) - assert "session_start_time" in metadata - assert "session_end_time" in metadata end_offset = (metadata["session_end_time"] - session_start).total_seconds() assert abs(end_offset - 245.0) < 1.0 - duration = ( - metadata["session_end_time"] - metadata["session_start_time"] - ).total_seconds() - assert abs(duration - 245.0) < 1.0 # max 250s and min 5s spike times + # max 250s and min 5s spike times + _assert_session_duration(metadata, 245.0) @pytest.mark.ai_generated @@ -734,15 +708,8 @@ def test_session_duration_with_events(tmp_path: Path) -> None: metadata = get_metadata(nwb_path) - # Check that session_end_time was calculated - assert "session_start_time" in metadata - assert "session_end_time" in metadata - # Duration should be 180 (100 + 80, max end) - 3 (min timestamp) = 177 seconds - duration = ( - metadata["session_end_time"] - metadata["session_start_time"] - ).total_seconds() - assert abs(duration - 157.0) < 1.0 # Allow small floating point errors + _assert_session_duration(metadata, 157.0) @mark_xfail_ontobee @@ -1259,11 +1226,20 @@ def test_ndtypes(ndtypes, asset_dict): assert metadata.variableMeasured[0].value == asset_dict.get(key)[0] -@mark.skipif_no_network -def test_nwb2asset(simple2_nwb: Path) -> None: +def _expected_simple2_asset( + *, + content_size: Any, + digest_value: str, + path: str, + blob_date_modified: Any, + strain: StrainType | None = None, +) -> BareAsset: + participant_kwargs: dict[str, Any] = {} + if strain is not None: + participant_kwargs["strain"] = strain # Classes with ANY_AWARE_DATETIME fields need to be constructed with # model_construct() - assert nwb2asset(simple2_nwb, digest=DUMMY_DANDI_ETAG) == BareAsset.model_construct( + return BareAsset.model_construct( schemaVersion=DANDI_SCHEMA_VERSION, keywords=["keyword1", "keyword 2"], access=[ @@ -1299,12 +1275,12 @@ def test_nwb2asset(simple2_nwb: Path) -> None: ], ), ], - contentSize=ANY_INT, + contentSize=content_size, encodingFormat="application/x-nwb", - digest={DigestType.dandi_etag: "dddddddddddddddddddddddddddddddd-1"}, - path=str(simple2_nwb), + digest={DigestType.dandi_etag: digest_value}, + path=path, dateModified=ANY_AWARE_DATETIME, - blobDateModified=ANY_AWARE_DATETIME, + blobDateModified=blob_date_modified, wasAttributedTo=[ Participant( identifier="mouse001", @@ -1324,7 +1300,7 @@ def test_nwb2asset(simple2_nwb: Path) -> None: identifier="http://purl.obolibrary.org/obo/NCBITaxon_10090", name="Mus musculus - House mouse", ), - strain=StrainType(schemaKey="StrainType", name="C57BL/6J"), + **participant_kwargs, ), ], variableMeasured=[], @@ -1334,6 +1310,17 @@ def test_nwb2asset(simple2_nwb: Path) -> None: ) +@mark.skipif_no_network +def test_nwb2asset(simple2_nwb: Path) -> None: + assert nwb2asset(simple2_nwb, digest=DUMMY_DANDI_ETAG) == _expected_simple2_asset( + content_size=ANY_INT, + digest_value="dddddddddddddddddddddddddddddddd-1", + path=str(simple2_nwb), + blob_date_modified=ANY_AWARE_DATETIME, + strain=StrainType(schemaKey="StrainType", name="C57BL/6J"), + ) + + @pytest.mark.timeout(120) @pytest.mark.xfail(reason="https://github.com/dandi/dandi-cli/issues/1450") def test_nwb2asset_remote_asset(nwb_dandiset: SampleDandiset) -> None: @@ -1343,73 +1330,9 @@ def test_nwb2asset_remote_asset(nwb_dandiset: SampleDandiset) -> None: mtime = ensure_datetime(asset.get_raw_metadata()["blobDateModified"]) assert isinstance(asset, RemoteBlobAsset) r = asset.as_readable() - # Classes with ANY_AWARE_DATETIME fields need to be constructed with - # model_construct() - assert nwb2asset(r, digest=digest) == BareAsset.model_construct( - schemaVersion=DANDI_SCHEMA_VERSION, - keywords=["keyword1", "keyword 2"], - access=[ - AccessRequirements( - schemaKey="AccessRequirements", status=AccessType.OpenAccess - ) - ], - wasGeneratedBy=[ - Session.model_construct( - schemaKey="Session", - identifier="session_id1", - name="session_id1", - description="session_description1", - startDate=ANY_AWARE_DATETIME, - ), - Activity.model_construct( - id=AnyFullmatch( - r"urn:uuid:[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}" - ), - schemaKey="Activity", - name="Metadata generation", - description="Metadata generated by DANDI cli", - startDate=ANY_AWARE_DATETIME, - endDate=ANY_AWARE_DATETIME, - wasAssociatedWith=[ - Software( - schemaKey="Software", - identifier="RRID:SCR_019009", - name="DANDI Command Line Interface", - version=__version__, - url="https://github.com/dandi/dandi-cli", - ) - ], - ), - ], - contentSize=ByteSize(asset.size), - encodingFormat="application/x-nwb", - digest={DigestType.dandi_etag: digest.value}, + assert nwb2asset(r, digest=digest) == _expected_simple2_asset( + content_size=ByteSize(asset.size), + digest_value=digest.value, path="sub-mouse001.nwb", - dateModified=ANY_AWARE_DATETIME, - blobDateModified=mtime, - wasAttributedTo=[ - Participant( - identifier="mouse001", - schemaKey="Participant", - age=PropertyValue( - schemaKey="PropertyValue", - unitText="ISO-8601 duration", - value="P135DT43200S", - valueReference=PropertyValue( - schemaKey="PropertyValue", - value=AgeReferenceType.BirthReference, - ), - ), - sex=SexType(schemaKey="SexType", name="Unknown"), - species=SpeciesType( - schemaKey="SpeciesType", - identifier="http://purl.obolibrary.org/obo/NCBITaxon_10090", - name="Mus musculus - House mouse", - ), - ), - ], - variableMeasured=[], - measurementTechnique=[], - approach=[], - relatedResource=[], + blob_date_modified=mtime, ) diff --git a/dandi/tests/test_move.py b/dandi/tests/test_move.py index 7b8c99ede..f5868f457 100644 --- a/dandi/tests/test_move.py +++ b/dandi/tests/test_move.py @@ -33,6 +33,15 @@ def moving_dandiset(new_dandiset: SampleDandiset) -> SampleDandiset: return new_dandiset +def _setup_move( + monkeypatch: pytest.MonkeyPatch, moving_dandiset: SampleDandiset, chdir_to: Path +) -> list[RemoteAsset]: + starting_assets = list(moving_dandiset.dandiset.get_assets()) + monkeypatch.chdir(chdir_to) + moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + return starting_assets + + def check_assets( sample_dandiset: SampleDandiset, starting_assets: list[RemoteAsset], @@ -170,9 +179,7 @@ def test_move( remapping: dict[str, str | None], work_on: MoveWorkOn, ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) move( *srcs, dest=dest, @@ -187,26 +194,39 @@ def test_move( @pytest.mark.parametrize( "work_on", [MoveWorkOn.LOCAL, MoveWorkOn.REMOTE, MoveWorkOn.BOTH] ) -def test_move_skip( +@pytest.mark.parametrize( + "existing,remapping", + [ + (MoveExisting.SKIP, {"file.txt": "subdir5/file.txt"}), + ( + MoveExisting.OVERWRITE, + { + "file.txt": "subdir5/file.txt", + "subdir4/foo.json": "subdir5/foo.json", + "subdir5/foo.json": None, + }, + ), + ], + ids=["skip", "overwrite"], +) +def test_move_existing( monkeypatch: pytest.MonkeyPatch, moving_dandiset: SampleDandiset, work_on: MoveWorkOn, + existing: MoveExisting, + remapping: dict[str, str | None], ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) move( "file.txt", "subdir4/foo.json", dest="subdir5", work_on=work_on, - existing=MoveExisting.SKIP, + existing=existing, dandi_instance=moving_dandiset.api.instance_id, devel_debug=True, ) - check_assets( - moving_dandiset, starting_assets, work_on, {"file.txt": "subdir5/file.txt"} - ) + check_assets(moving_dandiset, starting_assets, work_on, remapping) @pytest.mark.parametrize( @@ -219,9 +239,7 @@ def test_move_error( work_on: MoveWorkOn, kwargs: dict[str, Any], ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) with pytest.raises(ValueError) as excinfo: move( "file.txt", @@ -238,44 +256,10 @@ def test_move_error( check_assets(moving_dandiset, starting_assets, work_on, {}) -@pytest.mark.parametrize( - "work_on", [MoveWorkOn.LOCAL, MoveWorkOn.REMOTE, MoveWorkOn.BOTH] -) -def test_move_overwrite( - monkeypatch: pytest.MonkeyPatch, - moving_dandiset: SampleDandiset, - work_on: MoveWorkOn, -) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) - move( - "file.txt", - "subdir4/foo.json", - dest="subdir5", - work_on=work_on, - existing=MoveExisting.OVERWRITE, - devel_debug=True, - dandi_instance=moving_dandiset.api.instance_id, - ) - check_assets( - moving_dandiset, - starting_assets, - work_on, - { - "file.txt": "subdir5/file.txt", - "subdir4/foo.json": "subdir5/foo.json", - "subdir5/foo.json": None, - }, - ) - - def test_move_no_srcs( monkeypatch: pytest.MonkeyPatch, moving_dandiset: SampleDandiset ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) with pytest.raises(ValueError) as excinfo: move( dest="nowhere", @@ -289,9 +273,7 @@ def test_move_no_srcs( def test_move_regex_multisrcs( monkeypatch: pytest.MonkeyPatch, moving_dandiset: SampleDandiset ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) with pytest.raises(ValueError) as excinfo: move( r"\.txt", @@ -315,9 +297,7 @@ def test_move_multisrcs_file_dest( moving_dandiset: SampleDandiset, work_on: MoveWorkOn, ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) with pytest.raises(ValueError) as excinfo: move( "file.txt", @@ -341,9 +321,7 @@ def test_move_folder_src_file_dest( moving_dandiset: SampleDandiset, work_on: MoveWorkOn, ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) with pytest.raises(ValueError) as excinfo: move( "subdir1", @@ -363,9 +341,7 @@ def test_move_nonexistent_src( moving_dandiset: SampleDandiset, work_on: MoveWorkOn, ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) with pytest.raises(NotFoundError) as excinfo: move( "file.txt", @@ -390,45 +366,26 @@ def test_move_nonexistent_src( @pytest.mark.parametrize( "work_on", [MoveWorkOn.LOCAL, MoveWorkOn.REMOTE, MoveWorkOn.BOTH] ) -def test_move_file_slash_src( - monkeypatch: pytest.MonkeyPatch, - moving_dandiset: SampleDandiset, - work_on: MoveWorkOn, -) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) - with pytest.raises(ValueError) as excinfo: - move( - "file.txt", - "subdir1/apple.txt/", - dest="subdir2/", - work_on=work_on, - dandi_instance=moving_dandiset.api.instance_id, - ) - path_type = "Remote" if work_on == MoveWorkOn.REMOTE else "Local" - assert str(excinfo.value) == ( - f"{path_type} path 'subdir1/apple.txt/' is a file but a directory " - "was expected. Use a path ending with '/' for directories." - ) - check_assets(moving_dandiset, starting_assets, work_on, {}) - - @pytest.mark.parametrize( - "work_on", [MoveWorkOn.LOCAL, MoveWorkOn.REMOTE, MoveWorkOn.BOTH] + "srcs,dest", + [ + (["file.txt", "subdir1/apple.txt/"], "subdir2/"), + (["file.txt"], "subdir1/apple.txt/"), + ], + ids=["src", "dest"], ) -def test_move_file_slash_dest( +def test_move_file_slash( monkeypatch: pytest.MonkeyPatch, moving_dandiset: SampleDandiset, work_on: MoveWorkOn, + srcs: list[str], + dest: str, ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) with pytest.raises(ValueError) as excinfo: move( - "file.txt", - dest="subdir1/apple.txt/", + *srcs, + dest=dest, work_on=work_on, dandi_instance=moving_dandiset.api.instance_id, ) @@ -443,9 +400,7 @@ def test_move_file_slash_dest( def test_move_regex_no_match( monkeypatch: pytest.MonkeyPatch, moving_dandiset: SampleDandiset ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) with pytest.raises(ValueError) as excinfo: move( "no-match", @@ -464,9 +419,7 @@ def test_move_regex_no_match( def test_move_regex_collision( monkeypatch: pytest.MonkeyPatch, moving_dandiset: SampleDandiset ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) with pytest.raises(ValueError) as excinfo: move( r"^\w+/foo\.json$", @@ -492,9 +445,7 @@ def test_move_regex_some_to_self( moving_dandiset: SampleDandiset, work_on: MoveWorkOn, ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) move( r"(.+[123])/([^.]+)\.(.+)", dest=r"\1/\2.dat", @@ -534,9 +485,9 @@ def test_move_from_subdir( moving_dandiset: SampleDandiset, work_on: MoveWorkOn, ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath / "subdir1") - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move( + monkeypatch, moving_dandiset, moving_dandiset.dspath / "subdir1" + ) move( "../file.txt", "apple.txt", @@ -564,9 +515,9 @@ def test_move_in_subdir( moving_dandiset: SampleDandiset, work_on: MoveWorkOn, ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath / "subdir1") - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move( + monkeypatch, moving_dandiset, moving_dandiset.dspath / "subdir1" + ) move( "apple.txt", dest="macintosh.txt", @@ -590,9 +541,9 @@ def test_move_from_subdir_abspaths( moving_dandiset: SampleDandiset, work_on: MoveWorkOn, ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath / "subdir1") - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move( + monkeypatch, moving_dandiset, moving_dandiset.dspath / "subdir1" + ) with pytest.raises(NotFoundError) as excinfo: move( "file.txt", @@ -621,9 +572,9 @@ def test_move_from_subdir_as_dot( moving_dandiset: SampleDandiset, work_on: MoveWorkOn, ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath / "subdir1") - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move( + monkeypatch, moving_dandiset, moving_dandiset.dspath / "subdir1" + ) with pytest.raises(ValueError) as excinfo: move( ".", @@ -633,8 +584,7 @@ def test_move_from_subdir_as_dot( devel_debug=True, ) assert ( - str(excinfo.value) - == "Cannot move current working directory. " + str(excinfo.value) == "Cannot move current working directory. " "Change to a different directory before moving this location." ) check_assets(moving_dandiset, starting_assets, work_on, {}) @@ -648,9 +598,9 @@ def test_move_from_subdir_regex( moving_dandiset: SampleDandiset, work_on: MoveWorkOn, ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath / "subdir1") - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move( + monkeypatch, moving_dandiset, moving_dandiset.dspath / "subdir1" + ) move( r"\.txt", dest=".dat", @@ -676,9 +626,9 @@ def test_move_from_subdir_regex_no_changes( moving_dandiset: SampleDandiset, work_on: MoveWorkOn, ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath / "subdir1") - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move( + monkeypatch, moving_dandiset, moving_dandiset.dspath / "subdir1" + ) move( r"\.txt", dest=".txt", @@ -700,9 +650,7 @@ def test_move_dandiset_path( tmp_path: Path, work_on: MoveWorkOn, ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(tmp_path) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, tmp_path) move( "file.txt", "subdir2/banana.txt", @@ -730,9 +678,7 @@ def test_move_dandiset_url( tmp_path: Path, work_on: MoveWorkOn, ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(tmp_path) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, tmp_path) move( "file.txt", "subdir2/banana.txt", @@ -755,9 +701,7 @@ def test_move_dandiset_url( def test_move_work_on_auto( monkeypatch: pytest.MonkeyPatch, moving_dandiset: SampleDandiset, tmp_path: Path ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) move( "file.txt", "subdir2/banana.txt", @@ -796,9 +740,9 @@ def test_move_not_dandiset( def test_move_local_delete_empty_dirs( monkeypatch: pytest.MonkeyPatch, moving_dandiset: SampleDandiset ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath / "subdir4") - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move( + monkeypatch, moving_dandiset, moving_dandiset.dspath / "subdir4" + ) move( "../subdir1/apple.txt", "../subdir2/banana.txt", @@ -826,9 +770,7 @@ def test_move_both_src_path_not_in_local( monkeypatch: pytest.MonkeyPatch, moving_dandiset: SampleDandiset ) -> None: (moving_dandiset.dspath / "subdir2" / "banana.txt").unlink() - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) with pytest.raises(AssetMismatchError) as excinfo: move( "subdir2", @@ -851,9 +793,7 @@ def test_move_both_src_path_not_in_remote( monkeypatch: pytest.MonkeyPatch, moving_dandiset: SampleDandiset ) -> None: (moving_dandiset.dspath / "subdir2" / "mango.txt").write_text("Mango\n") - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) with pytest.raises(AssetMismatchError) as excinfo: move( "subdir2", @@ -876,9 +816,7 @@ def test_move_both_dest_path_not_in_remote( existing: MoveExisting, ) -> None: (moving_dandiset.dspath / "subdir2" / "file.txt").write_text("This is a file.\n") - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) with pytest.raises(AssetMismatchError) as excinfo: move( "file.txt", @@ -903,9 +841,7 @@ def test_move_both_dest_path_not_in_local( existing: MoveExisting, ) -> None: (moving_dandiset.dspath / "subdir2" / "banana.txt").unlink() - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) with pytest.raises(AssetMismatchError) as excinfo: move( "file.txt", @@ -932,9 +868,7 @@ def test_move_both_dest_mismatch( (moving_dandiset.dspath / "subdir1" / "apple.txt").unlink() (moving_dandiset.dspath / "subdir1" / "apple.txt").mkdir() (moving_dandiset.dspath / "subdir1" / "apple.txt" / "seeds").write_text("12345\n") - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) with pytest.raises(AssetMismatchError) as excinfo: move( "file.txt", @@ -957,46 +891,29 @@ def test_move_both_dest_mismatch( @pytest.mark.parametrize( "work_on", [MoveWorkOn.LOCAL, MoveWorkOn.REMOTE, MoveWorkOn.BOTH] ) -def test_move_pyout( - monkeypatch: pytest.MonkeyPatch, - moving_dandiset: SampleDandiset, - work_on: MoveWorkOn, -) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) - move( - "file.txt", - "subdir4/foo.json", - dest="subdir5", - work_on=work_on, - existing=MoveExisting.OVERWRITE, - devel_debug=False, - dandi_instance=moving_dandiset.api.instance_id, - ) - check_assets( - moving_dandiset, - starting_assets, - work_on, - { - "file.txt": "subdir5/file.txt", - "subdir4/foo.json": "subdir5/foo.json", - "subdir5/foo.json": None, - }, - ) - - @pytest.mark.parametrize( - "work_on", [MoveWorkOn.LOCAL, MoveWorkOn.REMOTE, MoveWorkOn.BOTH] + "dry_run,remapping", + [ + ( + False, + { + "file.txt": "subdir5/file.txt", + "subdir4/foo.json": "subdir5/foo.json", + "subdir5/foo.json": None, + }, + ), + (True, {}), + ], + ids=["moved", "dry_run"], ) -def test_move_pyout_dry_run( +def test_move_pyout( monkeypatch: pytest.MonkeyPatch, moving_dandiset: SampleDandiset, work_on: MoveWorkOn, + dry_run: bool, + remapping: dict[str, str | None], ) -> None: - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) move( "file.txt", "subdir4/foo.json", @@ -1004,10 +921,10 @@ def test_move_pyout_dry_run( work_on=work_on, existing=MoveExisting.OVERWRITE, devel_debug=False, - dry_run=True, + dry_run=dry_run, dandi_instance=moving_dandiset.api.instance_id, ) - check_assets(moving_dandiset, starting_assets, work_on, {}) + check_assets(moving_dandiset, starting_assets, work_on, remapping) @pytest.mark.parametrize( @@ -1020,9 +937,9 @@ def test_move_path_to_self( work_on: MoveWorkOn, ) -> None: (moving_dandiset.dspath / "newdir").mkdir() - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath / "subdir1") - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move( + monkeypatch, moving_dandiset, moving_dandiset.dspath / "subdir1" + ) move( "apple.txt", dest="../subdir1", @@ -1048,9 +965,7 @@ def test_move_remote_dest_is_local_dir_sans_slash( monkeypatch: pytest.MonkeyPatch, moving_dandiset: SampleDandiset ) -> None: (moving_dandiset.dspath / "newdir").mkdir() - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) move( "file.txt", dest="newdir", @@ -1067,9 +982,7 @@ def test_move_both_dest_is_local_dir_sans_slash( monkeypatch: pytest.MonkeyPatch, moving_dandiset: SampleDandiset ) -> None: (moving_dandiset.dspath / "newdir").mkdir() - starting_assets = list(moving_dandiset.dandiset.get_assets()) - monkeypatch.chdir(moving_dandiset.dspath) - moving_dandiset.api.monkeypatch_set_api_key_env(monkeypatch) + starting_assets = _setup_move(monkeypatch, moving_dandiset, moving_dandiset.dspath) move( "file.txt", dest="newdir", diff --git a/dandi/tests/test_upload.py b/dandi/tests/test_upload.py index 22e1a09de..eb436cd4c 100644 --- a/dandi/tests/test_upload.py +++ b/dandi/tests/test_upload.py @@ -466,10 +466,9 @@ def test_upload_bids_zarr( bids_zarr_dandiset.upload() -def test_upload_different_zarr(tmp_path: Path, zarr_dandiset: SampleDandiset) -> None: - asset = zarr_dandiset.dandiset.get_asset_by_path("sample.zarr") - assert isinstance(asset, RemoteZarrAsset) - zarr_id = asset.zarr +def _replace_zarr_and_check( + tmp_path: Path, zarr_dandiset: SampleDandiset, zarr_id: str +) -> None: rmtree(zarr_dandiset.dspath / "sample.zarr") zarr.save(zarr_dandiset.dspath / "sample.zarr", np.eye(5)) zarr_dandiset.upload() @@ -483,22 +482,18 @@ def test_upload_different_zarr(tmp_path: Path, zarr_dandiset: SampleDandiset) -> ) +def test_upload_different_zarr(tmp_path: Path, zarr_dandiset: SampleDandiset) -> None: + asset = zarr_dandiset.dandiset.get_asset_by_path("sample.zarr") + assert isinstance(asset, RemoteZarrAsset) + _replace_zarr_and_check(tmp_path, zarr_dandiset, asset.zarr) + + def test_upload_loose_zarr(tmp_path: Path, zarr_dandiset: SampleDandiset) -> None: asset = zarr_dandiset.dandiset.get_asset_by_path("sample.zarr") assert isinstance(asset, RemoteZarrAsset) zarr_id = asset.zarr asset.delete() - rmtree(zarr_dandiset.dspath / "sample.zarr") - zarr.save(zarr_dandiset.dspath / "sample.zarr", np.eye(5)) - zarr_dandiset.upload() - asset = zarr_dandiset.dandiset.get_asset_by_path("sample.zarr") - assert isinstance(asset, RemoteZarrAsset) - assert asset.zarr == zarr_id - download(zarr_dandiset.dandiset.version_api_url, tmp_path) - assert_dirtrees_eq( - zarr_dandiset.dspath / "sample.zarr", - tmp_path / zarr_dandiset.dandiset_id / "sample.zarr", - ) + _replace_zarr_and_check(tmp_path, zarr_dandiset, zarr_id) def test_upload_different_zarr_entry_conflicts( @@ -832,30 +827,35 @@ def mock_put(self, url, **kwargs): ), "All-same-type failures should be flagged as systematic" -@pytest.mark.ai_generated -def test_upload_rejects_dandidownload_paths( - new_dandiset: SampleDandiset, tmp_path: Path +def _assert_upload_rejects_dandidownload( + new_dandiset: SampleDandiset, badfile_path: Path, identifier: str ) -> None: - """Test that upload rejects assets with .dandidownload paths""" - dspath = new_dandiset.dspath - - # Test 1: Regular file with .dandidownload in path - badfile_path = dspath / f"test{DOWNLOAD_SUFFIX}" / "file.nwb" - badfile_path.parent.mkdir(parents=True) make_nwb_file( badfile_path, session_description="test session", - identifier="test123", + identifier=identifier, session_start_time=datetime(2017, 4, 15, 12, tzinfo=timezone.utc), subject=pynwb.file.Subject(subject_id="test"), ) - with pytest.raises( UploadError, match=f"contains {DOWNLOAD_SUFFIX} path which indicates incomplete download", ): new_dandiset.upload(allow_any_path=True) + +@pytest.mark.ai_generated +def test_upload_rejects_dandidownload_paths( + new_dandiset: SampleDandiset, tmp_path: Path +) -> None: + """Test that upload rejects assets with .dandidownload paths""" + dspath = new_dandiset.dspath + + # Test 1: Regular file with .dandidownload in path + badfile_path = dspath / f"test{DOWNLOAD_SUFFIX}" / "file.nwb" + badfile_path.parent.mkdir(parents=True) + _assert_upload_rejects_dandidownload(new_dandiset, badfile_path, "test123") + # Clean up for next test rmtree(badfile_path.parent) @@ -900,37 +900,15 @@ def test_upload_rejects_dandidownload_paths( @pytest.mark.ai_generated def test_upload_rejects_dandidownload_nwb_file(new_dandiset: SampleDandiset) -> None: """Test that upload rejects NWB files with .dandidownload in their path""" - dspath = new_dandiset.dspath - # Create an NWB file with .dandidownload in its name - bad_nwb_path = dspath / f"test{DOWNLOAD_SUFFIX}.nwb" - make_nwb_file( - bad_nwb_path, - session_description="test session", - identifier="test456", - session_start_time=datetime(2017, 4, 15, 12, tzinfo=timezone.utc), - subject=pynwb.file.Subject(subject_id="test"), - ) - - with pytest.raises( - UploadError, - match=f"contains {DOWNLOAD_SUFFIX} path which indicates incomplete download", - ): - new_dandiset.upload(allow_any_path=True) + bad_nwb_path = new_dandiset.dspath / f"test{DOWNLOAD_SUFFIX}.nwb" + _assert_upload_rejects_dandidownload(new_dandiset, bad_nwb_path, "test456") # ---------- Partial Zarr upload (patch mode) tests ---------- -@pytest.mark.ai_generated -def test_upload_zarr_patch_mode_no_delete( - tmp_path: Path, zarr_dandiset: SampleDandiset -) -> None: - """Upload with patch mode should NOT delete remote-only files.""" - asset = zarr_dandiset.dandiset.get_asset_by_path("sample.zarr") - assert isinstance(asset, RemoteZarrAsset) - - # Delete a local file so it becomes remote-only +def _remove_a_zarr_data_file(zarr_dandiset: SampleDandiset) -> str: local_zarr = zarr_dandiset.dspath / "sample.zarr" # Find and remove a data file (not .zgroup etc) data_files = [ @@ -940,6 +918,19 @@ def test_upload_zarr_patch_mode_no_delete( removed_file = data_files[0] removed_relpath = removed_file.relative_to(local_zarr).as_posix() removed_file.unlink() + return removed_relpath + + +@pytest.mark.ai_generated +def test_upload_zarr_patch_mode_no_delete( + tmp_path: Path, zarr_dandiset: SampleDandiset +) -> None: + """Upload with patch mode should NOT delete remote-only files.""" + asset = zarr_dandiset.dandiset.get_asset_by_path("sample.zarr") + assert isinstance(asset, RemoteZarrAsset) + + # Delete a local file so it becomes remote-only + removed_relpath = _remove_a_zarr_data_file(zarr_dandiset) # Upload in patch mode — remote file should be preserved zarr_dandiset.upload(zarr_mode="patch") @@ -961,14 +952,7 @@ def test_upload_zarr_full_mode_delete( assert isinstance(asset, RemoteZarrAsset) # Delete a local file - local_zarr = zarr_dandiset.dspath / "sample.zarr" - data_files = [ - f for f in list_paths(local_zarr) if f.is_file() and not f.name.startswith(".") - ] - assert data_files, "Expected data files in zarr" - removed_file = data_files[0] - removed_relpath = removed_file.relative_to(local_zarr).as_posix() - removed_file.unlink() + removed_relpath = _remove_a_zarr_data_file(zarr_dandiset) # Upload in full mode — remote file should be deleted zarr_dandiset.upload(zarr_mode="full") diff --git a/dandi/tests/test_utils.py b/dandi/tests/test_utils.py index 3a15043c5..1208e6351 100644 --- a/dandi/tests/test_utils.py +++ b/dandi/tests/test_utils.py @@ -172,21 +172,22 @@ def test_flatten() -> None: ] +SERVER_INFO = { + "version": "1.0.0", + "cli-minimal-version": "0.5.0", + "cli-bad-versions": [], + "services": { + "webui": {"url": "https://gui.dandi"}, + "api": {"url": "https://api.dandi"}, + "jupyterhub": {"url": "https://hub.dandi"}, + }, +} + + @responses.activate def test_get_instance_dandi_with_api() -> None: responses.add( - responses.GET, - "https://api.dandiarchive.org/api/info/", - json={ - "version": "1.0.0", - "cli-minimal-version": "0.5.0", - "cli-bad-versions": [], - "services": { - "webui": {"url": "https://gui.dandi"}, - "api": {"url": "https://api.dandi"}, - "jupyterhub": {"url": "https://hub.dandi"}, - }, - }, + responses.GET, "https://api.dandiarchive.org/api/info/", json=SERVER_INFO ) _get_instance.cache_clear() assert get_instance("dandi") == DandiInstance( @@ -198,20 +199,7 @@ def test_get_instance_dandi_with_api() -> None: @responses.activate def test_get_instance_url() -> None: - responses.add( - responses.GET, - "https://example.dandi/server-info", - json={ - "version": "1.0.0", - "cli-minimal-version": "0.5.0", - "cli-bad-versions": [], - "services": { - "webui": {"url": "https://gui.dandi"}, - "api": {"url": "https://api.dandi"}, - "jupyterhub": {"url": "https://hub.dandi"}, - }, - }, - ) + responses.add(responses.GET, "https://example.dandi/server-info", json=SERVER_INFO) _get_instance.cache_clear() assert get_instance("https://example.dandi/") == DandiInstance( name="api.dandi", diff --git a/tools/check-duplication.sh b/tools/check-duplication.sh new file mode 100755 index 000000000..fd88c70cc --- /dev/null +++ b/tools/check-duplication.sh @@ -0,0 +1,21 @@ +#!/bin/bash +# Code duplication detection. +# Run via: tox -e duplication +# Requires: npx (Node.js) + +set -eu + +# Pinned so a new jscpd release can't change behavior or fail CI on an +# otherwise-unchanged commit. Bump deliberately (and re-verify) when needed. +JSCPD_VERSION=5.3.3 + +if ! command -v npx >/dev/null 2>&1; then + echo "ERROR: npx not found. Install Node.js to run duplication checks." + exit 1 +fi + +echo "=== Code duplication check (jscpd ${JSCPD_VERSION}) ===" +echo + +# jscpd reads .jscpd.json for config (threshold, ignores, etc.) +npx --yes "jscpd@${JSCPD_VERSION}" dandi tools diff --git a/tox.ini b/tox.ini index f40c8230a..0fb5949b8 100644 --- a/tox.ini +++ b/tox.ini @@ -1,7 +1,7 @@ [tox] requires = tox-uv >= 1.11 -envlist = lint,typing,py3,py3-lowest +envlist = lint,typing,duplication,py3,py3-lowest [testenv] setenv = @@ -40,6 +40,12 @@ commands = codespell dandi docs tools setup.py flake8 --config=setup.cfg {posargs} dandi setup.py +[testenv:duplication] +skip_install = true +allowlist_externals = bash +description = Detect code duplication +commands = bash {toxinidir}/tools/check-duplication.sh + [testenv:typing] deps = mypy != 1.11.0