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

Allow setting (or skipping) new indexes in open_dataset #8051

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

benbovy
Copy link
Member

@benbovy benbovy commented Aug 7, 2023

This PR introduces a new boolean parameter set_indexes=True to xr.open_dataset(), which may be used to skip the creation of default (pandas) indexes when opening a dataset.

Currently works with the Zarr backend:

import numpy as np
import xarray as xr

# example dataset (real dataset may be much larger)
arr = np.random.random(size=1_000_000)
xr.Dataset({"x": arr}).to_zarr("dataset.zarr")


xr.open_dataset("dataset.zarr", set_indexes=False, engine="zarr")
# <xarray.Dataset>
# Dimensions:  (x: 1000000)
# Coordinates:
#     x        (x) float64 ...
# Data variables:
#     *empty*


xr.open_zarr("dataset.zarr", set_indexes=False)
# <xarray.Dataset>
# Dimensions:  (x: 1000000)
# Coordinates:
#     x        (x) float64 ...
# Data variables:
#     *empty*

I'll add it to the other Xarray backends as well, but I'd like to get your thoughts about the API first.

  1. Do we want to add yet another keyword parameter to xr.open_dataset()? There are already many...
  2. Do we want to add this parameter to the BackendEntrypoint.open_dataset() API?
  • I'm afraid we must do it if we want this parameter in xr.open_dataset()
  • this would also make it possible skipping the creation of custom indexes (if any) in custom IO backends
  • con: if we require set_indexes in the signature in addition to the drop_variables parameter, this is a breaking change for all existing 3rd-party backends. Or should we group set_indexes with the other xarray decoder kwargs? This would feel a bit odd to me as setting indexes is different from decoding data.
  1. Or should we leave this up to the backends?
  • pros: no breaking change, more flexible (3rd party backends may want to offer more control like choosing between custom indexes and default pandas indexes or skipping the creation of indexes by default)
  • cons: less discoverable, consistency is not enforced across 3rd party backends (although for such advanced case this is probably OK), not available by default in every backend.

Currently 1 and 2 are implemented in this PR, although as I write this comment I think that I would prefer 3. I guess this depends on whether we prefer open_*** vs. xr.open_dataset(engine="***") and unless I missed something there is still no real consensus about that? (e.g., #7496).

@github-actions github-actions bot added topic-backends topic-zarr Related to zarr storage library io labels Aug 7, 2023
@benbovy
Copy link
Member Author

benbovy commented Aug 22, 2023

@pydata/xarray any thoughts on which option among those above (top comment) would be best?

@rabernat
Copy link
Contributor

  1. Do we want to add yet another keyword parameter to xr.open_dataset()?

I vote for this.

@dcherian
Copy link
Contributor

Is there any way for packages to say they support V1 or V2 of an entrypoint?

I just realized that if we turned off indexes by default, it would be a big win for the open_mfdataset case in many cases.

@max-sixty
Copy link
Collaborator

  • con: if we require set_indexes in the signature in addition to the drop_variables parameter, this is a breaking change for all existing 3rd-party backends

Doesn't help with the problem at this moment, but could we add having **kwargs to the standard, so we can add parameters in future without breaking existing backends?

@benbovy
Copy link
Member Author

benbovy commented Nov 14, 2023

Agreed, adding **kwargs to the standard would help! However, to be honest I find it already a bit confusing how kwargs are handled in xarray.backends.api. #8447 may eventually help having a clearer separation between common and backend-specific options.

@shoyer
Copy link
Member

shoyer commented Nov 22, 2023

This looks great to me!

I agree with adding this into xarray.open_dataset() and BackendEntrypoint.open_dataset().

For what it's worth, I think it's OK to require backend developers to update their code more frequently -- we don't need the same level of stabily that we need for user level APIs.

@TomNicholas
Copy link
Member

I just realized that if we turned off indexes by default, it would be a big win for the open_mfdataset case in many cases.

I would love to see this merged so that I can try this out!

@keewis
Copy link
Collaborator

keewis commented Dec 5, 2023

be aware that merging now will break compatibility with any 3rd party backend, which I believe is not something we should do, even if we think that the transition window can be shorter than usual.

I my eyes the easiest way forward would be:

  1. extend Backend.open_dataset with **kwargs as suggested by @max-sixty
  2. wait a couple of versions (2?) to give backends the time to update and release
  3. add the new option

We don't have an easy way to contact all backend developers, unfortunately.

Edit: let's discuss in the meeting today

@keewis
Copy link
Collaborator

keewis commented Dec 6, 2023

In the meeting just now we decided to inspect the signature of the backend's open_dataset method and not pass the new option if it doesn't support it nor accepts **kwargs.

We should still change the spec to require **kwargs (in a new PR), and maybe emit a deprecation warning for all backends that don't have it already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
io topic-backends topic-zarr Related to zarr storage library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Opening dataset without loading any indexes?
7 participants