From 445c898eedddd6072cf953efc592de15b52920ce Mon Sep 17 00:00:00 2001 From: Kabilar Gunalan Date: Mon, 25 Nov 2024 16:14:33 -0600 Subject: [PATCH 1/6] Remove `NotImplementedError` to allow for uploading Zarr assets to embargoed Dandisets --- dandi/files/zarr.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/dandi/files/zarr.py b/dandi/files/zarr.py index 1c5140c08..9682cc61e 100644 --- a/dandi/files/zarr.py +++ b/dandi/files/zarr.py @@ -297,12 +297,6 @@ def iter_upload( ``"done"`` and an ``"asset"`` key containing the resulting `RemoteAsset`. """ - # So that older clients don't get away with doing the wrong thing once - # Zarr upload to embargoed Dandisets is implemented in the API: - if dandiset.embargo_status is EmbargoStatus.EMBARGOED: - raise NotImplementedError( - "Uploading Zarr assets to embargoed Dandisets is currently not implemented" - ) asset_path = metadata.setdefault("path", self.path) client = dandiset.client lgr.debug("%s: Producing asset", asset_path) From 07c840bde6a56b4a8d2c30396f05f8346f21a3df Mon Sep 17 00:00:00 2001 From: Kabilar Gunalan Date: Mon, 25 Nov 2024 16:46:14 -0600 Subject: [PATCH 2/6] Remove import --- dandi/files/zarr.py | 1 - 1 file changed, 1 deletion(-) diff --git a/dandi/files/zarr.py b/dandi/files/zarr.py index 9682cc61e..6b426fee4 100644 --- a/dandi/files/zarr.py +++ b/dandi/files/zarr.py @@ -22,7 +22,6 @@ ZARR_DELETE_BATCH_SIZE, ZARR_MIME_TYPE, ZARR_UPLOAD_BATCH_SIZE, - EmbargoStatus, ) from dandi.dandiapi import ( RemoteAsset, From c56665cb0ce8b53854f28fe0b66fd237b30ed90d Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Tue, 26 Nov 2024 16:54:34 -0500 Subject: [PATCH 3/6] RF + ENH: parametrize test_upload_sync_zarr to test against both embargoed and not dataset I had to duplicate initiation from zarr_dandiset fixture, but seems overall a small price to pay. Subsequent commit would refactor to make sweeping embargoed status on new_dataset easier. --- dandi/tests/test_upload.py | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/dandi/tests/test_upload.py b/dandi/tests/test_upload.py index 2ba5bf35f..05440f360 100644 --- a/dandi/tests/test_upload.py +++ b/dandi/tests/test_upload.py @@ -11,9 +11,9 @@ from pytest_mock import MockerFixture import zarr -from .fixtures import SampleDandiset +from .fixtures import SampleDandiset, SampleDandisetFactory from .test_helpers import assert_dirtrees_eq -from ..consts import ZARR_MIME_TYPE, dandiset_metadata_file +from ..consts import ZARR_MIME_TYPE, EmbargoStatus, dandiset_metadata_file from ..dandiapi import AssetType, RemoteBlobAsset, RemoteZarrAsset from ..dandiset import Dandiset from ..download import download @@ -250,7 +250,21 @@ def test_upload_bids_non_nwb_file(bids_dandiset: SampleDandiset) -> None: assert [asset.path for asset in bids_dandiset.dandiset.get_assets()] == ["README"] -def test_upload_sync_zarr(mocker, zarr_dandiset): +@pytest.mark.parametrize("embargo", [True, False]) +def test_upload_sync_zarr( + mocker, sample_dandiset_factory: SampleDandisetFactory, embargo: bool +) -> None: + zarr_dandiset = sample_dandiset_factory.mkdandiset( + f"Sample {'embargoed' if embargo else 'public'} Dandiset", + embargo=embargo, + ) + assert zarr_dandiset.dandiset.embargo_status == ( + EmbargoStatus.EMBARGOED if embargo else EmbargoStatus.OPEN + ) + zarr.save( + zarr_dandiset.dspath / "sample.zarr", np.arange(1000), np.arange(1000, 0, -1) + ) + zarr_dandiset.upload() rmtree(zarr_dandiset.dspath / "sample.zarr") zarr.save(zarr_dandiset.dspath / "identity.zarr", np.eye(5)) confirm_mock = mocker.patch("click.confirm", return_value=True) From 1deba3578c8061e9eb5556841488c646baf3a84d Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Tue, 26 Nov 2024 20:10:13 -0500 Subject: [PATCH 4/6] RF: make any "new_dandiset" fixture work together with @sweep_embargo --- dandi/tests/fixtures.py | 13 ++++++++++++- dandi/tests/test_upload.py | 16 +++------------- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/dandi/tests/fixtures.py b/dandi/tests/fixtures.py index 7e16ce3b1..ceaa2b7ab 100644 --- a/dandi/tests/fixtures.py +++ b/dandi/tests/fixtures.py @@ -610,12 +610,23 @@ def sample_dandiset_factory( return SampleDandisetFactory(local_dandi_api, tmp_path_factory) +# @sweep_embargo can be used along with `new_dandiset` fixture to +# run the test against both embargoed and non-embargoed Dandisets. +# "embargo: bool" argument should be added to the test function. +sweep_embargo = pytest.mark.parametrize("embargo", [True, False]) + + @pytest.fixture() def new_dandiset( request: pytest.FixtureRequest, sample_dandiset_factory: SampleDandisetFactory ) -> SampleDandiset: + kws = {} + if "embargo" in request.node.fixturenames: + kws["embargo"] = request.node.callspec.params["embargo"] return sample_dandiset_factory.mkdandiset( - f"Sample Dandiset for {request.node.name}" + f"Sample {'Embargoed' if kws.get('embargo') else 'Public'} " + f"Dandiset for {request.node.name}", + **kws, ) diff --git a/dandi/tests/test_upload.py b/dandi/tests/test_upload.py index 05440f360..4e0c50c96 100644 --- a/dandi/tests/test_upload.py +++ b/dandi/tests/test_upload.py @@ -11,7 +11,7 @@ from pytest_mock import MockerFixture import zarr -from .fixtures import SampleDandiset, SampleDandisetFactory +from .fixtures import SampleDandiset, sweep_embargo from .test_helpers import assert_dirtrees_eq from ..consts import ZARR_MIME_TYPE, EmbargoStatus, dandiset_metadata_file from ..dandiapi import AssetType, RemoteBlobAsset, RemoteZarrAsset @@ -250,21 +250,11 @@ def test_upload_bids_non_nwb_file(bids_dandiset: SampleDandiset) -> None: assert [asset.path for asset in bids_dandiset.dandiset.get_assets()] == ["README"] -@pytest.mark.parametrize("embargo", [True, False]) -def test_upload_sync_zarr( - mocker, sample_dandiset_factory: SampleDandisetFactory, embargo: bool -) -> None: - zarr_dandiset = sample_dandiset_factory.mkdandiset( - f"Sample {'embargoed' if embargo else 'public'} Dandiset", - embargo=embargo, - ) +@sweep_embargo +def test_upload_sync_zarr(mocker, zarr_dandiset: SampleDandiset, embargo: bool) -> None: assert zarr_dandiset.dandiset.embargo_status == ( EmbargoStatus.EMBARGOED if embargo else EmbargoStatus.OPEN ) - zarr.save( - zarr_dandiset.dspath / "sample.zarr", np.arange(1000), np.arange(1000, 0, -1) - ) - zarr_dandiset.upload() rmtree(zarr_dandiset.dspath / "sample.zarr") zarr.save(zarr_dandiset.dspath / "identity.zarr", np.eye(5)) confirm_mock = mocker.patch("click.confirm", return_value=True) From 60ccca22ff77facb19b0f302ae5791b38a16e004 Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Tue, 26 Nov 2024 20:20:29 -0500 Subject: [PATCH 5/6] ENH: Mark one more dataset testing on simpler files on embargo"ed version sync --- dandi/tests/test_upload.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/dandi/tests/test_upload.py b/dandi/tests/test_upload.py index 4e0c50c96..1bd372414 100644 --- a/dandi/tests/test_upload.py +++ b/dandi/tests/test_upload.py @@ -151,9 +151,10 @@ def test_upload_download_small_file( assert (tmp_path / dandiset_id / "file.txt").read_bytes() == contents +@sweep_embargo @pytest.mark.parametrize("confirm", [True, False]) def test_upload_sync( - confirm: bool, mocker: MockerFixture, text_dandiset: SampleDandiset + confirm: bool, mocker: MockerFixture, text_dandiset: SampleDandiset, embargo: bool ) -> None: (text_dandiset.dspath / "file.txt").unlink() confirm_mock = mocker.patch("click.confirm", return_value=confirm) From 2a1c5eddd45250ae3eaf7905788d938211f17d90 Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Wed, 27 Nov 2024 08:57:51 -0500 Subject: [PATCH 6/6] Add type annotation for mocker Co-authored-by: John T. Wodder II --- dandi/tests/test_upload.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/dandi/tests/test_upload.py b/dandi/tests/test_upload.py index 1bd372414..497310e48 100644 --- a/dandi/tests/test_upload.py +++ b/dandi/tests/test_upload.py @@ -252,7 +252,9 @@ def test_upload_bids_non_nwb_file(bids_dandiset: SampleDandiset) -> None: @sweep_embargo -def test_upload_sync_zarr(mocker, zarr_dandiset: SampleDandiset, embargo: bool) -> None: +def test_upload_sync_zarr( + mocker: MockerFixture, zarr_dandiset: SampleDandiset, embargo: bool +) -> None: assert zarr_dandiset.dandiset.embargo_status == ( EmbargoStatus.EMBARGOED if embargo else EmbargoStatus.OPEN )