introduce --dry-run run mode
#1881
Conversation
savingoyal
changed the title
--dry-run run mode
| @@ -901,6 +899,12 @@ def version(obj): | |||
| type=click.Choice(MONITOR_SIDECARS), | |||
| help="Monitoring backend type", | |||
| ) | |||
| @click.option( | |||
There was a problem hiding this comment.
I'm not a fan of having options cause other options to be ignored with no warning. I would add a check to see if there are conflicting options. dry-run is also a bit misleading (it's not dry run and things may happen, for example conda package caching, anything the code does as well, etc).
There was a problem hiding this comment.
also feel like --dry-run is a bit misleading as to what it actually does under the hood. Some alternatives that would maybe describe the behavior better could be --local-only or --local
There was a problem hiding this comment.
another thing that would speak for the rename is the following:
python HelloFlow.py --dry-run run --with kubernetes
results in an error
The @kubernetes decorator requires --datastore=s3 or --datastore=azure or --datastore=gs at the moment.
which would maybe make more sense with a renamed flag? e.g. "trying to run kubernetes locally, got an error".
With --dry-run my expectation at least is that remote decorators would be omitted instead
| ) | ||
| if all_decospecs: | ||
| decorators._attach_decorators(ctx.obj.flow, all_decospecs) | ||
| decospecs = list(decospecs or []) + list(ctx.obj.environment.decospecs() or []) |
There was a problem hiding this comment.
nit, use ctx.obj.decospecs (no point redoing the same computation and conversion)
| # words, for the same decorator, we want `myflow.py run --with foo` to | ||
| # take precedence over any other `foo` decospec | ||
| ctx.obj.tl_decospecs = list(decospecs or []) | ||
| # run/resume can add more decorators with --with, which take precedence |
There was a problem hiding this comment.
am fine with this (and associated change) but would re-add the comment about how precedence is determined by order right now (first one wins) so we need to save to apply in before_run. (ie: this comment is correct but doesn't say why we do it this way).
| @@ -901,6 +899,12 @@ def version(obj): | |||
| type=click.Choice(MONITOR_SIDECARS), | |||
| help="Monitoring backend type", | |||
| ) | |||
| @click.option( | |||
There was a problem hiding this comment.
also feel like --dry-run is a bit misleading as to what it actually does under the hood. Some alternatives that would maybe describe the behavior better could be --local-only or --local
| @@ -901,6 +899,12 @@ def version(obj): | |||
| type=click.Choice(MONITOR_SIDECARS), | |||
| help="Monitoring backend type", | |||
| ) | |||
| @click.option( | |||
There was a problem hiding this comment.
another thing that would speak for the rename is the following:
python HelloFlow.py --dry-run run --with kubernetes
results in an error
The @kubernetes decorator requires --datastore=s3 or --datastore=azure or --datastore=gs at the moment.
which would maybe make more sense with a renamed flag? e.g. "trying to run kubernetes locally, got an error".
With --dry-run my expectation at least is that remote decorators would be omitted instead
| # TODO: Consider setting monitor, tracer and logger to null too | ||
| ctx.params["metadata"] = metadata = "local" | ||
| ctx.params["datastore"] = datastore = "local" | ||
| echo(" in dry run mode", fg="magenta", nl=False) |
There was a problem hiding this comment.
not sure if there is any way around this, but exceptions raised in decorator init (f.ex. kubernetes checking for datastore type) will lead to this echo being eaten so there is no trace for the cause of a raised exception that the user has in their stdout, only if they check the command will it maybe be obvious
the rationale for this PR is to make it easier for folks to run tests through Metaflow without reporting anything on the progress to the UI or storing any artifacts.
also includes cosmetic changes for decorator handling in cli.py which can be safely ignored (or omitted or pulled out in a separate PR)