Added diff-view switch #4862

marijncv · 2021-10-01T20:06:40Z

Signed-off-by: Marijn Valk marijncv@hotmail.com

What changes are proposed in this pull request?

Added a checkbox for switching between diff-only view and regular view. closes #4819

How is this patch tested?

Create a couple of runs with params, metrics and tag of which some are different accross runs and some which are the same across runs. Mark/unmark the checkbox and see the columns disappear for which the value across all the runs is the same

Release Notes

Is this a user-facing change?

No. You can skip the rest of this section.
Yes. Give a description of this change to be included in the release notes for MLflow users.

Added a checkbox that will filter out all columns for which every run has the same value

What component(s), interfaces, languages, and integrations does this PR affect?

Components

Interface

area/uiux: Front-end, user experience, plotting, JavaScript, JavaScript dev server
area/docker: Docker use across MLflow's components, such as MLflow Projects and MLflow Models
area/sqlalchemy: Use of SQLAlchemy in the Tracking Service or Model Registry
area/windows: Windows support

Language

language/r: R APIs and clients
language/java: Java APIs and clients
language/new: Proposals for new client languages

Integrations

integrations/azure: Azure and Azure ML integrations
integrations/sagemaker: SageMaker integrations
integrations/databricks: Databricks integrations

How should the PR be classified in the release notes? Choose one:

rn/breaking-change - The PR will be mentioned in the "Breaking Changes" section
rn/none - No description will be included. The PR will be mentioned only by the PR number in the "Small Bugfixes and Documentation Updates" section
rn/feature - A new user-facing feature worth mentioning in the release notes
rn/bug-fix - A user-facing bug fix worth mentioning in the release notes
rn/documentation - A user-facing documentation change worth mentioning in the release notes

Signed-off-by: Marijn Valk <marijncv@hotmail.com>

harupy · 2021-10-04T03:51:28Z

Thanks for the PR, it looks great! Can we keep the source column to make it easier to jump to notebook/scripts?

diff-columns.mov

Scritp to generate test data:

import mlflow
import random


def random_float():
    return random.random()


def random_string():
    return random.choice(["a", "b", "c"])


for _ in range(100):
    with mlflow.start_run():
        mlflow.log_param("p1", 0)
        mlflow.log_param("p2", random_string())

        mlflow.log_metric("m1", 0)
        mlflow.log_metric("m2", random_float())

        mlflow.set_tag("t1", 0)
        mlflow.set_tag("t2", random_string())

Signed-off-by: Marijn Valk <marijncv@hotmail.com>

marijncv · 2021-10-04T11:28:02Z

Removed the source column from the diff view (it will be always visible unless unchecked by the user). Also moved the getCategorizedColumnsDiffView function to ExperimentViewUtil

Signed-off-by: Marijn Valk <marijncv@hotmail.com>

dbczumar · 2021-10-05T21:29:59Z

@marijncv Thanks a bunch for addressing comments from @harupy ; this is looking great! Before finalizing the PR, we've asked one of our UI/UX designers for input about the placement and text associated with the toggle. We'll provide a design mock with this information in the next few days.

marijncv · 2021-10-06T05:54:34Z

@dbczumar sounds good, looking forward to the design mock!

dbczumar · 2021-10-12T01:32:16Z

@dbczumar sounds good, looking forward to the design mock!

Hi @marijncv , apologies for the delay. Here is the mock:

Please ignore the slightly different UI styling used in our mockup tools. For consistency with the rest of the MLflow UI, we can keep using the existing toggle element from your PR. The only practical differences are the text next to the toggle and the text displayed on hover.

mlflow/server/js/src/experiment-tracking/components/ExperimentViewUtil.js

Signed-off-by: Marijn Valk <marijncv@hotmail.com>

marijncv · 2021-10-12T08:20:53Z

Thanks for the design @dbczumar! I've updated the PR accordingly

harupy · 2021-10-12T08:33:36Z

@marijncv Thanks for the updates! @dbczumar @jinzhang21 It looks like this now:

diff-column-view.mov

The toggle button looks great!
Diff columns are recomputed when we load more runs.

harupy · 2021-10-12T09:11:03Z

I found a bug that's related to column selection:

column-selection-bug.mov

In this video, I did the following

Enable the diff view (this hides the User column).
Select the User column in the column selector, but it doesn't show up.
Unselect the User column.
Select the User column again and it shows up this time.

Signed-off-by: Marijn Valk <marijncv@hotmail.com>

marijncv · 2021-10-12T12:11:44Z

@harupy thanks for pointing out the bug! I think it's fixed with my latest commit.

But I'm curious about a difference I see in your video and the code on my machine. For me the position of the column changes to the end of the list when I check/uncheck it, but for you that seems to not be the case (it just returns back to it's old position). Do you have any idea why that could be the case?

harupy · 2021-10-12T12:31:39Z

@marijncv Thanks for the quick fix!

Can you take a screen recording of what happens on your machine and share it with us?

marijncv · 2021-10-12T12:52:21Z

mlflow.mp4

In the video I use the switch, then select username, it shows up but at the end of the list of columns instead of in it's original position

harupy · 2021-10-12T13:03:05Z

@marijncv Thanks for the video. Let me pull the latest commit and try again.

harupy · 2021-10-12T13:06:06Z

column-select-2.mov

On my machine, the column shows up in the right position.

Signed-off-by: Marijn Valk <marijncv@hotmail.com>

mlflow/server/js/src/experiment-tracking/components/ExperimentView.js

…View.js correct comment Co-authored-by: Harutaka Kawamura <hkawamura0130@gmail.com> Signed-off-by: Marijn Valk <marijncv@hotmail.com>

mlflow/server/js/src/experiment-tracking/components/ExperimentViewUtil.js

Signed-off-by: Marijn Valk <marijncv@hotmail.com>

mlflow/server/js/src/experiment-tracking/components/ExperimentViewUtil.js

mlflow/server/js/src/experiment-tracking/components/ExperimentView.test.js

Signed-off-by: Marijn Valk <marijncv@hotmail.com>

harupy · 2021-10-20T04:46:53Z

Hey @marijncv, thanks for patiently applying our feedback!

bug-2.mov

We found another bug. Here are the steps to reproduce the issue.

Enable the diff-only view
Load more runs
Disable the diff-only view

The m1, p1, and t1 columns should show up after disabling the diff-only view but remain hidden. With the newly loaded runs, the result of getCategorizedUncheckedKeysDiffView doesn't contain m1, p1, and t1, which changes the result of getRestoredCategorizedUncheckedKeys.

Code to populate data

import mlflow
import random


def random_float():
    return random.random()


def random_string():
    return random.choice(["a", "b", "c"])


for _ in range(100):
    with mlflow.start_run():
        mlflow.log_param("p3", random_float())
        mlflow.log_metric("m3", random_float())
        mlflow.set_tag("t3", random_float())


for _ in range(100):
    with mlflow.start_run():
        mlflow.log_param("p1", 0)
        mlflow.log_param("p2", random_float())

        mlflow.log_metric("m1", 0)
        mlflow.log_metric("m2", random_float())

        mlflow.set_tag("t1", 0)
        mlflow.set_tag("t2", random_float())

harupy · 2021-10-20T07:28:10Z

mlflow/server/js/src/experiment-tracking/components/ExperimentViewUtil.js

+      [COLUMN_TYPES.ATTRIBUTES]: _.concat(
+        categorizedUncheckedKeys[COLUMN_TYPES.ATTRIBUTES],
+        attributeKeyList.filter((v, index) => {
+          return allEqual(attributes[index]);
+        }),


Is it possible to hide attribute columns only when they are empty?

Here's an example:

a1 a2 a3

- - 1

- 1 1

- 1 1

For the table above, the diff switch should hide the a1 column.

Yes it's possible. With that addition, should I also add the Models attribute to be considered to be removed if all values are empty? Right now only Run Name, User and Version are considered.

Version and User will never be empty so they can probably be left out of consideration all together. But on the other hand I can still image that the user would like to hide these columns if they contain the same value for each row.

What do you think?

should I also add the Models attribute to be considered to be removed if all values are empty?

Yes!

Version and User will never be empty so they can probably be left out of consideration all together.

User and Source, right? Version can be empty (e.g. run mlflow code in a non-git directory). Makes sense to exclude them from consideration.

But on the other hand I can still image that the user would like to hide these columns if they contain the same value for each row.

I do understand that some users prefer to hide constant attribute columns to obtain more space for param, metric, and tag columns. On the other hand, the version, user, and run-name columns seem useful even if they are constant.

user: tells us who creates the displayed runs

version: tells us the code that creates the displayed runs

run-name: ??? (I couldn't come up with a useful use case)

We could argue you can show attribute columns after turning on the diff switch though.

@dbczumar Any thoughts on this?

marijncv · 2021-10-20T08:46:54Z

Hey @marijncv, thanks for patiently applying our feedback!

bug-2.mov
We found another bug. Here are the steps to reproduce the issue.

Enable the diff-only view

Load more runs

Disable the diff-only view

The m1, p1, and t1 columns should show up after disabling the diff-only view but remain hidden. With the newly loaded runs, the result of getCategorizedUncheckedKeysDiffView doesn't contain m1, p1, and t1, which changes the result of getRestoredCategorizedUncheckedKeys.

Code to populate data
import mlflow
import random


def random_float():
    return random.random()


def random_string():
    return random.choice(["a", "b", "c"])


for _ in range(100):
    with mlflow.start_run():
        mlflow.log_param("p3", random_float())
        mlflow.log_metric("m3", random_float())
        mlflow.set_tag("t3", random_float())


for _ in range(100):
    with mlflow.start_run():
        mlflow.log_param("p1", 0)
        mlflow.log_param("p2", random_float())

        mlflow.log_metric("m1", 0)
        mlflow.log_metric("m2", random_float())

        mlflow.set_tag("t1", 0)
        mlflow.set_tag("t2", random_float())

No worries, it's a great learning experience :). Will look into this today.

Edit: it should be addressed by 9990804

Signed-off-by: Marijn Valk <marijncv@hotmail.com>

harupy · 2021-10-20T10:07:13Z

mlflow/server/js/src/experiment-tracking/components/ExperimentViewUtil.js

+   * Obtain the categorized columns for which the values in them
+   * have only a single value (or are undefined)
+   */
+  static getCategorizedUncheckedKeysDiffView({


btw I'm considering how we can improve/simplify this function. Here's my attempt:

Commit: harupy@6f61045
Branch: https://github.com/harupy/mlflow/tree/improve-diff-column-search

I like it! We could even add a short-circuit in the loop over runInfos if we find there are no longer any columns to consider.

And then for attributes we can add a dropNonEmptyColumns function and incorporate it in the same loop over runInfos

I applied your proposal and integrated the points I mentioned above. I'm doubting whether the toAttributesMap method should be made more generic (i.e. include all attributes). Also, maybe the comments in dropNonEmptyColumns might be a bit overkill since it's so similar to dropDiffColumns.

Signed-off-by: Marijn Valk <marijncv@hotmail.com>

dbczumar

LGTM! Thanks @marijncv !

harupy · 2021-10-21T01:10:25Z

@marijncv I found several unrelated CI checks failed. We can just ignore them in this PR.

harupy · 2021-10-21T02:13:35Z

mlflow/server/js/src/experiment-tracking/components/ExperimentViewUtil.js

+    const dropNonEmptyColumns = (columns, prevRow, currRow) => {
+      // # What each argument represents:
+      // | a   | b   | c   | d   | e   | <- columns
+      // | --- | --- | --- | --- | --- |
+      // | -   | 1   | -   | 1   | 1   | <- prevRow
+      // | -   | -   | 1   | 1   | 2   | <- currRow
+      // | ?   | ?   | ?   | ?   | ?   |
+
+      // a: may be an empty column, we need to take a look at the next row
+      // b: is not an empty column, we don't need to take a look at the next row
+      // c: is not an empty column
+      // d: is not an empty column
+      // e: is not an empty column
+
+      return columns.filter((col) => {
+        const prevValue = prevRow[col];
+        const currValue = currRow[col];
+        if ((!prevValue && !currValue) || (!currValue.length && !currValue.length)) {
+          // Case a
+          return true;
+        } else {
+          // Case b, c, d & e
+          return false;
+        }
+      });
+    };


For detecting empty attribute columns, I don't think we need to compare the previous and current rows. We can just take a look at the current row.

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>

harupy · 2021-10-21T04:30:19Z

@marijncv I pushed a commit to fix a couple of minior issues.

mlflow/server/js/src/experiment-tracking/components/ExperimentViewUtil.js

harupy

LGTM! Thanks for the contribution!

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>

Added diff-view checkbox (wip)

4a02387

Signed-off-by: Marijn Valk <marijncv@hotmail.com>

github-actions bot added area/uiux Front-end, user experience, plotting, JavaScript, JavaScript dev server rn/feature Mention under Features in Changelogs. labels Oct 1, 2021

marijncv added 4 commits October 2, 2021 13:31

Added unit tests

bdd9e66

Signed-off-by: Marijn Valk <marijncv@hotmail.com>

Move diff view checkbox to column dropdown

ee6e6dd

Signed-off-by: Marijn Valk <marijncv@hotmail.com>

Added Switch

932b3ce

Signed-off-by: Marijn Valk <marijncv@hotmail.com>

Remove not needed state in column dropdown

4e3ef7a

Signed-off-by: Marijn Valk <marijncv@hotmail.com>

marijncv marked this pull request as ready for review October 2, 2021 20:26

marijncv added 2 commits October 4, 2021 12:40

Merge branch 'master' into feature/diff-view

cae8a37

Removed source from diff view

2b78eb2

Signed-off-by: Marijn Valk <marijncv@hotmail.com>

marijncv changed the title ~~Added diff-view checkbox (WIP)~~ Added diff-view switch Oct 4, 2021

Small comment update

dc2d6e3

Signed-off-by: Marijn Valk <marijncv@hotmail.com>

harupy reviewed Oct 12, 2021

View reviewed changes

mlflow/server/js/src/experiment-tracking/components/ExperimentViewUtil.js Outdated Show resolved Hide resolved

marijncv added 2 commits October 12, 2021 08:43

Merge branch 'master' into feature/diff-view

a54246f

updated switch & tooltip text

e6e3aea

Signed-off-by: Marijn Valk <marijncv@hotmail.com>

bugfix for getCategorizedUncheckedKeys

4dc3d9a

Signed-off-by: Marijn Valk <marijncv@hotmail.com>

use persistedState to make onClear work

1cbccf9

Signed-off-by: Marijn Valk <marijncv@hotmail.com>

harupy mentioned this pull request Oct 14, 2021

Set applyColumnDefOrder for AgGridReact to make sure columns order matches column definitions order #4899

Merged

27 tasks

marijncv added 3 commits October 14, 2021 11:08

maintain pre-switch state

36b68a2

Signed-off-by: Marijn Valk <marijncv@hotmail.com>

Merge branch 'master' into feature/diff-view

d3bbbdb

Maintain state changes during diff view

7da86b8

Signed-off-by: Marijn Valk <marijncv@hotmail.com>

harupy reviewed Oct 19, 2021

View reviewed changes

mlflow/server/js/src/experiment-tracking/components/ExperimentView.js Outdated Show resolved Hide resolved

Update mlflow/server/js/src/experiment-tracking/components/Experiment…

701af4a

…View.js correct comment Co-authored-by: Harutaka Kawamura <hkawamura0130@gmail.com> Signed-off-by: Marijn Valk <marijncv@hotmail.com>

marijncv force-pushed the feature/diff-view branch from ff6c922 to 701af4a Compare October 19, 2021 08:37

harupy reviewed Oct 19, 2021

View reviewed changes

mlflow/server/js/src/experiment-tracking/components/ExperimentViewUtil.js Outdated Show resolved Hide resolved

harupy reviewed Oct 19, 2021

View reviewed changes

mlflow/server/js/src/experiment-tracking/components/ExperimentViewUtil.js Outdated Show resolved Hide resolved

marijncv added 2 commits October 19, 2021 12:39

added param description

2bdad77

Signed-off-by: Marijn Valk <marijncv@hotmail.com>

Merge branch 'master' into feature/diff-view

c9989a3

harupy reviewed Oct 19, 2021

View reviewed changes

mlflow/server/js/src/experiment-tracking/components/ExperimentViewUtil.js Outdated Show resolved Hide resolved

harupy reviewed Oct 19, 2021

View reviewed changes

mlflow/server/js/src/experiment-tracking/components/ExperimentViewUtil.js Outdated Show resolved Hide resolved

harupy reviewed Oct 19, 2021

View reviewed changes

mlflow/server/js/src/experiment-tracking/components/ExperimentView.test.js Outdated Show resolved Hide resolved

updated comments

2d10a60

Signed-off-by: Marijn Valk <marijncv@hotmail.com>

harupy reviewed Oct 20, 2021

View reviewed changes

Save postSwitch state instead of recalculating

9990804

Signed-off-by: Marijn Valk <marijncv@hotmail.com>

harupy reviewed Oct 20, 2021

View reviewed changes

Updated getCategorizedUncheckedKeysDiffView

af38fb9

Signed-off-by: Marijn Valk <marijncv@hotmail.com>

dbczumar approved these changes Oct 21, 2021

View reviewed changes

harupy reviewed Oct 21, 2021

View reviewed changes

minor fixes

12b0e9d

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>

harupy reviewed Oct 21, 2021

View reviewed changes

mlflow/server/js/src/experiment-tracking/components/ExperimentViewUtil.js Show resolved Hide resolved

harupy approved these changes Oct 21, 2021

View reviewed changes

comments

4c418bb

Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>

harupy merged commit 9e7c94d into mlflow:master Oct 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added diff-view switch #4862

Added diff-view switch #4862

marijncv commented Oct 1, 2021

harupy commented Oct 4, 2021 •

edited

Loading

marijncv commented Oct 4, 2021

dbczumar commented Oct 5, 2021 •

edited

Loading

marijncv commented Oct 6, 2021

dbczumar commented Oct 12, 2021 •

edited

Loading

marijncv commented Oct 12, 2021

harupy commented Oct 12, 2021 •

edited

Loading

harupy commented Oct 12, 2021 •

edited

Loading

marijncv commented Oct 12, 2021

harupy commented Oct 12, 2021 •

edited

Loading

marijncv commented Oct 12, 2021

harupy commented Oct 12, 2021

harupy commented Oct 12, 2021 •

edited

Loading

harupy commented Oct 20, 2021 •

edited

Loading

harupy Oct 20, 2021 •

edited

Loading

marijncv Oct 20, 2021

harupy Oct 20, 2021

harupy Oct 20, 2021 •

edited

Loading

marijncv commented Oct 20, 2021 •

edited

Loading

harupy Oct 20, 2021 •

edited

Loading

marijncv Oct 20, 2021 •

edited

Loading

marijncv Oct 20, 2021

dbczumar left a comment

harupy commented Oct 21, 2021

harupy Oct 21, 2021 •

edited

Loading

harupy commented Oct 21, 2021

harupy left a comment •

edited

Loading

Added diff-view switch #4862

Added diff-view switch #4862

Conversation

marijncv commented Oct 1, 2021

What changes are proposed in this pull request?

How is this patch tested?

Release Notes

Is this a user-facing change?

What component(s), interfaces, languages, and integrations does this PR affect?

How should the PR be classified in the release notes? Choose one:

harupy commented Oct 4, 2021 • edited Loading

Scritp to generate test data:

marijncv commented Oct 4, 2021

dbczumar commented Oct 5, 2021 • edited Loading

marijncv commented Oct 6, 2021

dbczumar commented Oct 12, 2021 • edited Loading

marijncv commented Oct 12, 2021

harupy commented Oct 12, 2021 • edited Loading

harupy commented Oct 12, 2021 • edited Loading

marijncv commented Oct 12, 2021

harupy commented Oct 12, 2021 • edited Loading

marijncv commented Oct 12, 2021

harupy commented Oct 12, 2021

harupy commented Oct 12, 2021 • edited Loading

harupy commented Oct 20, 2021 • edited Loading

harupy Oct 20, 2021 • edited Loading

Choose a reason for hiding this comment

marijncv Oct 20, 2021

Choose a reason for hiding this comment

harupy Oct 20, 2021

Choose a reason for hiding this comment

harupy Oct 20, 2021 • edited Loading

Choose a reason for hiding this comment

marijncv commented Oct 20, 2021 • edited Loading

harupy Oct 20, 2021 • edited Loading

Choose a reason for hiding this comment

marijncv Oct 20, 2021 • edited Loading

Choose a reason for hiding this comment

marijncv Oct 20, 2021

Choose a reason for hiding this comment

dbczumar left a comment

Choose a reason for hiding this comment

harupy commented Oct 21, 2021

harupy Oct 21, 2021 • edited Loading

Choose a reason for hiding this comment

harupy commented Oct 21, 2021

harupy left a comment • edited Loading

Choose a reason for hiding this comment

harupy commented Oct 4, 2021 •

edited

Loading

dbczumar commented Oct 5, 2021 •

edited

Loading

dbczumar commented Oct 12, 2021 •

edited

Loading

harupy commented Oct 12, 2021 •

edited

Loading

harupy commented Oct 12, 2021 •

edited

Loading

harupy commented Oct 12, 2021 •

edited

Loading

harupy commented Oct 12, 2021 •

edited

Loading

harupy commented Oct 20, 2021 •

edited

Loading

harupy Oct 20, 2021 •

edited

Loading

harupy Oct 20, 2021 •

edited

Loading

marijncv commented Oct 20, 2021 •

edited

Loading

harupy Oct 20, 2021 •

edited

Loading

marijncv Oct 20, 2021 •

edited

Loading

harupy Oct 21, 2021 •

edited

Loading

harupy left a comment •

edited

Loading