Add targets/stages argument to dvc exp show

[dberenbaum]

Let's try this out! Here's an initial proposal for how we might better show only relevant parameters and metrics in dvc exp show.

[jorgeorpinel]

To see the "rendered" proposal: https://github.com/iterative/enhancement-proposals/blob/exp-show-target/proposals/exp-show-target.md 👍

[jorgeorpinel]

What's the difference between this proposal and iterative/dvc#5451? Other than the nice format and summary, of course!

[jorgeorpinel]

Perhaps this was extracted from iterative/dvc#5451 (comment) and that issue is focused on introducing the --only-deps flag?

[dberenbaum]

Yeah, the idea was to keep iterative/dvc#5451 focused on the short-term fix of adding an option to show only tracked parameters in dvc metrics/params/exp show. This proposal builds off the discussion there to introduce a potentially larger change specifically for dvc exp show (adding an argument for the stage of interest and only showing the parameters and metrics associated with that stage). In part, this is a trial run to see the pros and cons of moving larger feature changes into enhancement proposals, although this one is still pretty small by design.

[jorgeorpinel]

adding an option to show only tracked parameters

Right that's what confused me, because "This proposal allows users to see only parameters and metrics relevant to specified pipeline stages," which seemed quite similar to iterative/dvc/pull/5550.

potentially larger change specifically for dvc exp show (adding an argument for the stage of interest...

OK I see so exp show clean would display all the same experiments as rows, but only metrics and params for the clean stage as rightmost columns?

[dberenbaum]

OK I see so exp show clean would display all the same experiments as rows, but only metrics and params for the clean stage as rightmost columns?

Correct. An "unresolved question" at the bottom of the doc is whether to include only the relevant experiments as rows.

[jorgeorpinel]

it's becoming useful for other purposes, like comparing multiple commits

Maybe we should discuss that too/separately (as a proposal) 🙂 Should this be a top-level command? Perhaps dvc compare (--exp[eriments])

Although, dvc compare is also the name I proposed to unify the different diff subcommands at the top-level (another possibly interesting proposal doc).

[jorgeorpinel]

discuss that too/separately (as a proposal)

Meta: how do we pick between / when to we move from https://github.com/iterative/dvc/discussions to https://github.com/iterative/enhancement-proposals ?

[dberenbaum]

Meta: how do we pick between / when to we move from https://github.com/iterative/dvc/discussions to https://github.com/iterative/enhancement-proposals ?

Yes, I'm struggling with that already! Let's discuss at retro.

[pmrowla]

This really belongs in a separate discussion, but it seems like we will eventually want to unify all of the diff commands.

exp diff exists as a convenience wrapper for params + metrics diff, but all of the individual dvc ... diff commands (including dvc diff itself) already support comparing any git commit or reference (including shortened DVC experiment names). It would probably make more sense to just have one diff command and where you can specify which output (data/metrics/params) you want to include/exclude (and nothing about this would be specific to experiments).

[dberenbaum]

Funny you say that: https://github.com/iterative/enhancement-proposals/tree/unified-show-diff 😉

[jorgeorpinel]

Same for metrics, I think (should they be mentioned?).

[pared]

I think that in case of metrics we don't have this problem: iterative/dvc#5451 (comment)
because metrics are files while params are loaded as files while they should be loaded as file entries/lines.

[jorgeorpinel]

So is the earlier bullet about metrics correct?

The command's output currently includes: ...

• All values in any referenced metrics files in any DVC stage.

[dberenbaum]

All metrics are outputs from a DVC stage, which means that:

• Unlike parameters, metrics are always tracked by some DVC stage.
• Metrics still may be irrelevant to a particular DVC stage, as mentioned in the bullets below.

[jorgeorpinel]

I'd go with targets for consistency with existing commands (e.g. repro), and since dvc.yaml files can be targets too.

Suggested change

	stages Stages for which to print experiment info. 'dvc.yaml'
	by default.
	usage: dvc experiments show [-h] [-q | -v] [-a] [-T] [-A] [-n <num>]
	...
	[targets [<target> ...]]
	targets Stages to include. 'dvc.yaml' by default.

[dberenbaum]

What about dvc params/metrics diff --targets, which use targets to mean filenames and not stages?

[jorgeorpinel]

I'm thinking of existing commands that target stages (again like repro which predate params and metrics BTW). But yeah --targets for some reason was designed to specifically accept file names and not param/metric stages, maybe something else to reconsider but seems unrelated here 🙂

[jorgeorpinel]

defined in any stage

Any stage in ./dvc.yaml only or any stage anywhere (like repro -P?

[dberenbaum]

Good point. I think any stage anywhere is better, but we need to work on terminology for this.

[jorgeorpinel]

I think the default targets for repro etc is just ./dvc.yaml

[dberenbaum]

Ah, thanks for pointing that out. In that case, I guess we should have a default of ./dvc.yaml and include a -P, --all-pipelines option. Seems like maybe I need to go through the repro options and determine all that should apply here.

[jorgeorpinel]

Why not just for those specific stages? Wouldn't mind incorporating the --[up/down]stream options to this proposal TBH. Full makeover 💇 💅

[jorgeorpinel]

Or --single[-item] as mentioned a bit further down.

[jorgeorpinel]

this is out of scope for now

The proposal format seems a bit long anyway, so why not cover as much as possible? Reuse the motivation

[dberenbaum]

The proposal format seems a bit long anyway, so why not cover as much as possible? Reuse the motivation

True, I'm used to trying to limit scope for issues, but one advantage of this format is to holistically discuss related issues in one cohesive doc. Thoughts on --single[-item]?

[jorgeorpinel]

Thoughts on --single[-item]?

If the idea here is to remove noise from the very busy exp show table maybe single-item should be the default and have --upstream instead (or some other names). No strong opinion though.

Would also throw in --downstream for consistency in either case.

[dberenbaum]

Again, my initial reaction is to follow the repro conventions, which would do upstream by default, but also include:

• -s, --single-item
• -p, --pipeline
• --downstream

[jorgeorpinel]

No strong opinion

[jorgeorpinel]

For this section I think something v useful would be to list the specific docs that could be affected by this (and how), and possibly new ones too.

[jorgeorpinel]

BTW lmk if you'd like me to propose a list.

[dberenbaum]

🙏 That would be great!

[jorgeorpinel]

This may be an advantage. Otherwise why have all these different options if they do the same thing?

[pared]

This may be an advantage.

I agree though we need to remember that we released a major version recently, and this would be kind of a breaking change. We don't know how many users actually want to see current behavior, and if changing it won't make users lose trust in our development process.

[pared]

Ok I guess its addressed down below.

[jorgeorpinel]

Another related-discussion/proposal: can we simplify the flags? E.g. merge the --include, --exclude, and --sort ones (may require them to accept 2 CLI args which I'm not sure is possible). E.g. exp show --inc :all auc,rank --sort p.tsv:epochs asc would include all params and just those 2 metrics, while inverse sorting by the epochs param.

[jorgeorpinel]

Or at least shorten the names. In any case, not sure it's so crowded in terms of functionality, since these 6 flags are all for similar purposes. Similar to dvc plots show which also has a bunch of display-related options.

[jorgeorpinel]

But DVC still has to read all those stages and values whereas with targets the operation could actually improve performance-wise (perhaps).

[jorgeorpinel]

Def. Another good proposal to include 👍 (or make an issue in the core repo directly?)

[jorgeorpinel]

exclude-params=params.json:local_config*

This specific case may already be possible? (Without the wildcard) by specifying a group/parent param path.

[dberenbaum]

Def. Another good proposal to include 👍 (or make an issue in the core repo directly?)

Added links to existing core dvc issues with some of these proposals.

This specific case may already be possible? (Without the wildcard) by specifying a group/parent param path.

I don't think it works without a fully specified param name, but let me know if you find otherwise.

[jorgeorpinel]

Seems to work:

$ dvc exp show --no-pager
┏━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━┳━━━━━━━┳━━━━━━━┳━━━┓
┃ Experiment    ┃ Created      ┃ p.one ┃ p.two ┃ q ┃
┡━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━╇━━━━━━━╇━━━━━━━╇━━━┩
│ workspace     │ -            │ 1     │ 2     │ q │
│ master        │ Mar 01, 2021 │ 1     │ 2     │ q │
│ └── exp-d662e │ Mar 01, 2021 │ 1     │ 2     │ q │
└───────────────┴──────────────┴───────┴───────┴───┘
$ dvc exp show --no-pager --include-params p
┏━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━┳━━━━━━━┳━━━━━━━┓
┃ Experiment    ┃ Created      ┃ p.one ┃ p.two ┃
┡━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━╇━━━━━━━╇━━━━━━━┩
│ workspace     │ -            │ 1     │ 2     │
│ master        │ Mar 01, 2021 │ 1     │ 2     │
│ └── exp-d662e │ Mar 01, 2021 │ 1     │ 2     │
└───────────────┴──────────────┴───────┴───────┘
$ dvc exp show --no-pager --include-params p.two
┏━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━┳━━━━━━━┓
┃ Experiment    ┃ Created      ┃ p.two ┃
┡━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━╇━━━━━━━┩
│ workspace     │ -            │ 2     │
│ master        │ Mar 01, 2021 │ 2     │
│ └── exp-d662e │ Mar 01, 2021 │ 2     │
└───────────────┴──────────────┴───────┘

[dberenbaum]

Oh, nice! I had tried with just a filename, which doesn't work, but I guess a yaml section does work.

[pmrowla]

The current include/exclude filtering implementation is naive - filtering is done at the UI level based on string comparisons for the column names.

Filtering by filename will work for cases where the table actually displays the filenames (meaning whenever a duplicate key exists in multiple files). But in normal cases where we do not need to display filenames, using filename: won't do anything.

So if your repo has params1.json:foo and params2.json:foo, the table will include the filenames as part of the column title, and exp show will also respect the filename: part of a column filter.

[dberenbaum]

See also iterative/dvc#5720

[jorgeorpinel]

Another great idea. BTW also similar to plots show/diff (plots templates).

[pared]

Just yesterday we talked about being able to group the plots into "report files".

[jorgeorpinel]

➕

[pared]

➕

[jorgeorpinel]

Although... Since there's already --show-json, TSC/CSV may be too much (you can easily convert from JSON to tabular).

[dberenbaum]

Although... Since there's already --show-json, TSC/CSV may be too much (you can easily convert from JSON to tabular).

This isn't true for dvc exp show, since the json is not structured like tabular data. Try it out and the results are kind of a mess (too messy to post in a comment here). The json could be cleaned up to make this easy, but it was specifically designed as output for downstream apps to consume instead of as a format that users can easily understand. So I guess it's less about having a csv format specifically, and more about having an easily understood output format that can be saved and parsed.

[jorgeorpinel]

Sounds like this def. covers the discussion about default behavior then, which I think is good (but may need clarification earlier on in the doc).

[jorgeorpinel]

Meta: maybe merge this section with Drawbacks?

[jorgeorpinel]

Sorry for the mixed input about the proposal format itself (meta), about statement clarifications, and about the actual proposal. It's a bit challenging to get out of the "I'm reviewing would-be-published doc" mentality into "I'm reviewing a proposal" under GH, PLUS trying to eval the format along the way 🙃

[pared]

Ok, so I had some comments, none that would be any kind of blocking concern. How do we treat PR's in this repo? I agree with current state of proposal = I approve it? Or the unresolved questions needs to be addressed before that?

[dberenbaum]

Ok, so I had some comments, none that would be any kind of blocking concern. How do we treat PR's in this repo? I agree with current state of proposal = I approve it? Or the unresolved questions needs to be addressed before that?

Good question! Maybe we treat it like any other PR? Approve once you think the current state of the proposal is in good enough shape to be merged? If you still have questions you think should be resolved first, then I would think you shouldn't approve yet.

[pmrowla]

Implementing this will require implementing it at the metrics and params level. exp show just calls the same metrics/params collection methods used by the metrics/params diff/show commands. So even if this doesn't get reflected in the actual metrics/params CLI UI, this is really still a core repo "add [pipeline] targets/stages support for metrics/params collection" issue, and is not specific to exp show.

Once the core metrics/params collection methods support pipeline targets, this should be trivial to implement - the table just shows whatever is returned by the metrics/params collection methods. So if those methods are only returning the desired specific pipeline stage(s), the existing exp show functionality will work out of the box.

---

Regarding doing anything more than that on the UI side of things:

In my opinion, we should only worry about providing a minimum level of functionality in exp show. Implementing more advanced filtering/sorting/etc UI should be done where it makes sense, like in the viewer and VSCode extension.

We already support (naively) filtering/excluding keys by yaml dictionary/hierarchy level, so given a params.yaml which is organized like

- featurize:
  - foo: 1
  - bar: 2
- train:
  - baz: 3

you can filter using featurize to match both featurize.foo and featurize.bar. So it is already possible for a user to mimic "targets" functionality if they organize their params/metrics in this way.

There is an existing issue to add shell glob support for this iterative/dvc#5642, and there is already an (unreviewed) contributor PR implementation.

Once the functionality for excluding untracked params is done (iterative/dvc#5451), I think this should be considered "good enough" for the state of exp show UI (in addition to the proposed core metrics/params targets functionality).

---

Implementing the core functionality for collecting metrics/params with specific targets/stages is fine and will clearly be beneficial for DVC, but doing anything more on the UI side should be left open to contributors (like with the shell globbing support). IMO the core team should not be spending too much time on extra "nice to have" UI functionality (at least for now given the current team size/bandwidth).

[dberenbaum]

@pmrowla Appreciate the detailed feedback!

It's probably good to explicitly set the expectation that feedback doesn't have to be limited to small tweaks. It should definitely include any debate of whether the proposal is worthwhile.

Once the functionality for excluding untracked params is done (iterative/dvc#5451), I think this should be considered "good enough" for the state of exp show UI (in addition to the proposed core metrics/params targets functionality).

I don't think that issue addresses the full scope of this proposal. In particular, this proposal adds support for stages as arguments, which not only filters out parameters that are untracked in any stage, but also filters parameters and metrics untracked by that particular stage (or upstream of it).

[pmrowla]

I don't think that issue addresses the full scope of this proposal. In particular, this proposal adds support for stages as arguments, which not only filters out parameters that are untracked in any stage, but also filters parameters and metrics untracked by that particular stage (or upstream of it).

To clarify, my "in addition to" point was that once the core metrics/params collection code is updated to support requesting a specific pipeline target (meaning current repro target, not params --targets), exp show will already work out of the box, and we should not need to do anything else on the UI side (other than adding the CLI flags to control whether or not upstream params/metrics get included)