From 5103535314d774af4dcb329c8fd7990ad6ebf836 Mon Sep 17 00:00:00 2001 From: Matt Seddon Date: Fri, 25 Nov 2022 15:02:18 +1100 Subject: [PATCH 1/7] give plots a linear update path --- extension/src/cli/dvc/reader.ts | 2 +- extension/src/plots/index.ts | 46 ++++++--- extension/src/plots/model/index.ts | 45 +++++---- extension/src/plots/paths/model.ts | 4 + extension/src/plots/webview/messages.ts | 36 +++---- .../test/suite/experiments/model/tree.test.ts | 6 +- extension/src/test/suite/plots/index.test.ts | 95 +++++++++++++------ 7 files changed, 154 insertions(+), 80 deletions(-) diff --git a/extension/src/cli/dvc/reader.ts b/extension/src/cli/dvc/reader.ts index 4ef291076c..ed6b7b5472 100644 --- a/extension/src/cli/dvc/reader.ts +++ b/extension/src/cli/dvc/reader.ts @@ -127,7 +127,7 @@ export class DvcReader extends DvcCli { } catch (error: unknown) { const msg = (error as MaybeConsoleError).stderr || (error as Error).message - Logger.error(`${args} failed with ${msg} retrying...`) + Logger.error(`${args} failed with ${msg}`) return { error: { msg, type: 'Caught error' } } } } diff --git a/extension/src/plots/index.ts b/extension/src/plots/index.ts index 0400ad7a6b..fa4b6386e9 100644 --- a/extension/src/plots/index.ts +++ b/extension/src/plots/index.ts @@ -11,13 +11,14 @@ import { ViewKey } from '../webview/constants' import { BaseRepository } from '../webview/repository' import { Experiments } from '../experiments' import { Resource } from '../resourceLocator' -import { InternalCommands } from '../commands/internal' +import { AvailableCommands, InternalCommands } from '../commands/internal' import { definedAndNonEmpty } from '../util/array' -import { ExperimentsOutput } from '../cli/dvc/contract' +import { ExperimentsOutput, PlotsOutput } from '../cli/dvc/contract' import { TEMP_PLOTS_DIR } from '../cli/dvc/constants' import { removeDir } from '../fileSystem' import { Toast } from '../vscode/toast' import { pickPaths } from '../path/selection/quickPick' +import { SelectedExperimentWithColor } from '../experiments/model' export type PlotsWebview = BaseWebview @@ -28,6 +29,7 @@ export class Plots extends BaseRepository { private readonly pathsChanged = this.dispose.track(new EventEmitter()) + private readonly internalCommands: InternalCommands private readonly experiments: Experiments private readonly plots: PlotsModel private readonly paths: PathsModel @@ -47,6 +49,8 @@ export class Plots extends BaseRepository { this.experiments = experiments + this.internalCommands = internalCommands + this.plots = this.dispose.track( new PlotsModel(this.dvcRoot, experiments, workspaceState) ) @@ -77,7 +81,7 @@ export class Plots extends BaseRepository { } public sendInitialWebviewData() { - return this.fetchMissingOrSendPlots() + return this.fetchMissingAndSendPlots() } public togglePathStatus(path: string) { @@ -130,21 +134,36 @@ export class Plots extends BaseRepository { private notifyChanged() { this.pathsChanged.fire() - this.fetchMissingOrSendPlots() + this.fetchMissingAndSendPlots() } - private async fetchMissingOrSendPlots() { + private async fetchMissingAndSendPlots( + overrideRevs?: SelectedExperimentWithColor[] + ) { await this.isReady() - if ( - this.paths.hasPaths() && - definedAndNonEmpty(this.plots.getUnfetchedRevisions()) - ) { - this.webviewMessages.sendCheckpointPlotsMessage() - return this.data.managedUpdate() + const selectedRevs = overrideRevs || this.experiments.getSelectedRevisions() + const selectedExperiments = this.experiments.getSelectedExperiments() + + const missingRevs = this.plots.getMissingRevisions(selectedRevs) + + if (this.paths.hasPaths() && definedAndNonEmpty(missingRevs)) { + const data = await this.internalCommands.executeCommand( + AvailableCommands.PLOTS_DIFF, + this.dvcRoot, + ...missingRevs + ) + + await Promise.all([ + this.plots.transformAndSetPlots(data, missingRevs), + this.paths.transformAndSet(data) + ]) } - return this.webviewMessages.sendWebviewMessage() + return this.webviewMessages.sendWebviewMessage( + selectedRevs, + selectedExperiments + ) } private createWebviewMessageHandler( @@ -184,13 +203,14 @@ export class Plots extends BaseRepository { private setupExperimentsListener(experiments: Experiments) { this.dispose.track( experiments.onDidChangeExperiments(async data => { + const selectedRevs = experiments.getSelectedRevisions() if (data) { await this.plots.transformAndSetExperiments(data) } this.plots.setComparisonOrder() - this.fetchMissingOrSendPlots() + this.fetchMissingAndSendPlots(selectedRevs) }) ) } diff --git a/extension/src/plots/model/index.ts b/extension/src/plots/model/index.ts index d0f44cc154..8b80e4be31 100644 --- a/extension/src/plots/model/index.ts +++ b/extension/src/plots/model/index.ts @@ -39,6 +39,7 @@ import { MultiSourceEncoding, MultiSourceVariations } from '../multiSource/collect' +import { SelectedExperimentWithColor } from '../../experiments/model' export class PlotsModel extends ModelWithPersistence { private readonly experiments: Experiments @@ -105,6 +106,10 @@ export class PlotsModel extends ModelWithPersistence { } public async transformAndSetPlots(data: PlotsOutput, revs: string[]) { + if (data.error) { + return + } + const cliIdToLabel = this.getCLIIdToLabel() this.fetchedRevs = new Set([ @@ -147,15 +152,15 @@ export class PlotsModel extends ModelWithPersistence { this.deferred.resolve() } - public getCheckpointPlots() { + public getCheckpointPlots(overrideRevs?: SelectedExperimentWithColor[]) { if (!this.checkpointPlots) { return } const colors = getColorScale( - this.experiments - .getSelectedExperiments() - .map(({ displayColor, id: revision }) => ({ displayColor, revision })) + (overrideRevs || this.experiments.getSelectedExperiments()).map( + ({ displayColor, id: revision }) => ({ displayColor, revision }) + ) ) if (!colors) { @@ -176,19 +181,19 @@ export class PlotsModel extends ModelWithPersistence { this.deleteRevisionData(id) } - public getUnfetchedRevisions() { - return this.getSelectedRevisions().filter( + public getUnfetchedRevisions(overrideRevs?: SelectedExperimentWithColor[]) { + return this.getSelectedRevisions(overrideRevs).filter( revision => !this.fetchedRevs.has(revision) ) } - public getMissingRevisions() { + public getMissingRevisions(overrideRevs?: SelectedExperimentWithColor[]) { const cachedRevisions = new Set([ ...Object.keys(this.comparisonData), ...Object.keys(this.revisionData) ]) - return this.getSelectedRevisions() + return this.getSelectedRevisions(overrideRevs) .filter(label => !cachedRevisions.has(label)) .map(label => this.getCLIId(label)) } @@ -216,12 +221,15 @@ export class PlotsModel extends ModelWithPersistence { ) } - public getTemplatePlots(order: TemplateOrder | undefined) { + public getTemplatePlots( + order: TemplateOrder | undefined, + overrideRevs?: SelectedExperimentWithColor[] + ) { if (!definedAndNonEmpty(order)) { return } - const selectedRevisions = this.getSelectedRevisions() + const selectedRevisions = this.getSelectedRevisions(overrideRevs) if (!definedAndNonEmpty(selectedRevisions)) { return @@ -230,12 +238,15 @@ export class PlotsModel extends ModelWithPersistence { return this.getSelectedTemplatePlots(order, selectedRevisions) } - public getComparisonPlots(paths: string[] | undefined) { + public getComparisonPlots( + paths: string[] | undefined, + overrideRevs?: SelectedExperimentWithColor[] + ) { if (!paths) { return } - const selectedRevisions = this.getSelectedRevisions() + const selectedRevisions = this.getSelectedRevisions(overrideRevs) if (!definedAndNonEmpty(selectedRevisions)) { return } @@ -317,6 +328,12 @@ export class PlotsModel extends ModelWithPersistence { return this.multiSourceEncoding } + public getSelectedRevisions(overrideRevs?: SelectedExperimentWithColor[]) { + return (overrideRevs || this.experiments.getSelectedRevisions()).map( + ({ label }) => label + ) + } + private removeStaleData() { return Promise.all([ this.removeStaleBranches(), @@ -379,10 +396,6 @@ export class PlotsModel extends ModelWithPersistence { return this.branchRevisions[label] || label } - private getSelectedRevisions() { - return this.experiments.getSelectedRevisions().map(({ label }) => label) - } - private getPlots( checkpointPlots: CheckpointPlot[], selectedExperiments: string[] diff --git a/extension/src/plots/paths/model.ts b/extension/src/plots/paths/model.ts index 43d015fc0d..d27b7f9bcc 100644 --- a/extension/src/plots/paths/model.ts +++ b/extension/src/plots/paths/model.ts @@ -27,6 +27,10 @@ export class PathsModel extends PathSelectionModel { } public transformAndSet(data: PlotsOutput) { + if (data.error) { + return + } + const paths = collectPaths(this.data, data) this.setNewStatuses(paths) diff --git a/extension/src/plots/webview/messages.ts b/extension/src/plots/webview/messages.ts index cc268c9963..33bec78b0b 100644 --- a/extension/src/plots/webview/messages.ts +++ b/extension/src/plots/webview/messages.ts @@ -21,6 +21,7 @@ import { PathsModel } from '../paths/model' import { BaseWebview } from '../../webview' import { getModifiedTime } from '../../fileSystem' import { definedAndNonEmpty } from '../../util/array' +import { SelectedExperimentWithColor } from '../../experiments/model' export class WebviewMessages { private readonly paths: PathsModel @@ -47,21 +48,18 @@ export class WebviewMessages { this.updateData = updateData } - public sendWebviewMessage() { + public sendWebviewMessage( + selectedRevs: SelectedExperimentWithColor[], + selectedExperiments: SelectedExperimentWithColor[] + ) { this.getWebview()?.show({ - checkpoint: this.getCheckpointPlots(), - comparison: this.getComparisonPlots(), + checkpoint: this.getCheckpointPlots(selectedExperiments), + comparison: this.getComparisonPlots(selectedRevs), hasPlots: !!this.paths?.hasPaths(), hasSelectedPlots: definedAndNonEmpty(this.paths.getSelected()), sectionCollapsed: this.plots.getSectionCollapsed(), selectedRevisions: this.plots.getSelectedRevisionDetails(), - template: this.getTemplatePlots() - }) - } - - public sendCheckpointPlotsMessage() { - this.getWebview()?.show({ - checkpoint: this.getCheckpointPlots() + template: this.getTemplatePlots(selectedRevs) }) } @@ -96,6 +94,12 @@ export class WebviewMessages { } } + private sendCheckpointPlotsMessage() { + this.getWebview()?.show({ + checkpoint: this.getCheckpointPlots() + }) + } + private setSelectedMetrics(metrics: string[]) { this.plots.setSelectedMetrics(metrics) this.sendCheckpointPlotsAndEvent(EventName.VIEWS_PLOTS_METRICS_SELECTED) @@ -230,9 +234,9 @@ export class WebviewMessages { }) } - private getTemplatePlots() { + private getTemplatePlots(overrideRevs?: SelectedExperimentWithColor[]) { const paths = this.paths?.getTemplateOrder() - const plots = this.plots?.getTemplatePlots(paths) + const plots = this.plots?.getTemplatePlots(paths, overrideRevs) if (!this.plots || !plots || isEmpty(plots)) { return null @@ -244,9 +248,9 @@ export class WebviewMessages { } } - private getComparisonPlots() { + private getComparisonPlots(overrideRevs?: SelectedExperimentWithColor[]) { const paths = this.paths.getComparisonPaths() - const comparison = this.plots.getComparisonPlots(paths) + const comparison = this.plots.getComparisonPlots(paths, overrideRevs) if (!this.plots || !comparison || isEmpty(comparison)) { return null } @@ -284,7 +288,7 @@ export class WebviewMessages { } } - private getCheckpointPlots() { - return this.plots?.getCheckpointPlots() || null + private getCheckpointPlots(overrideRevs?: SelectedExperimentWithColor[]) { + return this.plots?.getCheckpointPlots(overrideRevs) || null } } diff --git a/extension/src/test/suite/experiments/model/tree.test.ts b/extension/src/test/suite/experiments/model/tree.test.ts index 53fba711ed..fa927dd4af 100644 --- a/extension/src/test/suite/experiments/model/tree.test.ts +++ b/extension/src/test/suite/experiments/model/tree.test.ts @@ -396,10 +396,10 @@ suite('Experiments Tree Test Suite', () => { 'the missing revisions have been requested' ).to.be.calledWithExactly( dvcDemoPath, - '1ee5f2e', - '2173124', '23250b3', - 'd1343a8' + 'd1343a8', + '1ee5f2e', + '2173124' ) }).timeout(WEBVIEW_TEST_TIMEOUT) diff --git a/extension/src/test/suite/plots/index.test.ts b/extension/src/test/suite/plots/index.test.ts index 49313ef4ee..ca28cc919c 100644 --- a/extension/src/test/suite/plots/index.test.ts +++ b/extension/src/test/suite/plots/index.test.ts @@ -89,11 +89,12 @@ suite('Plots Test Suite', () => { it('should call plots diff with new experiment revisions but not checkpoints', async () => { const mockNow = getMockNow() - const { mockPlotsDiff, data, experiments } = await buildPlots( + const { plots, mockPlotsDiff, experiments } = await buildPlots( disposable, plotsDiffFixture ) mockPlotsDiff.resetHistory() + await plots.showWebview() const updatedExpShowFixture = merge( cloneDeep(expShowFixtureWithoutErrors), @@ -116,7 +117,9 @@ suite('Plots Test Suite', () => { ) const dataUpdateEvent = new Promise(resolve => - disposable.track(data.onDidUpdate(() => resolve(undefined))) + disposable.track( + experiments.onDidChangeExperiments(() => resolve(undefined)) + ) ) bypassProcessManagerDebounce(mockNow) @@ -129,11 +132,21 @@ suite('Plots Test Suite', () => { }) it('should call plots diff with the branch name whenever the current branch commit changes', async () => { - const mockNow = getMockNow() - const { data, experiments, mockPlotsDiff } = await buildPlots( - disposable, - plotsDiffFixture + const { plots, experiments, mockPlotsDiff, webviewMessages } = + await buildPlots(disposable, plotsDiffFixture) + + const mockSendPlots = stub(webviewMessages, 'sendWebviewMessage') + + const plotsSentEvent = new Promise(resolve => + mockSendPlots.callsFake(() => { + resolve(undefined) + }) ) + await plots.showWebview() + + await plotsSentEvent + mockSendPlots.resetBehavior() + mockPlotsDiff.resetHistory() const committedExperiment = { @@ -152,30 +165,38 @@ suite('Plots Test Suite', () => { workspace: committedExperiment } - const dataUpdateEvent = new Promise(resolve => - disposable.track(data.onDidUpdate(() => resolve(undefined))) + const plotsResent = new Promise(resolve => + mockSendPlots.callsFake(() => { + resolve(undefined) + }) ) - bypassProcessManagerDebounce(mockNow) experiments.setState(updatedExpShowFixture) - await dataUpdateEvent + await plotsResent expect(mockPlotsDiff).to.be.calledOnce expect(mockPlotsDiff).to.be.calledWithExactly( dvcDemoPath, - '9235a02', - 'workspace' + 'workspace', + '9235a02' ) }) it('should re-fetch data when moving between branches', async () => { - const mockNow = getMockNow() - const { data, experiments, mockPlotsDiff, plotsModel, webviewMessages } = + const { plots, experiments, mockPlotsDiff, plotsModel, webviewMessages } = await buildPlots(disposable, plotsDiffFixture) - mockPlotsDiff.resetHistory() - const mockSendPlots = stub(webviewMessages, 'sendWebviewMessage') + const plotsSentEvent = new Promise(resolve => + mockSendPlots.callsFake(() => { + resolve(undefined) + }) + ) + await plots.showWebview() + await plotsSentEvent + mockSendPlots.resetBehavior() + + mockPlotsDiff.resetHistory() mockPlotsDiff .onFirstCall() @@ -214,14 +235,14 @@ suite('Plots Test Suite', () => { const branchChangedEvent = new Promise(resolve => disposable.track( - data.onDidUpdate(() => { + experiments.onDidChangeExperiments(() => { if (plotsModel) { resolve(undefined) } }) ) ) - const plotsSentEvent = new Promise(resolve => + const plotsResentEvent = new Promise(resolve => mockSendPlots.callsFake(() => { if (isEqual(plotsModel.getMissingRevisions(), ['9235a02'])) { resolve(undefined) @@ -229,35 +250,36 @@ suite('Plots Test Suite', () => { }) ) - bypassProcessManagerDebounce(mockNow) experiments.setState(updatedExpShowFixture) await branchChangedEvent - await plotsSentEvent + await plotsResentEvent + mockSendPlots.resetBehavior() expect(mockPlotsDiff).to.be.calledOnce expect(mockPlotsDiff).to.be.calledWithExactly( dvcDemoPath, - '9235a02', - 'workspace' + 'workspace', + '9235a02' ) - bypassProcessManagerDebounce(mockNow, 2) - const dataUpdateEvent = new Promise(resolve => - disposable.track(data.onDidUpdate(() => resolve(undefined))) + const originalPlotsSentEvent = new Promise(resolve => + mockSendPlots.callsFake(() => { + resolve(undefined) + }) ) experiments.setState(expShowFixtureWithoutErrors) - await dataUpdateEvent + await originalPlotsSentEvent expect(mockPlotsDiff).to.be.calledTwice expect(mockPlotsDiff).to.be.calledWithExactly( dvcDemoPath, - '1ba7bcd', - '42b8736', - '4fb124a', + 'workspace', '53c3851', - 'workspace' + '4fb124a', + '42b8736', + '1ba7bcd' ) }).timeout(WEBVIEW_TEST_TIMEOUT) @@ -766,17 +788,28 @@ suite('Plots Test Suite', () => { const { experiments, plots, messageSpy, mockPlotsDiff } = await buildPlots(disposable, multiSourcePlotsDiffFixture) + messageSpy.restore() + stub(experiments, 'getSelectedRevisions').returns([ { label: 'workspace' }, { label: 'main' } ] as SelectedExperimentWithColor[]) const webview = await plots.showWebview() + const mockShow = stub(webview, 'show') + const messageReceived = new Promise(resolve => + mockShow.callsFake(() => { + resolve(undefined) + return Promise.resolve(true) + }) + ) + await webview.isReady() + await messageReceived expect(mockPlotsDiff).to.be.called - const { template: templateData } = getFirstArgOfLastCall(messageSpy) + const { template: templateData } = getFirstArgOfLastCall(mockShow) const [singleViewSection, multiViewSection] = ( templateData as TemplatePlotsData From bfee9f1fdd7c4bb33bdb38b3399f3f143c1665bd Mon Sep 17 00:00:00 2001 From: Matt Seddon Date: Fri, 25 Nov 2022 17:30:07 +1100 Subject: [PATCH 2/7] remove experiments from plots model --- extension/src/plots/data/index.ts | 17 +- extension/src/plots/index.ts | 43 ++++- extension/src/plots/model/index.test.ts | 40 ++--- extension/src/plots/model/index.ts | 149 +++++++++--------- extension/src/plots/webview/messages.ts | 68 +++++--- .../test/suite/experiments/model/tree.test.ts | 8 +- .../src/test/suite/plots/data/index.test.ts | 13 +- extension/src/test/suite/plots/index.test.ts | 12 +- 8 files changed, 215 insertions(+), 135 deletions(-) diff --git a/extension/src/plots/data/index.ts b/extension/src/plots/data/index.ts index 1b02d38722..f7d3cd4b7d 100644 --- a/extension/src/plots/data/index.ts +++ b/extension/src/plots/data/index.ts @@ -2,6 +2,7 @@ import { EventEmitter } from 'vscode' import { PlotsOutput } from '../../cli/dvc/contract' import { AvailableCommands, InternalCommands } from '../../commands/internal' import { BaseData } from '../../data' +import { Experiments } from '../../experiments' import { definedAndNonEmpty, flattenUnique, @@ -10,12 +11,14 @@ import { import { PlotsModel } from '../model' export class PlotsData extends BaseData<{ data: PlotsOutput; revs: string[] }> { - private readonly model: PlotsModel + private plots: PlotsModel + private experiments: Experiments constructor( dvcRoot: string, internalCommands: InternalCommands, - model: PlotsModel, + plots: PlotsModel, + experiments: Experiments, updatesPaused: EventEmitter ) { super( @@ -30,13 +33,15 @@ export class PlotsData extends BaseData<{ data: PlotsOutput; revs: string[] }> { ], ['dvc.yaml', 'dvc.lock'] ) - this.model = model + + this.plots = plots + this.experiments = experiments } public async update(): Promise { const revs = flattenUnique([ - this.model.getMissingRevisions(), - this.model.getMutableRevisions() + this.plots.getMissingRevisions(this.experiments.getSelectedRevisions()), + this.experiments.getMutableRevisions() ]) if ( @@ -72,7 +77,7 @@ export class PlotsData extends BaseData<{ data: PlotsOutput; revs: string[] }> { private getArgs(revs: string[]) { const cliWillThrowError = sameContents(revs, ['workspace']) - if (this.model && cliWillThrowError) { + if (this.plots && cliWillThrowError) { return [] } diff --git a/extension/src/plots/index.ts b/extension/src/plots/index.ts index fa4b6386e9..c59df0973d 100644 --- a/extension/src/plots/index.ts +++ b/extension/src/plots/index.ts @@ -52,7 +52,7 @@ export class Plots extends BaseRepository { this.internalCommands = internalCommands this.plots = this.dispose.track( - new PlotsModel(this.dvcRoot, experiments, workspaceState) + new PlotsModel(this.dvcRoot, workspaceState) ) this.paths = this.dispose.track( new PathsModel(this.dvcRoot, workspaceState) @@ -65,7 +65,13 @@ export class Plots extends BaseRepository { ) this.data = this.dispose.track( - new PlotsData(dvcRoot, internalCommands, this.plots, updatesPaused) + new PlotsData( + dvcRoot, + internalCommands, + this.plots, + this.experiments, + updatesPaused + ) ) this.onDidUpdateData() @@ -108,7 +114,9 @@ export class Plots extends BaseRepository { Toast.infoWithOptions( 'Attempting to refresh plots for selected experiments.' ) - for (const { revision } of this.plots.getSelectedRevisionDetails()) { + for (const { revision } of this.plots.getSelectedRevisionDetails( + this.experiments.getSelectedRevisions() + )) { this.plots.setupManualRefresh(revision) } this.data.managedUpdate() @@ -137,6 +145,7 @@ export class Plots extends BaseRepository { this.fetchMissingAndSendPlots() } + // eslint-disable-next-line sonarjs/cognitive-complexity private async fetchMissingAndSendPlots( overrideRevs?: SelectedExperimentWithColor[] ) { @@ -155,7 +164,11 @@ export class Plots extends BaseRepository { ) await Promise.all([ - this.plots.transformAndSetPlots(data, missingRevs), + this.plots.transformAndSetPlots( + data, + missingRevs, + this.experiments.getSelectedRevisions() + ), this.paths.transformAndSet(data) ]) } @@ -205,10 +218,15 @@ export class Plots extends BaseRepository { experiments.onDidChangeExperiments(async data => { const selectedRevs = experiments.getSelectedRevisions() if (data) { - await this.plots.transformAndSetExperiments(data) + await this.plots.transformAndSetExperiments( + data, + experiments.getBranchRevisions(), + experiments.getRevisions(), + experiments.hasCheckpoints() + ) } - this.plots.setComparisonOrder() + this.plots.setComparisonOrder(selectedRevs) this.fetchMissingAndSendPlots(selectedRevs) }) @@ -216,7 +234,12 @@ export class Plots extends BaseRepository { } private async initializeData(data: ExperimentsOutput) { - await this.plots.transformAndSetExperiments(data) + await this.plots.transformAndSetExperiments( + data, + this.experiments.getBranchRevisions(), + this.experiments.getRevisions(), + this.experiments.hasCheckpoints() + ) this.data.managedUpdate() await Promise.all([ this.data.isReady(), @@ -230,7 +253,11 @@ export class Plots extends BaseRepository { this.dispose.track( this.data.onDidUpdate(async ({ data, revs }) => { await Promise.all([ - this.plots.transformAndSetPlots(data, revs), + this.plots.transformAndSetPlots( + data, + revs, + this.experiments.getSelectedRevisions() + ), this.paths.transformAndSet(data) ]) this.notifyChanged() diff --git a/extension/src/plots/model/index.test.ts b/extension/src/plots/model/index.test.ts index c0aca7f906..27a70da5b2 100644 --- a/extension/src/plots/model/index.test.ts +++ b/extension/src/plots/model/index.test.ts @@ -6,7 +6,6 @@ import { Section } from '../webview/contract' import { buildMockMemento } from '../../test/util' -import { Experiments } from '../../experiments' import { PersistenceKey } from '../../persistence/constants' const mockedRevisions = [ @@ -29,14 +28,7 @@ describe('plotsModel', () => { const mockedGetSelectedRevisions = jest.fn() beforeEach(() => { - model = new PlotsModel( - exampleDvcRoot, - { - getSelectedRevisions: mockedGetSelectedRevisions, - isReady: () => Promise.resolve(undefined) - } as unknown as Experiments, - memento - ) + model = new PlotsModel(exampleDvcRoot, memento) jest.clearAllMocks() }) @@ -116,7 +108,7 @@ describe('plotsModel', () => { const mementoUpdateSpy = jest.spyOn(memento, 'update') const newOrder = ['71f31cf', 'e93c7e6', 'ffbe811', 'workspace', 'main'] - model.setComparisonOrder(newOrder) + model.setComparisonOrder(mockedGetSelectedRevisions(), newOrder) expect(mementoUpdateSpy).toHaveBeenCalledTimes(1) expect(mementoUpdateSpy).toHaveBeenCalledWith( @@ -125,7 +117,9 @@ describe('plotsModel', () => { ) expect( - model.getSelectedRevisionDetails().map(({ revision }) => revision) + model + .getSelectedRevisionDetails(mockedGetSelectedRevisions()) + .map(({ revision }) => revision) ).toStrictEqual(newOrder) }) @@ -134,10 +128,12 @@ describe('plotsModel', () => { const newOrder = ['71f31cf', 'e93c7e6'] - model.setComparisonOrder(newOrder) + model.setComparisonOrder(mockedGetSelectedRevisions(), newOrder) expect( - model.getSelectedRevisionDetails().map(({ revision }) => revision) + model + .getSelectedRevisionDetails(mockedGetSelectedRevisions()) + .map(({ revision }) => revision) ).toStrictEqual([ ...newOrder, ...mockedRevisions @@ -160,22 +156,28 @@ describe('plotsModel', () => { .mockReturnValueOnce(revisionReAdded) const initialOrder = ['workspace', 'main', '71f31cf'] - model.setComparisonOrder(initialOrder) + model.setComparisonOrder(mockedGetSelectedRevisions(), initialOrder) expect( - model.getSelectedRevisionDetails().map(({ revision }) => revision) + model + .getSelectedRevisionDetails(mockedGetSelectedRevisions()) + .map(({ revision }) => revision) ).toStrictEqual(initialOrder) - model.setComparisonOrder() + model.setComparisonOrder(mockedGetSelectedRevisions()) expect( - model.getSelectedRevisionDetails().map(({ revision }) => revision) + model + .getSelectedRevisionDetails(mockedGetSelectedRevisions()) + .map(({ revision }) => revision) ).toStrictEqual(initialOrder.filter(revision => revision !== 'main')) - model.setComparisonOrder() + model.setComparisonOrder(mockedGetSelectedRevisions()) expect( - model.getSelectedRevisionDetails().map(({ revision }) => revision) + model + .getSelectedRevisionDetails(mockedGetSelectedRevisions()) + .map(({ revision }) => revision) ).toStrictEqual(['workspace', '71f31cf', 'main']) }) }) diff --git a/extension/src/plots/model/index.ts b/extension/src/plots/model/index.ts index 8b80e4be31..d374c8412e 100644 --- a/extension/src/plots/model/index.ts +++ b/extension/src/plots/model/index.ts @@ -26,7 +26,6 @@ import { PlotSizeNumber } from '../webview/contract' import { ExperimentsOutput, PlotsOutput } from '../../cli/dvc/contract' -import { Experiments } from '../../experiments' import { getColorScale, truncateVerticalTitle } from '../vega/util' import { definedAndNonEmpty, reorderObjectList } from '../../util/array' import { removeMissingKeysFromObject } from '../../util/object' @@ -42,8 +41,6 @@ import { import { SelectedExperimentWithColor } from '../../experiments/model' export class PlotsModel extends ModelWithPersistence { - private readonly experiments: Experiments - private plotSizes: Record private sectionCollapsed: SectionCollapsed private branchRevisions: Record = {} @@ -63,13 +60,8 @@ export class PlotsModel extends ModelWithPersistence { private selectedMetrics?: string[] private metricOrder: string[] - constructor( - dvcRoot: string, - experiments: Experiments, - workspaceState: Memento - ) { + constructor(dvcRoot: string, workspaceState: Memento) { super(dvcRoot, workspaceState) - this.experiments = experiments this.plotSizes = this.revive( PersistenceKey.PLOT_SIZES, @@ -87,10 +79,18 @@ export class PlotsModel extends ModelWithPersistence { this.metricOrder = this.revive(PersistenceKey.PLOT_METRIC_ORDER, []) } - public async transformAndSetExperiments(data: ExperimentsOutput) { + public async transformAndSetExperiments( + data: ExperimentsOutput, + branchRevisions: { + id: string + sha: string | undefined + }[], + revisions: string[], + hasCheckpoints = false + ) { const [checkpointPlots, workspaceRunningCheckpoint] = await Promise.all([ collectCheckpointPlotsData(data), - collectWorkspaceRunningCheckpoint(data, this.experiments.hasCheckpoints()) + collectWorkspaceRunningCheckpoint(data, hasCheckpoints) ]) if (!this.selectedMetrics && checkpointPlots) { @@ -102,19 +102,23 @@ export class PlotsModel extends ModelWithPersistence { this.setMetricOrder() - return this.removeStaleData() + return this.removeStaleData(branchRevisions, revisions) } - public async transformAndSetPlots(data: PlotsOutput, revs: string[]) { + public async transformAndSetPlots( + data: PlotsOutput, + newRevs: string[], + selectedRevs: SelectedExperimentWithColor[] + ) { if (data.error) { return } - const cliIdToLabel = this.getCLIIdToLabel() + const cliIdToLabel = this.getCLIIdToLabel(selectedRevs) this.fetchedRevs = new Set([ ...this.fetchedRevs, - ...revs.map(rev => cliIdToLabel[rev]) + ...newRevs.map(rev => cliIdToLabel[rev]) ]) const [{ comparisonData, revisionData }, templates, multiSourceVariations] = @@ -147,31 +151,34 @@ export class PlotsModel extends ModelWithPersistence { this.multiSourceVariations ) - this.setComparisonOrder() + this.setComparisonOrder(selectedRevs) this.deferred.resolve() } - public getCheckpointPlots(overrideRevs?: SelectedExperimentWithColor[]) { + public getCheckpointPlots( + selectedExperiments: SelectedExperimentWithColor[] + ) { if (!this.checkpointPlots) { return } const colors = getColorScale( - (overrideRevs || this.experiments.getSelectedExperiments()).map( - ({ displayColor, id: revision }) => ({ displayColor, revision }) - ) + selectedExperiments.map(({ displayColor, id: revision }) => ({ + displayColor, + revision + })) ) if (!colors) { return } - const { domain: selectedExperiments } = colors + const { domain } = colors return { colors, - plots: this.getPlots(this.checkpointPlots, selectedExperiments), + plots: this.getPlots(this.checkpointPlots, domain), selectedMetrics: this.getSelectedMetrics(), size: this.getPlotSize(Section.CHECKPOINT_PLOTS) } @@ -181,72 +188,60 @@ export class PlotsModel extends ModelWithPersistence { this.deleteRevisionData(id) } - public getUnfetchedRevisions(overrideRevs?: SelectedExperimentWithColor[]) { - return this.getSelectedRevisions(overrideRevs).filter( - revision => !this.fetchedRevs.has(revision) - ) - } - - public getMissingRevisions(overrideRevs?: SelectedExperimentWithColor[]) { + public getMissingRevisions(selectedRevs: SelectedExperimentWithColor[]) { const cachedRevisions = new Set([ ...Object.keys(this.comparisonData), ...Object.keys(this.revisionData) ]) - return this.getSelectedRevisions(overrideRevs) + return this.getSelectedRevisions(selectedRevs) .filter(label => !cachedRevisions.has(label)) .map(label => this.getCLIId(label)) } - public getMutableRevisions() { - return this.experiments.getMutableRevisions() - } - - public getRevisionColors() { - return getColorScale(this.getSelectedRevisionDetails()) + public getRevisionColors(selectedRevs: SelectedExperimentWithColor[]) { + return getColorScale(this.getSelectedRevisionDetails(selectedRevs)) } - public getSelectedRevisionDetails() { + public getSelectedRevisionDetails( + selectedRevs: SelectedExperimentWithColor[] + ) { return reorderObjectList( this.comparisonOrder, - this.experiments - .getSelectedRevisions() - .map(({ label, displayColor, logicalGroupName, id }) => ({ - displayColor, - group: logicalGroupName, - id, - revision: label - })), + selectedRevs.map(({ label, displayColor, logicalGroupName, id }) => ({ + displayColor, + group: logicalGroupName, + id, + revision: label + })), 'revision' ) } public getTemplatePlots( order: TemplateOrder | undefined, - overrideRevs?: SelectedExperimentWithColor[] + selectedRevs: SelectedExperimentWithColor[] ) { if (!definedAndNonEmpty(order)) { return } - const selectedRevisions = this.getSelectedRevisions(overrideRevs) - - if (!definedAndNonEmpty(selectedRevisions)) { + if (!definedAndNonEmpty(selectedRevs)) { return } - return this.getSelectedTemplatePlots(order, selectedRevisions) + return this.getSelectedTemplatePlots(order, selectedRevs) } public getComparisonPlots( paths: string[] | undefined, - overrideRevs?: SelectedExperimentWithColor[] + selectedRevs: SelectedExperimentWithColor[] ) { if (!paths) { return } - const selectedRevisions = this.getSelectedRevisions(overrideRevs) + const selectedRevisions = this.getSelectedRevisions(selectedRevs) if (!definedAndNonEmpty(selectedRevisions)) { return } @@ -254,8 +249,11 @@ export class PlotsModel extends ModelWithPersistence { return this.getSelectedComparisonPlots(paths, selectedRevisions) } - public setComparisonOrder(revisions: string[] = this.comparisonOrder) { - const currentRevisions = this.getSelectedRevisions() + public setComparisonOrder( + selectedRevs: SelectedExperimentWithColor[], + revisions: string[] = this.comparisonOrder + ) { + const currentRevisions = this.getSelectedRevisions(selectedRevs) this.comparisonOrder = revisions.filter(revision => currentRevisions.includes(revision) @@ -328,22 +326,24 @@ export class PlotsModel extends ModelWithPersistence { return this.multiSourceEncoding } - public getSelectedRevisions(overrideRevs?: SelectedExperimentWithColor[]) { - return (overrideRevs || this.experiments.getSelectedRevisions()).map( - ({ label }) => label - ) + public getSelectedRevisions(selectedRevs: SelectedExperimentWithColor[]) { + return selectedRevs.map(({ label }) => label) } - private removeStaleData() { + private removeStaleData( + branchRevisions: { + id: string + sha: string | undefined + }[], + revisions: string[] + ) { return Promise.all([ - this.removeStaleBranches(), - this.removeStaleRevisions() + this.removeStaleBranches(branchRevisions), + this.removeStaleRevisions(revisions) ]) } - private removeStaleRevisions() { - const revisions = this.experiments.getRevisions() - + private removeStaleRevisions(revisions: string[]) { this.comparisonData = removeMissingKeysFromObject( revisions, this.comparisonData @@ -361,10 +361,13 @@ export class PlotsModel extends ModelWithPersistence { } } - private removeStaleBranches() { - const currentBranchRevisions = collectBranchRevisionDetails( - this.experiments.getBranchRevisions() - ) + private removeStaleBranches( + branchRevisions: { + id: string + sha: string | undefined + }[] + ) { + const currentBranchRevisions = collectBranchRevisionDetails(branchRevisions) for (const id of Object.keys(this.branchRevisions)) { if (this.branchRevisions[id] !== currentBranchRevisions[id]) { this.deleteRevisionData(id) @@ -382,10 +385,10 @@ export class PlotsModel extends ModelWithPersistence { this.fetchedRevs.delete(id) } - private getCLIIdToLabel() { + private getCLIIdToLabel(selectedRevs: SelectedExperimentWithColor[]) { const mapping: { [shortSha: string]: string } = {} - for (const rev of this.getSelectedRevisions()) { + for (const rev of this.getSelectedRevisions(selectedRevs)) { mapping[this.getCLIId(rev)] = rev } @@ -452,15 +455,15 @@ export class PlotsModel extends ModelWithPersistence { private getSelectedTemplatePlots( order: TemplateOrder, - selectedRevisions: string[] + selectedRevs: SelectedExperimentWithColor[] ) { return collectSelectedTemplatePlots( order, - selectedRevisions, + this.getSelectedRevisions(selectedRevs), this.templates, this.revisionData, this.getPlotSize(Section.TEMPLATE_PLOTS), - this.getRevisionColors(), + this.getRevisionColors(selectedRevs), this.multiSourceEncoding ) } diff --git a/extension/src/plots/webview/messages.ts b/extension/src/plots/webview/messages.ts index 33bec78b0b..7d00ecb837 100644 --- a/extension/src/plots/webview/messages.ts +++ b/extension/src/plots/webview/messages.ts @@ -58,7 +58,7 @@ export class WebviewMessages { hasPlots: !!this.paths?.hasPaths(), hasSelectedPlots: definedAndNonEmpty(this.paths.getSelected()), sectionCollapsed: this.plots.getSectionCollapsed(), - selectedRevisions: this.plots.getSelectedRevisionDetails(), + selectedRevisions: this.plots.getSelectedRevisionDetails(selectedRevs), template: this.getTemplatePlots(selectedRevs) }) } @@ -72,11 +72,20 @@ export class WebviewMessages { case MessageFromWebviewType.TOGGLE_PLOTS_SECTION: return this.setSectionCollapsed(message.payload) case MessageFromWebviewType.REORDER_PLOTS_COMPARISON: - return this.setComparisonOrder(message.payload) + return this.setComparisonOrder( + this.experiments.getSelectedRevisions(), + message.payload + ) case MessageFromWebviewType.REORDER_PLOTS_COMPARISON_ROWS: - return this.setComparisonRowsOrder(message.payload) + return this.setComparisonRowsOrder( + this.experiments.getSelectedRevisions(), + message.payload + ) case MessageFromWebviewType.REORDER_PLOTS_TEMPLATES: - return this.setTemplateOrder(message.payload) + return this.setTemplateOrder( + this.experiments.getSelectedRevisions(), + message.payload + ) case MessageFromWebviewType.REORDER_PLOTS_METRICS: return this.setMetricOrder(message.payload) case MessageFromWebviewType.SELECT_PLOTS: @@ -96,7 +105,9 @@ export class WebviewMessages { private sendCheckpointPlotsMessage() { this.getWebview()?.show({ - checkpoint: this.getCheckpointPlots() + checkpoint: this.getCheckpointPlots( + this.experiments.getSelectedExperiments() + ) }) } @@ -124,9 +135,12 @@ export class WebviewMessages { ) } - private setComparisonOrder(order: string[]) { - this.plots.setComparisonOrder(order) - this.sendComparisonPlots() + private setComparisonOrder( + selectedRevs: SelectedExperimentWithColor[], + order: string[] + ) { + this.plots.setComparisonOrder(selectedRevs, order) + this.sendComparisonPlots(selectedRevs) sendTelemetryEvent( EventName.VIEWS_PLOTS_REVISIONS_REORDERED, undefined, @@ -134,9 +148,12 @@ export class WebviewMessages { ) } - private setComparisonRowsOrder(order: string[]) { + private setComparisonRowsOrder( + selectedRevs: SelectedExperimentWithColor[], + order: string[] + ) { this.paths.setComparisonPathsOrder(order) - this.sendComparisonPlots() + this.sendComparisonPlots(selectedRevs) sendTelemetryEvent( EventName.VIEWS_PLOTS_COMPARISON_ROWS_REORDERED, undefined, @@ -144,9 +161,12 @@ export class WebviewMessages { ) } - private setTemplateOrder(order: PlotsTemplatesReordered) { + private setTemplateOrder( + selectedRevs: SelectedExperimentWithColor[], + order: PlotsTemplatesReordered + ) { this.paths.setTemplateOrder(order) - this.sendTemplatePlots() + this.sendTemplatePlots(selectedRevs) sendTelemetryEvent( EventName.VIEWS_REORDER_PLOTS_TEMPLATES, undefined, @@ -221,22 +241,22 @@ export class WebviewMessages { }) } - private sendComparisonPlots() { + private sendComparisonPlots(selectedRevs: SelectedExperimentWithColor[]) { this.getWebview()?.show({ - comparison: this.getComparisonPlots(), - selectedRevisions: this.plots?.getSelectedRevisionDetails() + comparison: this.getComparisonPlots(selectedRevs), + selectedRevisions: this.plots?.getSelectedRevisionDetails(selectedRevs) }) } - private sendTemplatePlots() { + private sendTemplatePlots(selectedRevs: SelectedExperimentWithColor[]) { this.getWebview()?.show({ - template: this.getTemplatePlots() + template: this.getTemplatePlots(selectedRevs) }) } - private getTemplatePlots(overrideRevs?: SelectedExperimentWithColor[]) { + private getTemplatePlots(selectedRevs: SelectedExperimentWithColor[]) { const paths = this.paths?.getTemplateOrder() - const plots = this.plots?.getTemplatePlots(paths, overrideRevs) + const plots = this.plots?.getTemplatePlots(paths, selectedRevs) if (!this.plots || !plots || isEmpty(plots)) { return null @@ -248,9 +268,9 @@ export class WebviewMessages { } } - private getComparisonPlots(overrideRevs?: SelectedExperimentWithColor[]) { + private getComparisonPlots(selectedRevs: SelectedExperimentWithColor[]) { const paths = this.paths.getComparisonPaths() - const comparison = this.plots.getComparisonPlots(paths, overrideRevs) + const comparison = this.plots.getComparisonPlots(paths, selectedRevs) if (!this.plots || !comparison || isEmpty(comparison)) { return null } @@ -288,7 +308,9 @@ export class WebviewMessages { } } - private getCheckpointPlots(overrideRevs?: SelectedExperimentWithColor[]) { - return this.plots?.getCheckpointPlots(overrideRevs) || null + private getCheckpointPlots( + selectedExperiments: SelectedExperimentWithColor[] + ) { + return this.plots?.getCheckpointPlots(selectedExperiments) || null } } diff --git a/extension/src/test/suite/experiments/model/tree.test.ts b/extension/src/test/suite/experiments/model/tree.test.ts index fa927dd4af..6ac002173e 100644 --- a/extension/src/test/suite/experiments/model/tree.test.ts +++ b/extension/src/test/suite/experiments/model/tree.test.ts @@ -294,7 +294,9 @@ suite('Experiments Tree Test Suite', () => { await buildPlots(disposable, plotsDiffFixture) await plots.showWebview() - const initiallySelectedRevisions = plotsModel.getSelectedRevisionDetails() + const initiallySelectedRevisions = plotsModel.getSelectedRevisionDetails( + experiments.getSelectedRevisions() + ) const setSelectionModeSpy = spy( ExperimentsModel.prototype, @@ -345,7 +347,9 @@ suite('Experiments Tree Test Suite', () => { 'auto-apply filters to experiment selection is not enabled when the user selects to use the most recent' ).to.be.false expect( - plotsModel.getSelectedRevisionDetails(), + plotsModel.getSelectedRevisionDetails( + experiments.getSelectedRevisions() + ), 'all running and the most recent experiments are now selected' ).to.deep.equal([ { diff --git a/extension/src/test/suite/plots/data/index.test.ts b/extension/src/test/suite/plots/data/index.test.ts index f50fbdce40..2e7567197d 100644 --- a/extension/src/test/suite/plots/data/index.test.ts +++ b/extension/src/test/suite/plots/data/index.test.ts @@ -6,6 +6,7 @@ import { PlotsData } from '../../../../plots/data' import { PlotsModel } from '../../../../plots/model' import { dvcDemoPath } from '../../../util' import { buildDependencies } from '../../util' +import { Experiments } from '../../../../experiments' suite('Plots Data Test Suite', () => { const disposable = Disposable.fn() @@ -31,17 +32,25 @@ suite('Plots Data Test Suite', () => { const mockGetMissingRevisions = stub().returns(missingRevisions) const mockGetMutableRevisions = stub().returns(mutableRevisions) + const mockGetSelectedRevisions = stub().returns( + missingRevisions.map(rev => ({ label: rev })) + ) const mockPlotsModel = { - getMissingRevisions: mockGetMissingRevisions, - getMutableRevisions: mockGetMutableRevisions + getMissingRevisions: mockGetMissingRevisions } as unknown as PlotsModel + const mockExperiments = { + getMutableRevisions: mockGetMutableRevisions, + getSelectedRevisions: mockGetSelectedRevisions + } as unknown as Experiments + const data = disposable.track( new PlotsData( dvcDemoPath, internalCommands, mockPlotsModel, + mockExperiments, updatesPaused ) ) diff --git a/extension/src/test/suite/plots/index.test.ts b/extension/src/test/suite/plots/index.test.ts index ca28cc919c..fb1a84bca5 100644 --- a/extension/src/test/suite/plots/index.test.ts +++ b/extension/src/test/suite/plots/index.test.ts @@ -244,7 +244,14 @@ suite('Plots Test Suite', () => { ) const plotsResentEvent = new Promise(resolve => mockSendPlots.callsFake(() => { - if (isEqual(plotsModel.getMissingRevisions(), ['9235a02'])) { + if ( + isEqual( + plotsModel.getMissingRevisions( + experiments.getSelectedRevisions() + ), + ['9235a02'] + ) + ) { resolve(undefined) } }) @@ -413,7 +420,7 @@ suite('Plots Test Suite', () => { }).timeout(WEBVIEW_TEST_TIMEOUT) it('should handle a comparison revisions reordered message from the webview', async () => { - const { plots, plotsModel, messageSpy } = await buildPlots( + const { experiments, plots, plotsModel, messageSpy } = await buildPlots( disposable, plotsDiffFixture ) @@ -440,6 +447,7 @@ suite('Plots Test Suite', () => { expect(mockSetComparisonOrder).to.be.calledOnce expect(mockSetComparisonOrder).to.be.calledWithExactly( + experiments.getSelectedRevisions(), mockComparisonOrder ) expect(messageSpy).to.be.calledOnce From 9fc1ecf373d7cbeef1b8f33c3fe5f8072df30c9d Mon Sep 17 00:00:00 2001 From: Matt Seddon Date: Mon, 28 Nov 2022 10:07:05 +1100 Subject: [PATCH 3/7] fix class members --- extension/src/plots/data/index.ts | 20 ++++++++++++-------- extension/src/plots/index.ts | 13 +++---------- 2 files changed, 15 insertions(+), 18 deletions(-) diff --git a/extension/src/plots/data/index.ts b/extension/src/plots/data/index.ts index f7d3cd4b7d..8af2eb5ad5 100644 --- a/extension/src/plots/data/index.ts +++ b/extension/src/plots/data/index.ts @@ -11,8 +11,8 @@ import { import { PlotsModel } from '../model' export class PlotsData extends BaseData<{ data: PlotsOutput; revs: string[] }> { - private plots: PlotsModel - private experiments: Experiments + private readonly plots: PlotsModel + private readonly experiments: Experiments constructor( dvcRoot: string, @@ -54,17 +54,13 @@ export class PlotsData extends BaseData<{ data: PlotsOutput; revs: string[] }> { } const args = this.getArgs(revs) - const data = await this.internalCommands.executeCommand( - AvailableCommands.PLOTS_DIFF, - this.dvcRoot, - ...args - ) + const data = await this.fetch(args) const files = this.collectFiles({ data }) this.compareFiles(files) - return this.notifyChanged({ data, revs }) + this.notifyChanged({ data, revs }) } public managedUpdate() { @@ -75,6 +71,14 @@ export class PlotsData extends BaseData<{ data: PlotsOutput; revs: string[] }> { return Object.keys(data) } + public fetch(revs: string[]) { + return this.internalCommands.executeCommand( + AvailableCommands.PLOTS_DIFF, + this.dvcRoot, + ...revs + ) + } + private getArgs(revs: string[]) { const cliWillThrowError = sameContents(revs, ['workspace']) if (this.plots && cliWillThrowError) { diff --git a/extension/src/plots/index.ts b/extension/src/plots/index.ts index c59df0973d..d4677841da 100644 --- a/extension/src/plots/index.ts +++ b/extension/src/plots/index.ts @@ -11,9 +11,9 @@ import { ViewKey } from '../webview/constants' import { BaseRepository } from '../webview/repository' import { Experiments } from '../experiments' import { Resource } from '../resourceLocator' -import { AvailableCommands, InternalCommands } from '../commands/internal' +import { InternalCommands } from '../commands/internal' import { definedAndNonEmpty } from '../util/array' -import { ExperimentsOutput, PlotsOutput } from '../cli/dvc/contract' +import { ExperimentsOutput } from '../cli/dvc/contract' import { TEMP_PLOTS_DIR } from '../cli/dvc/constants' import { removeDir } from '../fileSystem' import { Toast } from '../vscode/toast' @@ -29,7 +29,6 @@ export class Plots extends BaseRepository { private readonly pathsChanged = this.dispose.track(new EventEmitter()) - private readonly internalCommands: InternalCommands private readonly experiments: Experiments private readonly plots: PlotsModel private readonly paths: PathsModel @@ -49,8 +48,6 @@ export class Plots extends BaseRepository { this.experiments = experiments - this.internalCommands = internalCommands - this.plots = this.dispose.track( new PlotsModel(this.dvcRoot, workspaceState) ) @@ -157,11 +154,7 @@ export class Plots extends BaseRepository { const missingRevs = this.plots.getMissingRevisions(selectedRevs) if (this.paths.hasPaths() && definedAndNonEmpty(missingRevs)) { - const data = await this.internalCommands.executeCommand( - AvailableCommands.PLOTS_DIFF, - this.dvcRoot, - ...missingRevs - ) + const data = await this.data.fetch(missingRevs) await Promise.all([ this.plots.transformAndSetPlots( From ea733d0ba0e5230a05e87df3a71599474eb97dc0 Mon Sep 17 00:00:00 2001 From: Matt Seddon Date: Mon, 28 Nov 2022 10:09:05 +1100 Subject: [PATCH 4/7] use static select revisions for running experiments --- extension/src/plots/index.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/extension/src/plots/index.ts b/extension/src/plots/index.ts index d4677841da..2f77e12b8b 100644 --- a/extension/src/plots/index.ts +++ b/extension/src/plots/index.ts @@ -157,11 +157,7 @@ export class Plots extends BaseRepository { const data = await this.data.fetch(missingRevs) await Promise.all([ - this.plots.transformAndSetPlots( - data, - missingRevs, - this.experiments.getSelectedRevisions() - ), + this.plots.transformAndSetPlots(data, missingRevs, selectedRevs), this.paths.transformAndSet(data) ]) } From 165a82708ac6b12f23320f6b00cc649971c99d18 Mon Sep 17 00:00:00 2001 From: Matt Seddon Date: Mon, 28 Nov 2022 10:26:23 +1100 Subject: [PATCH 5/7] simplify test by removing mock --- extension/src/plots/model/index.test.ts | 36 +++++++++---------------- 1 file changed, 12 insertions(+), 24 deletions(-) diff --git a/extension/src/plots/model/index.test.ts b/extension/src/plots/model/index.test.ts index 27a70da5b2..c560fd75bb 100644 --- a/extension/src/plots/model/index.test.ts +++ b/extension/src/plots/model/index.test.ts @@ -7,6 +7,7 @@ import { } from '../webview/contract' import { buildMockMemento } from '../../test/util' import { PersistenceKey } from '../../persistence/constants' +import { SelectedExperimentWithColor } from '../../experiments/model' const mockedRevisions = [ { displayColor: 'white', label: 'workspace' }, @@ -14,7 +15,7 @@ const mockedRevisions = [ { displayColor: 'blue', label: '71f31cf' }, { displayColor: 'black', label: 'e93c7e6' }, { displayColor: 'brown', label: 'ffbe811' } -] +] as unknown as SelectedExperimentWithColor[] describe('plotsModel', () => { let model: PlotsModel @@ -25,7 +26,6 @@ describe('plotsModel', () => { persistedSelectedMetrics, [PersistenceKey.PLOT_SIZES + exampleDvcRoot]: DEFAULT_SECTION_SIZES }) - const mockedGetSelectedRevisions = jest.fn() beforeEach(() => { model = new PlotsModel(exampleDvcRoot, memento) @@ -104,11 +104,9 @@ describe('plotsModel', () => { }) it('should reorder comparison revisions after receiving a message to reorder', () => { - mockedGetSelectedRevisions.mockReturnValue(mockedRevisions) - const mementoUpdateSpy = jest.spyOn(memento, 'update') const newOrder = ['71f31cf', 'e93c7e6', 'ffbe811', 'workspace', 'main'] - model.setComparisonOrder(mockedGetSelectedRevisions(), newOrder) + model.setComparisonOrder(mockedRevisions, newOrder) expect(mementoUpdateSpy).toHaveBeenCalledTimes(1) expect(mementoUpdateSpy).toHaveBeenCalledWith( @@ -118,21 +116,19 @@ describe('plotsModel', () => { expect( model - .getSelectedRevisionDetails(mockedGetSelectedRevisions()) + .getSelectedRevisionDetails(mockedRevisions) .map(({ revision }) => revision) ).toStrictEqual(newOrder) }) it('should always send new revisions to the end of the list', () => { - mockedGetSelectedRevisions.mockReturnValue(mockedRevisions) - const newOrder = ['71f31cf', 'e93c7e6'] - model.setComparisonOrder(mockedGetSelectedRevisions(), newOrder) + model.setComparisonOrder(mockedRevisions, newOrder) expect( model - .getSelectedRevisionDetails(mockedGetSelectedRevisions()) + .getSelectedRevisionDetails(mockedRevisions) .map(({ revision }) => revision) ).toStrictEqual([ ...newOrder, @@ -147,36 +143,28 @@ describe('plotsModel', () => { const revisionDropped = allRevisions.filter(({ label }) => label !== 'main') const revisionReAdded = allRevisions - mockedGetSelectedRevisions - .mockReturnValueOnce(allRevisions) - .mockReturnValueOnce(allRevisions) - .mockReturnValueOnce(revisionDropped) - .mockReturnValueOnce(revisionDropped) - .mockReturnValueOnce(revisionReAdded) - .mockReturnValueOnce(revisionReAdded) - const initialOrder = ['workspace', 'main', '71f31cf'] - model.setComparisonOrder(mockedGetSelectedRevisions(), initialOrder) + model.setComparisonOrder(allRevisions, initialOrder) expect( model - .getSelectedRevisionDetails(mockedGetSelectedRevisions()) + .getSelectedRevisionDetails(allRevisions) .map(({ revision }) => revision) ).toStrictEqual(initialOrder) - model.setComparisonOrder(mockedGetSelectedRevisions()) + model.setComparisonOrder(revisionDropped) expect( model - .getSelectedRevisionDetails(mockedGetSelectedRevisions()) + .getSelectedRevisionDetails(revisionDropped) .map(({ revision }) => revision) ).toStrictEqual(initialOrder.filter(revision => revision !== 'main')) - model.setComparisonOrder(mockedGetSelectedRevisions()) + model.setComparisonOrder(revisionReAdded) expect( model - .getSelectedRevisionDetails(mockedGetSelectedRevisions()) + .getSelectedRevisionDetails(revisionReAdded) .map(({ revision }) => revision) ).toStrictEqual(['workspace', '71f31cf', 'main']) }) From a5ee9d96b8b438f3fc1f1fa9021a5294d08cbe90 Mon Sep 17 00:00:00 2001 From: Matt Seddon Date: Mon, 28 Nov 2022 11:16:23 +1100 Subject: [PATCH 6/7] remove linter comment --- extension/src/plots/index.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/extension/src/plots/index.ts b/extension/src/plots/index.ts index 2f77e12b8b..51bff3f2b5 100644 --- a/extension/src/plots/index.ts +++ b/extension/src/plots/index.ts @@ -142,7 +142,6 @@ export class Plots extends BaseRepository { this.fetchMissingAndSendPlots() } - // eslint-disable-next-line sonarjs/cognitive-complexity private async fetchMissingAndSendPlots( overrideRevs?: SelectedExperimentWithColor[] ) { From 44aed9448a252bb398119cc05e0a0260361d8ee6 Mon Sep 17 00:00:00 2001 From: Matt Seddon Date: Mon, 28 Nov 2022 12:13:08 +1100 Subject: [PATCH 7/7] sync webview after data has been sent --- extension/src/plots/index.ts | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/extension/src/plots/index.ts b/extension/src/plots/index.ts index 51bff3f2b5..1c5488f971 100644 --- a/extension/src/plots/index.ts +++ b/extension/src/plots/index.ts @@ -12,7 +12,7 @@ import { BaseRepository } from '../webview/repository' import { Experiments } from '../experiments' import { Resource } from '../resourceLocator' import { InternalCommands } from '../commands/internal' -import { definedAndNonEmpty } from '../util/array' +import { definedAndNonEmpty, sameContents } from '../util/array' import { ExperimentsOutput } from '../cli/dvc/contract' import { TEMP_PLOTS_DIR } from '../cli/dvc/constants' import { removeDir } from '../fileSystem' @@ -161,10 +161,20 @@ export class Plots extends BaseRepository { ]) } - return this.webviewMessages.sendWebviewMessage( - selectedRevs, - selectedExperiments - ) + this.webviewMessages.sendWebviewMessage(selectedRevs, selectedExperiments) + return this.syncWebview(selectedRevs) + } + + private syncWebview(selectedRevs: SelectedExperimentWithColor[]) { + if ( + sameContents( + this.experiments.getSelectedRevisions().map(({ label }) => label), + selectedRevs.map(({ label }) => label) + ) + ) { + return + } + this.fetchMissingAndSendPlots() } private createWebviewMessageHandler(