params/metrics/exp diff: consistent output

[jorgeorpinel]

Extracted from https://github.com/iterative/dvc.org/pull/2200#discussion_r581757584

• metrics diff currently shows old, new, change
• params diff currently shows old, new
• exp diff currently shows new, change

Should they all be Old, New, Change? (Possibly with some flag to skip Old or Change)

[jorgeorpinel]

cc @dberenbaum

[dberenbaum]

Thanks, @jorgeorpinel! Let's refer this back to #5392.

Should they all be Old, New, Change?

👍

[jorgeorpinel]

What do you mean @dberenbaum? 🙂

[jorgeorpinel]

BTW @pmrowla do you know if we've considered merging dvc params/metrics/exp diff into a single command (e.g. dvc compare)? Or maybe as flags of dvc diff.

[pmrowla]

@jorgeorpinel the idea has come up before as something that we could consider doing in the future, but not seriously considered yet

[dberenbaum]

What do you mean @dberenbaum? 🙂

I just wanted to mention it so we have all of these UI issues linked in that one issue. No other action needed!

[daavoo]

Was about to comment this in #5392 (haven't seen any other related issues). I think that the terms Old / New used in diff sometimes could be confusing, given that users can pass refs in any order.

For example they could run dvc metrics diff {newer commit hash} {older commit hash} and the table will show the values of the newer commit under the column Old.

[dberenbaum]

cc @skshetry

I'd prefer having the ref names instead of Old and New.

[daavoo]

Two questions:

A) Should Change be included by default ??

I might be biased but, in my personal experience (and current docs examples), the "Change" value is kind of awkward most of the times.

I would vote for hiding it by default and making it optional.

B) Should there be an option to hide a_rev (previously "Old") at all?

IMO the only case where it could make sense to not show a_rev is when calling dvc {X} diff {b_rev} (where a_rev is set to HEAD by default.)

Maybe instead of a flag for opting out a_rev, we could just hide it when equals to HEAD.

[dberenbaum]

@daavoo @jorgeorpinel Is there anything left to do on this issue?

[daavoo]

@daavoo @jorgeorpinel Is there anything left to do on this issue?

Showing Change in dvc params diff (If we agree that Change should be showed by default)

[jorgeorpinel]

Not sure, I'd expect some PR to close this at some point but feel free to do so manually when you think it's addressed. Thanks

[dberenbaum]

Params are not always numeric, so I'm not sure what should be shown in Change. Maybe it's a boolean for non-numeric params?

I agree that Change is not generally something I find that useful, but I also don't see it as really harmful.

[daavoo]

Params are not always numeric, so I'm not sure what should be shown in Change. Maybe it's a boolean for non-numeric params?

We are showing Change in the params section of dvc exp diff treating them as scalars https://dvc.org/doc/command-reference/exp/diff#examples 🤷

[dberenbaum]

Ah, okay, it's handled like:

$ dvc exp diff
Path         Param    HEAD    workspace    Change
params.yaml  foo      a       b            diff not supported

I'd vote to keep Change since I see little harm and it seems like a less breaking change, but I don't feel that strongly about it.

[daavoo]

Ah, okay, it's handled like:

$ dvc exp diff
Path         Param    HEAD    workspace    Change
params.yaml  foo      a       b            diff not supported

I'd vote to keep Change since I see little harm and it seems like a less breaking change, but I don't feel that strongly about it.

We could improve the non scalar support by replacing the 'diff not supported' with bool resulting of direct comparisson

[daavoo]

To clarify, what's left to close the issue is:

• Showing Change in dvc params diff

[mesejo]

If this is still open, I would like to take it.

[filippopellizzari]

If no one is currently working on this, I would like to take it.