-
Notifications
You must be signed in to change notification settings - Fork 12
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
[ENH] Add global config interface #149
base: main
Are you sure you want to change the base?
Conversation
@@ -1014,33 +1046,184 @@ | |||
self.c = 84 | |||
|
|||
|
|||
def test_set_get_config(): | |||
"""Test logic behind get_config, set_config. | |||
class AnotherConfigTester(BaseObject): |
Check warning
Code scanning / CodeQL
`__eq__` not overridden when adding attributes
self.c = 84 | ||
|
||
|
||
class ConfigExtensionInterfaceTester(BaseObject): |
Check warning
Code scanning / CodeQL
`__eq__` not overridden when adding attributes
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #149 +/- ##
==========================================
+ Coverage 82.68% 84.24% +1.55%
==========================================
Files 32 37 +5
Lines 2327 2589 +262
==========================================
+ Hits 1924 2181 +257
- Misses 403 408 +5
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@fkiraly I plan on adding a few more simple tests of uncovered edge cases to increase coverage. But this is ready for feedback in the interim. |
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.
Hm - looks good but I am not sure I fully understand the design.
If this were a downstream package, I would be agreed.
But what ist the extension case here? That is, someone using skbase
to make a package.
I think this is going to be problematic, as it now ties every descendant of BaseObject
to the global config in skbase
.
Instead, I would invert the pointer, say, add a method get_global_config
or similar that by default does nothing in the downstream package, but can be linked to a global config in the downstream package (which in turn is templated by skbase
).
@fkiraly good questions.
There are two things at play:
I've included more details on both below. Reliance of downstream packages to the config or options of an upstream package.There are multiple cases where downstream Python packages need to call an upstream package's config/options to change behavior in a way that affects downstream (I'm showing For example, packages that use For example: from sklearn import set_config
from sklearn.base import BaseEstimator
class SomeDownstreamPackageClass(BaseEstimator):
"""This class implements some awesome estimator."""
def __init__(self, some_param=7):
self.some_param = some_param
downstream_estimator = SomeDownstreamPackageClass()
print(downstream_estimator)
# prints SomeDownstreamPackageClass()
set_config(print_changed_only=False) # True is default
print(downstream_estimator)
# prints SomeDownstreamPackageClass(some_param=7) The first part of the global config interface (part importable from But this has the downside that it means the local Should we let users override global
|
# When calling the method with invalid value it should raise user warning | ||
# And the warning should start with `msg` if it is passed | ||
with pytest.warns(UserWarning, match=r"some message.*"): | ||
returned_value = some_config_param.get_valid_param_or_default( |
Check notice
Code scanning / CodeQL
Unused local variable
set_config(do_something_else=True) | ||
|
||
with pytest.raises(TypeError): | ||
set_config(True) |
Check failure
Code scanning / CodeQL
Wrong number of arguments in a call
def test_set_config_invalid_keyword_argument(): | ||
"""Test set_config behavior when invalid keyword argument passed.""" | ||
with pytest.raises(TypeError): | ||
set_config(do_something_else=True) |
Check failure
Code scanning / CodeQL
Wrong name for an argument in a call
Reference Issues/PRs
What does this implement/fix? Explain your changes.
This adds a global config interface. It allows the retrieval of the global config via
get_config
, updating the global config viaset_config
, finding out whatskbase
's default config is viaget_default_config
, resetting the config to the default viareset_config
, and a config context manager viaconfig_context
.The PR also updates the local config interface's config retrieval (e.g.,
BaseObject().get_config()
) so that it will retrieve the global config and then update it based on local config.It also adds the
__skbase_get_config__
extension point. If it is defined on a descendent class and returns a dict, then the returned dict is used to update the local copy of the global config. This is useful for downstream packages that useskbase
. They can implement their own extension toskbase
that reads their own package's global config, letting the local config retrieveskbase
's config and the downstream package config. It also allows the local config (e.g. BaseObject.set_config or config defined in BaseObject._config) to override both theskbase
config and the descendent package's config (which is what we want so that the interface works for downstream users).Order of precedence is:
__skbase_get_config__
is defined and returns a dict, then it is used to update the copy of the global configOne point I was not entirely sure on was how to treat the local configuration (override of global config) using invalid values for the
skbase
global configuration variables. For example,print_changed_only
is askbase
global config (that can optionally be configured on an object or globally) to determine if the pretty printed representations ofBaseObject
s shoudl print only parameters that are different from their default or all parameters. It is boolean.Suppose that someone tries to do:
Should we stop that from occuring in the case where we know it is invalid (we only know that for
skbase
configurable parameters, not extension configurations)? Right now, I'm allowing the assignment to occur, but when callingget_config
, I'm not letting the local config override the global config when the local value is invalid (in this case not boolean).Does your contribution introduce a new dependency? If yes, which one?
There is not any new dependencies added.
What should a reviewer concentrate their feedback on?
Does design make sense? Do tests cover any cases we can think of?
Any other comments?
PR checklist
For all contributions
the PR topic is related to enhancement, CI/CD, maintenance, documentation, or a bug.
For code contributions