exp show: Add `--only-changed` option #6867

daavoo · 2021-10-25T19:47:54Z

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

dvc.org P.R: treeverse/dvc.org#2966

Also, pre-requisite for #4455

Demo:

$ dvc exp show
┏━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━┳━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━┓
┃ Experiment            ┃ Created      ┃     auc ┃ featurize.max_fea… ┃ featurize.ngrams ┃ prepare.seed ┃ prepare.split ┃ train.n_estimators ┃ train.seed ┃
┡━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━╇━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━┩
│ workspace             │ -            │ 0.61314 │ 1500               │ 2                │ 20170428     │ 0.2           │ 50                 │ 20170428   │
│ 10-bigrams-experiment │ Jun 20, 2020 │ 0.61314 │ 1500               │ 2                │ 20170428     │ 0.2           │ 50                 │ 20170428   │
│ ├── exp-e6c97         │ Oct 21, 2020 │ 0.61314 │ 1500               │ 2                │ 20170428     │ 0.2           │ 50                 │ 20170428   │
│ ├── exp-1dad0         │ Oct 09, 2020 │ 0.57756 │ 2000               │ 2                │ 20170428     │ 0.2           │ 50                 │ 20170428   │
│ └── exp-1df77         │ Oct 09, 2020 │ 0.51676 │ 500                │ 2                │ 20170428     │ 0.2           │ 50                 │ 20170428   │
└───────────────────────┴──────────────┴─────────┴────────────────────┴──────────────────┴──────────────┴───────────────┴────────────────────┴────────────┘

$ dvc exp show --only-changed
┏━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━┳━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━━━┓
┃ Experiment            ┃ Created      ┃     auc ┃ featurize.max_features ┃
┡━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━╇━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━┩
│ workspace             │ -            │ 0.61314 │ 1500                   │
│ 10-bigrams-experiment │ Jun 20, 2020 │ 0.61314 │ 1500                   │
│ ├── exp-e6c97         │ Oct 21, 2020 │ 0.61314 │ 1500                   │
│ ├── exp-1dad0         │ Oct 09, 2020 │ 0.57756 │ 2000                   │
│ └── exp-1df77         │ Oct 09, 2020 │ 0.51676 │ 500                    │
└───────────────────────┴──────────────┴─────────┴────────────────────────┘

tests/unit/test_tabular_data.py

dberenbaum

I'm not sure about the name or description. There are two possible meaning of changed values:

Whether the table column has 0 variance/all the same values. This is the current feature AFAIK.
Whether the experiment value changed from its parent commit. I think this is technically closer to what was requested in #5966 and #6778.

The behavior will be similar since I think exp show always includes the parent commit with its associated experiments. I think the only time it will be different is if two parent commits have different values but the experiments don't modify those values.

Since this is pretty minor, the current functionality makes sense and is intuitive. However, I do think we should be precise about the meaning, especially since we might have other functionality to show changes from the parent commit (we could use highlighting for that, for example).

What do you think? Is there a better option name or description to summarize the behavior?

dberenbaum · 2021-10-26T15:17:40Z

How does this interact with other --include/--exclude options? Should we specify and test for those?

daavoo · 2021-10-26T15:43:05Z

How does this interact with other --include/--exclude options? Should we specify and test for those?

This is the last operation applied before rendering the table. It is applied to the TabularData whereas --include/--exclude options are applied before TabularData is created.

We might need to specify that (in the current state of the P.R.) --only-changed might "ignore" the --include flags For example, if you --include some param/metric that doesn't vary across experiments, it will not be shown. For --exclude the two flags are just complementary.

dberenbaum · 2021-10-26T15:49:10Z

We might need to specify that (in the current state of the P.R.) --only-changed might "ignore" the --include flags

Do you think this makes sense from a user perspective? I would probably expect --include to override since it's the more specific option.

skshetry · 2021-10-26T15:53:01Z

One question that I have, what do the users care more about: looking through the entire history of parameters/metrics or just a few recent experiments? I know there are use-cases for both of those but just wanted to understand which one do you guys think is more important.

Looking at the current complexity of dvc exp show and our plans to add pipelines/data changes into it, maybe we are optimizing the wrong commands instead of improving exp diff and exp list?

daavoo · 2021-10-26T15:59:31Z

We might need to specify that (in the current state of the P.R.) --only-changed might "ignore" the --include flags

Do you think this makes sense from a user perspective? I would probably expect --include to override since it's the more specific option.

Before introducing this command, I have always considered both --include and --exclude useful for filtering out metrics/params (because --include actually means --only-include).

However, I understand that users might expect --include to prevail when combined with this new option. I will update the P.R. to handle that.

skshetry · 2021-10-26T16:06:39Z

dvc/compare.py

            {k: self._columns[k][i] for k in keys} for i in range(len(self))
        ]

+    def drop_duplicates(self, axis: str):


My plan for TabularData was to evolve the APIs to mirror those of like pandas and then split it into a separate library. I love that you are also taking the inspiration.

I did make a mistake on the internals of it, and by locking the type to just str instead of arbitrary data (and, no support for NaN :(), which I did mostly for performance reasons as we'd have to coerce during rendering. If you are fighting too much with the internals, please feel free to change it. As I said, my plan is to split it as a pure-python lightweight pandas library in the future when we improve APIs and internals 🙂 (we could separate it today too).

That's very good to hear.

In fact, preserving the original type was one of the next steps I was about to bring up for discussion. It is not a fully blocker for the parallel coordinates but it would definitely make it easier to integrate.

I also felt that TabularData was becoming a good candidate to be one of the core components of the future experiments.api; I assume you also felt that way?

The arbitrary-type support would allow us to support custom serializers for NaNs (instead of hacking around with fill_value), float values (instead of putting the responsibility on the caller) and composite objects (dict/list). Though at the expense of performance.

I also felt that TabularData was becoming a good candidate to be one of the core components of the future experiments.api

I am not really sure what experiments.api is, or what its scope is tbh. One thing to keep in mind though, the CLI that we render is actually a nested table, a table for each commit, and each table having their n number of experiments, which is flattened later. This is why the --include/--exclude and --sort-by does not really translate well to the TabularData. pandas does support nested tables, but I don't think it's worth complicating it here. I am not sure if we need to distinguish between that for experiments.api.

daavoo · 2021-10-26T16:17:31Z

I'm not sure about the name or description. There are two possible meaning of changed values:
1. Whether the table column has 0 variance/all the same values. This is the current feature AFAIK.

2. Whether the experiment value changed from its parent commit. I think this is technically closer to what was requested in #5966 and #6778.
The behavior will be similar since I think exp show always includes the parent commit with its associated experiments. I think the only time it will be different is if two parent commits have different values but the experiments don't modify those values.

Since this is pretty minor, the current functionality makes sense and is intuitive. However, I do think we should be precise about the meaning, especially since we might have other functionality to show changes from the parent commit (we could use highlighting for that, for example).

What do you think? Is there a better option name or description to summarize the behavior?

I'm not fully understanding the meaning of point number 2. Could you please share an example of what would be expected behavior with some fake data?

dberenbaum · 2021-10-26T20:28:37Z

I'm not fully understanding the meaning of point number 2. Could you please share an example of what would be expected behavior with some fake data?

Let's say the output with the current functionality looks like:

$ dvc exp show --only-changed
┏━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━┳━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━━━┓
┃ Experiment            ┃ Created      ┃     auc ┃ featurize.max_features ┃
┡━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━╇━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━┩
│ workspace             │ -            │ 0.61314 │ 1500                   │
│ 10-bigrams-experiment │ Jun 20, 2020 │ 0.61314 │ 1500                   │
│ ├── exp-e6c97         │ Oct 21, 2020 │ 0.61314 │ 1500                   │
│ ├── exp-1dad0         │ Oct 09, 2020 │ 0.57756 │ 1500                   │
│ └── exp-1df77         │ Oct 09, 2020 │ 0.51676 │ 1500                   │
│ 9-bigrams-model       │ Jun 20, 2020 │ 0.53238 │ 2000                   │
│ └── exp-f780a         │ Oct 21, 2020 │ 0.54398 │ 2000                   │
└───────────────────────┴──────────────┴─────────┴────────────────────────┘

Technically, no experiments being shown change featurize.max_features from their parent commit. It would be weird to not show the featurize.max_features column in this case, so I'm definitely not suggesting changing the behavior, but I think we need to explain it. Actually, I think the the description in treeverse/dvc.org#2966 is pretty accurate:

 - `--only-changed` - Only show metrics/params with values varying across the
  selected experiments.

dberenbaum · 2021-10-26T20:31:26Z

Looking at the current complexity of dvc exp show and our plans to add pipelines/data changes into it, maybe we are optimizing the wrong commands instead of improving exp diff and exp list?

Good point. Should we discuss in a separate issue/discussion?

daavoo · 2021-10-27T10:14:18Z

Looking at the current complexity of dvc exp show and our plans to add pipelines/data changes into it, maybe we are optimizing the wrong commands instead of improving exp diff and exp list?

Good point. Should we discuss in a separate issue/discussion?

Referenced in #6451

pared

I am fine with the current state,
but the question is whether we want to discuss @skshetry comment first?

dberenbaum · 2021-10-27T14:37:04Z

I am fine with the current state,
but the question is whether we want to discuss @skshetry comment first?

I don't think it should block this PR, especially since this PR is pretty well aligned with the concerns raised by @skshetry. It's not like it's adding more to the output of exp show.

efiop · 2021-10-29T12:29:38Z

@daavoo Please rebase

This method mimics pandas `drop_duplicates` when axis=rows. For axis=cols this method will drop columns with 0-variance (only 1 unique value). NaNs (`_fill_value`) won't be counted as unique values. So the following column (`-` would be the `_fill_value`) will be dropped: foo - foo foo

Use TabularData `drop_duplicates` to only show metrics/params with values varying across the selected experiments. Fixes #5966

daavoo · 2021-10-29T12:51:09Z

@daavoo Please rebase

That's all I do these days 🧐

daavoo requested a review from a team as a code owner October 25, 2021 19:47

daavoo requested a review from pared October 25, 2021 19:47

daavoo added the feature is a feature label Oct 25, 2021

daavoo self-assigned this Oct 25, 2021

daavoo mentioned this pull request Oct 25, 2021

ref: exp show --only-changed treeverse/dvc.org#2966

Merged

pared suggested changes Oct 26, 2021

View reviewed changes

tests/unit/test_tabular_data.py Show resolved Hide resolved

dberenbaum reviewed Oct 26, 2021

View reviewed changes

skshetry reviewed Oct 26, 2021

View reviewed changes

daavoo mentioned this pull request Oct 27, 2021

show: revisiting table format? #6451

Closed

daavoo force-pushed the exp-show-only-changed branch 3 times, most recently from 1b67f26 to a6f470c Compare October 27, 2021 11:45

dberenbaum approved these changes Oct 27, 2021

View reviewed changes

pared reviewed Oct 27, 2021

View reviewed changes

pared approved these changes Oct 27, 2021

View reviewed changes

daavoo changed the title ~~Introduce exp show --only-changed~~ exp show: Add --only-changed option Oct 27, 2021

daavoo force-pushed the exp-show-only-changed branch from a6f470c to dd986f1 Compare October 27, 2021 17:26

daavoo added A: experiments Related to dvc exp A: diff/show labels Oct 27, 2021

daavoo added 2 commits October 29, 2021 14:48

exp show: Add --only-changed option.

c597e98

Use TabularData `drop_duplicates` to only show metrics/params with values varying across the selected experiments. Fixes #5966

daavoo force-pushed the exp-show-only-changed branch from dd986f1 to c597e98 Compare October 29, 2021 12:49

daavoo requested a review from efiop October 29, 2021 15:04

daavoo mentioned this pull request Nov 5, 2021

exp show: Add parallel coordinates plot. #6933

Merged

2 tasks

efiop merged commit 72066f2 into master Nov 10, 2021

efiop deleted the exp-show-only-changed branch November 10, 2021 14:52

iesahin mentioned this pull request Dec 8, 2021

check documentation for dvc 2.9 release treeverse/dvc.org#3074

Closed

21 tasks

skshetry mentioned this pull request Dec 22, 2021

TabularData: Use None internally #7167

Closed

exp show: Add --only-changed option #6867

exp show: Add --only-changed option #6867

Uh oh!

Conversation

daavoo commented Oct 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

dberenbaum left a comment

Choose a reason for hiding this comment

Uh oh!

dberenbaum commented Oct 26, 2021

Uh oh!

daavoo commented Oct 26, 2021

Uh oh!

dberenbaum commented Oct 26, 2021

Uh oh!

skshetry commented Oct 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daavoo commented Oct 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skshetry Oct 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daavoo Oct 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skshetry Oct 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daavoo commented Oct 26, 2021

Uh oh!

dberenbaum commented Oct 26, 2021

Uh oh!

dberenbaum commented Oct 26, 2021

Uh oh!

daavoo commented Oct 27, 2021

Uh oh!

pared left a comment

Choose a reason for hiding this comment

Uh oh!

dberenbaum commented Oct 27, 2021

Uh oh!

efiop commented Oct 29, 2021

Uh oh!

daavoo commented Oct 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

exp show: Add `--only-changed` option #6867

exp show: Add `--only-changed` option #6867

daavoo commented Oct 25, 2021 •

edited

Loading

skshetry commented Oct 26, 2021 •

edited

Loading

daavoo commented Oct 26, 2021 •

edited

Loading

skshetry Oct 26, 2021 •

edited

Loading

daavoo Oct 26, 2021 •

edited

Loading

skshetry Oct 26, 2021 •

edited

Loading