dvc.yaml: make persist/cache bool flags in outs/metrics #3804
Conversation
efiop
changed the title
| METRICS = "metrics" | ||
| NO_CACHE = "outs_no_cache" | ||
| OUTS = "outs" | ||
| PARAM_OUTS_PERSIST = "outs_persist" |
There was a problem hiding this comment.
Btw, might be worth renaming it to persist instead of outs_persist. Looks a lot nicer that way.
There was a problem hiding this comment.
may be in this case we can make it a flag within the outs item?
There was a problem hiding this comment.
@shcheklein It is that already :) But we also keep plain format where it mimics CLI options. So I wonder if we should make it persist and --persist in dvc run.
There was a problem hiding this comment.
yep, sounds good to me! never liked these JAVA-style - --outs-enterprisy-long-name-include-everything :)
| }, | ||
| ) | ||
| ], | ||
| Optional(StageParams.PARAM_OUTS_NO_CACHE): [str], |
There was a problem hiding this comment.
Being a bit restrictive here and not allowing additional flags (e.g. persist=True) for such fields.
| ], | ||
| Optional(StageParams.PARAM_OUTS_NO_CACHE): [str], | ||
| Optional(StageParams.PARAM_METRICS): [ | ||
| Any(str, {str: {Optional(BaseOutput.PARAM_CACHE): bool}}) |
There was a problem hiding this comment.
Not supporting persist for metrics because it is the way we currently generate it during the serialization. Plus we don't really have a CLI option for it. But seems like persistent metrics does make sense, especially when we talk about plots that might accumulate data from run to run. But let's wait for someone to ask for it.
Note that in old dvc-files that(persistent metrics) was supported if user set metric: True, persist=True by hand.
| bucket_key += ["persist"] | ||
| extra = {} | ||
| if o.persist: | ||
| extra["persist"] = True |
There was a problem hiding this comment.
Just matching existing style in this method, so using plain strings. Maybe we should discuss strings vs constants one more time, to see how people feel about it these days.
More explicit CLI-like options `metrics/persist_no_cache` are still supported, but we use flags when generating dvc.yaml. Part of iterative#3409, prerequisite for `plot`.
efiop
changed the title
|
Aaand I wondered why there is no |
More explicit CLI-like options
metrics/persist_no_cacheare still supported,but we use flags when generating dvc.yaml.
Part of #3409, prerequisite for
plot.❗ I have followed the Contributing to DVC checklist.
📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here. If the CLI API is changed, I have updated tab completion scripts.
❌ I will check DeepSource, CodeClimate, and other sanity checks below. (We consider them recommendatory and don't expect everything to be addressed. Please fix things that actually improve code or fix bugs.)
Thank you for the contribution - we'll try to review it as soon as possible. 🙏