Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Append mode for StoreToZarr #721
Append mode for StoreToZarr #721
Changes from all commits
b74905b
8e3d55e
9d18ba8
342dd26
401ee46
ee41b76
b9f6b8b
9defde9
9eff610
9a3cb4b
cd13679
d0cbb3f
3b4c31e
5b43dae
99e41ed
4bf2eae
468f326
5ce2d62
7eb866c
6822f56
ffd21b9
a51d3cc
aaa5a69
c75a274
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Just realizing that this, while seemingly necessary to avoid overwriting non-
append_dim
coords, may prevent us from benefiting from the dimension consistency checks offered byds.to_zarr
... we may need to re-implement such checks inStoreToZarr.__post_init__
if we want them.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.
Thinking about this further... in
StoreToZarr.__post_init__
we actually don't know what the coordinates in the new data are (aside from the user-providedappend_dim
), because we haven't actually fetched and opened any source files yet.So if we want to check for consistency, we could actually do it right here. Before dropping non-append coords from the schema, we would just need to open the target dataset and check that all coords match first. Could be a good idea.
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.
Having just reviewed xarray's checks again, I actually don't see that this is done there, so may be overly strict. I think we can move forward with what we have for now.
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.
Amazing how easy it was to modify this step to allow appending.
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.
In future PR's to this, do you see a good path forward to making append safe/idempotent?
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.
Good question! For future readers, we had a good discussion of this in today's Coordination meeting, minutes here.