-
Notifications
You must be signed in to change notification settings - Fork 2
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
Simplify Config #500
Merged
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
November 16, 2022 21:53
b323453
to
3a58992
Compare
Bento007
force-pushed
the
tsmith/119-config
branch
from
November 16, 2022 22:00
3a58992
to
e7c830f
Compare
Bento007
force-pushed
the
tsmith/119-config
branch
from
November 16, 2022 22:59
959a254
to
39cc42e
Compare
Bento007
commented
Nov 17, 2022
Bento007
force-pushed
the
tsmith/119-config
branch
from
November 17, 2022 19:34
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_config
anddataset_config
intoapp_config
. This adds more complexity than needed.app_config
using 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_config
andsecret_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.external
section from config. This is part of removing the external config.dataset
config parameter todefault_dataset
. This is a better description of what it is.dataroots
config parameter where multiple dataroots can be specified. To simplify the requirement thatdataroot
can be both a string and dictionary,dataroots
was added to contain the dictionary version ofdataroot
. This simplifies the configuration and validation logic and adds minimal overhead.s3: region_name
config parameter tos3_region_name
. This simplifies the config by reducing the nesting level for a single key dictionary.MatrixDataLoader
toDataLoader
. This was a TODO in the code.current_app.app_config
in the API handlers rather than passingapp_config
as a parameter. This reduces the complexity of the interface for some of the API handlers.per_dataset_config
because it's not used and adds significant complexity.allowed_matrix_types
because only CXG is supported.get_title
andget_about
fromapp_config
. All it does is wrap the equivalent data_adapter function.Testing
Checked that it works on rdev
Future