iterative
plots: templates option is clunky
yes, it's more about this command's interface in this case. Feels like this list {simple,linear,confusion,confusion_normalized,scatter,smooth} will be growing / changing. Why do we have to specify it on the parser level, it's not very common to limit that large number of targets there.
Originally posted by @shcheklein in https://github.com/iterative/dvc.org/pull/3691#discussion_r907907003
Would this be enough to simplify it?
$ dvc plots templates --help
usage: dvc plots templates [-h] [-q | -v] [-l] [-o <path>] [<template>] # Simplify usage
...
$ dvc plots templates -l # Add -l to show templates
simple
linear
...
$ dvc plots templates simple
Templates have been written into '.dvc/plots'.
$ dvc plots templates invalid # Show appropriate error if invalid template arg passed.
ERROR: 'invalid' is not a valid template.
Yep, it's not clear what why we need -l in this case. It can behave similar to git tag, git branch, dvc cache dir, etc - show something useful by default.
Make sense to add --all? not sure.
I was trying to avoid breaking changes here.
Also, I think of it as closer to dvc config where the command primarily serves to write but also has an option to show info and exit 🤷 .
show something useful by default.
dvc plots templates by default dumps all built-in plots templates. I think it's expected that this is useful and probably the most common usage pattern. That way, you can inspect not only the template names but also their contents.
Make sense to add
--all?
What would --all do? Are you suggesting that dvc plots templates should only act on a single template unless --all is provided? It currently supports a template name as an optional argument but acts on all templates otherwise.
Ultimately, I would have no problem redesigning the CLI if starting from scratch. Is there enough reason to make breaking changes here?
All things I'm saying are not strong opinions, command is not critical. It just feel too niche and interface indeed doesn't remind anything we have in DVC and in general feels unusual.
I was trying to avoid breaking changes here.
It's not even released yet, right?
dvc plots templates by default dumps
got it. do you expect by chance that you would need a bit more? modify, list, etc?
It means also, that we have to specify / hard code the list somewhere (since there is no list command). Initial suggestion to make it just template won't work. But it feels unusual and not scalable.
(sorry, still feels like we have a global command for a very niche / narrow / utility like scenario)
Are you suggesting that dvc plots templates should only act on a single template
Yes, I didn't check the default behavior now. Since it dumps files can be a bit too intrusive?
Yeah, not critical, and maybe this command isn't necessary 🤷 . I'm trying to minimize additional work and changes on it 😅, but we should make changes if we identify obvious improvements.
It's not even released yet, right?
Documentation is lagging on it, but it was included several minor versions ago and has been recommended to users who have asked. Not a big deal, but obviously let's err on the side of consistency until we identify changes worth making.
got it. do you expect by chance that you would need a bit more? modify, list, etc?
I would expect more that we may need more dump flags like --force or the existing -o, which is why I'm not crazy about listing by default and adding --dump. Dumping by default seems unusual and intrusive, but I think users often need it so they can inspect and modify the JSON files.
Do you envision a broader set of template commands? Is it because of the command name (maybe dvc init plots/templates would be more intuitive)? Template subcommands seem intuitive but also could be premature, and we have so far avoided a 3rd level of nesting like dvc plots templates list/dump.
It means also, that we have to specify / hard code the list somewhere (since there is no list command).
Dumping them is the way to see them (and inspect or modify them) -- similar to how we used to point to .dvc/plots to see them. A couple ideas how we can improve it:
-
dvc remote add --helplinks to the docs to show all supported remote urls. We could do the same for templates.
$ dvc remote add --help
...
positional arguments:
name Name of the remote
url Remote location. See full list of supported URLs at <https://man.dvc.org/remote>
...
- What if we list the templates that were dumped?
$ dvc plots templates
Templates have been written into '.dvc/plots':
simple
linear
confusion
...
A couple ideas how we can improve it:
We can, but still, looking 3rd time into the command, I still feel that this default behavior is too intrusive (write things into files by default ... and yes, it's better to have a message like Templates have been written into '.dvc/plots' at least).
but I think users often need it so they can inspect and modify the JSON files.
my 2cs - still don't feel it justifies this default :) Also, we should be consistent (not sure we have some good analogs in this case though).
I would just change it to list and would add -dump-all or just -all to dump them all.
Do you envision a broader set of template commands?
no, I don't :) agreed that it's premature to do things like 4th level sub-commands, dvc init templates, etc. But even by doing list by default it feels you will have more flexibility in the future if needed (?) - this is just intuition primarily.
https://github.com/iterative/dvc-render/issues/8 is an example where we would have to update the list of templates in a few places?
iterative/dvc-render#8 is an example where we would have to update the list of templates in a few places?
I think we should just replace the hardcoded names with something like :
from dvc_render.vega_templates import TEMPLATES
TEMPLATE_CHOICES = [x.default_name for x in TEMPLATES]
@shcheklein I'm still struggling to envision the command 🤔 .
The default could look like:
$ dvc plots templates
simple
linear
confusion
...
If you are listing them all by default, --all seems like a non-intuitive option to dump them, so maybe just --dump?
$ dvc plots templates --dump
Templates have been written into '.dvc/plots'
simple
linear
confusion
...
Would you keep a way to get a single template? It's probably not strictly needed but is nice to have if you know you only want to modify a specific template. dvc plots templates linear wouldn't make much sense if it just lists the template though. Or should dvc plots templates linear print the template to stdout (it complies with the https://clig.dev/#the-basics rule to send output to stdout)?
Example syntax:
$ dvc plots templates simple
{
"$schema": "https://vega.github.io/schema/vega-lite/v5.json",
"data": {
"values": "<DVC_METRIC_DATA>"
},
...
What about -o? Again, it's probably not strictly needed, but it's helpful if you want to dump a template without fear of deleting/overriding existing templates. Maybe --dump isn't needed and -o can signal wanting to dump to disk? I prefer that to having --dump and -o, but it could mean getting rid of the default output dir, which may be useful if we are keeping that as a default plots directory. Also, would it be obvious that -o/--dump could be used either for an individual template or for all templates at once?
Example syntax:
$ dvc plots templates -o plots_dir
Templates have been written into:
plots_dir/simple.json
plots_dir/linear.json
...
$ dvc plots templates simple -o plots_dir
Templates have been written into:
plots_dir/simple.json
Overall, there are some changes I like but it still feels clunky to me since the default is to list the templates while every other option involves dumping.
good stuff @dberenbaum !
If you are listing them all by default, --all seems like a non-intuitive option to dump them
agreed
Would you keep a way to get a single template?
yep, feels it's nice to have, I would try to make it happen
dvc plots templates linear print the template to stdout
🤔 sound reasonable
What about -o?
again, sounds reasonable
Overall, there are some changes I like but it still feels clunky to me since the default is to list the templates while every other option involves dumping.
yes, still feels like a great improvement
probably we can find a lot of example where default does one thing and specifying a target another.
e.g.:
If --list is given, or if there are no non-option arguments, existing branches are listed; the current branch will be highlighted in green and marked with an asterisk. ...
if you pass a target it'll create a new branch.
@shcheklein tldr you prefer the suggested syntax to the current one? 😄
I have no strong preference except that we nail down exactly the expected syntax before we spend more time on this command.
@pared WDYT?
I think that the simplest seems to be just list available templates on dvc plots templates and dvc plots templates {name} would print to stdout. No dumping all templates, no .dvc/plots behavior - just let user do whatever they want. That is simple and easy to understand.
