-
Notifications
You must be signed in to change notification settings - Fork 57
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
Add parameter alias to inherits_config_params #193
Add parameter alias to inherits_config_params #193
Conversation
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'd like you to write a document :)
test/test_config_params.py
Outdated
@@ -25,6 +25,12 @@ class Inherited(gokart.TaskOnKart): | |||
param_b = luigi.Parameter(default='overrided') | |||
|
|||
|
|||
@inherits_config_params(ConfigClass, param_config2task={'param_a': 'param_d'}) |
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.
When would you be happy with this feature?
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.
@vaaaaanquish
This would be useful when ConfigClass
and Wrapped class has different parameter name but want to have common value.
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.
@vaaaaanquish
I've added use case to the docs.
https://github.com/m3dev/gokart/pull/193/files#diff-bd31f137cd4066a176d3de6d9729841a660ed1416d3a26cf9bd0c820211ad8b8R57
gokart/config_params.py
Outdated
import luigi | ||
|
||
import gokart | ||
|
||
|
||
class inherits_config_params: | ||
def __init__(self, config_class: luigi.Config): | ||
self.config_class: luigi.Config = config_class | ||
def __init__(self, config_class: luigi.Config, param_config2task: Optional[Dict[str, str]] = None): |
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.
@mski-iksm [nits] Plz write docstring about args 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.
@Hi-king
I've changed param_config2task
to parameter_alias
. Please review!
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.
LGTM
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.
LGTM!
This update allows to replace parameter name of inherited config Task, when using
@inherits_config_params
decorator.Currently, when using
@inherits_config_params
, parameter name of inheriting config Task and decorated Task must be same. After this PR, parameter name can be replaced by passing parameter names dictionaryparam_config2task
to@inherits_config_params
decorator.Please review!
@hirosassa @vaaaaanquish @e-mon @Hi-king