From 7d65929197215e0b9bb1887376bf5f0383b29e59 Mon Sep 17 00:00:00 2001 From: Matt Seddon Date: Tue, 21 Mar 2023 14:18:22 +1100 Subject: [PATCH 1/3] Accomodate breaking plots diff changes --- demo | 2 +- extension/src/cli/dvc/contract.ts | 13 ++- extension/src/plots/data/collect.ts | 6 +- extension/src/plots/model/collect.test.ts | 5 +- extension/src/plots/model/collect.ts | 6 +- extension/src/plots/multiSource/collect.ts | 3 +- extension/src/plots/paths/collect.test.ts | 100 +++++++++--------- extension/src/plots/paths/collect.ts | 16 ++- extension/src/plots/paths/model.test.ts | 52 ++++----- .../src/test/fixtures/plotsDiff/index.ts | 10 +- .../test/fixtures/plotsDiff/multiSource.ts | 4 +- .../test/suite/experiments/model/tree.test.ts | 17 +-- .../src/test/suite/plots/data/index.test.ts | 42 ++++---- extension/src/test/suite/plots/index.test.ts | 3 +- .../src/test/suite/plots/paths/tree.test.ts | 2 +- extension/src/test/suite/plots/util.ts | 4 +- 16 files changed, 154 insertions(+), 131 deletions(-) diff --git a/demo b/demo index 8d6107e3f2..70d82c56e2 160000 --- a/demo +++ b/demo @@ -1 +1 @@ -Subproject commit 8d6107e3f2bea907ade1c758db22b0cddda616f1 +Subproject commit 70d82c56e231d3777a46e50d96977bbd2266962f diff --git a/extension/src/cli/dvc/contract.ts b/extension/src/cli/dvc/contract.ts index 5eb5a9ec63..5402f88497 100644 --- a/extension/src/cli/dvc/contract.ts +++ b/extension/src/cli/dvc/contract.ts @@ -96,8 +96,19 @@ export interface ExperimentsOutput { } } -export interface PlotsOutput { +export interface PlotsData { [path: string]: Plot[] } +type PlotError = { + name: string + rev: string + source?: string +} & ErrorContents + +export interface PlotsOutput { + data: { [path: string]: Plot[] } + errors?: PlotError[] +} + export type PlotsOutputOrError = PlotsOutput | DvcError diff --git a/extension/src/plots/data/collect.ts b/extension/src/plots/data/collect.ts index 012c2f542d..9bf95919e0 100644 --- a/extension/src/plots/data/collect.ts +++ b/extension/src/plots/data/collect.ts @@ -46,14 +46,16 @@ const collectKeyData = (acc: string[], key: string, plots: Plot[]): void => { } export const collectFiles = ( - data: PlotsOutputOrError, + output: PlotsOutputOrError, collectedFiles: string[] ): string[] => { const acc = [...collectedFiles] - if (isDvcError(data)) { + if (isDvcError(output)) { return acc } + const { data } = output + for (const [key, plots] of Object.entries(data)) { collectKeyData(acc, key, plots) } diff --git a/extension/src/plots/model/collect.test.ts b/extension/src/plots/model/collect.test.ts index 295beb3e65..60ba9efd5e 100644 --- a/extension/src/plots/model/collect.test.ts +++ b/extension/src/plots/model/collect.test.ts @@ -30,7 +30,8 @@ import { Experiment } from '../../experiments/webview/contract' const logsLossPath = join('logs', 'loss.tsv') -const logsLossPlot = (plotsDiffFixture[logsLossPath][0] || {}) as TemplatePlot +const logsLossPlot = (plotsDiffFixture.data[logsLossPath][0] || + {}) as TemplatePlot describe('collectCustomPlots', () => { const defaultFuncArgs = { @@ -147,7 +148,7 @@ describe('collectData', () => { expect(_1ba7bcd_heatmap).toBeDefined() expect(_1ba7bcd_heatmap).toStrictEqual( - plotsDiffFixture[heatmapPlot].find(({ revisions }) => + plotsDiffFixture.data[heatmapPlot].find(({ revisions }) => sameContents(revisions as string[], ['1ba7bcd']) ) ) diff --git a/extension/src/plots/model/collect.ts b/extension/src/plots/model/collect.ts index 7612e164ae..8b64c2b686 100644 --- a/extension/src/plots/model/collect.ts +++ b/extension/src/plots/model/collect.ts @@ -290,9 +290,10 @@ const collectPathData = ( } export const collectData = ( - data: PlotsOutput, + output: PlotsOutput, cliIdToLabel: CLIRevisionIdToLabel ): DataAccumulator => { + const { data } = output const acc = { comparisonData: {}, revisionData: {} @@ -319,7 +320,8 @@ const collectTemplate = ( acc[path] = template } -export const collectTemplates = (data: PlotsOutput): TemplateAccumulator => { +export const collectTemplates = (output: PlotsOutput): TemplateAccumulator => { + const { data } = output const acc: TemplateAccumulator = {} for (const [path, plots] of Object.entries(data)) { diff --git a/extension/src/plots/multiSource/collect.ts b/extension/src/plots/multiSource/collect.ts index cac5f870f5..63de57b9e3 100644 --- a/extension/src/plots/multiSource/collect.ts +++ b/extension/src/plots/multiSource/collect.ts @@ -86,9 +86,10 @@ const collectPathMultiSourceVariations = ( } export const collectMultiSourceVariations = ( - data: PlotsOutput, + output: PlotsOutput, acc: Record[]> ) => { + const { data } = output for (const [path, plots] of Object.entries(data)) { collectPathMultiSourceVariations(acc, path, plots) } diff --git a/extension/src/plots/paths/collect.test.ts b/extension/src/plots/paths/collect.test.ts index 42152ab53d..5357cf5f1b 100644 --- a/extension/src/plots/paths/collect.test.ts +++ b/extension/src/plots/paths/collect.test.ts @@ -90,21 +90,23 @@ describe('collectPath', () => { const updatedPaths = collectPaths( collectedPaths, { - [remainingPath]: [ - { - content: {}, - datapoints: { - [EXPERIMENT_WORKSPACE_ID]: [ - { - loss: '2.43323', - step: '0' - } - ] - }, - revisions: [EXPERIMENT_WORKSPACE_ID], - type: PlotsType.VEGA - } - ] + data: { + [remainingPath]: [ + { + content: {}, + datapoints: { + [EXPERIMENT_WORKSPACE_ID]: [ + { + loss: '2.43323', + step: '0' + } + ] + }, + revisions: [EXPERIMENT_WORKSPACE_ID], + type: PlotsType.VEGA + } + ] + } }, [EXPERIMENT_WORKSPACE_ID], {} @@ -138,39 +140,41 @@ describe('collectPath', () => { it('should handle more complex paths', () => { const revisions = [EXPERIMENT_WORKSPACE_ID] const mockPlotsDiff = { - [join('logs', 'scalars', 'acc.tsv')]: [ - { - content: {}, - datapoints: { [EXPERIMENT_WORKSPACE_ID]: [{}] }, - revisions, - type: PlotsType.VEGA - } - ], - [join('logs', 'scalars', 'loss.tsv')]: [ - { - content: {}, - datapoints: { [EXPERIMENT_WORKSPACE_ID]: [{}] }, - revisions, - type: PlotsType.VEGA - } - ], - [join('plots', 'heatmap.png')]: [ - { - revisions, - type: PlotsType.IMAGE, - url: join('plots', 'heatmap.png') - } - ], - 'predictions.json': [ - { - content: { - facet: { field: 'rev', type: 'nominal' } - } as VisualizationSpec, - datapoints: { [EXPERIMENT_WORKSPACE_ID]: [{}] }, - revisions, - type: PlotsType.VEGA - } - ] + data: { + [join('logs', 'scalars', 'acc.tsv')]: [ + { + content: {}, + datapoints: { [EXPERIMENT_WORKSPACE_ID]: [{}] }, + revisions, + type: PlotsType.VEGA + } + ], + [join('logs', 'scalars', 'loss.tsv')]: [ + { + content: {}, + datapoints: { [EXPERIMENT_WORKSPACE_ID]: [{}] }, + revisions, + type: PlotsType.VEGA + } + ], + [join('plots', 'heatmap.png')]: [ + { + revisions, + type: PlotsType.IMAGE, + url: join('plots', 'heatmap.png') + } + ], + 'predictions.json': [ + { + content: { + facet: { field: 'rev', type: 'nominal' } + } as VisualizationSpec, + datapoints: { [EXPERIMENT_WORKSPACE_ID]: [{}] }, + revisions, + type: PlotsType.VEGA + } + ] + } } expect(collectPaths([], mockPlotsDiff, revisions, {})).toStrictEqual([ diff --git a/extension/src/plots/paths/collect.ts b/extension/src/plots/paths/collect.ts index ee7edccedc..7ee076847e 100644 --- a/extension/src/plots/paths/collect.ts +++ b/extension/src/plots/paths/collect.ts @@ -5,7 +5,11 @@ import { TemplatePlot, TemplatePlotGroup } from '../webview/contract' -import { EXPERIMENT_WORKSPACE_ID, PlotsOutput } from '../../cli/dvc/contract' +import { + EXPERIMENT_WORKSPACE_ID, + PlotsData, + PlotsOutput +} from '../../cli/dvc/contract' import { getParent, getPath, getPathArray } from '../../fileSystem/util' import { splitMatchedOrdered, definedAndNonEmpty } from '../../util/array' import { isMultiViewPlot } from '../vega/util' @@ -50,7 +54,7 @@ const collectType = (plots: Plot[]) => { } const getType = ( - data: PlotsOutput, + data: PlotsData, hasChildren: boolean, path: string ): Set | undefined => { @@ -105,7 +109,7 @@ const collectTemplateRevisions = ( } const collectPathRevisions = ( - data: PlotsOutput, + data: PlotsData, path: string, cliIdToLabel: { [id: string]: string } ): Set => { @@ -125,7 +129,7 @@ const collectPathRevisions = ( const collectOrderedPath = ( acc: PlotPath[], - data: PlotsOutput, + data: PlotsData, revisions: Set, pathArray: string[], idx: number @@ -163,12 +167,14 @@ const collectOrderedPath = ( export const collectPaths = ( existingPaths: PlotPath[], - data: PlotsOutput, + output: PlotsOutput, fetchedRevs: string[], cliIdToLabel: { [id: string]: string } ): PlotPath[] => { let acc: PlotPath[] = filterWorkspaceIfFetched(existingPaths, fetchedRevs) + const { data } = output + const paths = Object.keys(data) for (const path of paths) { diff --git a/extension/src/plots/paths/model.test.ts b/extension/src/plots/paths/model.test.ts index 4735d7c1c5..be40890be1 100644 --- a/extension/src/plots/paths/model.test.ts +++ b/extension/src/plots/paths/model.test.ts @@ -83,29 +83,31 @@ describe('PathsModel', () => { const commitBeforePlots = '4c4318d' const previousPlotPath = join('dvclive', 'plots', 'metrics', 'loss.tsv') const previousPlotFixture = { - [previousPlotPath]: [ - { - content: {}, - datapoints: { - [commitBeforePlots]: [ - { - loss: '2.29', - step: '0' - }, - { - loss: '2.27', - step: '1' - }, - { - loss: '2.25', - step: '2' - } - ] - }, - revisions: [commitBeforePlots], - type: PlotsType.VEGA - } - ] + data: { + [previousPlotPath]: [ + { + content: {}, + datapoints: { + [commitBeforePlots]: [ + { + loss: '2.29', + step: '0' + }, + { + loss: '2.27', + step: '1' + }, + { + loss: '2.25', + step: '2' + } + ] + }, + revisions: [commitBeforePlots], + type: PlotsType.VEGA + } + ] + } } const multiViewGroup = { @@ -205,7 +207,7 @@ describe('PathsModel', () => { const model = new PathsModel(mockDvcRoot, buildMockMemento()) model.transformAndSet( - { ...plotsDiffFixture, ...previousPlotFixture }, + { data: { ...plotsDiffFixture.data, ...previousPlotFixture.data } }, [...revisions, commitBeforePlots], {} ) @@ -243,7 +245,7 @@ describe('PathsModel', () => { it('should move plots which do not have selected revisions to the end when a reordering occurs', () => { const model = new PathsModel(mockDvcRoot, buildMockMemento()) model.transformAndSet( - { ...plotsDiffFixture, ...previousPlotFixture }, + { data: { ...plotsDiffFixture.data, ...previousPlotFixture.data } }, [...revisions, commitBeforePlots], {} ) diff --git a/extension/src/test/fixtures/plotsDiff/index.ts b/extension/src/test/fixtures/plotsDiff/index.ts index fe44d3e839..2ab240b536 100644 --- a/extension/src/test/fixtures/plotsDiff/index.ts +++ b/extension/src/test/fixtures/plotsDiff/index.ts @@ -451,12 +451,14 @@ const getImageData = (baseUrl: string, joinFunc = join) => ({ }) export const getOutput = (baseUrl: string): PlotsOutput => ({ - ...getImageData(baseUrl), - ...basicVega, - ...require('./vega').default + data: { + ...getImageData(baseUrl), + ...basicVega, + ...require('./vega').default + } }) -export const getMinimalOutput = (): PlotsOutput => ({ ...basicVega }) +export const getMinimalOutput = (): PlotsOutput => ({ data: { ...basicVega } }) export const getMultiSourceOutput = (): PlotsOutput => ({ ...require('./multiSource').default diff --git a/extension/src/test/fixtures/plotsDiff/multiSource.ts b/extension/src/test/fixtures/plotsDiff/multiSource.ts index a60fba277b..757a895d33 100644 --- a/extension/src/test/fixtures/plotsDiff/multiSource.ts +++ b/extension/src/test/fixtures/plotsDiff/multiSource.ts @@ -1,4 +1,4 @@ -import { EXPERIMENT_WORKSPACE_ID } from '../../../cli/dvc/contract' +import { EXPERIMENT_WORKSPACE_ID, PlotsOutput } from '../../../cli/dvc/contract' import { join } from '../../util/path' const data = { @@ -356214,4 +356214,4 @@ const data = { ] } -export default data +export default { data } as PlotsOutput diff --git a/extension/src/test/suite/experiments/model/tree.test.ts b/extension/src/test/suite/experiments/model/tree.test.ts index d19e6e4346..1f9b1692e6 100644 --- a/extension/src/test/suite/experiments/model/tree.test.ts +++ b/extension/src/test/suite/experiments/model/tree.test.ts @@ -26,7 +26,6 @@ import { import { buildPlots, getExpectedCustomPlotsData } from '../../plots/util' import customPlotsFixture from '../../../fixtures/expShow/base/customPlots' import expShowFixture from '../../../fixtures/expShow/base/output' -import plotsRevisionsFixture from '../../../fixtures/plotsDiff/revisions' import { ExperimentsTree } from '../../../../experiments/model/tree' import { buildExperiments, @@ -46,11 +45,7 @@ import { WorkspaceExperiments } from '../../../../experiments/workspace' import { ExperimentItem } from '../../../../experiments/model/collect' import { EXPERIMENT_WORKSPACE_ID } from '../../../../cli/dvc/contract' import { DvcReader } from '../../../../cli/dvc/reader' -import { - ColorScale, - CustomPlotType, - DEFAULT_SECTION_COLLAPSED -} from '../../../../plots/webview/contract' +import { ColorScale, CustomPlotType } from '../../../../plots/webview/contract' suite('Experiments Tree Test Suite', () => { const disposable = getTimeSafeDisposer() @@ -114,21 +109,15 @@ suite('Experiments Tree Test Suite', () => { expect( messageSpy, - 'when there are no experiments selected we dont send checkpoint type plots' + "when there are no experiments selected we don't send checkpoint trend plots" ).to.be.calledWithMatch({ - comparison: null, custom: { ...customPlotsFixture, colors: undefined, plots: customPlotsFixture.plots.filter( plot => plot.type !== CustomPlotType.CHECKPOINT ) - }, - hasPlots: false, - hasUnselectedPlots: false, - sectionCollapsed: DEFAULT_SECTION_COLLAPSED, - selectedRevisions: plotsRevisionsFixture.slice(0, 2), - template: null + } }) messageSpy.resetHistory() diff --git a/extension/src/test/suite/plots/data/index.test.ts b/extension/src/test/suite/plots/data/index.test.ts index 49e6c14898..cc83f7e318 100644 --- a/extension/src/test/suite/plots/data/index.test.ts +++ b/extension/src/test/suite/plots/data/index.test.ts @@ -165,26 +165,28 @@ suite('Plots Data Test Suite', () => { const mockExecuteCommand = (command: CommandId) => { if (command === AvailableCommands.PLOTS_DIFF) { return Promise.resolve({ - 'dvc.yaml::Accuracy': [ - { - datapoints: { - workspace: [ - { - dvc_data_version_info: { - field: join('train', 'acc'), - filename: collectedFile, - revision: EXPERIMENT_WORKSPACE_ID - }, - dvc_inferred_y_value: '0.2707333333333333', - step: '0', - [join('train', 'acc')]: '0.2707333333333333' - } - ] - }, - revisions: [EXPERIMENT_WORKSPACE_ID], - type: 'vega' - } - ] + data: { + 'dvc.yaml::Accuracy': [ + { + datapoints: { + workspace: [ + { + dvc_data_version_info: { + field: join('train', 'acc'), + filename: collectedFile, + revision: EXPERIMENT_WORKSPACE_ID + }, + dvc_inferred_y_value: '0.2707333333333333', + step: '0', + [join('train', 'acc')]: '0.2707333333333333' + } + ] + }, + revisions: [EXPERIMENT_WORKSPACE_ID], + type: 'vega' + } + ] + } }) } } diff --git a/extension/src/test/suite/plots/index.test.ts b/extension/src/test/suite/plots/index.test.ts index 1c49a409de..33bf3d8579 100644 --- a/extension/src/test/suite/plots/index.test.ts +++ b/extension/src/test/suite/plots/index.test.ts @@ -64,7 +64,8 @@ suite('Plots Test Suite', () => { describe('Plots', () => { it('should call plots diff once on instantiation with missing revisions if there are no plots', async () => { const { mockPlotsDiff, messageSpy, plots, data } = await buildPlots( - disposable + disposable, + { data: {} } ) const managedUpdateSpy = spy(data, 'managedUpdate') diff --git a/extension/src/test/suite/plots/paths/tree.test.ts b/extension/src/test/suite/plots/paths/tree.test.ts index 33daf14434..f1647b6f32 100644 --- a/extension/src/test/suite/plots/paths/tree.test.ts +++ b/extension/src/test/suite/plots/paths/tree.test.ts @@ -37,7 +37,7 @@ suite('Plots Paths Tree Test Suite', () => { }) it('should be able to toggle whether a plot is selected with dvc.views.plotsPathsTree.toggleStatus', async () => { - const [path] = Object.keys(plotsDiffFixture) + const [path] = Object.keys(plotsDiffFixture.data) const { plots, messageSpy } = await buildPlots( disposable, plotsDiffFixture diff --git a/extension/src/test/suite/plots/util.ts b/extension/src/test/suite/plots/util.ts index 3ac226b408..339c086758 100644 --- a/extension/src/test/suite/plots/util.ts +++ b/extension/src/test/suite/plots/util.ts @@ -23,12 +23,12 @@ import { BaseWorkspaceWebviews } from '../../../webview/workspace' import { WebviewMessages } from '../../../plots/webview/messages' import { ExperimentsModel } from '../../../experiments/model' import { Experiment } from '../../../experiments/webview/contract' -import { EXPERIMENT_WORKSPACE_ID } from '../../../cli/dvc/contract' +import { EXPERIMENT_WORKSPACE_ID, PlotsOutput } from '../../../cli/dvc/contract' import { isCheckpointPlot } from '../../../plots/model/custom' export const buildPlots = async ( disposer: Disposer, - plotsDiff = {}, + plotsDiff: PlotsOutput | undefined = undefined, expShow = expShowFixtureWithoutErrors ) => { const { From a215088be31f9335d040e69881d0116783adfc08 Mon Sep 17 00:00:00 2001 From: Matt Seddon Date: Sun, 2 Apr 2023 09:21:08 +1000 Subject: [PATCH 2/3] Update min version of DVC required to 2.52.0 (plots diff breaking change) --- demo | 2 +- extension/src/cli/dvc/contract.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/demo b/demo index 70d82c56e2..dd4b7efc23 160000 --- a/demo +++ b/demo @@ -1 +1 @@ -Subproject commit 70d82c56e231d3777a46e50d96977bbd2266962f +Subproject commit dd4b7efc23a88234d5fd14f241d770733a33fc24 diff --git a/extension/src/cli/dvc/contract.ts b/extension/src/cli/dvc/contract.ts index 5402f88497..59e1169112 100644 --- a/extension/src/cli/dvc/contract.ts +++ b/extension/src/cli/dvc/contract.ts @@ -1,7 +1,7 @@ import { Plot } from '../../plots/webview/contract' -export const MIN_CLI_VERSION = '2.30.0' -export const LATEST_TESTED_CLI_VERSION = '2.51.0' +export const MIN_CLI_VERSION = '2.52.0' +export const LATEST_TESTED_CLI_VERSION = '2.52.0' export const MAX_CLI_VERSION = '3' type ErrorContents = { type: string; msg: string } From 6fdc993d6551d881159caa19359b0801c5e04a26 Mon Sep 17 00:00:00 2001 From: Matt Seddon Date: Sun, 2 Apr 2023 11:20:40 +1000 Subject: [PATCH 3/3] Remove final experiment row requirement from e2e test --- extension/src/test/e2e/extension.test.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/extension/src/test/e2e/extension.test.ts b/extension/src/test/e2e/extension.test.ts index 82ece08b07..03ce4651c1 100644 --- a/extension/src/test/e2e/extension.test.ts +++ b/extension/src/test/e2e/extension.test.ts @@ -37,7 +37,6 @@ describe('Experiments Table Webview', function () { const webview = new ExperimentsWebview('experiments') const epochs = 15 - const finalExperimentRow = 1 const headerRows = 3 const workspaceRow = 1 const commitRows = 3 @@ -89,7 +88,7 @@ describe('Experiments Table Webview', function () { async () => { await webview.expandAllRows() const currentRows = await webview.row$$ - return currentRows.length >= initialRows + epochs + finalExperimentRow + return currentRows.length >= initialRows + epochs }, { interval: 5000, timeout: 180000 } ) @@ -100,9 +99,7 @@ describe('Experiments Table Webview', function () { const finalRows = await webview.row$$ - expect(finalRows.length).toStrictEqual( - initialRows + epochs + finalExperimentRow - ) + expect(finalRows.length).toStrictEqual(initialRows + epochs) await webview.unfocus() await closeAllEditors() await waitForDvcToFinish()