Skip to content

Conversation

@EddyCMWF
Copy link
Contributor

@EddyCMWF EddyCMWF commented Dec 3, 2025

  1. open_dataset -> open_zarr for succinct kwarg passing
  2. Option for parsing kwargs into open_zarr command
  3. decrypt kwargs recursivley

@EddyCMWF EddyCMWF requested a review from malmans2 December 3, 2025 15:03
Copy link
Member

@malmans2 malmans2 left a comment

Choose a reason for hiding this comment

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

Approving, but with a couple of comments:

  1. I was under the impression that xarray's long-term plan was to deprecate open_zarr. I cannot see a warning in the documentation though, so I may be wrong. It's no big deal if you are sure that you will always open all ARCO data with zarr-python.

  2. I would have handled the decryption differently, by explicitly decoding only the credential arguments. The blind recursive decryption seems a bit error prone to me. I am also not a big fan of the ignore_errors option in decrypt. I only added it to comply with the requirements, but I would avoid using it when possible.

@EddyCMWF
Copy link
Contributor Author

EddyCMWF commented Dec 3, 2025

Thanks @malmans2 ,
For 1, I will change back, I was not 100% sure on that, and your suspicion is enough for me to revert. I can then make engine: "zarr" a default kwarg which can be over-ridden if need be in the future... not sure it ever will be

For 2, understood but happy to go with it as it is as the exact credential config is still being discussed and this gives us a nice flexibility. I will open an issue to come back in the future.

@malmans2
Copy link
Member

malmans2 commented Dec 3, 2025

Sounds good.
BTW, I think you might also want to add this default, which will default to Dask arrays.

open_dataset_kwargs.setdefault("chunks", {})

@EddyCMWF EddyCMWF merged commit c12d30c into main Dec 4, 2025
9 checks passed
@EddyCMWF EddyCMWF deleted the feature/kwargs-for-zarr-open_dataset branch December 4, 2025 10:34
@EddyCMWF
Copy link
Contributor Author

EddyCMWF commented Dec 4, 2025

Sounds good. BTW, I think you might also want to add this default, which will default to Dask arrays.

open_dataset_kwargs.setdefault("chunks", {})

I'll investigate with configs then make default if needed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants