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

Unify Dataset IO #535

Merged
merged 13 commits into from
Feb 1, 2022
Merged

Unify Dataset IO #535

merged 13 commits into from
Feb 1, 2022

Conversation

TonioF
Copy link
Contributor

@TonioF TonioF commented Sep 21, 2021

This PR addresses #516. It ensures that, if a Configuration with Datasets is passed to a ServiceContext, each dataset is assigned to a datastore that will be used to open the data (unless the dataset is in-memory). It is attempted to find common roots for data stores to group datasets located at same/similar locations together.

When reviewing this, please consider especially the function get_data_store_pool of the ServerContext: This function used to remove a data store pool if there were no datastores explicitly configured. I had to change this, but I don't fully understand what was the original reasoning behind this.

This is a follow-up PR to erroneously merged PR #527 .

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
  • AppVeyor CI passes
  • Test coverage remains or increases (target 100%)

Remember to close associated issues after merge!

Sorry, something went wrong.

@TonioF TonioF requested a review from forman September 21, 2021 10:15
@TonioF TonioF self-assigned this Sep 21, 2021
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.

Where are the changes in xcube.webapi.context?

Also I don't understand your comment:

When reviewing this, please consider especially the function get_data_store_pool of the ServerContext: This function used to remove a data store pool if there were no datastores explicitly configured. I had to change this, but I don't fully understand what was the original reasoning behind this.

I cannot see what you are referring to.

@TonioF
Copy link
Contributor Author

TonioF commented Oct 8, 2021

Also I don't understand your comment:

When reviewing this, please consider especially the function get_data_store_pool of the ServerContext: This function used to remove a data store pool if there were no datastores explicitly configured. I had to change this, but I don't fully understand what was the original reasoning behind this.

I cannot see what you are referring to.

In get_data_store_pool, there were the following lines:

if not data_store_configs:
self._data_store_pool = None`
I had to change this part, as it would cause that an existing data pool would be removed if data_store_configs was None or empty. However, I do not understand what was the original reasoning behind this part, so I felt a bit hesitant to remove it. Please see whether I broke something here (which I don't believe I did).

@TonioF TonioF requested a review from forman October 8, 2021 12:40
},
{
'Identifier': 'z_4',
'FileSystem': 'obs',
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should stop using "obs" when referring to s3 object storage - and exchange it to "s3" as used in the DataStores configurations.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct - but for now, we'll move this to another PR.

@codecov-commenter
Copy link

Codecov Report

Merging #535 (9797cf9) into master (ca1718a) will increase coverage by 0.14%.
The diff coverage is 97.64%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #535      +/-   ##
==========================================
+ Coverage   91.63%   91.77%   +0.14%     
==========================================
  Files         292      292              
  Lines       26587    26742     +155     
==========================================
+ Hits        24362    24543     +181     
+ Misses       2225     2199      -26     
Impacted Files Coverage Δ
test/core/gridmapping/test_base.py 100.00% <ø> (ø)
test/webapi/test_config.py 100.00% <ø> (ø)
xcube/webapi/config.py 100.00% <ø> (ø)
xcube/core/store/storepool.py 98.14% <91.66%> (+0.13%) ⬆️
xcube/webapi/context.py 81.62% <96.11%> (+6.24%) ⬆️
test/core/store/test_storepool.py 100.00% <100.00%> (ø)
test/webapi/test_context.py 100.00% <100.00%> (ø)
xcube/webapi/handlers.py 92.03% <100.00%> (+0.29%) ⬆️
xcube/core/mldataset.py 78.61% <0.00%> (-0.87%) ⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca1718a...9797cf9. Read the comment docs.

@TonioF TonioF merged commit b575ead into master Feb 1, 2022
@TonioF TonioF deleted the toniof-516-unify-datasetio2 branch February 1, 2022 14:08
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.

None yet

4 participants