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

Don't overwrite indexes for region writes, always #8589

Closed
dcherian opened this issue Jan 4, 2024 · 2 comments · Fixed by #8877
Closed

Don't overwrite indexes for region writes, always #8589

dcherian opened this issue Jan 4, 2024 · 2 comments · Fixed by #8877
Labels

Comments

@dcherian
Copy link
Contributor

dcherian commented Jan 4, 2024

What happened?

Currently we don't overwrite indexes when region="auto"

xarray/xarray/backends/api.py

Lines 1769 to 1770 in e6ccedb

if region_was_autodetected:
dataset = dataset.drop_vars(dataset.indexes)

I propose we do this for all region writes and completely disallow modifying indexes with a region write.

This would match the map_blocks model, where all indexes are specified in the template and no changes by the mapped function are allowed.

@dcherian dcherian added bug needs triage Issue that has not been reviewed by xarray team member and removed needs triage Issue that has not been reviewed by xarray team member labels Jan 4, 2024
@dcherian dcherian mentioned this issue Jan 4, 2024
4 tasks
@max-sixty
Copy link
Collaborator

Very much agree!

IIRC, currently it's disallowed to write indexes that don't contain the region (writing any var that doesn't contain the region is disallowed)

So this is only an issue for indexes which do contain the region. Historically the index would generally have been dropped manually; makes sense to exclude it.


(I also thought about whether this was an issue for other variables which had fewer bigger chunks. But actually this is disallowed in zarr, it's only the index that can have different chunks)

@slevang
Copy link
Contributor

slevang commented Jan 5, 2024

There was a bunch of discussion about this on #8434 - I'm in favor. I decided to take the smaller step on that PR for only region="auto" writes (where this is definitely not safe for distributed use) because I hadn't thought through all the edge cases.

dcherian added a commit to dcherian/xarray that referenced this issue Mar 25, 2024
dcherian added a commit to dcherian/xarray that referenced this issue Mar 25, 2024
dcherian added a commit to dcherian/xarray that referenced this issue Mar 25, 2024
dcherian added a commit that referenced this issue Mar 27, 2024
* Don't allow overwriting indexes with region writes

Closes #8589

* Fix typing

* one more typing fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants