[core-change] Top level options from step decorators. #2002
Conversation
valayDave
commented
Aug 28, 2024
valayDave
commented
- CLI options injected by step decorators in the Top level CLI
- Step decorators exposing an additional hooks in the lifecycle to accept options passed down from top level
- top level option injection is only done by step decorators that are statically set in the code
…tors. - CLI options injected by step decorators in the Top level CLI - Step decorators exposing an additional hooks in the lifecycle to accept options passed down from top level - top level option injection is only done by step decorators that are statically set in the code
valayDave
changed the title
| """ | ||
| Return a list of option-value pairs that correspond to top-level | ||
| options that should be passed to subprocesses (tasks). The option | ||
| names should be a subset of the keys in self.options. |
There was a problem hiding this comment.
Needed all of this so that click can set step decorator based Options in cli.py
| @@ -843,6 +843,7 @@ def version(obj): | |||
|
|
|||
| @tracing.cli_entrypoint("cli/start") | |||
| @decorators.add_decorator_options | |||
| @decorators.add_step_decorator_options | |||
There was a problem hiding this comment.
Top level CLI Options added for only statically set decorators in code.
| @@ -1009,6 +1010,8 @@ def start( | |||
| deco_options, | |||
| ) | |||
|
|
|||
| decorators._inject_step_decorator_options(ctx.obj.flow, deco_options) | |||
There was a problem hiding this comment.
todo: fixme : give better name
| @@ -257,6 +281,34 @@ class MyDecorator(StepDecorator): | |||
| pass them around with every lifecycle call. | |||
| """ | |||
|
|
|||
| cli_options = {} | |||
There was a problem hiding this comment.
named it cli_options because options seemed a like a fairly common attributes that user defined step decorators could also have
| @@ -186,7 +186,7 @@ def from_cli(cls, flow_file: str, cli_collection: Callable) -> Callable: | |||
| flow_cls = extract_flow_class_from_file(flow_file) | |||
| flow_parameters = [p for _, p in flow_cls._get_parameters()] | |||
| with flow_context(flow_cls) as _: | |||
| add_decorator_options(cli_collection) | |||
| add_step_decorator_options(add_decorator_options(cli_collection)) | |||
There was a problem hiding this comment.
added this here because i assumed that low level API will also need it .
There was a problem hiding this comment.
Minor nits. I am also not convinced with the UX but maybe I just need to see a use-case you are thinking. I know this argument could also be made for flow-level decorators but it feels like this could be passed using configs. With step decorators in multiple places, something feels a bit weird providing a top level option to control the setting of all the decorators. If there is such a setting, could it not be placed at the flow level? Again, don't have a concrete use case example so hard for me to speculate but the "single setting" "multiple places" feels a bit weird to me.
| for deco_name in step_deco_names: | ||
| deco = step_deco_dict[deco_name] | ||
| for option, kwargs in deco.cli_options.items(): | ||
| if option in seen: |
There was a problem hiding this comment.
this would need to be global (across step and flow level decorators)
| # pass options to the step decorator from the cli. | ||
|
|
||
| @classmethod | ||
| def step_options_init(cls, flow, options_dict): |
There was a problem hiding this comment.
This can probably be combined with the config thing (ie: we probably don't need to add two new hooks)
