Introduce stage:list command with autocompletions #5282

skshetry · 2021-01-18T09:25:09Z

Autocompletion only limited to the dvc repro for now.

❗ 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.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

skshetry · 2021-01-18T09:26:08Z

dvc/command/completion.py

+}
+_dvc_compadd_stages() {
+    # this will also show up the description of the stages
+    _describe 'stages' "($(_dvc_stages_output))"


Maybe it'd be better to create a hidden flag for completions instead of using awk?

I think it makes sense, that would make completions "test"-able to some extent.

pared

Played around a bit on my own machine. There seems to be a bit of delay, but its much better than when we were testing it on #5191 🚀

skshetry · 2021-01-18T11:39:48Z

There seems to be a bit of delay

Sadly, most of the time is just the startup time. If you are not in shtab >= 1.3.4 version, then it might be pkg_resources as well. Or, if you are in Python3.9, don't forget to pip install ruamel.yaml.clib.

For the example-get-started, it takes ~300ms and for a quite large dvc.yaml file with ~100 stages, it takes ~400ms on my machine (just to compare).

pared · 2021-01-18T12:13:55Z

@skshetry I am using 3.7, shtab==1.3.4. Don't get me wrong, in my case the delay is small, so that I don't feel fear from using completions, which was the case last time we have been measuring it. So I would say it's pretty great.

shcheklein · 2021-01-18T20:14:34Z

dvc/command/stage.py

+        formatter_class=argparse.RawDescriptionHelpFormatter,
+    )
+    stage_list_parser.add_argument(
+        "targets", nargs="*", default=["dvc.yaml"], help="Stages to list from",


Files to list from?

@shcheklein, it can also be a directory/foreach-stage-name/stage-name.

kk, my point that this combination default=["dvc.yaml"] and "Stages to list from" sounds strange a bit and it's not clear what does it exactly expect/can take.

This has been a common problem in most of our commands these days as they all support these kinds of target spec. I don't have a good idea to make it concise and also document it clearly.

I changed it to the following (removed or a stage name to keep it short for now):

Show stages from a dvc.yaml/.dvc file or a directory. 'dvc.yaml' by default.

I looked into pip install and git add, and they mostly seem to not explain positional arguments.

pip

$ pip install -h Usage: pip install [options] <requirement specifier> [package-index-options] ... pip install [options] -r <requirements file> [package-index-options] ... pip install [options] [-e] <vcs project url> ... pip install [options] [-e] <local project path> ... pip install [options] <archive url/path> ...

git

$ git add -h usage: git add [<options>] [--] <pathspec>...

maybe we could do similar to pip in our commands?

You are right, but in this case it should be less generic as targets. In case of pip local project path is a good example. It doesn't require any further explanations.

Git is so-so. They use pathspec but they are very consistent and it's easy to google it (it's very specific). Also, they have examples usually around.

We could also replace similar to the pip with:

$ dvc stage list -h Usage: dvc stage list [path to dvc.yaml]... dvc stage list -R <dir>... dvc stage list [path to dvc.yaml][:<stage name>]... dvc stage list --all

shcheklein · 2021-01-19T21:26:31Z

A few quick notes on the workflow:

since it affects UI/UX - it would be great to see some summary to review and ask @jorgeorpinel or @dberenbaum to review messages/descriptions
do we have a ticket for dvc.org on this? (it's checked, but I don't see it)

jorgeorpinel

Stage list description UI text suggestions:

dvc/command/stage.py

jorgeorpinel · 2021-01-20T02:56:16Z

dvc/command/stage.py

+
+    plots_and_metrics = list(filter(is_plot_or_metric, stage.outs))
+    if plots_and_metrics:
+        desc.append("Generates " + part_desc(plots_and_metrics))


"Outputs metric " ?

That's too long. I'd only like to add a single verb here.

I tried this in a test repo I have and there's no indication about what "Produces/Outputs" and "Generates" mean.

$ dvc stage list ab Produces ab abm Generates ab.met

Maybe also use "Outputs" (single verb) and append "(metric)" or "(plot)" at the end of the line?

ab Outputs ab abm Outputs ab.met (metric)

Adding metric and plot is one more word for each of the outputs. Would be better to find a single word to explain all these kinds of output.

Remember that user will have the output path that is already quite lengthy. I am trying to keep it within a single line.

Oops sorry super ~~long~~ late reply. I know this is merged so OK. But for the record, keeping "produces" and "generates" seems to me like the worst alternative. It's unclear why the former refers to regular outputs and the latter to metrics/plots.

jorgeorpinel · 2021-01-20T02:58:06Z

(Most of the strings in https://github.com/iterative/dvc/pull/5282/files#diff-cd4d8ab7bbb98bdac7ebf8f89f4c65961526eea6bd78a142e11d4da9835e2bf7 are not covered in tests according to Codecov BTW, not sure if that's a problem)

jorgeorpinel

stage list -h string suggestions and some Qs:

jorgeorpinel · 2021-01-20T04:55:06Z

dvc/command/stage.py

+        "targets",
+        nargs="*",
+        default=["dvc.yaml"],
+        help=(
+            "Show stages from a dvc.yaml/.dvc file or a directory. "
+            "'dvc.yaml' by default"


"Limit command scope to the stages in these .dvc or dvc.yaml files."

Similar to https://dvc.org/doc/command-reference/repro (and others)

We can explain the default and about directories in the options and in docs.

But wait, why .dvc files? What stages are there?

And, so this doesn't accept stage names? E.g. to confirm that a stage exists (like regular list for artifacts)

And, so this doesn't accept stage names?

It does, but I removed it to keep it short.

"Limit command scope to the stages in these .dvc or dvc.yaml files."

I don't think limit applies here, it's more like changing the scope completely. Though I don't like the repro's either.

"Limit" applies as much as in all the existing targets arg descriptions in other commands, I think. It's usually dvc.yaml by default, and the base use case of this is to narrow it down.

I don't prefer Limit here as the main use of targets here is to change the scope of dvc.yaml to be used, rather than narrowing it.

OK then something like `.dvc file or dvc.yaml files to show stages from". Should def. be plural at least since it's "targets".

jorgeorpinel · 2021-01-20T05:09:36Z

dvc/command/stage.py

+        "--all",
+        action="store_true",
+        default=False,
+        help="List all of the stages in the repo.",


"List all of the stages in the repo regardless of `targets`." (if I'm understanding this correctly)

I find "regardless of targets" to be redundant due to the "all of the stages" there.

It's good to be redundant sometimes. We need to be extra clear.

We often describe things correctly but people still get confused. All the commands an options are not obvious at all and as the developers we are biased because we already deeply understand them.

jorgeorpinel · 2021-01-20T05:11:34Z

dvc/command/stage.py

+        "--fail",
+        action="store_true",
+        default=False,
+        help="Fail immediately if there's an error.",


Doesn't error imply failing already?
Not sure how to try this locally, what kind of error? Thanks

Syntax Errors in yaml when using --all.

OK. So my point was that "fails if there's an error" doesn't explain much/ seems redundant. Maybe we can update these messages in some upcoming PR?

I have opened #5390 regarding discussion of UI messages. Will update the flags as that will most likely affect it.

Yeah I found that issue after checking this. Cool, thanks!

Tried to address this on #5499, please take a look.

#5499 LGTM 👍

dvc/command/stage.py

jorgeorpinel · 2021-02-05T03:51:59Z

dvc/command/stage.py

+class CmdStageList(CmdBase):
+    def _get_stages(self) -> Iterable["Stage"]:
+        if self.args.all:
+            stages: List["Stage"] = self.repo.stages  # type: ignore


One last post-merge comment here. Should it be dvc stages add/list? In plural like params/metrics/plots/experiments (Only remote is in singular, which is OK for Git familiarity I think)

Good point. Subcommand should usually be plural, which we should better discuss on #5392 as a guideline.

skshetry added enhancement Enhances DVC feature is a feature labels Jan 18, 2021

skshetry requested review from pmrowla, efiop and pared January 18, 2021 09:25

skshetry self-assigned this Jan 18, 2021

skshetry commented Jan 18, 2021

View reviewed changes

pared approved these changes Jan 18, 2021

View reviewed changes

skshetry force-pushed the stage-list branch from e1b242d to 962da8e Compare January 18, 2021 13:36

Implement stage:list

962da8e

shcheklein reviewed Jan 18, 2021

View reviewed changes

change targets help message

4c38942

efiop approved these changes Jan 19, 2021

View reviewed changes

skshetry merged commit 372d6f0 into iterative:master Jan 19, 2021

skshetry deleted the stage-list branch January 19, 2021 11:50

jorgeorpinel reviewed Jan 20, 2021

View reviewed changes

jorgeorpinel reviewed Feb 5, 2021

View reviewed changes

jorgeorpinel mentioned this pull request Feb 5, 2021

UI/UX of the stage:list command #5390

Closed

skshetry mentioned this pull request Feb 19, 2021

stage:list: update output and help text #5499

Merged

2 tasks

dberenbaum mentioned this pull request Feb 19, 2021

Common UI improvements #5392

Closed

1 task

Introduce stage:list command with autocompletions #5282

Introduce stage:list command with autocompletions #5282

Conversation

skshetry commented Jan 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pared left a comment • edited Loading

Choose a reason for hiding this comment

skshetry commented Jan 18, 2021 • edited Loading

pared commented Jan 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skshetry Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

shcheklein commented Jan 19, 2021

jorgeorpinel left a comment • edited Loading

Choose a reason for hiding this comment

jorgeorpinel Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

skshetry Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

This comment was marked as resolved.

jorgeorpinel Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgeorpinel Feb 5, 2021 • edited Loading

Choose a reason for hiding this comment

jorgeorpinel commented Jan 20, 2021

jorgeorpinel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgeorpinel Feb 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgeorpinel Feb 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skshetry Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

jorgeorpinel Feb 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pared left a comment •

edited

Loading

skshetry commented Jan 18, 2021 •

edited

Loading

skshetry Jan 20, 2021 •

edited

Loading

jorgeorpinel left a comment •

edited

Loading

jorgeorpinel Jan 20, 2021 •

edited

Loading

skshetry Jan 20, 2021 •

edited

Loading

jorgeorpinel Jan 20, 2021 •

edited

Loading

jorgeorpinel Feb 5, 2021 •

edited

Loading

jorgeorpinel Feb 5, 2021 •

edited

Loading

jorgeorpinel Feb 22, 2021 •

edited

Loading

skshetry Jan 20, 2021 •

edited

Loading

jorgeorpinel Feb 5, 2021 •

edited

Loading