-
Notifications
You must be signed in to change notification settings - Fork 37
Add support for partial Zarr download and upload #1816
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
base: master
Are you sure you want to change the base?
Changes from all commits
b6de389
c168643
16b25ec
a9d61f4
7e306ff
e32fb86
d43f363
d63131b
1a52291
4c24ad0
8a3efee
815fa19
a0d2038
f78e8c3
697cd9f
e63a301
741730b
bc632ad
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,7 +12,7 @@ | |
| ) | ||
| from ..consts import SyncMode | ||
| from ..exceptions import UploadValidationError | ||
| from ..upload import UploadExisting, UploadValidation | ||
| from ..upload import UploadExisting, UploadValidation, ZarrMode | ||
|
|
||
|
|
||
| @click.command() | ||
|
|
@@ -58,6 +58,16 @@ | |
| default="require", | ||
| show_default=True, | ||
| ) | ||
| @click.option( | ||
| "--zarr-mode", | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same comment here. we are increasing the CLI surface and should ask if these should be there or inferred.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and the same reply here: it is not about the fact that we are dealing with zarr, but rather an option specific if working with zarr(s). Let's also add shortcut |
||
| type=click.Choice(list(ZarrMode)), | ||
| default="full", | ||
| help=( | ||
| "Zarr sync mode: 'full' (default) syncs completely; " | ||
| "'patch' uploads/updates without deleting remote files." | ||
| ), | ||
| show_default=True, | ||
| ) | ||
| @click.argument("paths", nargs=-1) # , type=click.Path(exists=True, dir_okay=False)) | ||
| # & | ||
| # Development options: Set DANDI_DEVEL for them to become available | ||
|
|
@@ -85,6 +95,7 @@ def upload( | |
| dandi_instance: str, | ||
| existing: UploadExisting, | ||
| validation: UploadValidation, | ||
| zarr_mode: ZarrMode, | ||
| # Development options should come as kwargs | ||
| allow_any_path: bool = False, | ||
| upload_dandiset_metadata: bool = False, | ||
|
|
@@ -120,19 +131,21 @@ def upload( | |
| validation_companion_path(ctx.obj.logfile) if ctx.obj is not None else None | ||
| ) | ||
|
|
||
|
|
||
| try: | ||
| upload_( | ||
| paths, | ||
| existing=existing, | ||
| validation=validation, | ||
| dandi_instance=dandi_instance, | ||
| allow_any_path=allow_any_path, | ||
| upload_dandiset_metadata=upload_dandiset_metadata, | ||
| devel_debug=devel_debug, | ||
| jobs=jobs, | ||
| jobs_per_file=jobs_per_file, | ||
| sync=SyncMode(sync) if sync is not None else None, | ||
| validation_log_path=companion, | ||
| ) | ||
| upload_( | ||
| paths, | ||
| existing=existing, | ||
| validation=validation, | ||
| dandi_instance=dandi_instance, | ||
| allow_any_path=allow_any_path, | ||
| upload_dandiset_metadata=upload_dandiset_metadata, | ||
| devel_debug=devel_debug, | ||
| jobs=jobs, | ||
| jobs_per_file=jobs_per_file, | ||
| sync=SyncMode(sync) if sync is not None else None, | ||
| zarr_mode=zarr_mode, | ||
| validation_log_path=companion, | ||
| ) | ||
| except UploadValidationError as exc: | ||
| raise click.ClickException(str(exc)) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,6 +47,7 @@ | |
| REDIRECT_HEAD_TIMEOUT, | ||
| RETRY_STATUSES, | ||
| VERSION_REGEX, | ||
| ZARR_EXTENSIONS, | ||
| DandiInstance, | ||
| known_instances, | ||
| ) | ||
|
|
@@ -446,6 +447,72 @@ def get_assets( | |
| ) | ||
|
|
||
|
|
||
| def split_zarr_location(location: str) -> tuple[str, str] | None: | ||
| """Split a location into ``(asset_path, zarr_subpath)`` if it crosses a zarr boundary. | ||
|
|
||
| Scans path components for segments ending with a Zarr extension | ||
| (``.zarr``, ``.ngff``). If found **and** there are remaining components | ||
| after the boundary, returns the split; otherwise returns ``None``. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| location : str | ||
| A POSIX-style path, e.g. ``"sub-1/file.ome.zarr/0/0/0"``. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. zarr doesn't have to have a
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if sharded -- don't we upload it "as is" with all its shardiness? it is a new info for me and I guess refers to good to know but I wonder if that is an issue bigger than this "docstring fix" e.g.
likely worth a concerted effort to support such ones (unless I am wrong and we already do). Pragmatically, does any of the actively working with DANDI groups use alternative separators @satra @kabilar ? I think worth a separate issue, and if you see what to change here specifically -- please leave a suggestion @satra |
||
|
|
||
| Returns | ||
| ------- | ||
| tuple[str, str] | None | ||
| ``(asset_path, zarr_subpath)`` when a zarr boundary with a subpath | ||
| is detected, otherwise ``None``. | ||
|
|
||
| Examples | ||
| -------- | ||
| >>> split_zarr_location("sub-1/file.ome.zarr/0/0/0") | ||
| ('sub-1/file.ome.zarr', '0/0/0') | ||
| >>> split_zarr_location("sub-1/file.ome.zarr") # no subpath | ||
| >>> split_zarr_location("sub-1/file.nwb") # no zarr extension | ||
| """ | ||
| parts = [p for p in location.split("/") if p] | ||
| for i, part in enumerate(parts): | ||
| if any(part.endswith(ext) for ext in ZARR_EXTENSIONS): | ||
| asset_path = "/".join(parts[: i + 1]) | ||
| zarr_subpath = "/".join(parts[i + 1 :]) | ||
| return (asset_path, zarr_subpath) if zarr_subpath else None | ||
| return None | ||
|
|
||
|
|
||
| @dataclass | ||
| class AssetZarrEntryURL(SingleAssetURL): | ||
| """Parsed from a URL that points into entries within a Zarr asset. | ||
|
|
||
| For example, ``dandi://dandi/000108/sub-1/file.ome.zarr/0/0/0`` would | ||
| produce ``asset_path="sub-1/file.ome.zarr"`` and ``zarr_subpath="0/0/0"``. | ||
| """ | ||
|
|
||
| asset_path: str # e.g., "sub-1/file.ome.zarr" | ||
| zarr_subpath: str # e.g., "0/0/0" | ||
|
|
||
| def get_assets( | ||
| self, client: DandiAPIClient, order: str | None = None, strict: bool = False | ||
| ) -> Iterator[BaseRemoteAsset]: | ||
| """Yield the zarr asset whose path equals ``asset_path``. | ||
|
|
||
| If the asset does not exist, a `NotFoundError` is raised when | ||
| ``strict`` is true; otherwise nothing is yielded. | ||
| """ | ||
| try: | ||
| dandiset = self.get_dandiset(client, lazy=not strict) | ||
| assert dandiset is not None | ||
| dandiset.version_id # Force version evaluation | ||
| except NotFoundError: | ||
| if strict: | ||
| raise | ||
| else: | ||
| return | ||
| with _maybe_strict(strict): | ||
| yield dandiset.get_asset_by_path(self.asset_path) | ||
|
|
||
|
|
||
| @dataclass | ||
| class AssetFolderURL(MultiAssetURL): | ||
| """ | ||
|
|
@@ -846,12 +913,24 @@ def parse( | |
| path=location, | ||
| ) | ||
| else: | ||
| parsed_url = AssetItemURL( | ||
| instance=instance, | ||
| dandiset_id=dandiset_id, | ||
| version_id=version_id, | ||
| path=location, | ||
| ) | ||
| # Check if location crosses a zarr boundary | ||
| zarr_split = split_zarr_location(location) | ||
| if zarr_split is not None: | ||
| asset_path, zarr_subpath = zarr_split | ||
| parsed_url = AssetZarrEntryURL( | ||
| instance=instance, | ||
| dandiset_id=dandiset_id, | ||
| version_id=version_id, | ||
| asset_path=asset_path, | ||
| zarr_subpath=zarr_subpath, | ||
| ) | ||
| else: | ||
| parsed_url = AssetItemURL( | ||
| instance=instance, | ||
| dandiset_id=dandiset_id, | ||
| version_id=version_id, | ||
| path=location, | ||
| ) | ||
| elif asset_id: | ||
| if dandiset_id is None: | ||
| parsed_url = BaseAssetIDURL(instance=instance, asset_id=asset_id) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be an explicit option or inferred from metadata like neuroglancer does? or a hint when the user/agent knows specifically to do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we infer already based on extensions IIRC; this is just an a new option for filter... I guess we might better call it that
--zarr-filter(with--z-fshortcut)