-
Notifications
You must be signed in to change notification settings - Fork 18
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
Toniof 516 unify datasetio #527
Conversation
Codecov Report
@@ Coverage Diff @@
## master #527 +/- ##
==========================================
+ Coverage 91.51% 91.69% +0.17%
==========================================
Files 288 288
Lines 26271 26426 +155
==========================================
+ Hits 24043 24230 +187
+ Misses 2228 2196 -32
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe, this is very good!
However I have some trouble understanding the base algorithm implemented here. The code urgently needs some explanation, may it be docstrings, comments, or by refactorings. Please have a look at the comments.
return self._dataset_configs | ||
|
||
def _maybe_assign_store_instance_ids(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Long, complex method. Please refactor and/or add comments so one can understand the algorithm implementation. It takes me too much time to reverse-engineer the intentions coded here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I tried.
xcube/core/store/storepool.py
Outdated
def get_store_instance_id(self, | ||
store_config: DataStoreConfig, | ||
strict_check: bool = False | ||
) \ | ||
-> Optional[str]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code style:
def get_store_instance_id(self, | |
store_config: DataStoreConfig, | |
strict_check: bool = False | |
) \ | |
-> Optional[str]: | |
def get_store_instance_id(self, | |
store_config: DataStoreConfig, | |
strict_check: bool = False | |
) \ | |
-> Optional[str]: |
def get_store_instance_id(self, | |
store_config: DataStoreConfig, | |
strict_check: bool = False | |
) \ | |
-> Optional[str]: | |
def get_store_instance_id( | |
self, | |
store_config: DataStoreConfig, | |
strict_check: bool = False | |
) -> Optional[str]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GitHub has a bug here: it doubles the suggestions! Only the second is valid!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It even doubles every comment!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just wanted to check our code style for this issue. I did not find it, I just recall we already talked about this before.
But are you certain that the suggestion below is right? Going to the lowest indention level with the method header?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not find it, I just recall we already talked about this before.
PEP-8. And yes, we talked about this before, see https://www.python.org/dev/peps/pep-0008/#indentation
Going to the lowest indention level with the method header?
No that's not right, it happened per accident.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, changed it according to pep8
# Conflicts: # examples/serve/demo/config-with-stores.yml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good, let's talk about _maybe_assign_store_instance_ids(self)
tomorrow!
…-datasetio"" This reverts commit 4c67c0a
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.
Checklist:
[ ] Add docstrings and API docs for any new/modified user-facing classes and functions[ ] New/modified features documented indocs/source/*
CHANGES.md
Remember to close associated issues after merge!