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

Paths as URIs #243

Merged
merged 78 commits into from
Dec 3, 2024
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
78 commits
Select commit Hold shift + click to select a range
c787da4
validate that paths can be coerced to valid URIs
TomNicholas Oct 2, 2024
b2f3f26
add a test that paths are converted to URIs
TomNicholas Oct 2, 2024
6b15d9e
added test and better error if local path is not absolute
TomNicholas Oct 2, 2024
045cc7a
raise more informative error if path is not absolute
TomNicholas Oct 2, 2024
ddc6b7f
test that empty paths are allowed
TomNicholas Oct 2, 2024
61bf5d6
add failing test for raising on malformed paths
TomNicholas Oct 2, 2024
2896bd8
fix paths in tests
TomNicholas Oct 2, 2024
584adf9
fix more tests
TomNicholas Oct 2, 2024
14b8ca3
remove the file:/// prefix when writing to kerchunk format
TomNicholas Oct 2, 2024
bad0b1b
Merge branch 'main' into paths_as_uris
TomNicholas Oct 19, 2024
739a6bd
absolute paths in recently-added tests
TomNicholas Oct 19, 2024
06f3a4d
absolute paths in recently-added tests
TomNicholas Oct 19, 2024
3d50def
fix one more test
TomNicholas Oct 20, 2024
b37f8d0
Merge branch 'main' into paths_as_uris
TomNicholas Nov 21, 2024
a9bd0a3
stop wrapping specific error in less useful one
TomNicholas Nov 21, 2024
f0bc445
moved remaining kerchunk parsing logic out into translator file
TomNicholas Nov 21, 2024
b224af8
add fs_root parameter to validation fn
TomNicholas Nov 21, 2024
c8d513c
demote ChunkEntry to a TypedDict to separate validation fn
TomNicholas Nov 21, 2024
4ad26cf
actually instead add new constructor method to TypedDict
TomNicholas Nov 21, 2024
7aaaabf
test kerchunk writer with absolute paths
TomNicholas Nov 21, 2024
f66ed79
make kerchunk reader tests pass
TomNicholas Nov 21, 2024
e30ab05
try to implement the fs_root concatenation
TomNicholas Nov 21, 2024
0a4a11d
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Nov 21, 2024
98644a7
implementation using cloudpathlib working
TomNicholas Nov 22, 2024
4499cf9
check that fs_root doesn't have a filetype suffix
TomNicholas Nov 22, 2024
d27bdf6
add cloudpathlib to dependencies
TomNicholas Nov 22, 2024
fefab90
allow http paths, and require file suffixes
TomNicholas Nov 22, 2024
927121c
unit tests for path validation
TomNicholas Nov 22, 2024
c561a5d
test whether we can catch malformed paths
TomNicholas Nov 22, 2024
d42ff33
test fs_root
TomNicholas Nov 22, 2024
4b7cd2d
add (unimplemented) validate_entries kwarg to .from_arrays
TomNicholas Nov 23, 2024
0f8b550
add .keys(), .values(), .items()
TomNicholas Nov 23, 2024
8055ba7
test validate_paths during .from_arrays
TomNicholas Nov 23, 2024
f18bf59
ensure validation actually normalizes paths to uris
TomNicholas Nov 23, 2024
96f6360
test .rename_paths correctly validates paths
TomNicholas Nov 23, 2024
b2ed1cd
some release notes
TomNicholas Nov 23, 2024
87fad80
remove now-superfluous coercion to URI in icechunk writer
TomNicholas Nov 23, 2024
1f41d69
added link to icechunk writer performance benchmark
TomNicholas Nov 23, 2024
41ec08f
add reader_kwargs argument to open_virtual_dataset, and pass it down …
TomNicholas Nov 23, 2024
b4cc617
Merge branch 'main' into reader_kwargs
TomNicholas Nov 23, 2024
ffff93e
Merge branch 'main' into paths_as_uris
TomNicholas Nov 23, 2024
26e8950
Merge branch 'main' into paths_as_uris
TomNicholas Nov 23, 2024
19c13ae
ensure relative paths containing .. can be normalized
TomNicholas Nov 23, 2024
6ce7ebd
ensure HDF5 reader always returns absolute URIs
TomNicholas Nov 23, 2024
24b6ea7
ensure HDF reader always returns absolute URIs
TomNicholas Nov 23, 2024
5f0f362
add relative path handling to other kerchunk-based readers
TomNicholas Nov 23, 2024
001f95b
Merge branch 'reader_kwargs' into paths_as_uris
TomNicholas Nov 23, 2024
15ae139
add dmrpp relative path integration test
ayushnag Nov 24, 2024
b3432c6
Merge branch 'main' into paths_as_uris
TomNicholas Nov 26, 2024
ae00ac3
Merge branch 'main' into paths_as_uris
TomNicholas Nov 27, 2024
ddc19c3
Merge branch 'main' into filepath_test_dmrpp
TomNicholas Nov 27, 2024
8353a9b
fix kerchunk relative paths test by pluggin through fs_root kwarg
TomNicholas Nov 27, 2024
251469f
Merge branch 'filepath_test_dmrpp' of https://github.com/ayushnag/Vir…
TomNicholas Nov 27, 2024
0915433
fix dmrpp tests by using absolute filepaths in DMR++ contents
TomNicholas Nov 27, 2024
3ca8f7f
clarify new dmrpp test
TomNicholas Nov 27, 2024
8a90ec7
test handling of relative filepaths to dmrpp files
TomNicholas Nov 27, 2024
4afa34c
group related tests
TomNicholas Nov 27, 2024
da9ee10
removed cloudpathlib from validation code
TomNicholas Dec 1, 2024
3557008
fix bug but restrict fs_root to only handle filesystem paths, not buc…
TomNicholas Dec 2, 2024
ca09096
global list of recognized URI prefixes
TomNicholas Dec 2, 2024
7727d4e
cleanup
TomNicholas Dec 2, 2024
03aa6bc
remove cloudpathlib from dependencies
TomNicholas Dec 2, 2024
4d8ceea
fix/ignore some typing errors
TomNicholas Dec 2, 2024
cd5bec1
rewrite tests to use a new dmrparser_factory
TomNicholas Dec 3, 2024
e521e20
rewrite using global dict of XML strings
TomNicholas Dec 3, 2024
4ff17d0
fix final test by explicitly passing in tmp_path instead of using a f…
TomNicholas Dec 3, 2024
7e0be24
Merge branch 'dmrpp_tests_tmp_path' into paths_as_uris
TomNicholas Dec 3, 2024
bf0bb87
fix bug with not converting Path objects to strings
TomNicholas Dec 3, 2024
25acdf9
Merge branch 'dmrpp_tests_tmp_path' into paths_as_uris
TomNicholas Dec 3, 2024
ac04cef
dmrpp relative paths tests passing
TomNicholas Dec 3, 2024
39bbfaf
fix type hint for filetype kwarg
TomNicholas Dec 3, 2024
9b6906b
Merge branch 'main' into paths_as_uris
TomNicholas Dec 3, 2024
0127f05
user documentation on fs_root
TomNicholas Dec 3, 2024
cd5da5d
change example manifests to use URIs
TomNicholas Dec 3, 2024
df6663f
reminder that rename_paths exists
TomNicholas Dec 3, 2024
be20994
Merge branch 'paths_as_uris' of https://github.com/TomNicholas/Virtua…
TomNicholas Dec 3, 2024
528f722
update release notes
TomNicholas Dec 3, 2024
a8d5a15
remove note about .rename_paths
TomNicholas Dec 3, 2024
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
19 changes: 0 additions & 19 deletions virtualizarr/manifests/array.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
_isnan,
)
from virtualizarr.manifests.manifest import ChunkManifest
from virtualizarr.types.kerchunk import KerchunkArrRefs
from virtualizarr.zarr import ZArray


Expand Down Expand Up @@ -62,24 +61,6 @@ def __init__(
self._zarray = _zarray
self._manifest = _chunkmanifest

@classmethod
def _from_kerchunk_refs(cls, arr_refs: KerchunkArrRefs) -> "ManifestArray":
from virtualizarr.translators.kerchunk import (
fully_decode_arr_refs,
parse_array_refs,
)

decoded_arr_refs = fully_decode_arr_refs(arr_refs)

chunk_dict, zarray, _zattrs = parse_array_refs(decoded_arr_refs)
manifest = ChunkManifest._from_kerchunk_chunk_dict(chunk_dict)

obj = object.__new__(cls)
obj._manifest = manifest
obj._zarray = zarray

return obj

@property
def manifest(self) -> ChunkManifest:
return self._manifest
Expand Down
167 changes: 101 additions & 66 deletions virtualizarr/manifests/manifest.py
Original file line number Diff line number Diff line change
@@ -1,65 +1,118 @@
import dataclasses
import json
import re
from collections.abc import Iterable, Iterator
from typing import Any, Callable, Dict, NewType, Tuple, TypedDict, cast
from pathlib import Path
from typing import Any, Callable, NewType, Tuple, TypedDict, cast

import numpy as np

from virtualizarr.types import ChunkKey

VALID_URI_PREFIXES = {
"s3://",
"gs://",
"azure://",
"r2://",
"cos://",
"minio://",
"file:///",
}
_INTEGER = (
r"([1-9]+\d*|0)" # matches 0 or an unsigned integer that does not begin with zero
)
_SEPARATOR = r"\."
_CHUNK_KEY = rf"^{_INTEGER}+({_SEPARATOR}{_INTEGER})*$" # matches 1 integer, optionally followed by more integers each separated by a separator (i.e. a period)


class ChunkDictEntry(TypedDict):
class ChunkEntry(TypedDict):
path: str
offset: int
length: int

@classmethod
def with_validation(
cls, *, path: str, offset: int, length: int, fs_root: str | None = None
) -> "ChunkEntry":
"""
Constructor which validates each part of the chunk entry.

ChunkDict = NewType("ChunkDict", dict[ChunkKey, ChunkDictEntry])


@dataclasses.dataclass(frozen=True)
class ChunkEntry:
"""
Information for a single chunk in the manifest.
Parameters
----------
fs_root
The root of the filesystem on which these references were generated.
Required if any (likely kerchunk-generated) paths are relative in order to turn them into absolute paths (which virtualizarr requires).
"""

Stored in the form `{"path": "s3://bucket/foo.nc", "offset": 100, "length": 100}`.
"""
# note: we can't just use `__init__` or a dataclass' `__post_init__` because we need `fs_root` to be an optional kwarg

path: str # TODO stricter typing/validation of possible local / remote paths?
offset: int
length: int
path = validate_and_normalize_path_to_uri(path, fs_root=fs_root)

@classmethod
def from_kerchunk(
cls, path_and_byte_range_info: tuple[str] | tuple[str, int, int]
) -> "ChunkEntry":
from upath import UPath
if isinstance(offset, np.integer):
_offset = int(offset)
elif isinstance(offset, int):
_offset = offset
else:
raise TypeError(
f"chunk entry byte offset must of type int, but got type {type(offset)}"
)
if _offset < 0:
raise ValueError(
f"chunk entry byte offset must be a positive integer, but got offset={_offset}"
)

if len(path_and_byte_range_info) == 1:
path = path_and_byte_range_info[0]
offset = 0
length = UPath(path).stat().st_size
if isinstance(length, np.integer):
_length = int(length)
elif isinstance(length, int):
_length = length
else:
path, offset, length = path_and_byte_range_info
raise TypeError(
f"chunk entry byte offset must of type int, but got type {type(length)}"
)
if _length < 0:
raise ValueError(
f"chunk entry byte offset must be a positive integer, but got offset={_length}"
)

return ChunkEntry(path=path, offset=offset, length=length)

def to_kerchunk(self) -> tuple[str, int, int]:
"""Write out in the format that kerchunk uses for chunk entries."""
return (self.path, self.offset, self.length)

def dict(self) -> ChunkDictEntry:
return ChunkDictEntry(
path=self.path,
offset=self.offset,
length=self.length,
)
def validate_and_normalize_path_to_uri(path: str, fs_root: str | None = None) -> str:
"""
Makes all paths into fully-qualified absolute URIs, or raises

See https://en.wikipedia.org/wiki/File_URI_scheme

Parameters
----------
fs_root
The root of the filesystem on which these references were generated.
Required if any (likely kerchunk-generated) paths are relative in order to turn them into absolute paths (which virtualizarr requires).
"""
if not any(path.startswith(prefix) for prefix in VALID_URI_PREFIXES) and path != "":
# TODO refactor this logic?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen people use urlparse (https://docs.python.org/3/library/urllib.parse.html#urllib.parse.urlparse). Not sure if that would be easier to read or not.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that does look neater, thanks!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually handling all these cases is such a PITA that maybe I should just use cloudpathlib.AnyPath

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using cloudpathlib did make this very easy, at the cost of adding a new dependency. Luckily for this use case (string validation) it's only a pure-python standalone dependency.

try:
return str(Path(path).as_uri())
except ValueError as e:
if str(e) == "relative path can't be expressed as a file URI":
# problem is that path is relative instead of absolute
if fs_root is not None:
# use knowledge of filesystem root to convert to absolute path
return str(fs_root / Path(path).as_uri())
else:
# add context to error message that relative paths are forbidden
raise ValueError(
f"paths in the manifest must be absolute, but got {path}, and fs_root was not specified"
) from e
else:
# must be some other problem with the path
raise
else:
# (empty paths are allowed through as they represent missing chunks)
# TODO should we do other validation here? e.g. to prevent a malformed path like `file:///directory//filename.nc`?
return path


ChunkDict = NewType("ChunkDict", dict[ChunkKey, ChunkEntry])


class ChunkManifest:
Expand Down Expand Up @@ -124,20 +177,20 @@ def __init__(self, entries: dict, shape: tuple[int, ...] | None = None) -> None:

# populate the arrays
for key, entry in entries.items():
try:
path, offset, length = entry.values()
entry = ChunkEntry(path=path, offset=offset, length=length)
except (ValueError, TypeError) as e:
if not isinstance(entry, dict) or len(entry) != 3:
msg = (
"Each chunk entry must be of the form dict(path=<str>, offset=<int>, length=<int>), "
f"but got {entry}"
)
raise ValueError(msg) from e
raise ValueError(msg)

path, offset, length = entry.values()
entry = ChunkEntry.with_validation(path=path, offset=offset, length=length)

split_key = split(key)
paths[split_key] = entry.path
offsets[split_key] = entry.offset
lengths[split_key] = entry.length
paths[split_key] = entry["path"]
offsets[split_key] = entry["offset"]
lengths[split_key] = entry["length"]

self._paths = paths
self._offsets = offsets
Expand Down Expand Up @@ -249,12 +302,12 @@ def dict(self) -> ChunkDict: # type: ignore[override]

The returned dict will be of the form

{
"0.0.0": {"path": "s3://bucket/foo.nc", "offset": 100, "length": 100},
"0.0.1": {"path": "s3://bucket/foo.nc", "offset": 200, "length": 100},
"0.1.0": {"path": "s3://bucket/foo.nc", "offset": 300, "length": 100},
"0.1.1": {"path": "s3://bucket/foo.nc", "offset": 400, "length": 100},
}
| {
| "0.0.0": {"path": "s3://bucket/foo.nc", "offset": 100, "length": 100},
| "0.0.1": {"path": "s3://bucket/foo.nc", "offset": 200, "length": 100},
| "0.1.0": {"path": "s3://bucket/foo.nc", "offset": 300, "length": 100},
| "0.1.1": {"path": "s3://bucket/foo.nc", "offset": 400, "length": 100},
| }

Entries whose path is an empty string will be interpreted as missing chunks and omitted from the dictionary.
"""
Expand Down Expand Up @@ -301,24 +354,6 @@ def to_zarr_json(self, filepath: str) -> None:
with open(filepath, "w") as json_file:
json.dump(entries, json_file, indent=4, separators=(", ", ": "))

@classmethod
def _from_kerchunk_chunk_dict(
cls,
# The type hint requires `Dict` instead of `dict` due to
# the conflicting ChunkManifest.dict method.
kerchunk_chunk_dict: Dict[ChunkKey, str | tuple[str] | tuple[str, int, int]],
) -> "ChunkManifest":
chunk_entries: dict[ChunkKey, ChunkDictEntry] = {}
for k, v in kerchunk_chunk_dict.items():
if isinstance(v, (str, bytes)):
raise NotImplementedError(
"Reading inlined reference data is currently not supported. [ToDo]"
)
elif not isinstance(v, (tuple, list)):
raise TypeError(f"Unexpected type {type(v)} for chunk value: {v}")
chunk_entries[k] = ChunkEntry.from_kerchunk(v).dict()
return ChunkManifest(entries=chunk_entries)

def rename_paths(
self,
new: str | Callable[[str], str],
Expand Down
13 changes: 9 additions & 4 deletions virtualizarr/readers/hdf/hdf.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,13 @@

import numpy as np
import xarray as xr
from xarray import Index, Variable
from xarray import Dataset, Index, Variable

from virtualizarr.manifests import ChunkEntry, ChunkManifest, ManifestArray
from virtualizarr.manifests import (
ChunkEntry,
ChunkManifest,
ManifestArray,
)
from virtualizarr.readers.common import (
VirtualBackend,
construct_virtual_dataset,
Expand Down Expand Up @@ -99,11 +103,12 @@ def _dataset_chunk_manifest(path: str, dataset: Dataset) -> Optional[ChunkManife
else:
key_list = [0] * (len(dataset.shape) or 1)
key = ".".join(map(str, key_list))
chunk_entry = ChunkEntry(

chunk_entry = ChunkEntry.with_validation(
path=path, offset=dsid.get_offset(), length=dsid.get_storage_size()
)
chunk_key = ChunkKey(key)
chunk_entries = {chunk_key: chunk_entry.dict()}
chunk_entries = {chunk_key: chunk_entry}
chunk_manifest = ChunkManifest(entries=chunk_entries)
return chunk_manifest
else:
Expand Down
2 changes: 1 addition & 1 deletion virtualizarr/tests/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ def create_manifestarray(
def entry_from_chunk_key(ind: tuple[int, ...]) -> dict[str, str | int]:
"""Generate a (somewhat) unique manifest entry from a given chunk key"""
entry = {
"path": f"file.{str(join(ind))}.nc",
"path": f"/foo.{str(join(ind))}.nc",
"offset": offset_from_chunk_key(ind),
"length": length_from_chunk_key(ind),
}
Expand Down
6 changes: 3 additions & 3 deletions virtualizarr/tests/test_integration.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@
def test_kerchunk_roundtrip_in_memory_no_concat():
# Set up example xarray dataset
chunks_dict = {
"0.0": {"path": "foo.nc", "offset": 100, "length": 100},
"0.1": {"path": "foo.nc", "offset": 200, "length": 100},
"0.0": {"path": "/foo.nc", "offset": 100, "length": 100},
"0.1": {"path": "/foo.nc", "offset": 200, "length": 100},
}
manifest = ChunkManifest(entries=chunks_dict)
marr = ManifestArray(
Expand Down Expand Up @@ -239,7 +239,7 @@ def test_non_dimension_coordinates(self, tmpdir, format, hdf_backend):

def test_datetime64_dtype_fill_value(self, tmpdir, format):
chunks_dict = {
"0.0.0": {"path": "foo.nc", "offset": 100, "length": 100},
"0.0.0": {"path": "/foo.nc", "offset": 100, "length": 100},
}
manifest = ChunkManifest(entries=chunks_dict)
chunks = (1, 1, 1)
Expand Down
Loading
Loading