Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add targets/stages argument to dvc exp show #1
base: main
Are you sure you want to change the base?
Add targets/stages argument to dvc exp show #1
Changes from all commits
2a6eeb8
3cbdf53
dac9c00
738d770
6c73ed1
1f46c0c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 differentdiff
subcommands at the top-level (another possibly interesting proposal doc).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meta: how do we pick between / when to we move from https://github.com/iterative/dvc/discussions to https://github.com/iterative/enhancement-proposals ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'm struggling with that already! Let's discuss at retro.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 individualdvc ... diff
commands (includingdvc diff
itself) already support comparing any git commit or reference (including shortened DVC experiment names). It would probably make more sense to just have onediff
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).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Funny you say that: https://github.com/iterative/enhancement-proposals/tree/unified-show-diff 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for metrics, I think (should they be mentioned?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So is the earlier bullet about metrics correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All metrics are outputs from a DVC stage, which means that:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd go with
targets
for consistency with existing commands (e.g.repro
), and since dvc.yaml files can be targets too.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about
dvc params/metrics diff --targets
, which usetargets
to mean filenames and not stages?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🙂There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any stage in ./dvc.yaml only or any stage anywhere (like
repro -P
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I think any stage anywhere is better, but we need to work on terminology for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the default targets for
repro
etc is just ./dvc.yamlThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just for those specific stages? Wouldn't mind incorporating the
--[up/down]stream
options to this proposal TBH. Full makeover 💇 💅There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or
--single[-item]
as mentioned a bit further down.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The proposal format seems a bit long anyway, so why not cover as much as possible? Reuse the motivation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No strong opinion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW lmk if you'd like me to propose a list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙏 That would be great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be an advantage. Otherwise why have all these different options if they do the same thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I guess its addressed down below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theepochs
param.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But DVC still has to read all those stages and values whereas with
targets
the operation could actually improve performance-wise (perhaps).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another great idea. BTW also similar to
plots show/diff
(plots templates).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just yesterday we talked about being able to group the plots into "report files".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meta: maybe merge this section with Drawbacks?