Raise if missing virtual chunk containers#774
Raise if missing virtual chunk containers#774TomNicholas merged 29 commits intozarr-developers:mainfrom
Conversation
for more information, see https://pre-commit.ci
| assert diff_is_empty(diff), diff | ||
|
|
||
|
|
||
| def diff_is_empty(diff: "Diff") -> bool: | ||
| # TODO would be nicer to implement __bool__ on icechunk's Diff class | ||
| return not any( | ||
| [ | ||
| bool(diff.deleted_arrays), | ||
| bool(diff.deleted_groups), | ||
| bool(diff.new_arrays), | ||
| bool(diff.new_groups), | ||
| bool(diff.updated_arrays), | ||
| bool(diff.updated_chunks), | ||
| bool(diff.updated_groups), | ||
| ] | ||
| ) |
There was a problem hiding this comment.
@paraseba if Diff had this as a new method
class Diff:
@property
def __bool__(self) -> bool:
"""Returns true if there are any changes whatsoever."""
return any(
[
bool(diff.deleted_arrays),
bool(diff.deleted_groups),
bool(diff.new_arrays),
bool(diff.new_groups),
bool(diff.updated_arrays),
bool(diff.updated_chunks),
bool(diff.updated_groups),
]
)then I could simplify my check to just
| assert diff_is_empty(diff), diff | |
| def diff_is_empty(diff: "Diff") -> bool: | |
| # TODO would be nicer to implement __bool__ on icechunk's Diff class | |
| return not any( | |
| [ | |
| bool(diff.deleted_arrays), | |
| bool(diff.deleted_groups), | |
| bool(diff.new_arrays), | |
| bool(diff.new_groups), | |
| bool(diff.updated_arrays), | |
| bool(diff.updated_chunks), | |
| bool(diff.updated_groups), | |
| ] | |
| ) | |
| assert not diff, diff |
There was a problem hiding this comment.
Request tracked upstream in earth-mover/icechunk#1165
virtualizarr/writers/icechunk.py
Outdated
| virtual_datasets = [pair[1] for pair in paths_and_virtual_datasets] | ||
|
|
||
| store_path = StorePath(store, path="" if at_root else tree.relative_to(vdt)) | ||
| validate_virtual_chunk_containers(store.session.config, virtual_datasets) |
There was a problem hiding this comment.
This line requires an unreleased change in icechunk: earth-mover/icechunk#1164
There was a problem hiding this comment.
Otherwise you can't do session.config
| ) | ||
|
|
||
|
|
||
| def validate_virtual_chunk_containers( |
There was a problem hiding this comment.
I kinda feel like I should be able to just call some public Icechunk API here to do this instead of doing it myself. Like there is currently no way to validate a virtual reference without then immediately setting it.
There was a problem hiding this comment.
Raised earth-mover/icechunk#1167 (comment) upstream to track this idea.
|
The upstream tests are now passing, which means that oncee Icechunk is released (see #774 (comment)) this can be merged (and then this PR would need to pin to that new IC release). |
for more information, see https://pre-commit.ci
|
IC was just released |
…ithub.com/TomNicholas/VirtualiZarr into raise_if_missing_virtual_chunk_containers
for more information, see https://pre-commit.ci
…ithub.com/TomNicholas/VirtualiZarr into raise_if_missing_virtual_chunk_containers
|
@TomNicholas I think this should also be listed as a breaking change in the release notes, since we're using True as the default. It would be helpful to include a short description of why this default change is needed. |
We were already using But I agree that this effectively constitutes a breaking change - I will add it now. |
Closes #763 using suggestion in #763 (comment), and supercedes #767.
docs/releases.rstNew functions/methods are listed inapi.rst