Clean up column path logic in custom state (#3460)

iterative · Mar 14, 2023 · e461677 · e461677
1 parent 244076a
commit e461677
Show file tree

Hide file tree

Showing 7 changed files with 83 additions and 64 deletions.
diff --git a/extension/src/plots/model/collect.ts b/extension/src/plots/model/collect.ts
@@ -40,7 +40,8 @@ import { extractColumns } from '../../experiments/columns/extract'
 import {
   decodeColumn,
   appendColumnToPath,
-  splitColumnPath
+  splitColumnPath,
+  FILE_SEPARATOR
 } from '../../experiments/columns/paths'
 import {
   ColumnType,
@@ -270,18 +271,23 @@ const collectMetricVsParamPlot = (
   param: string,
   experiments: Experiment[]
 ): MetricVsParamPlot => {
-  const splitUpMetricPath = splitColumnPath(metric)
-  const splitUpParamPath = splitColumnPath(param)
+  const splitUpMetricPath = splitColumnPath(
+    ColumnType.METRICS + FILE_SEPARATOR + metric
+  )
+  const splitUpParamPath = splitColumnPath(
+    ColumnType.PARAMS + FILE_SEPARATOR + param
+  )
   const plotData: MetricVsParamPlot = {
     id: getCustomPlotId(metric, param),
-    metric: metric.slice(ColumnType.METRICS.length + 1),
-    param: param.slice(ColumnType.PARAMS.length + 1),
+    metric,
+    param,
     type: CustomPlotType.METRIC_VS_PARAM,
     values: []
   }
 
   for (const experiment of experiments) {
     const metricValue = get(experiment, splitUpMetricPath) as number | undefined
+
     const paramValue = get(experiment, splitUpParamPath) as number | undefined
 
     if (metricValue !== undefined && paramValue !== undefined) {
@@ -305,7 +311,7 @@ export const collectCustomPlots = (
     .map((plotOrderValue): CustomPlot => {
       if (isCheckpointValue(plotOrderValue.type)) {
         const { metric } = plotOrderValue
-        return checkpointPlots[metric.slice(ColumnType.METRICS.length + 1)]
+        return checkpointPlots[metric]
       }
       const { metric, param } = plotOrderValue
       return collectMetricVsParamPlot(metric, param, experiments)

diff --git a/extension/src/plots/model/custom.ts b/extension/src/plots/model/custom.ts
@@ -1,3 +1,4 @@
+import { ColumnType } from '../../experiments/webview/contract'
 import { CheckpointPlot, CustomPlot, CustomPlotType } from '../webview/contract'
 
 export const CHECKPOINTS_PARAM = 'epoch'
@@ -23,3 +24,23 @@ export const doesCustomPlotAlreadyExist = (
   order.some(value => {
     return value.param === param && value.metric === metric
   })
+
+const removeColumnTypeFromPath = (
+  columnPath: string,
+  type: string,
+  fileSep: string
+) =>
+  columnPath.startsWith(type + fileSep)
+    ? columnPath.slice(type.length + 1)
+    : columnPath
+
+export const cleanupOldOrderValue = (
+  { param, metric, type }: CustomPlotsOrderValue,
+  fileSep: string
+): CustomPlotsOrderValue => ({
+  // previous column paths have the "TYPE:" prefix
+  metric: removeColumnTypeFromPath(metric, ColumnType.METRICS, fileSep),
+  param: removeColumnTypeFromPath(param, ColumnType.PARAMS, fileSep),
+  // previous values didn't have a type
+  type: type || CustomPlotType.METRIC_VS_PARAM
+})
diff --git a/extension/src/plots/model/index.ts b/extension/src/plots/model/index.ts
@@ -16,7 +16,11 @@ import {
   collectCustomPlotData
 } from './collect'
 import { getRevisionFirstThreeColumns } from './util'
-import { CustomPlotsOrderValue, isCheckpointPlot } from './custom'
+import {
+  cleanupOldOrderValue,
+  CustomPlotsOrderValue,
+  isCheckpointPlot
+} from './custom'
 import {
   CheckpointPlot,
   ComparisonPlots,
@@ -31,8 +35,7 @@ import {
   CustomPlot,
   ColorScale,
   DEFAULT_HEIGHT,
-  DEFAULT_NB_ITEMS_PER_ROW,
-  CustomPlotType
+  DEFAULT_NB_ITEMS_PER_ROW
 } from '../webview/contract'
 import {
   ExperimentsOutput,
@@ -53,6 +56,7 @@ import {
   MultiSourceVariations
 } from '../multiSource/collect'
 import { isDvcError } from '../../cli/dvc/reader'
+import { FILE_SEPARATOR } from '../../experiments/columns/paths'
 
 export type CustomCheckpointPlots = { [metric: string]: CheckpointPlot }
 
@@ -186,13 +190,8 @@ export class PlotsModel extends ModelWithPersistence {
   }
 
   public getCustomPlotsOrder() {
-    return this.customPlotsOrder.map(
-      ({ type, ...rest }) =>
-        ({
-          ...rest,
-          // type is possibly undefined if state holds an older version of custom plots
-          type: type || CustomPlotType.METRIC_VS_PARAM
-        } as CustomPlotsOrderValue)
+    return this.customPlotsOrder.map(value =>
+      cleanupOldOrderValue(value, FILE_SEPARATOR)
     )
   }
 

diff --git a/extension/src/plots/webview/messages.ts b/extension/src/plots/webview/messages.ts
@@ -29,8 +29,6 @@ import {
   pickMetricAndParam
 } from '../model/quickPick'
 import { Title } from '../../vscode/title'
-import { ColumnType } from '../../experiments/webview/contract'
-import { FILE_SEPARATOR } from '../../experiments/columns/paths'
 import { reorderObjectList } from '../../util/array'
 import {
   CHECKPOINTS_PARAM,
@@ -271,19 +269,14 @@ export class WebviewMessages {
       return
     }
 
-    const buildMetricOrParamPath = (type: string, path: string) =>
-      type + FILE_SEPARATOR + path
-
     const newOrder: CustomPlotsOrderValue[] = reorderObjectList(
       plotIds,
       customPlots,
       'id'
-    ).map(plot => ({
-      metric: buildMetricOrParamPath(ColumnType.METRICS, plot.metric),
-      param: isCheckpointValue(plot.type)
-        ? plot.param
-        : buildMetricOrParamPath(ColumnType.PARAMS, plot.param),
-      type: plot.type
+    ).map(({ metric, param, type }) => ({
+      metric,
+      param,
+      type
     }))
     this.plots.setCustomPlotsOrder(newOrder)
     this.sendCustomPlotsAndEvent(EventName.VIEWS_REORDER_PLOTS_CUSTOM)

diff --git a/extension/src/test/fixtures/expShow/base/customPlots.ts b/extension/src/test/fixtures/expShow/base/customPlots.ts
@@ -12,22 +12,22 @@ import {
 
 export const customPlotsOrderFixture: CustomPlotsOrderValue[] = [
   {
-    metric: 'metrics:summary.json:loss',
-    param: 'params:params.yaml:dropout',
+    metric: 'summary.json:loss',
+    param: 'params.yaml:dropout',
     type: CustomPlotType.METRIC_VS_PARAM
   },
   {
-    metric: 'metrics:summary.json:accuracy',
-    param: 'params:params.yaml:epochs',
+    metric: 'summary.json:accuracy',
+    param: 'params.yaml:epochs',
     type: CustomPlotType.METRIC_VS_PARAM
   },
   {
-    metric: 'metrics:summary.json:loss',
+    metric: 'summary.json:loss',
     param: CHECKPOINTS_PARAM,
     type: CustomPlotType.CHECKPOINT
   },
   {
-    metric: 'metrics:summary.json:accuracy',
+    metric: 'summary.json:accuracy',
     param: CHECKPOINTS_PARAM,
     type: CustomPlotType.CHECKPOINT
   }
@@ -125,7 +125,7 @@ const data: CustomPlotsData = {
   },
   plots: [
     {
-      id: 'custom-metrics:summary.json:loss-params:params.yaml:dropout',
+      id: 'custom-summary.json:loss-params.yaml:dropout',
       metric: 'summary.json:loss',
       param: 'params.yaml:dropout',
       type: CustomPlotType.METRIC_VS_PARAM,
@@ -149,7 +149,7 @@ const data: CustomPlotsData = {
       yTitle: 'summary.json:loss'
     },
     {
-      id: 'custom-metrics:summary.json:accuracy-params:params.yaml:epochs',
+      id: 'custom-summary.json:accuracy-params.yaml:epochs',
       metric: 'summary.json:accuracy',
       param: 'params.yaml:epochs',
       type: CustomPlotType.METRIC_VS_PARAM,

diff --git a/extension/src/test/suite/plots/index.test.ts b/extension/src/test/suite/plots/index.test.ts
@@ -431,8 +431,8 @@ suite('Plots Test Suite', () => {
       const webview = await plots.showWebview()
 
       const mockNewCustomPlotsOrder = [
-        'custom-metrics:summary.json:accuracy-params:params.yaml:epochs',
-        'custom-metrics:summary.json:loss-params:params.yaml:dropout'
+        'custom-summary.json:accuracy-params.yaml:epochs',
+        'custom-summary.json:loss-params.yaml:dropout'
       ]
 
       stub(plotsModel, 'getCustomPlots')
@@ -454,13 +454,13 @@ suite('Plots Test Suite', () => {
       expect(mockSetCustomPlotsOrder).to.be.calledOnce
       expect(mockSetCustomPlotsOrder).to.be.calledWithExactly([
         {
-          metric: 'metrics:summary.json:accuracy',
-          param: 'params:params.yaml:epochs',
+          metric: 'summary.json:accuracy',
+          param: 'params.yaml:epochs',
           type: CustomPlotType.METRIC_VS_PARAM
         },
         {
-          metric: 'metrics:summary.json:loss',
-          param: 'params:params.yaml:dropout',
+          metric: 'summary.json:loss',
+          param: 'params.yaml:dropout',
           type: CustomPlotType.METRIC_VS_PARAM
         }
       ])
@@ -781,8 +781,8 @@ suite('Plots Test Suite', () => {
       const mockGetMetric = stub(customPlotQuickPickUtil, 'pickMetric')
 
       const mockMetricVsParamOrderValue = {
-        metric: 'metrics:summary.json:accuracy',
-        param: 'params:params.yaml:dropout',
+        metric: 'summary.json:accuracy',
+        param: 'params.yaml:dropout',
         type: CustomPlotType.METRIC_VS_PARAM
       }
 
@@ -825,7 +825,7 @@ suite('Plots Test Suite', () => {
       )
 
       const mockCheckpointsOrderValue = {
-        metric: 'metrics:summary.json:val_loss',
+        metric: 'summary.json:val_loss',
         param: CHECKPOINTS_PARAM,
         type: CustomPlotType.CHECKPOINT
       }
@@ -877,15 +877,15 @@ suite('Plots Test Suite', () => {
         mockSelectCustomPlots.callsFake(() => {
           resolve(undefined)
           return Promise.resolve([
-            'custom-metrics:summary.json:loss-params:params.yaml:dropout'
+            'custom-summary.json:loss-params.yaml:dropout'
           ])
         })
       )
 
       stub(plotsModel, 'getCustomPlotsOrder').returns([
         {
-          metric: 'metrics:summary.json:loss',
-          param: 'params:params.yaml:dropout',
+          metric: 'summary.json:loss',
+          param: 'params.yaml:dropout',
           type: CustomPlotType.METRIC_VS_PARAM
         }
       ])

diff --git a/webview/src/plots/components/App.test.tsx b/webview/src/plots/components/App.test.tsx
@@ -645,8 +645,8 @@ describe('App', () => {
     let plots = screen.getAllByTestId(/summary\.json/)
 
     expect(plots.map(plot => plot.id)).toStrictEqual([
-      'custom-metrics:summary.json:loss-params:params.yaml:dropout',
-      'custom-metrics:summary.json:accuracy-params:params.yaml:epochs',
+      'custom-summary.json:loss-params.yaml:dropout',
+      'custom-summary.json:accuracy-params.yaml:epochs',
       'custom-summary.json:loss-epoch',
       'custom-summary.json:accuracy-epoch'
     ])
@@ -656,8 +656,8 @@ describe('App', () => {
     plots = screen.getAllByTestId(/summary\.json/)
 
     expect(plots.map(plot => plot.id)).toStrictEqual([
-      'custom-metrics:summary.json:accuracy-params:params.yaml:epochs',
-      'custom-metrics:summary.json:loss-params:params.yaml:dropout',
+      'custom-summary.json:accuracy-params.yaml:epochs',
+      'custom-summary.json:loss-params.yaml:dropout',
       'custom-summary.json:loss-epoch',
       'custom-summary.json:accuracy-epoch'
     ])
@@ -671,8 +671,8 @@ describe('App', () => {
 
     const plots = screen.getAllByTestId(/summary\.json/)
     expect(plots.map(plot => plot.id)).toStrictEqual([
-      'custom-metrics:summary.json:loss-params:params.yaml:dropout',
-      'custom-metrics:summary.json:accuracy-params:params.yaml:epochs',
+      'custom-summary.json:loss-params.yaml:dropout',
+      'custom-summary.json:accuracy-params.yaml:epochs',
       'custom-summary.json:loss-epoch',
       'custom-summary.json:accuracy-epoch'
     ])
@@ -683,8 +683,8 @@ describe('App', () => {
 
     const expectedOrder = [
       'custom-summary.json:loss-epoch',
-      'custom-metrics:summary.json:loss-params:params.yaml:dropout',
-      'custom-metrics:summary.json:accuracy-params:params.yaml:epochs',
+      'custom-summary.json:loss-params.yaml:dropout',
+      'custom-summary.json:accuracy-params.yaml:epochs',
       'custom-summary.json:accuracy-epoch'
     ]
 
@@ -710,8 +710,8 @@ describe('App', () => {
     expect(
       screen.getAllByTestId(/summary\.json/).map(plot => plot.id)
     ).toStrictEqual([
-      'custom-metrics:summary.json:loss-params:params.yaml:dropout',
-      'custom-metrics:summary.json:accuracy-params:params.yaml:epochs',
+      'custom-summary.json:loss-params.yaml:dropout',
+      'custom-summary.json:accuracy-params.yaml:epochs',
       'custom-summary.json:loss-epoch'
     ])
 
@@ -722,8 +722,8 @@ describe('App', () => {
     expect(
       screen.getAllByTestId(/summary\.json/).map(plot => plot.id)
     ).toStrictEqual([
-      'custom-metrics:summary.json:loss-params:params.yaml:dropout',
-      'custom-metrics:summary.json:accuracy-params:params.yaml:epochs',
+      'custom-summary.json:loss-params.yaml:dropout',
+      'custom-summary.json:accuracy-params.yaml:epochs',
       'custom-summary.json:loss-epoch',
       'custom-summary.json:accuracy-epoch'
     ])
@@ -738,8 +738,8 @@ describe('App', () => {
     expect(
       screen.getAllByTestId(/summary\.json/).map(plot => plot.id)
     ).toStrictEqual([
-      'custom-metrics:summary.json:loss-params:params.yaml:dropout',
-      'custom-metrics:summary.json:accuracy-params:params.yaml:epochs',
+      'custom-summary.json:loss-params.yaml:dropout',
+      'custom-summary.json:accuracy-params.yaml:epochs',
       'custom-summary.json:loss-epoch',
       'custom-summary.json:accuracy-epoch'
     ])
@@ -754,7 +754,7 @@ describe('App', () => {
     expect(
       screen.getAllByTestId(/summary\.json/).map(plot => plot.id)
     ).toStrictEqual([
-      'custom-metrics:summary.json:accuracy-params:params.yaml:epochs',
+      'custom-summary.json:accuracy-params.yaml:epochs',
       'custom-summary.json:loss-epoch',
       'custom-summary.json:accuracy-epoch'
     ])
@@ -997,10 +997,10 @@ describe('App', () => {
     dragAndDrop(plots[0], screen.getByTestId('custom-plots'))
 
     const expectedOrder = [
-      'custom-metrics:summary.json:accuracy-params:params.yaml:epochs',
+      'custom-summary.json:accuracy-params.yaml:epochs',
       'custom-summary.json:loss-epoch',
       'custom-summary.json:accuracy-epoch',
-      'custom-metrics:summary.json:loss-params:params.yaml:dropout'
+      'custom-summary.json:loss-params.yaml:dropout'
     ]
 
     expect(