Better S3 Support for MagViews #1278
Conversation
…o better_s3_mag_views
| ) | ||
| ) | ||
| or (endpoint_url := path.storage_options.get("endpoint_url", None)) | ||
| or (endpoint_url := os.environ.get("S3_ENDPOINT_URL", None)) |
There was a problem hiding this comment.
Check whether this is required, would be better to always infer endpoint_url from UPath.
There was a problem hiding this comment.
I think we should rely on the UPath
|
@normanrz @markbader I think we should discuss this PR. The proposed solution changes/redefines the behavior of Maybe we should discuss what the expected behavior should be and how to create a new dataset layer with some (not all) of its mags pointing to mags of other (remote) datasets? |
| ) | ||
| ) | ||
| or (endpoint_url := path.storage_options.get("endpoint_url", None)) | ||
| or (endpoint_url := os.environ.get("S3_ENDPOINT_URL", None)) |
There was a problem hiding this comment.
I think we should rely on the UPath
| @@ -780,6 +787,7 @@ def create(cls, path: Path, array_info: ArrayInfo) -> "Zarr3Array": | |||
| ], | |||
| }, | |||
| "create": True, | |||
| "open": True, | |||
| if maybe_layer_path and is_remote | ||
| else self.dataset.path / self.name | ||
| ) | ||
| return self.dataset.path / self.name |
There was a problem hiding this comment.
Have you checked the usages of Layer.path in wklibs and vx?
| self._properties.mags.append( | ||
| MagViewProperties( | ||
| mag=mag_view.mag, | ||
| path=str(mag_view.path), |
There was a problem hiding this comment.
Just casting str(..) probably doesn't work, because wk expects a different syntax for S3 paths, i.e. s3://{endpoint_url/{bucket_name}/{prefix} instead of s3://{bucket_name}/{prefix}
Description:
layer.pathfrom the paths of the layer's mags, which leads to problems for layers with both remote and local magslayer.pathto always bedataset_path / layer.nameTodos:
Make sure to delete unnecessary points or to check all before merging: