Simplify Config #500
Merged
Simplify Config #500
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
# Conflicts: # server/tests/unit/common/config/test_app_config.py
Codecov Report
@@ Coverage Diff @@
## main #500 +/- ##
==========================================
- Coverage 78.32% 77.94% -0.39%
==========================================
Files 91 84 -7
Lines 7059 6456 -603
==========================================
- Hits 5529 5032 -497
+ Misses 1530 1424 -106
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
# Conflicts: # server/default_config.py
User current_app.app_config in the rest API functionds
Remove allow_matrix_types and any checking around the type of matrix. The matrix msut be cxg.
Bento007
force-pushed
the
tsmith/119-config
branch
5 times, most recently
from
b323453
to
3a58992
Compare
Bento007
force-pushed
the
tsmith/119-config
branch
from
3a58992
to
e7c830f
Compare
Bento007
force-pushed
the
tsmith/119-config
branch
from
959a254
to
39cc42e
Compare
Bento007
commented
Nov 17, 2022
Bento007
force-pushed
the
tsmith/119-config
branch
from
df4261f
to
0f1e811
Compare
ebezzi
approved these changes
Nov 23, 2022
Merged
Bento007
added a commit
that referenced
this pull request
Dec 1, 2022
- Merge `server_config` and `dataset_config` into `app_config`. This adds more complexity than needed. - Validate `app_config` using pydantic. This greatly simplifies the config validation processes and uses existing python type hinting to do it. - modify `AppConfig.__init__` to accept a file name as a parameter. - Remove `external_config` and `secret_config`. This added more complexity that we weren't really using. Pydnatic support this functionality and we can add it back in the future if needed. - Remove getting config from environment variables. This is part of removing the external config. - Remove the `external` section from config. This is part of removing the external config. - Rename the `dataset` config parameter to `default_dataset`. This is a better description of what it is. - Add `dataroots` config parameter where multiple dataroots can be specified. To simplify the requirement that `dataroot` can be both a string and dictionary, `dataroots` was added to contain the dictionary version of `dataroot`. This simplifies the configuration and validation logic and adds minimal overhead. - Rename `s3: region_name` config parameter to `s3_region_name`. This simplifies the config by reducing the nesting level for a single key dictionary. - Rename `MatrixDataLoader` to `DataLoader`. This was a TODO in the code. - Use `current_app.app_config` in the API handlers rather than passing `app_config` as a parameter. This reduces the complexity of the interface for some of the API handlers. - Remove `per_dataset_config` because it's not used and adds significant complexity. - Remove `allowed_matrix_types` because only CXG is supported. - Remove `get_title` and `get_about` from `app_config`. All it does is wrap the equivalent data_adapter function. - Remove obsolete tests. Co-authored-by: Andrew Tolopko <atolopko-czi@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#119
Reviewers
Functional:
Readability:
Changes
server_configanddataset_configintoapp_config. This adds more complexity than needed.app_configusing pydantic. This greatly simplifies the config validation processes and uses existing python type hinting to do it.AppConfig.__init__to accept a file name as a parameter.external_configandsecret_config. This added more complexity that we weren't really using. Pydnatic support this functionality and we can add it back in the future if needed.externalsection from config. This is part of removing the external config.datasetconfig parameter todefault_dataset. This is a better description of what it is.datarootsconfig parameter where multiple dataroots can be specified. To simplify the requirement thatdatarootcan be both a string and dictionary,datarootswas added to contain the dictionary version ofdataroot. This simplifies the configuration and validation logic and adds minimal overhead.s3: region_nameconfig parameter tos3_region_name. This simplifies the config by reducing the nesting level for a single key dictionary.MatrixDataLoadertoDataLoader. This was a TODO in the code.current_app.app_configin the API handlers rather than passingapp_configas a parameter. This reduces the complexity of the interface for some of the API handlers.per_dataset_configbecause it's not used and adds significant complexity.allowed_matrix_typesbecause only CXG is supported.get_titleandget_aboutfromapp_config. All it does is wrap the equivalent data_adapter function.Testing
Checked that it works on rdev
Future