Add Compare Runs metric section #2798

Gkrumbach07 · 2024-05-09T22:32:15Z

closes: https://issues.redhat.com/browse/RHOAIENG-4772
closes: https://issues.redhat.com/browse/RHOAIENG-4771

Description

Adds compare runs section for the following metrics:

scalar metric table
confusion matrix
roc curve
I added a markdown tab but kept it disabled until fetching from s3 is done

Scalar metric table

should be able to see all scalar metrics for each execution.
will react to selected runs
horizontal scroll
ability to hide same metrics

Confusion matrix

horizontal scroll
has expand button for graph which opens graph by itself.
- i moved away from the mocks on how the dropdown works in this state becuase the dropdown is a checkbox. @yannnz you can clarify what is intended on this

roc curve

curve has legend that matches table under it
table can be sorted and filtered

How Has This Been Tested?

compare runs that have all metrics. use pipeline linked
Pipeline metrics-visualization-pipeline.yaml.zip
go to metrics section and verify description

Test Impact

Tests are complicated becuase they involve graphs and mlmd. I am working on a solution now but i want to get this PR up so it unblocks

Request review criteria:

Self checklist (all need to be checked):

The developer has manually tested the changes and verified that the changes work
Commits have been squashed into descriptive, self-contained units of work (e.g. 'WIP' and 'Implements feedback' style messages have been removed)
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

Included any necessary screenshots or gifs if it was a UI change.
Included tags to the UX team if it was a UI/UX change (find relevant UX in the SMEs section).

After the PR is posted & before it merges:

The developer has tested their solution on a cluster by using the image produced by the PR to main

cc @yannnz

jpuzz0

Using some existing runs that probably don't have matrix data to display, I'm seeing an infinite loading spinner:

frontend/src/concepts/pipelines/apiHooks/mlmd/useGetArtifactById.ts

frontend/src/concepts/pipelines/apiHooks/mlmd/useGetArtifactsList.ts

frontend/src/concepts/pipelines/content/artifacts/charts/ConfusionMatrix.tsx

...epts/pipelines/content/compareRuns/metricsSection/confusionMatrix/ConfusionMatrixCompare.tsx

frontend/src/concepts/pipelines/content/compareRuns/metricsSection/roc/utils.ts

frontend/src/concepts/pipelines/content/compareRuns/metricsSection/scalar/ScalarMetricTable.tsx

frontend/src/concepts/pipelines/content/compareRuns/metricsSection/scalar/utils.ts

frontend/src/concepts/pipelines/content/compareRuns/metricsSection/types.ts

frontend/src/pages/pipelines/global/experiments/compareRuns/CompareRunsMetricsSection.tsx

frontend/src/concepts/pipelines/content/artifacts/charts/ConfusionMatrix.tsx

jpuzz0 · 2024-05-14T18:41:15Z

Are you sure the intention of Expand link was to just span the whole width of the drawer? Based on this Figma conversation, it seems like the intention for all Expands might be to open them in a modal, although I'm questioning that in this conversation and waiting for Yan's response.

e.g. https://www.figma.com/proto/etbN0ccCcZR1qKCpJXQNAg/Pipeline-phase-2?page-id=615%3A34927&type=design&node-id=1682-101538&viewport=1131%2C-244%2C0.1&t=I8pPcuoDXIu1XQas-1&scaling=scale-down-width&starting-point-node-id=1585%3A95719

jpuzz0 · 2024-05-14T18:44:55Z

Why would anything be hidden if only 1 run is shown here?

Also, why is only 1 run shown, when 3 are selected in this comparison example?

jpuzz0 · 2024-05-14T19:06:42Z

When selecting the already checked/selected item for the Confusion matrix, this is what I see:

Seems like maybe the click handler should do nothing when attempting to click an item that is already selected to prevent this.

jpuzz0 · 2024-05-14T19:08:37Z

Should we not show empty state when there are no parameters with differences?

jpuzz0 · 2024-05-14T19:12:49Z

Wondering why we would use "Series" instead of the run name for the ROC curve legend:

If we do keep Series, it seems like there should be a tooltip for each series color in the table showing which series corresponds to the color:

jpuzz0 · 2024-05-14T19:14:36Z

Should we still show ROC curve data if the search finds none?

Gkrumbach07 · 2024-05-14T20:51:09Z

Are you sure the intention of Expand link was to just span the whole width of the drawer? Based on this Figma conversation, it seems like the intention for all Expands might be to open them in a modal, although I'm questioning that in this conversation and waiting for Yan's response.

The UX is different on the compare runs page vs the run details for expand

Gkrumbach07 · 2024-05-14T20:51:50Z

Should we still show ROC curve data if the search finds none?

This is what is done in KF, and i did not have a ref for what yan wanted so i fell back on what upstream did

Gkrumbach07 · 2024-05-14T20:53:21Z

Wondering why we would use "Series" instead of the run name for the ROC curve legend:

there could be multiple per run so it would have to be run name > execution name > artifact name.... this can get large so i assume thats why its just using series name. this can be iterated on late if it is confusing

Gkrumbach07 · 2024-05-14T20:54:22Z

Should we not show empty state when there are no parameters with differences?

We could, this is what upstream had and there was no mock for what to do in our case

Gkrumbach07 · 2024-05-14T20:55:29Z

Why would anything be hidden if only 1 run is shown here?

I can remove the switch if there is only one run

Gkrumbach07 · 2024-05-14T21:01:06Z

Also, why is only 1 run shown, when 3 are selected in this comparison example?

it only shows the runs with scalar metrics

Gkrumbach07 · 2024-05-14T21:04:31Z

When selecting the already checked/selected item for the Confusion matrix, this is what I see:

I think that would be confusing to the user if they could not deselect it.

The blank skeleton was the closest PF like thing i could use to simulate what KF used.

I think a lot of the concerns need UX to decide what is best, maybe we can wait until demo and @yannnz can give her opinion

jpuzz0 · 2024-05-14T21:13:07Z

When selecting the already checked/selected item for the Confusion matrix, this is what I see:

I think that would be confusing to the user if they could not deselect it.

The blank skeleton was the closest PF like thing i could use to simulate what KF used.

I think a lot of the concerns need UX to decide what is best, maybe we can wait until demo and @yannnz can give her opinion

The skeleton is used for loading states, but this doesn't load anything, so I think if we are meant to deselect the item, the content should instead show empty state?

codecov · 2024-05-14T21:37:33Z

Codecov Report

Attention: Patch coverage is 29.93197% with 309 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (main@424378e). Click here to learn what that means.

❗ Current head 5c20c13 differs from pull request most recent head c1291d3. Consider uploading reports for the commit c1291d3 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2798   +/-   ##
=======================================
  Coverage        ?   77.35%           
=======================================
  Files           ?     1093           
  Lines           ?    23021           
  Branches        ?     5794           
=======================================
  Hits            ?    17808           
  Misses          ?     5213           
  Partials        ?        0

Files	Coverage Δ
...tend/src/concepts/pipelines/apiHooks/mlmd/types.ts	`100.00% <ø> (ø)`
...epts/pipelines/apiHooks/mlmd/useGetArtifactById.ts	`11.11% <ø> (ø)`
...pts/pipelines/apiHooks/mlmd/useGetArtifactsList.ts	`6.25% <ø> (ø)`
...elines/content/compareRuns/metricsSection/const.ts	`100.00% <100.00%> (ø)`
...ents/artifacts/ArtifactDetails/ArtifactDetails.tsx	`83.33% <ø> (ø)`
...ifacts/ArtifactDetails/ArtifactOverviewDetails.tsx	`87.50% <100.00%> (ø)`
...nes/global/experiments/artifacts/ArtifactsList.tsx	`75.00% <ø> (ø)`
...es/global/experiments/artifacts/ArtifactsTable.tsx	`40.35% <ø> (ø)`
...xperiments/compareRuns/CompareRunParamsSection.tsx	`97.67% <ø> (ø)`
...global/experiments/compareRuns/CompareRunsPage.tsx	`100.00% <ø> (ø)`
... and 21 more

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 424378e...c1291d3. Read the comment docs.

yannnz · 2024-05-15T13:43:17Z

For the ROC Curve, there is an interaction been missed. When the user hover the legend of the series, the corresponding curve highlighted with a thicker curve.

Experiment column of the run table list should be clickable if we keep parity with KFP UI.

Not quite sure why the Markdown tab is disable. If it is because there is no markdown, I think we need an empty states.

On my screen, the space between the Metrics section and the Parameter section can be larger to keep the space consistency between sections. The space between the Run table list under the ROC Curve and the ROC Curve graph can be smaller because they related with each other.

yannnz · 2024-05-15T14:05:12Z

Should we still show ROC curve data if the search finds none?

I wonder how KFP does it? I still cannot figure out how their filters work. I can see how to use this filter is to filter the listed artifact names, if there are several results matching the results, the UI will only display those filtered ones, right? If this is the logic for filtering, filtering ended with no result means nothing to show on the graph.

yannnz · 2024-05-15T14:06:23Z

Wondering why we would use "Series" instead of the run name for the ROC curve legend:

If we do keep Series, it seems like there should be a tooltip for each series color in the table showing which series corresponds to the color:

I still don't get what are Series TBH. Are those runs? We just followed KFP UI....

yannnz · 2024-05-15T14:10:33Z

Should we not show empty state when there are no parameters with differences?

Yes I don't think we need to show empty state for this scenario, because there are still parameters. It might confuse the user. WDYT?

yannnz · 2024-05-15T14:12:48Z

When selecting the already checked/selected item for the Confusion matrix, this is what I see:

Seems like maybe the click handler should do nothing when attempting to click an item that is already selected to prevent this.

Thanks for raising this. Yes this is not a good experience when you already selected something, but it started reloading just because you clicked it again.

yannnz · 2024-05-15T14:14:24Z

Why would anything be hidden if only 1 run is shown here?
Also, why is only 1 run shown, when 3 are selected in this comparison example?

It is weird. Is it because of the KFP API behavior?

jpuzz0 · 2024-05-15T14:16:26Z

Wondering why we would use "Series" instead of the run name for the ROC curve legend:
If we do keep Series, it seems like there should be a tooltip for each series color in the table showing which series corresponds to the color:

I still don't get what are Series TBH. Are those runs? We just followed KFP UI....

@yannnz I'm not sure either. I understood it as them being the runs since that's what is unique for each table row in this case, but I could be wrong.

jpuzz0 · 2024-05-15T14:27:48Z

Why would anything be hidden if only 1 run is shown here?

Also, why is only 1 run shown, when 3 are selected in this comparison example?

It is weird. Is it because of the KFP API behavior?

@yannnz I don't think so. At least how the Parameters compare table works, if 1 run has data, the other 2 show to compare against empty values. This should be achievable here as well, but not sure what was expected.

jpuzz0 · 2024-05-15T14:30:50Z

Should we not show empty state when there are no parameters with differences?

Yes I don't think we need to show empty state for this scenario, because there are still parameters. It might confuse the user. WDYT?

@yannnz Does it still have parameters? I think the 2nd row here doesn't represent a parameter, just the execution name.

This has a parameter called accuracy:

This has no parameters as far as my understanding goes:

jpuzz0 · 2024-05-15T14:36:49Z

Should we still show ROC curve data if the search finds none?

I wonder how KFP does it? I still cannot figure out how their filters work. I can see how to use this filter is to filter the listed artifact names, if there are several results matching the results, the UI will only display those filtered ones, right? If this is the logic for filtering, filtering ended with no result means nothing to show on the graph.

@yannnz In KF UI, When adding filter text to the search box and no results are found, they don't update the table data, and instead still show the rows. In our case, we allow users to filter, which hides the rows, but yet they're still selected:

KF UI:

jpuzz0 · 2024-05-15T14:56:49Z

frontend/src/concepts/pipelines/content/compareRuns/metricsSection/roc/RocCurveCompare.tsx

+      .filter((confidenceMetrics) => !!confidenceMetrics.data)
+      .map(({ data, fullArtifactPath }, i) => {
+        // validate the custom properties
+        const confidenceMetricsArray = data?.list as unknown[];


Why is this case to unknown?

becuase it is coming from custom properties

i changed the logic to be based on the actual value now

jpuzz0 · 2024-05-15T15:43:36Z

When the width is small enough and there's enough data, using the maxDimension prop - set to 500 here, the x-axis values overlap for the ROC curve:

Gkrumbach07 · 2024-05-15T16:17:01Z

For the ROC Curve, there is an interaction been missed. When the user hover the legend of the series, the corresponding curve highlighted with a thicker curve.

This is not possible in victory charts

Gkrumbach07 · 2024-05-15T16:33:46Z

@yannnz I don't think so. At least how the Parameters compare table works, if 1 run has data, the other 2 show to compare against empty values. This should be achievable here as well, but not sure what was expected.

KF does not show empty ones, just ones that gave the param. otherwise it may give the impression that the execution has that scalar metric. And if we do go down this route, then we would be showing a bunch of empty cells

Gkrumbach07 · 2024-05-15T16:35:21Z

@yannnz In KF UI, When adding filter text to the search box and no results are found, they don't update the table data, and instead still show the rows. In our case, we allow users to filter, which hides the rows, but yet they're still selected:

filtering should not deselect things from the checkboxes. that is not our behavior in other areas so it should not be here. i guess the idea is to help find your artifact to show if you have a lot?

… and types.ts Refactor imports and remove unused code in ArtifactDetails.spec.tsx and ArtifactsTable.spec.tsx Refactor imports and remove unused code in ArtifactDetails.spec.tsx and ArtifactsTable.spec.tsx Add TestArtifacts component to GlobalPipelineExperimentsRoutes Refactor ConfusionMatrix component to improve readability and styling Add ConfusionMatrix component and buildConfusionMatrixConfig function Add ConfusionMatrix and ROCCurve components Remove TestArtifacts component and update related routes Refactor ROC curve table components, update imports, and remove unnecessary code Refactor ConfusionMatrixSelect component and add skeleton loading Refactor ROC curve table components and update imports Refactor ROC curve table components and add related constants Refactor ROC curve table components and add related constants Refactor CompareRunsMetricsSection component, update imports, and remove console.log statements Refactor CompareRunsMetricsSection component, update imports, and add ROC curve tab Refactor Dockerfile to use linux/amd64 platform Refactor CompareRunsMetricsSection component, update imports, and add confusion matrix tab Refactor CompareRunsMetricsSection component, add MetricSectionTabLabels enum, and update related types and imports Refactor CompareRunsMetricsSection component, add MetricSectionTabLabels enum, and update related types and imports Refactor CompareRunsMetricsSection component and add MetricSectionTabLabels enum Refactor and remove unused code in usePipelinesUiRoute.ts and ArtifactUriLink.tsx Refactor MLMD API hooks and remove unused code Refactor components to improve readability and add loading spinners Refactor components to improve readability and add loading spinners clean up Refactor imports and remove unused code in ArtifactDetails.spec.tsx and ArtifactsTable.spec.tsx Refactor ConfusionMatrix component to improve readability and styling Refactor ROC curve table components and update imports

jpuzz0 · 2024-05-15T16:51:03Z

frontend/src/concepts/pipelines/content/artifacts/charts/ROCCurve.tsx

+};
+
+const ROCCurve: React.FC<ROCCurveProps> = ({ configs, maxDimension }) => {
+  const width = Math.min(maxDimension || 800, 500);


Maybe this was added just to test out 500 px? Won't this always return 500?

jpuzz0

/lgtm /approve

Approving with the expectation for any UX discussions to be made into a separate story and followed up on.

openshift-ci · 2024-05-15T16:53:31Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot requested review from DaoDaoNoCode and dpanshug May 9, 2024 22:32

Gkrumbach07 force-pushed the story/RHOAIENG-2976-spike-determine-artifact-rendering-details branch from 4cae1ff to 9a969fb Compare May 10, 2024 12:24

jpuzz0 reviewed May 13, 2024

View reviewed changes

Gkrumbach07 force-pushed the story/RHOAIENG-2976-spike-determine-artifact-rendering-details branch 2 times, most recently from 2fd73b2 to e25ca4e Compare May 14, 2024 15:32

jpuzz0 reviewed May 14, 2024

View reviewed changes

frontend/src/concepts/pipelines/content/artifacts/charts/ConfusionMatrix.tsx Outdated Show resolved Hide resolved

jpuzz0 reviewed May 14, 2024

View reviewed changes

frontend/src/concepts/pipelines/content/artifacts/charts/ConfusionMatrix.tsx Outdated Show resolved Hide resolved

Gkrumbach07 force-pushed the story/RHOAIENG-2976-spike-determine-artifact-rendering-details branch 2 times, most recently from 1967ca6 to 5c20c13 Compare May 14, 2024 21:12

jpuzz0 reviewed May 15, 2024

View reviewed changes

Gkrumbach07 force-pushed the story/RHOAIENG-2976-spike-determine-artifact-rendering-details branch from 5c20c13 to c1291d3 Compare May 15, 2024 16:38

jpuzz0 reviewed May 15, 2024

View reviewed changes

jpuzz0 added lgtm approved labels May 15, 2024

openshift-merge-bot bot merged commit 8645fe6 into opendatahub-io:main May 15, 2024
6 checks passed

Add Compare Runs metric section #2798

Add Compare Runs metric section #2798

Conversation

Gkrumbach07 commented May 9, 2024 • edited Loading

Description

How Has This Been Tested?

Test Impact

Request review criteria:

jpuzz0 left a comment

Choose a reason for hiding this comment

jpuzz0 commented May 14, 2024 • edited Loading

jpuzz0 commented May 14, 2024

jpuzz0 commented May 14, 2024

jpuzz0 commented May 14, 2024

jpuzz0 commented May 14, 2024 • edited Loading

jpuzz0 commented May 14, 2024

Gkrumbach07 commented May 14, 2024

Gkrumbach07 commented May 14, 2024

Gkrumbach07 commented May 14, 2024

Gkrumbach07 commented May 14, 2024

Gkrumbach07 commented May 14, 2024

Gkrumbach07 commented May 14, 2024

Gkrumbach07 commented May 14, 2024

jpuzz0 commented May 14, 2024

codecov bot commented May 14, 2024 • edited Loading

Codecov Report

yannnz commented May 15, 2024 • edited Loading

yannnz commented May 15, 2024

yannnz commented May 15, 2024 • edited Loading

yannnz commented May 15, 2024

yannnz commented May 15, 2024

yannnz commented May 15, 2024

jpuzz0 commented May 15, 2024

jpuzz0 commented May 15, 2024

jpuzz0 commented May 15, 2024

jpuzz0 commented May 15, 2024 • edited Loading

jpuzz0 May 15, 2024

Choose a reason for hiding this comment

Gkrumbach07 May 15, 2024

Choose a reason for hiding this comment

Gkrumbach07 May 15, 2024

Choose a reason for hiding this comment

jpuzz0 commented May 15, 2024 • edited Loading

Gkrumbach07 commented May 15, 2024 • edited Loading

Gkrumbach07 commented May 15, 2024

Gkrumbach07 commented May 15, 2024

jpuzz0 May 15, 2024

Choose a reason for hiding this comment

jpuzz0 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented May 15, 2024

Gkrumbach07 commented May 9, 2024 •

edited

Loading

jpuzz0 commented May 14, 2024 •

edited

Loading

jpuzz0 commented May 14, 2024 •

edited

Loading

codecov bot commented May 14, 2024 •

edited

Loading

yannnz commented May 15, 2024 •

edited

Loading

yannnz commented May 15, 2024 •

edited

Loading

jpuzz0 commented May 15, 2024 •

edited

Loading

jpuzz0 commented May 15, 2024 •

edited

Loading

Gkrumbach07 commented May 15, 2024 •

edited

Loading