Move schema loading/validating code out of config #9371
Merged
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
jdavcs
added
status/WIP
kind/refactoring
nsoranzo
reviewed
Feb 13, 2020
jdavcs
force-pushed
the
sg_config51
branch
4 times, most recently
from
5712495
to
7d30c0c
Compare
nsoranzo
reviewed
Feb 17, 2020
nsoranzo
reviewed
Feb 17, 2020
nsoranzo
reviewed
Feb 17, 2020
nsoranzo
reviewed
Feb 18, 2020
Add integration test; update several unit tests
1. Modify test setup code to adjust to edits in previous commit. Do NOT change the tests. 2. Simplify tests; get rid of mocking where possible (use real objects).
Rationale: group related items together. Constants OPTION_DEFAULTS and UNKNOWN_OPTION are closely related and are used in the same context. OPTION_DEFAULTS is only used in `schema.py` and `config_manage.py`; the latter imports items from `schema.py`. Thus, it is reasonable to place both constants in the same module. (cherry-picked from closed galaxyproject#9307)
This commit drops the exception that guarded against accidental overwriting of existing attributes, and instead ignores the setattr call, logging a debug message (which was initialy proposed in galaxyproject#8728). Rationale: with the previous version, base configuration properties (config_dir, data_dir, etc.) had to be set *after* this method was called (otherwise an exeption was raised), but *before* any path resolving code (because these properties are used for path resolution). As a result, config loading required a rigid sequence of steps: 1: load defaults and kwargs, create all config attributes. 2: set base configuration properties [CANNOT MOVE THIS] 3: resolve paths, do the rest: load individual properties, etc. However, setting base configuration properties is a special case that is *independent of schema loading*, is handled separately (in set_config_base), and takes precendence over schema loading procedures. This modification makes config loading more flexible, and logical: 1. set base configuration properties 2. do the rest (now any refactoring can go here) (cherry-picked from closed galaxyproject#9307)
GalaxyAppConfiguration has a reference to an AppSchema object (`schema`
instance variable), which provides access to relevant schema data. So we
can simplify:
`self.appschema[key].get('path_resolves_to')` >> `self.schema.paths_to_resolve.get(key)`
Accessing the application schema mapping directly is straightforward:
`self.schema.app_schema`
Add schema var to galaxy mock as a MockSchema stub object
Provide stub methods for TS config implementation as a temporary measure. No refactoring of TS config in this commit.
This will prevent an error in TS if the base config class `_update_raw_config_from_kwargs()` method is invoked.
Following the principle of least surprise. The stub methods are not invoked because the parent's __init__() method is not called. They can be safely removed. However, if a future commit adds a call to parent's __init__, without these stub methods the parent's methods will be called, which may result in a surprise (e.g., attributes would be created for all schema properties).
natefoo
approved these changes
Mar 9, 2020
|
I unresolved a couple resolved comments that I added comments to |
Except base config attributes set in `_set_config_base()`
Instead of app_schema['foo'].get('default') - as per code review
natefoo
approved these changes
Mar 10, 2020
|
Thank you for the detailed review @natefoo! |
|
Ah, sorry, missed that one. This is cool! Thanks @ic4f! |
|
@bgruening No worries, and thank you! |
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.
This replaces #9307.
Motivation:
config/__init__.pyis doing too much. Last year's modifications to the config system, while simplifying configuration loading, have done the opposite to the configuration module's code, and especially to theGalaxyAppConfigurationclass. This is an attempt to address this problem by separating 2 different responsibilities of theGalaxyAppConfigurationclass: (1) automatically loading and processing all config properties from the schema; and (2) setting individual config properties and handling special cases. My first attempt (#9307) was to delegate the former to a ConfigurationLoader module and use it for any further schema processing, while using GalaxyAppConfiguration for setting individual config properties (as it did before).However, concerns were raised during the code review: (a) the schema loader object was manipulating the internal structure of the config object; and (b) such tight coupling of 2 objects suggested the separation may have been not necessary (#9307 (review)).
This, I think, is a better approach. Instead of delegating everything to a new "builder" object, I've split up the stuff that IMO does not belong in
GalaxyAppConfigurationinto 2 parts: (a) schema-related; and (b) configuration-related. All schema-related functionality has been moved to the schema object: loading collections of schema data + validating that data are now the schema object's responsibilities. All the configuration-related stuff that deals with all attributes + their processing, and does not include setting individual, galaxy-specific properties, has been moved up to the base config class.I've updated the tests; I've also simplified the mocking in the unit tests in
test/unit/config.I expect this will simplify the config code and make modifications easier. Also, as a side benefit, we can now eliminate much redundant code from TS's config (e.g., #9379, if needed).
Ping @nsoranzo, @bgruening.
(EDIT: more details in config messages.)