-
Notifications
You must be signed in to change notification settings - Fork 774
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -843,6 +843,7 @@ def version(obj): | |
|
||
@tracing.cli_entrypoint("cli/start") | ||
@decorators.add_decorator_options | ||
@decorators.add_step_decorator_options | ||
@click.command( | ||
cls=click.CommandCollection, | ||
sources=[cli] + plugins.get_plugin_cli(), | ||
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. todo: fixme : give better name |
||
|
||
# In the case of run/resume, we will want to apply the TL decospecs | ||
# *after* the run decospecs so that they don't take precedence. In other | ||
# words, for the same decorator, we want `myflow.py run --with foo` to | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -207,6 +207,7 @@ def add_decorator_options(cmd): | |
flow_cls = getattr(current_flow, "flow_cls", None) | ||
if flow_cls is None: | ||
return cmd | ||
|
||
for deco in flow_decorators(flow_cls): | ||
for option, kwargs in deco.options.items(): | ||
if option in seen: | ||
|
@@ -220,6 +221,29 @@ def add_decorator_options(cmd): | |
else: | ||
seen[option] = deco.name | ||
cmd.params.insert(0, click.Option(("--" + option,), **kwargs)) | ||
|
||
return cmd | ||
|
||
|
||
def add_step_decorator_options(cmd): | ||
step_deco_dict = _get_all_step_decos() | ||
seen = set() | ||
step_deco_names = getattr(current_flow, "unique_step_decos_in_flow", None) | ||
if step_deco_names is None: | ||
return cmd | ||
|
||
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 commentThe reason will be displayed to describe this comment to others. Learn more. this would need to be global (across step and flow level decorators) |
||
msg = ( | ||
"Step decorator '%s' uses an option '%s' which is also " | ||
"used by another step decorator. " % (deco.name, option) | ||
) | ||
raise MetaflowInternalError(msg) | ||
else: | ||
seen.add(option) | ||
cmd.params.insert(0, click.Option(("--" + option,), **kwargs)) | ||
return cmd | ||
|
||
|
||
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. named it |
||
# `cli_options` is similar to the one the flow decorator. It will be used to | ||
# 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 commentThe 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) |
||
""" | ||
Called right after `flow_init` to pass down the options set in the cli. | ||
Since step decorators can inject options via `cli_options`, this callback | ||
helps set these options for statically set decorators since it is called before | ||
the dynamically set decorators are attached (ie. decorators set via `--with`). | ||
|
||
Its a class method to ensure that any decorator even attached via `--with` | ||
(given its statically present in the code too) can access the options set in the cli. | ||
""" | ||
pass | ||
|
||
def get_top_level_options(self): | ||
""" | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. todo: fix me |
||
|
||
If the decorator has a non-empty set of options in `self.cli_options`, you | ||
probably want to return the assigned values in this method. | ||
""" | ||
return [] | ||
|
||
def step_init( | ||
self, flow, graph, step_name, decorators, environment, flow_datastore, logger | ||
): | ||
|
@@ -510,6 +562,14 @@ def _attach_decorators_to_step(step, decospecs): | |
step.decorators.append(deco) | ||
|
||
|
||
def _inject_step_decorator_options(flow, deco_options): | ||
for step in flow: | ||
for deco in step.decorators: | ||
deco.step_options_init(flow, deco_options) | ||
|
||
return | ||
|
||
|
||
def _init_flow_decorators( | ||
flow, graph, environment, flow_datastore, metadata, logger, echo, deco_options | ||
): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Needed all of this so that |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,7 +32,7 @@ | |
UUIDParameterType, | ||
) | ||
from metaflow._vendor.typeguard import TypeCheckError, check_type | ||
from metaflow.decorators import add_decorator_options | ||
from metaflow.decorators import add_decorator_options, add_step_decorator_options | ||
from metaflow.exception import MetaflowException | ||
from metaflow.includefile import FilePathClass | ||
from metaflow.parameters import JSONTypeClass, flow_context | ||
|
@@ -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 commentThe 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 . |
||
|
||
class_dict = {"__module__": "metaflow", "_API_NAME": flow_file} | ||
command_groups = cli_collection.sources | ||
|
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.