Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove NotImplementedError to allow for uploading Zarr assets to embargoed Dandisets #1540

Merged
merged 6 commits into from
Nov 27, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 0 additions & 7 deletions dandi/files/zarr.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
ZARR_DELETE_BATCH_SIZE,
ZARR_MIME_TYPE,
ZARR_UPLOAD_BATCH_SIZE,
EmbargoStatus,
)
from dandi.dandiapi import (
RemoteAsset,
Expand Down Expand Up @@ -297,12 +296,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:
kabilar marked this conversation as resolved.
Show resolved Hide resolved
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)
Expand Down
13 changes: 12 additions & 1 deletion dandi/tests/fixtures.py
Original file line number Diff line number Diff line change
Expand Up @@ -610,12 +610,23 @@
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"]

Check warning on line 625 in dandi/tests/fixtures.py

View check run for this annotation

Codecov / codecov/patch

dandi/tests/fixtures.py#L623-L625

Added lines #L623 - L625 were not covered by tests
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,
)


Expand Down
13 changes: 9 additions & 4 deletions dandi/tests/test_upload.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@
from pytest_mock import MockerFixture
import zarr

from .fixtures import SampleDandiset
from .fixtures import SampleDandiset, sweep_embargo
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
Expand Down Expand Up @@ -151,9 +151,10 @@
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)
Expand Down Expand Up @@ -250,7 +251,11 @@
assert [asset.path for asset in bids_dandiset.dandiset.get_assets()] == ["README"]


def test_upload_sync_zarr(mocker, zarr_dandiset):
@sweep_embargo
def test_upload_sync_zarr(mocker, zarr_dandiset: SampleDandiset, embargo: bool) -> None:
yarikoptic marked this conversation as resolved.
Show resolved Hide resolved
assert zarr_dandiset.dandiset.embargo_status == (

Check warning on line 256 in dandi/tests/test_upload.py

View check run for this annotation

Codecov / codecov/patch

dandi/tests/test_upload.py#L256

Added line #L256 was not covered by tests
EmbargoStatus.EMBARGOED if embargo else EmbargoStatus.OPEN
)
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)
Expand Down
Loading