From beeb5c34fca5f008d4ad74143a055a3ce7e95a5e Mon Sep 17 00:00:00 2001 From: julieg18 Date: Mon, 13 Mar 2023 19:22:06 -0500 Subject: [PATCH 1/2] check that weird error shows remotely --- extension/src/plots/model/collect.ts | 18 ++++++---- extension/src/plots/model/custom.ts | 19 +++++++++++ extension/src/plots/model/index.ts | 18 ++++------ extension/src/plots/webview/messages.ts | 15 +++----- .../test/fixtures/expShow/base/customPlots.ts | 16 ++++----- webview/src/plots/components/App.test.tsx | 34 +++++++++---------- 6 files changed, 67 insertions(+), 53 deletions(-) diff --git a/extension/src/plots/model/collect.ts b/extension/src/plots/model/collect.ts index 7e56fb004b..8e23831469 100644 --- 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 index 410198456b..ca7f3cfa82 100644 --- a/extension/src/plots/model/custom.ts +++ b/extension/src/plots/model/custom.ts @@ -1,3 +1,5 @@ +import { FILE_SEPARATOR } from '../../experiments/columns/paths' +import { ColumnType } from '../../experiments/webview/contract' import { CheckpointPlot, CustomPlot, CustomPlotType } from '../webview/contract' export const CHECKPOINTS_PARAM = 'epoch' @@ -23,3 +25,20 @@ export const doesCustomPlotAlreadyExist = ( order.some(value => { return value.param === param && value.metric === metric }) + +const removeColumnTypeFromPath = (columnPath: string, type: string) => + columnPath.startsWith(type + FILE_SEPARATOR) + ? columnPath.slice(type.length + 1) + : columnPath + +export const cleanupOldOrderValue = ({ + param, + metric, + type +}: CustomPlotsOrderValue): CustomPlotsOrderValue => ({ + // previous column paths have the "TYPE:" prefix + metric: removeColumnTypeFromPath(metric, ColumnType.METRICS), + param: removeColumnTypeFromPath(param, ColumnType.PARAMS), + // 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 index 216227a342..0d3b533652 100644 --- 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, @@ -186,14 +189,7 @@ 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(cleanupOldOrderValue) } public updateCustomPlotsOrder(plotsOrder: CustomPlotsOrderValue[]) { diff --git a/extension/src/plots/webview/messages.ts b/extension/src/plots/webview/messages.ts index b3b55c2c57..a4664b2b9e 100644 --- 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 index d4b415eacf..9e40d4db4f 100644 --- 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/webview/src/plots/components/App.test.tsx b/webview/src/plots/components/App.test.tsx index 7898659240..a8494b1052 100644 --- 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( From d8c50bdfb6925e33a6c57995d1421faeb84996ab Mon Sep 17 00:00:00 2001 From: julieg18 Date: Mon, 13 Mar 2023 20:51:49 -0500 Subject: [PATCH 2/2] Fix error --- extension/src/plots/model/custom.ts | 22 ++++++++++-------- extension/src/plots/model/index.ts | 5 +++- extension/src/test/suite/plots/index.test.ts | 24 ++++++++++---------- 3 files changed, 28 insertions(+), 23 deletions(-) diff --git a/extension/src/plots/model/custom.ts b/extension/src/plots/model/custom.ts index ca7f3cfa82..5bd051ff52 100644 --- a/extension/src/plots/model/custom.ts +++ b/extension/src/plots/model/custom.ts @@ -1,4 +1,3 @@ -import { FILE_SEPARATOR } from '../../experiments/columns/paths' import { ColumnType } from '../../experiments/webview/contract' import { CheckpointPlot, CustomPlot, CustomPlotType } from '../webview/contract' @@ -26,19 +25,22 @@ export const doesCustomPlotAlreadyExist = ( return value.param === param && value.metric === metric }) -const removeColumnTypeFromPath = (columnPath: string, type: string) => - columnPath.startsWith(type + FILE_SEPARATOR) +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): CustomPlotsOrderValue => ({ +export const cleanupOldOrderValue = ( + { param, metric, type }: CustomPlotsOrderValue, + fileSep: string +): CustomPlotsOrderValue => ({ // previous column paths have the "TYPE:" prefix - metric: removeColumnTypeFromPath(metric, ColumnType.METRICS), - param: removeColumnTypeFromPath(param, ColumnType.PARAMS), + 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 index 0d3b533652..6d474229ee 100644 --- a/extension/src/plots/model/index.ts +++ b/extension/src/plots/model/index.ts @@ -56,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 } @@ -189,7 +190,9 @@ export class PlotsModel extends ModelWithPersistence { } public getCustomPlotsOrder() { - return this.customPlotsOrder.map(cleanupOldOrderValue) + return this.customPlotsOrder.map(value => + cleanupOldOrderValue(value, FILE_SEPARATOR) + ) } public updateCustomPlotsOrder(plotsOrder: CustomPlotsOrderValue[]) { diff --git a/extension/src/test/suite/plots/index.test.ts b/extension/src/test/suite/plots/index.test.ts index fa449f68bb..bb45c6033d 100644 --- 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 } ])