Add run links popover to metrics plot

[harupy]

What changes are proposed in this pull request?

Resolves #2279

How is this patch tested?

(Details)

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.

(Details in 1-2 sentences. You can just refer to another PR with a description if this PR is part of a larger change.)

What component(s) does this PR affect?

• UI
• CLI
• API
• REST-API
• Examples
• Docs
• Tracking
• Projects
• Artifacts
• Models
• Scoring
• Serving
• R
• Java
• Python

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

[harupy]

The plot area gets initialized when the popover shows up. I'm wondering if it's possible to prevent the plot area from getting initialized by re-rendering.

[juntai-zheng]

Hey @harupy, @smurching and I took a look at what was causing the issue. We've isolated it to the metrics prop changing in the MetricsPlotView, causing it to refresh whenever there is a click and the popover is created. This also happens when you're not zoomed in, though it's not noticeable because it doesn't change the graph. You can see how we found the bug in this screenshot:

We used the hook from here to log which prop was changed.

Do you think you could take a closer look as to why metrics is changing? If that doesn't change then the zoom in problem might be solved. Thanks!

[harupy]

@juntai-zheng Thanks for the investigation. The actual cause is when toggling the popover, the state popoverVisible changes. This change triggers re-rendering and the plot gets zoomed out.

[juntai-zheng]

@harupy ah sorry, forgot to mention that we had actually removed popoverVisible as another part of our investigation since we saw that it was causing re-rendering and didn't seem needed in MetricsPlotView. When we did, metrics was changing as well (causing the re-rendering).

[harupy]

@juntai-zheng Can you share the branch you're working on?

[juntai-zheng]

@harupy Here it is: https://github.com/juntai-zheng/mlflow/tree/popover

Just checking, the links created by the popover link me back to the MLflow home page. Is linking to the appropriate run a WIP?

[harupy]

@juntai-zheng

Is linking to the appropriate run a WIP?

Yes

[harupy]

@juntai-zheng

I checked your branch. In JavaScript, === compares identity for objects, not equality.

> const a = {};
> const b = {};
> const c = a;

> a === b
false

>>> a === c
true

The comparison below doesn't work when val is an object or a list.

prevProps[key] !== val

So the metrics wasn't changing.

[harupy]

One way to freeze the plot when toggling the popover is to use shouldComponentUpdate in MetricsPlotView (which is a stateless component) and prevent the re-render if the next props equal to the current ones.

[juntai-zheng]

Hi @harupy, you were correct in the hook being incorrect and metrics changing. We did some more digging and found out that because the state of the popover was contained in the state of MetricsPlotPanel, the changes to the state of the popover would also change the state of MetricsPlotPanel. Since MetricsPlotPanel is a parent to the popover and MetricsPlotView, whenever MetricsPlotPanel would re-render, so would MetricsPlotView. To avoid this, we made a ref to the popover in MetricsPlotPanel and only changed the state of RunLinksPopover, which subsequently only re-renders the popover and left the plot alone.

We made rough changes to my branch https://github.com/juntai-zheng/mlflow/tree/popover, so you can check it out from there.

[harupy]

@juntai-zheng Awesome. I'll reflect the change on this branch.

[harupy]

Now it looks like:

[juntai-zheng]

@harupy the popover looks great! Looks like a couple test are failing because of extra props being passed with our modifications. Do you think you could apply a fix for the tests?

[harupy]

@juntai-zheng I will fix the tests!

@gioa
Any thoughts on the popover design?

[harupy]

Added the following updates.

• Display the popover on the left to prevent it from overlapping the tooltips on the plot.
• Display metric values after run names.

[harupy]

Maybe we should truncate run uuid in the run name (for example displaying only the first 8 characters) because the plot width is reduced when comparing multiple runs.

[Zangr]

@harupy , the changes look merge ready, would you like to resolve the conflicts?

[harupy]

@Zangr Fixed the conflict!

[harupy]

Maybe we should truncate run uuid in the run name (for example displaying only the first 8 characters) because the plot width is reduced when comparing multiple runs.

@Zangr What do you think about this?

[Zangr]

Maybe we should truncate run uuid in the run name (for example displaying only the first 8 characters) because the plot width is reduced when comparing multiple runs.

Agree, I think we can show a short version and show the full id in the tooltip. cc @gioa

[harupy]

Yes, that makes sense. I'll open a PR.

[harupy]

@Zangr @smurching
I found a bug. The popover shows up after relayout by double click. I'll open a PR to fix this bug.