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

Add data_type to open_data method in DataStore class #1037

Merged

Conversation

konstntokas
Copy link
Contributor

@konstntokas konstntokas commented Jul 9, 2024

Closes #1030

Checklist:

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • New/modified features documented in docs/source/*
  • Changes documented in CHANGES.md
  • GitHub CI passes
  • AppVeyor CI passes
  • Test coverage remains or increases (target 100%)

@konstntokas konstntokas requested a review from forman July 9, 2024 12:46
Copy link
Member

@forman forman left a comment

Choose a reason for hiding this comment

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

Very good work, thanks! See my change requests and please adjust accordingly.

self,
data_id: str,
opener_id: str = None,
data_type: DataTypeLike = None,
Copy link
Member

Choose a reason for hiding this comment

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

Adding keyword data_type is a change that breaks compatibility with any data store developed against older versions of xcube.

We should instead document the parameter here - as you did - and update data store implementations subsequently. Only after we have migrated all of our own data stores and after next xcube major release, we should define the kwarg here as you did.

Therefore, the xcube-stac store should pop data_type out of the open_params for time being.

CHANGES.md Outdated
Comment on lines 20 to 22
* Added `data_type` to `open_data` method in the `DataStore` class, which determines
the return value of the data set. Note that `opener_id` includes the `data_type`
at its first position and will override the `date_type` argument. (#1030)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Added `data_type` to `open_data` method in the `DataStore` class, which determines
the return value of the data set. Note that `opener_id` includes the `data_type`
at its first position and will override the `date_type` argument. (#1030)
* The `open_data` method of xcube's default `xcube.core.store.DataStore` implementations
now support a keyword argument `data_type`, which determines
data type of the return value. Note that `opener_id` includes the `data_type`
at its first position and will override the `date_type` argument.
To preserve backward compatibility, the the keyword argument `data_type`
has not yet been added to the `open_data()` method arguments. (#1030)

If *opener_id* is given, the identified data opener will be used
to open the data resource and *open_params* must comply with the
schema of the opener's parameters. Note that some store
implementations may not support using different openers or just
support a single one.

If *data_type* is provided, the return value will be in the specified
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If *data_type* is provided, the return value will be in the specified
Implementations are advised to support an additional optional keyword
argument `data_type: DataTypeLike = None`.
If *data_type* is provided, the return value should be in the specified

@forman
Copy link
Member

forman commented Jul 9, 2024

FYI @TonioF (xcube-cci), @pont-us (xcube-cds), @TejasMorbagal (xcube-cmems)

@konstntokas
Copy link
Contributor Author

As suggested, I included in keyword argument data_type in **open_params now. Further I added testing to the open_data method in the ReferenceDataStore class. Please check if I understood correctly, that the open_data method in the ReferenceDataStore class only returns xr.Dataset. However most of these tests are skipped because kerchunk is not installed (see e.g. https://github.com/xcube-dev/xcube/blob/main/test/core/store/ref/test_store.py#L178). Should we maybe add kerchunk to the testing section in the environment.yml ?

@konstntokas konstntokas requested a review from forman July 9, 2024 15:52
@forman
Copy link
Member

forman commented Jul 9, 2024

Please check if I understood correctly, that the open_data method in the ReferenceDataStore class only returns xr.Dataset.

True.

However most of these tests are skipped because kerchunk is not installed (see e.g. https://github.com/xcube-dev/xcube/blob/main/test/core/store/ref/test_store.py#L178). Should we maybe add kerchunk to the testing section in the environment.yml ?

Let's do that!

@konstntokas konstntokas merged commit 7809dd8 into main Jul 10, 2024
3 checks passed
@konstntokas konstntokas deleted the konstntokas-1030-add_data_type_open_data_store_framework branch July 10, 2024 05:59
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.

Data store framework enhancement: add the optional argument data_type to the open_data method
2 participants