-
Notifications
You must be signed in to change notification settings - Fork 762
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
[core-change] Top level options from step decorators. #2002
base: master
Are you sure you want to change the base?
Conversation
valayDave
commented
Aug 28, 2024
- 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
""" | ||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo: fix me
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added this here because i assumed that low level API will also need it .
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can probably be combined with the config thing (ie: we probably don't need to add two new hooks)