diff --git a/extension/src/experiments/index.ts b/extension/src/experiments/index.ts index d209f26f3b..2ebeaaf8ef 100644 --- a/extension/src/experiments/index.ts +++ b/extension/src/experiments/index.ts @@ -453,12 +453,6 @@ export class Experiments extends BaseRepository { return experiment?.name || experiment?.label } - public getMutableRevisions() { - return this.experiments.getMutableRevisions( - this.checkpoints.hasCheckpoints() - ) - } - public getRevisions() { return this.experiments.getRevisions() } diff --git a/extension/src/experiments/model/collect.test.ts b/extension/src/experiments/model/collect.test.ts index efe1668257..dc59035124 100644 --- a/extension/src/experiments/model/collect.test.ts +++ b/extension/src/experiments/model/collect.test.ts @@ -1,10 +1,7 @@ -import { collectExperiments, collectMutableRevisions } from './collect' +import { collectExperiments } from './collect' import { Experiment } from '../webview/contract' import modifiedFixture from '../../test/fixtures/expShow/modified/output' -import { - ExperimentStatus, - EXPERIMENT_WORKSPACE_ID -} from '../../cli/dvc/contract' +import { EXPERIMENT_WORKSPACE_ID } from '../../cli/dvc/contract' import { COMMITS_SEPARATOR } from '../../cli/git/constants' describe('collectExperiments', () => { @@ -233,62 +230,3 @@ describe('collectExperiments', () => { ).toStrictEqual(3) }) }) - -describe('collectMutableRevisions', () => { - const baseExperiments = [ - { label: 'branch-A', selected: false, status: ExperimentStatus.SUCCESS }, - { - label: EXPERIMENT_WORKSPACE_ID, - selected: false, - status: ExperimentStatus.FAILED - } - ] as Experiment[] - - it('should return the workspace when there is an unselected running checkpoint experiment', () => { - const experiments = [ - { - label: 'exp-123', - selected: false, - status: ExperimentStatus.RUNNING - }, - ...baseExperiments - ] as Experiment[] - - const mutableRevisions = collectMutableRevisions(experiments, true) - expect(mutableRevisions).toStrictEqual([EXPERIMENT_WORKSPACE_ID]) - }) - - it('should return the workspace when there are no checkpoints', () => { - const experiments = [ - { label: 'branch-A', selected: false, status: ExperimentStatus.SUCCESS }, - { - label: EXPERIMENT_WORKSPACE_ID, - selected: false, - status: ExperimentStatus.SUCCESS - } - ] as Experiment[] - - const mutableRevisions = collectMutableRevisions(experiments, false) - expect(mutableRevisions).toStrictEqual([EXPERIMENT_WORKSPACE_ID]) - }) - - it('should return all running experiments when there are checkpoints', () => { - const experiments = [ - { label: 'branch-A', selected: false, status: ExperimentStatus.SUCCESS }, - { - label: EXPERIMENT_WORKSPACE_ID, - selected: false, - status: ExperimentStatus.SUCCESS - }, - { label: 'running-1', selected: false, status: ExperimentStatus.RUNNING }, - { label: 'running-2', selected: true, status: ExperimentStatus.RUNNING } - ] as Experiment[] - - const mutableRevisions = collectMutableRevisions(experiments, false) - expect(mutableRevisions).toStrictEqual([ - EXPERIMENT_WORKSPACE_ID, - 'running-1', - 'running-2' - ]) - }) -}) diff --git a/extension/src/experiments/model/collect.ts b/extension/src/experiments/model/collect.ts index a250cb9df3..f0fd24d05e 100644 --- a/extension/src/experiments/model/collect.ts +++ b/extension/src/experiments/model/collect.ts @@ -23,7 +23,6 @@ import { ExperimentStatus } from '../../cli/dvc/contract' import { addToMapArray } from '../../util/map' -import { uniqueValues } from '../../util/array' import { RegisteredCommands } from '../../commands/external' import { Resource } from '../../resourceLocator' import { shortenForLabel } from '../../util/string' @@ -408,31 +407,6 @@ export const collectExperiments = ( return acc } -const getDefaultMutableRevision = (): string[] => [EXPERIMENT_WORKSPACE_ID] - -const collectMutableRevision = ( - acc: string[], - { label, status }: Experiment, - hasCheckpoints: boolean -) => { - if (isRunning(status) && !hasCheckpoints) { - acc.push(label) - } -} - -export const collectMutableRevisions = ( - experiments: Experiment[], - hasCheckpoints: boolean -): string[] => { - const acc = getDefaultMutableRevision() - - for (const experiment of experiments) { - collectMutableRevision(acc, experiment, hasCheckpoints) - } - - return uniqueValues(acc) -} - type DeletableExperimentAccumulator = { [dvcRoot: string]: Set } const initializeAccumulatorRoot = ( diff --git a/extension/src/experiments/model/index.ts b/extension/src/experiments/model/index.ts index 61c955ae55..39d4d1b6c2 100644 --- a/extension/src/experiments/model/index.ts +++ b/extension/src/experiments/model/index.ts @@ -6,7 +6,7 @@ import { splitExperimentsByFilters, getFilterId } from './filterBy' -import { collectExperiments, collectMutableRevisions } from './collect' +import { collectExperiments } from './collect' import { collectFiltered, collectFilteredCounts, @@ -264,13 +264,6 @@ export class ExperimentsModel extends ModelWithPersistence { return this.getCombinedList().map(({ label }) => label) } - public getMutableRevisions(hasCheckpoints: boolean) { - return collectMutableRevisions( - this.getRecordsWithoutCheckpoints(), - hasCheckpoints - ) - } - public getSelectedRevisions() { return this.getSelectedFromList(() => this.getCombinedList()) } diff --git a/extension/src/plots/data/collect.ts b/extension/src/plots/data/collect.ts index aac95ede34..c0f2485486 100644 --- a/extension/src/plots/data/collect.ts +++ b/extension/src/plots/data/collect.ts @@ -1,8 +1,4 @@ -import { - EXPERIMENT_WORKSPACE_ID, - ExperimentsOutput, - PlotsOutputOrError -} from '../../cli/dvc/contract' +import { ExperimentsOutput, PlotsOutputOrError } from '../../cli/dvc/contract' import { isDvcError } from '../../cli/dvc/reader' import { sortCollectedArray, uniqueValues } from '../../util/array' import { isImagePlot, Plot, TemplatePlot } from '../webview/contract' @@ -76,26 +72,3 @@ export const collectMetricsFiles = ( return sortCollectedArray(metricsFiles) } - -const collectRev = (acc: string[], rev: string): void => { - if (rev !== EXPERIMENT_WORKSPACE_ID && !acc.includes(rev)) { - acc.push(rev) - } -} - -export const collectRevs = ( - missingRevisions: string[], - mutableRevisions: string[] -): string[] => { - const acc: string[] = [] - - for (const missingRevision of missingRevisions) { - collectRev(acc, missingRevision) - } - - for (const mutableRevision of mutableRevisions) { - collectRev(acc, mutableRevision) - } - - return [EXPERIMENT_WORKSPACE_ID, ...sortCollectedArray(acc)] -} diff --git a/extension/src/plots/data/index.ts b/extension/src/plots/data/index.ts index 5614b117ce..6e16679cd8 100644 --- a/extension/src/plots/data/index.ts +++ b/extension/src/plots/data/index.ts @@ -1,5 +1,5 @@ -import { EventEmitter } from 'vscode' -import { collectMetricsFiles, collectFiles, collectRevs } from './collect' +import { Event, EventEmitter } from 'vscode' +import { collectMetricsFiles, collectFiles } from './collect' import { EXPERIMENT_WORKSPACE_ID, ExperimentsOutput, @@ -14,10 +14,16 @@ export class PlotsData extends BaseData<{ data: PlotsOutputOrError revs: string[] }> { + public readonly onDidTrigger: Event + private readonly model: PlotsModel private metricFiles: string[] = [] + private readonly triggered: EventEmitter = this.dispose.track( + new EventEmitter() + ) + constructor( dvcRoot: string, internalCommands: InternalCommands, @@ -37,13 +43,12 @@ export class PlotsData extends BaseData<{ ['dvc.yaml', 'dvc.lock'] ) this.model = model + this.onDidTrigger = this.triggered.event } public async update(): Promise { - const revs = collectRevs( - this.model.getMissingRevisions(), - this.model.getMutableRevisions() - ) + this.notifyTriggered() + const revs = this.model.getSelectedOrderedCliIds() const args = this.getArgs(revs) const data = await this.internalCommands.executeCommand( @@ -76,6 +81,10 @@ export class PlotsData extends BaseData<{ this.collectedFiles = collectFiles(data, this.collectedFiles) } + private notifyTriggered() { + this.triggered.fire() + } + private getArgs(revs: string[]) { const cliWillThrowError = sameContents(revs, [EXPERIMENT_WORKSPACE_ID]) if (this.model && cliWillThrowError) { diff --git a/extension/src/plots/index.ts b/extension/src/plots/index.ts index 71c48e058b..198f218bbf 100644 --- a/extension/src/plots/index.ts +++ b/extension/src/plots/index.ts @@ -14,7 +14,6 @@ import { BaseRepository } from '../webview/repository' import { Experiments } from '../experiments' import { Resource } from '../resourceLocator' import { InternalCommands } from '../commands/internal' -import { definedAndNonEmpty } from '../util/array' import { TEMP_PLOTS_DIR } from '../cli/dvc/constants' import { removeDir } from '../fileSystem' import { Toast } from '../vscode/toast' @@ -69,6 +68,7 @@ export class Plots extends BaseRepository { new PlotsData(dvcRoot, internalCommands, this.plots, updatesPaused) ) + this.onDidTriggerDataUpdate() this.onDidUpdateData() this.waitForInitialData(experiments) @@ -105,10 +105,7 @@ export class Plots extends BaseRepository { void Toast.infoWithOptions( 'Attempting to refresh plots for selected experiments.' ) - for (const { revision } of this.plots.getSelectedRevisionDetails()) { - this.plots.setupManualRefresh(revision) - } - void this.data.managedUpdate() + this.triggerDataUpdate() } public getChildPaths(path: string | undefined) { @@ -130,7 +127,7 @@ export class Plots extends BaseRepository { } protected sendInitialWebviewData() { - return this.fetchMissingOrSendPlots() + return this.sendPlots() } private notifyChanged() { @@ -140,19 +137,18 @@ export class Plots extends BaseRepository { this.errors.getErrorPaths(selectedRevisions) ) this.pathsChanged.fire() - void this.fetchMissingOrSendPlots() + + if (this.plots.requiresUpdate()) { + this.triggerDataUpdate() + return + } + + void this.sendPlots() } - private async fetchMissingOrSendPlots() { + private async sendPlots() { await this.isReady() - if ( - this.paths.hasPaths() && - definedAndNonEmpty(this.plots.getUnfetchedRevisions()) - ) { - void this.data.managedUpdate() - } - return this.webviewMessages.sendWebviewMessage() } @@ -214,7 +210,7 @@ export class Plots extends BaseRepository { private async initializeData() { await this.plots.transformAndSetExperiments() - void this.data.managedUpdate() + this.triggerDataUpdate() await Promise.all([ this.data.isReady(), this.plots.isReady(), @@ -223,6 +219,19 @@ export class Plots extends BaseRepository { this.deferred.resolve() } + private triggerDataUpdate() { + void this.data.managedUpdate() + } + + private onDidTriggerDataUpdate() { + const sendCachedDataToWebview = () => { + this.plots.resetFetched() + this.plots.setComparisonOrder() + return this.sendPlots() + } + this.dispose.track(this.data.onDidTrigger(() => sendCachedDataToWebview())) + } + private onDidUpdateData() { this.dispose.track( this.data.onDidUpdate(async ({ data, revs }) => { diff --git a/extension/src/plots/model/collect.test.ts b/extension/src/plots/model/collect.test.ts index dc145aa5ff..416e0067f0 100644 --- a/extension/src/plots/model/collect.test.ts +++ b/extension/src/plots/model/collect.test.ts @@ -4,7 +4,8 @@ import { collectData, collectTemplates, collectOverrideRevisionDetails, - collectCustomPlots + collectCustomPlots, + collectOrderedRevisions } from './collect' import { isCheckpointPlot } from './custom' import plotsDiffFixture from '../../test/fixtures/plotsDiff/output' @@ -525,3 +526,56 @@ describe('collectOverrideRevisionDetails', () => { ]) }) }) + +describe('collectOrderedRevisions', () => { + it('should return the expected value from the test fixture', () => { + const main = { Created: '2020-11-21T19:58:22', id: 'main', label: 'main' } + const workspace = { + id: EXPERIMENT_WORKSPACE_ID, + label: EXPERIMENT_WORKSPACE_ID + } + const _4fb124a = { + Created: '2020-12-29T15:31:52', + id: 'exp-e7a67', + label: '4fb124a' + } + const _42b8736 = { + Created: '2020-12-29T15:28:59', + id: 'test-branch', + label: '42b8736' + } + const _1ba7bcd = { + Created: '2020-12-29T15:27:02', + id: 'exp-83425', + label: '1ba7bcd' + } + const orderedRevisions = collectOrderedRevisions([ + _1ba7bcd, + _42b8736, + _4fb124a, + main, + workspace + ]) + expect(orderedRevisions).toStrictEqual([ + workspace, + _4fb124a, + _42b8736, + _1ba7bcd, + main + ]) + }) + + it('should order the provided revisions by workspace and then Created', () => { + const a = { Created: '2023-03-23T16:27:20', id: 'a', label: 'a' } + const b = { Created: '2023-03-23T15:27:20', id: 'b', label: 'b' } + const c = { Created: '2023-03-23T12:10:13', id: 'c', label: 'c' } + const d = { Created: '2020-11-21T19:58:22', id: 'd', label: 'd' } + const workspace = { + id: EXPERIMENT_WORKSPACE_ID, + label: EXPERIMENT_WORKSPACE_ID + } + const orderedRevisions = collectOrderedRevisions([b, c, workspace, d, a]) + + expect(orderedRevisions).toStrictEqual([workspace, a, b, c, d]) + }) +}) diff --git a/extension/src/plots/model/collect.ts b/extension/src/plots/model/collect.ts index 9eca392b2f..62bd5eddea 100644 --- a/extension/src/plots/model/collect.ts +++ b/extension/src/plots/model/collect.ts @@ -724,3 +724,21 @@ export const collectOverrideRevisionDetails = ( overrideRevisions: selectedWithOverrides } } + +export const collectOrderedRevisions = ( + revisions: { + id: string + label: string + Created?: string | null + }[] +): { id: string; label: string; Created?: string | null }[] => { + return [...revisions].sort((a, b) => { + if (a.id === 'workspace') { + return -1 + } + if (b.id === 'workspace') { + return 1 + } + return (b.Created || '').localeCompare(a.Created || '') + }) +} diff --git a/extension/src/plots/model/index.ts b/extension/src/plots/model/index.ts index 76c992e29a..8c72816527 100644 --- a/extension/src/plots/model/index.ts +++ b/extension/src/plots/model/index.ts @@ -11,7 +11,8 @@ import { collectCommitRevisionDetails, collectOverrideRevisionDetails, collectCustomPlots, - getCustomPlotId + getCustomPlotId, + collectOrderedRevisions } from './collect' import { getRevisionFirstThreeColumns } from './util' import { @@ -40,7 +41,11 @@ import { } from '../../cli/dvc/contract' import { Experiments } from '../../experiments' import { getColorScale } from '../vega/util' -import { definedAndNonEmpty, reorderObjectList } from '../../util/array' +import { + definedAndNonEmpty, + reorderObjectList, + sameContents +} from '../../util/array' import { removeMissingKeysFromObject } from '../../util/object' import { TemplateOrder } from '../paths/collect' import { PersistenceKey } from '../../persistence/constants' @@ -137,10 +142,7 @@ export class PlotsModel extends ModelWithPersistence { this.setComparisonOrder() - this.fetchedRevs = new Set([ - ...this.fetchedRevs, - ...revs.map(rev => cliIdToLabel[rev]) - ]) + this.fetchedRevs = new Set(revs.map(rev => cliIdToLabel[rev])) this.experiments.setRevisionCollected(revs) @@ -217,10 +219,6 @@ export class PlotsModel extends ModelWithPersistence { this.setCustomPlotsOrder(newCustomPlotsOrder) } - public setupManualRefresh(id: string) { - this.deleteRevisionData(id) - } - public getOverrideRevisionDetails() { const finishedExperiments = this.experiments.getFinishedExperiments() @@ -245,27 +243,6 @@ export class PlotsModel extends ModelWithPersistence { } } - public getUnfetchedRevisions() { - return this.getSelectedRevisions().filter( - revision => !this.fetchedRevs.has(revision) - ) - } - - public getMissingRevisions() { - const cachedRevisions = new Set([ - ...Object.keys(this.comparisonData), - ...Object.keys(this.revisionData) - ]) - - return this.getSelectedRevisions() - .filter(label => !cachedRevisions.has(label)) - .map(label => this.getCLIId(label)) - } - - public getMutableRevisions() { - return this.experiments.getMutableRevisions() - } - public getRevisionColors(overrideRevs?: Revision[]) { return getColorScale(overrideRevs || this.getSelectedRevisionDetails()) } @@ -332,6 +309,10 @@ export class PlotsModel extends ModelWithPersistence { return this.getSelectedComparisonPlots(paths, selectedRevisions) } + public requiresUpdate() { + return !sameContents([...this.fetchedRevs], this.getSelectedRevisions()) + } + public getComparisonRevisions() { return reorderObjectList( this.comparisonOrder, @@ -360,6 +341,12 @@ export class PlotsModel extends ModelWithPersistence { return this.experiments.getSelectedRevisions().map(({ label }) => label) } + public getSelectedOrderedCliIds() { + return collectOrderedRevisions(this.experiments.getSelectedRevisions()).map( + ({ label }) => this.getCLIId(label) + ) + } + public setNbItemsPerRowOrWidth(section: PlotsSection, nbItemsPerRow: number) { this.nbItemsPerRowOrWidth[section] = nbItemsPerRow this.persist( @@ -380,6 +367,10 @@ export class PlotsModel extends ModelWithPersistence { this.persist(PersistenceKey.PLOT_HEIGHT, this.height) } + public resetFetched() { + this.fetchedRevs = new Set() + } + public getHeight(section: PlotsSection) { return this.height[section] } @@ -442,12 +433,6 @@ export class PlotsModel extends ModelWithPersistence { revisions, this.revisionData ) - - for (const fetchedRev of this.fetchedRevs) { - if (!revisions.includes(fetchedRev)) { - this.fetchedRevs.delete(fetchedRev) - } - } } private removeStaleCommits() { diff --git a/extension/src/plots/paths/model.test.ts b/extension/src/plots/paths/model.test.ts index b1e4c6149d..48c26a4173 100644 --- a/extension/src/plots/paths/model.test.ts +++ b/extension/src/plots/paths/model.test.ts @@ -375,19 +375,19 @@ describe('PathsModel', () => { expect(rootChildren).toStrictEqual([ { - descendantStatuses: [2, 2, 2], + descendantStatuses: [2, 2], hasChildren: true, parentPath: undefined, - path: 'plots', + path: 'logs', revisions: new Set(revisions), status: 2, tooltip: undefined }, { - descendantStatuses: [2, 2], + descendantStatuses: [2, 2, 2], hasChildren: true, parentPath: undefined, - path: 'logs', + path: 'plots', revisions: new Set(revisions), status: 2, tooltip: undefined @@ -410,7 +410,7 @@ describe('PathsModel', () => { descendantStatuses: [], hasChildren: false, parentPath: 'logs', - path: logsLoss, + path: logsAcc, revisions: new Set(revisions), status: 2, tooltip: undefined, @@ -420,7 +420,7 @@ describe('PathsModel', () => { descendantStatuses: [], hasChildren: false, parentPath: 'logs', - path: logsAcc, + path: logsLoss, revisions: new Set(revisions), status: 2, tooltip: undefined, @@ -441,7 +441,7 @@ describe('PathsModel', () => { descendantStatuses: [], hasChildren: true, parentPath: 'logs', - path: logsLoss, + path: logsAcc, revisions: new Set(revisions), status: 2, tooltip: undefined, @@ -451,7 +451,7 @@ describe('PathsModel', () => { descendantStatuses: [], hasChildren: true, parentPath: 'logs', - path: logsAcc, + path: logsLoss, revisions: new Set(revisions), status: 2, tooltip: undefined, diff --git a/extension/src/plots/paths/model.ts b/extension/src/plots/paths/model.ts index 39d1576b0e..d76d415625 100644 --- a/extension/src/plots/paths/model.ts +++ b/extension/src/plots/paths/model.ts @@ -79,13 +79,15 @@ export class PathsModel extends PathSelectionModel { path: string | undefined, multiSourceEncoding: MultiSourceEncoding = {} ) { - return this.filterChildren(path).map(element => ({ - ...element, - descendantStatuses: this.getTerminalNodeStatuses(element.path), - hasChildren: this.getHasChildren(element, multiSourceEncoding), - status: this.status[element.path], - tooltip: this.getTooltip(element.path) - })) + return this.filterChildren(path) + .map(element => ({ + ...element, + descendantStatuses: this.getTerminalNodeStatuses(element.path), + hasChildren: this.getHasChildren(element, multiSourceEncoding), + status: this.status[element.path], + tooltip: this.getTooltip(element.path) + })) + .sort(({ path: aPath }, { path: bPath }) => aPath.localeCompare(bPath)) } public getTerminalNodes(): (PlotPath & { selected: boolean })[] { diff --git a/extension/src/plots/webview/messages.ts b/extension/src/plots/webview/messages.ts index 8301be7f55..9c47a789df 100644 --- a/extension/src/plots/webview/messages.ts +++ b/extension/src/plots/webview/messages.ts @@ -67,8 +67,10 @@ export class WebviewMessages { const { overrideComparison, overrideRevisions } = this.plots.getOverrideRevisionDetails() + const comparison = this.getComparisonPlots(overrideComparison) + void this.getWebview()?.show({ - comparison: this.getComparisonPlots(overrideComparison), + comparison, custom: this.getCustomPlots(), hasPlots: !!this.paths.hasPaths(), hasUnselectedPlots: this.paths.getHasUnselectedPlots(), @@ -328,7 +330,6 @@ export class WebviewMessages { void Toast.infoWithOptions( `Attempting to refresh plots data for ${revision}.` ) - this.plots.setupManualRefresh(revision) void this.updateData() sendTelemetryEvent( EventName.VIEWS_PLOTS_MANUAL_REFRESH, @@ -339,9 +340,6 @@ export class WebviewMessages { private attemptToRefreshSelectedData(revisions: string[]) { void Toast.infoWithOptions('Attempting to refresh visible plots data.') - for (const revision of revisions) { - this.plots.setupManualRefresh(revision) - } void this.updateData() sendTelemetryEvent( EventName.VIEWS_PLOTS_MANUAL_REFRESH, diff --git a/extension/src/test/suite/experiments/model/tree.test.ts b/extension/src/test/suite/experiments/model/tree.test.ts index 1f9b1692e6..aae8068e1d 100644 --- a/extension/src/test/suite/experiments/model/tree.test.ts +++ b/extension/src/test/suite/experiments/model/tree.test.ts @@ -13,7 +13,9 @@ import { import { ExperimentType } from '../../../../experiments/model' import { UNSELECTED } from '../../../../experiments/model/status' import { + bypassProcessManagerDebounce, getFirstArgOfLastCall, + getMockNow, getTimeSafeDisposer, spyOnPrivateMethod, stubPrivatePrototypeMethod @@ -70,6 +72,8 @@ suite('Experiments Tree Test Suite', () => { }) it('should be able to toggle whether an experiment is shown in the plots webview with dvc.views.experiments.toggleStatus', async () => { + const mockNow = getMockNow() + const { plots, messageSpy } = await buildPlots(disposable) const expectedDomain = [...domain] @@ -79,6 +83,7 @@ suite('Experiments Tree Test Suite', () => { await webview.isReady() + let updateCall = 1 while (expectedDomain.length > 0) { const expectedData = getExpectedCustomPlotsData( expectedDomain, @@ -96,6 +101,7 @@ suite('Experiments Tree Test Suite', () => { const id = expectedDomain.pop() expectedRange.pop() + bypassProcessManagerDebounce(mockNow, updateCall) const unSelected = await commands.executeCommand( RegisteredCommands.EXPERIMENT_TOGGLE, { @@ -103,6 +109,7 @@ suite('Experiments Tree Test Suite', () => { id } ) + updateCall = updateCall + 1 expect(unSelected).to.equal(UNSELECTED) } @@ -124,6 +131,7 @@ suite('Experiments Tree Test Suite', () => { expectedDomain.push(domain[0]) expectedRange.push(range[0]) + bypassProcessManagerDebounce(mockNow, updateCall) const selected = await commands.executeCommand( RegisteredCommands.EXPERIMENT_TOGGLE, { diff --git a/extension/src/test/suite/plots/data/index.test.ts b/extension/src/test/suite/plots/data/index.test.ts index c75a8deabe..afe6659f93 100644 --- a/extension/src/test/suite/plots/data/index.test.ts +++ b/extension/src/test/suite/plots/data/index.test.ts @@ -34,19 +34,14 @@ suite('Plots Data Test Suite', () => { return disposable.disposeAndFlush() }) - const buildPlotsData = ( - missingRevisions: string[] = [], - mutableRevisions: string[] = [] - ) => { + const buildPlotsData = (selectedRevisions: string[] = []) => { const { internalCommands, updatesPaused, mockPlotsDiff } = buildDependencies(disposable) - const mockGetMissingRevisions = stub().returns(missingRevisions) - const mockGetMutableRevisions = stub().returns(mutableRevisions) + const mockGetSelectedOrderedCliIds = stub().returns(selectedRevisions) const mockPlotsModel = { - getMissingRevisions: mockGetMissingRevisions, - getMutableRevisions: mockGetMutableRevisions + getSelectedOrderedCliIds: mockGetSelectedOrderedCliIds } as unknown as PlotsModel const data = disposable.track( @@ -66,7 +61,7 @@ suite('Plots Data Test Suite', () => { describe('PlotsData', () => { it('should call plots diff when there are no revisions to fetch and no experiment is running (workspace updates)', async () => { - const { data, mockPlotsDiff } = buildPlotsData([], []) + const { data, mockPlotsDiff } = buildPlotsData([]) await data.update() @@ -74,24 +69,8 @@ suite('Plots Data Test Suite', () => { expect(mockPlotsDiff).to.be.calledWithExactly(dvcDemoPath) }) - it('should always call plots diff with workspace as the first argument to get the correct template (caching)', async () => { - const { data, mockPlotsDiff } = buildPlotsData([], ['53c3851', '4fb124a']) - - await data.update() - - expect(mockPlotsDiff).to.be.calledWithExactly( - dvcDemoPath, - EXPERIMENT_WORKSPACE_ID, - '4fb124a', - '53c3851' - ) - }) - it('should call plots diff when an experiment is running in the workspace (live updates)', async () => { - const { data, mockPlotsDiff } = buildPlotsData( - [], - [EXPERIMENT_WORKSPACE_ID] - ) + const { data, mockPlotsDiff } = buildPlotsData([EXPERIMENT_WORKSPACE_ID]) await data.update() @@ -99,53 +78,31 @@ suite('Plots Data Test Suite', () => { }) it('should call plots diff when an experiment is running in a temporary directory (live updates)', async () => { - const { data, mockPlotsDiff } = buildPlotsData([], ['a7739b5']) + const { data, mockPlotsDiff } = buildPlotsData(['a7739b5']) await data.update() expect(mockPlotsDiff).to.be.calledOnce - expect(mockPlotsDiff).to.be.calledWithExactly( - dvcDemoPath, - EXPERIMENT_WORKSPACE_ID, - 'a7739b5' - ) + expect(mockPlotsDiff).to.be.calledWithExactly(dvcDemoPath, 'a7739b5') }) it('should call plots diff when an experiment is running and there are missing revisions (checkpoints)', async () => { - const { data, mockPlotsDiff } = buildPlotsData( - ['53c3851', '4fb124a', '42b8736', '1ba7bcd'], - [] - ) - - await data.update() - - expect(mockPlotsDiff).to.be.calledOnce - expect(mockPlotsDiff).to.be.calledWithExactly( - dvcDemoPath, - EXPERIMENT_WORKSPACE_ID, - '1ba7bcd', - '42b8736', + const { data, mockPlotsDiff } = buildPlotsData([ + '53c3851', '4fb124a', - '53c3851' - ) - }) - - it('should call plots diff when an experiment is running and there are missing revisions and one of them is mutable', async () => { - const { data, mockPlotsDiff } = buildPlotsData( - ['53c3851', '4fb124a', '42b8736', '1ba7bcd'], - ['1ba7bcd'] - ) + '42b8736', + '1ba7bcd' + ]) await data.update() expect(mockPlotsDiff).to.be.calledOnce expect(mockPlotsDiff).to.be.calledWithExactly( dvcDemoPath, - EXPERIMENT_WORKSPACE_ID, - '1ba7bcd', - '42b8736', + '53c3851', '4fb124a', - '53c3851' + '42b8736', + '1ba7bcd' ) }) @@ -198,8 +155,7 @@ suite('Plots Data Test Suite', () => { executeCommand: mockExecuteCommand } as unknown as InternalCommands, { - getMissingRevisions: () => [], - getMutableRevisions: () => [] + getSelectedOrderedCliIds: () => [] } as unknown as PlotsModel, disposable.track(new EventEmitter()) ) diff --git a/extension/src/test/suite/plots/index.test.ts b/extension/src/test/suite/plots/index.test.ts index 2f41e53fbf..7ba362cd1e 100644 --- a/extension/src/test/suite/plots/index.test.ts +++ b/extension/src/test/suite/plots/index.test.ts @@ -74,9 +74,9 @@ suite('Plots Test Suite', () => { expect(mockPlotsDiff).to.be.calledWithExactly( dvcDemoPath, EXPERIMENT_WORKSPACE_ID, - '1ba7bcd', - '42b8736', '4fb124a', + '42b8736', + '1ba7bcd', '53c3851' ) mockPlotsDiff.resetHistory() @@ -117,7 +117,8 @@ suite('Plots Test Suite', () => { checkpoint_tip: 'experiment', executor: 'dvc-task', name: 'exp-e1new', - status: ExperimentStatus.RUNNING + status: ExperimentStatus.RUNNING, + timestamp: '2023-03-23T09:02:20' } } } @@ -137,11 +138,15 @@ suite('Plots Test Suite', () => { expect(mockPlotsDiff).to.be.calledWithExactly( dvcDemoPath, EXPERIMENT_WORKSPACE_ID, - 'experim' + 'experim', + '4fb124a', + '42b8736', + '1ba7bcd', + '53c3851' ) }) - it('should call plots diff with the branch name (if available) whenever the current commit changes', async () => { + it('should call plots diff with the commit whenever the current commit changes', async () => { const mockNow = getMockNow() const { data, experiments, mockPlotsDiff } = await buildPlots( disposable, @@ -597,13 +602,11 @@ suite('Plots Test Suite', () => { }).timeout(WEBVIEW_TEST_TIMEOUT) it('should handle a message to manually refresh a revision from the webview', async () => { - const { data, plots, plotsModel, mockPlotsDiff } = await buildPlots( + const { data, plots, mockPlotsDiff } = await buildPlots( disposable, plotsDiffFixture ) - const removeDataSpy = spy(plotsModel, 'setupManualRefresh') - const webview = await plots.showWebview() mockPlotsDiff.resetHistory() @@ -621,7 +624,6 @@ suite('Plots Test Suite', () => { await dataUpdateEvent - expect(removeDataSpy).to.be.calledOnce expect(mockSendTelemetryEvent).to.be.calledOnce expect(mockSendTelemetryEvent).to.be.calledWithExactly( EventName.VIEWS_PLOTS_MANUAL_REFRESH, @@ -632,18 +634,24 @@ suite('Plots Test Suite', () => { expect(mockPlotsDiff).to.be.calledWithExactly( dvcDemoPath, EXPERIMENT_WORKSPACE_ID, + '4fb124a', + '42b8736', + '1ba7bcd', '53c3851' ) }).timeout(WEBVIEW_TEST_TIMEOUT) it('should handle a message to manually refresh all visible plots from the webview', async () => { - const { data, plots, mockPlotsDiff } = await buildPlots( + const { data, plots, mockPlotsDiff, messageSpy } = await buildPlots( disposable, plotsDiffFixture ) + messageSpy.restore() + const webview = await plots.showWebview() mockPlotsDiff.resetHistory() + const instanceMessageSpy = spy(webview, 'show') const mockSendTelemetryEvent = stub(Telemetry, 'sendTelemetryEvent') const mockMessageReceived = getMessageReceivedEmitter(webview) @@ -675,11 +683,15 @@ suite('Plots Test Suite', () => { expect(mockPlotsDiff).to.be.calledWithExactly( dvcDemoPath, EXPERIMENT_WORKSPACE_ID, - '1ba7bcd', - '42b8736', '4fb124a', + '42b8736', + '1ba7bcd', '53c3851' ) + expect( + instanceMessageSpy, + 'should call the plots webview with cached data before refreshing with data from the CLI' + ).to.be.calledTwice }).timeout(WEBVIEW_TEST_TIMEOUT) it('should be able to make the plots webview visible', async () => { diff --git a/extension/src/test/suite/plots/paths/tree.test.ts b/extension/src/test/suite/plots/paths/tree.test.ts index f1647b6f32..3e5bb6dfe9 100644 --- a/extension/src/test/suite/plots/paths/tree.test.ts +++ b/extension/src/test/suite/plots/paths/tree.test.ts @@ -150,9 +150,9 @@ suite('Plots Paths Tree Test Suite', () => { expect(mockPlotsDiff).to.be.calledWithExactly( dvcDemoPath, EXPERIMENT_WORKSPACE_ID, - '1ba7bcd', - '42b8736', '4fb124a', + '42b8736', + '1ba7bcd', '53c3851' ) }).timeout(WEBVIEW_TEST_TIMEOUT) diff --git a/webview/src/plots/components/LoadingSection.tsx b/webview/src/plots/components/LoadingSection.tsx index 521cd1df4f..a6221d2d9f 100644 --- a/webview/src/plots/components/LoadingSection.tsx +++ b/webview/src/plots/components/LoadingSection.tsx @@ -2,9 +2,13 @@ import { Revision } from 'dvc/src/plots/webview/contract' import React from 'react' import { EmptyState } from '../../shared/components/emptyState/EmptyState' -export const sectionIsLoading = (selectedRevisions: Revision[]): boolean => +export const sectionIsLoading = ( + selectedRevisions: Revision[], + hasData: boolean +): boolean => selectedRevisions.length > 0 && - !selectedRevisions.some(({ fetched }) => fetched) + !selectedRevisions.some(({ fetched }) => fetched) && + !hasData export const LoadingSection: React.FC = () => ( Loading... diff --git a/webview/src/plots/components/comparisonTable/ComparisonTableCell.tsx b/webview/src/plots/components/comparisonTable/ComparisonTableCell.tsx index e581014784..9fe04a9726 100644 --- a/webview/src/plots/components/comparisonTable/ComparisonTableCell.tsx +++ b/webview/src/plots/components/comparisonTable/ComparisonTableCell.tsx @@ -13,14 +13,39 @@ type ComparisonTableCellProps = { plot?: ComparisonPlot & { fetched: boolean } } +const MissingPlotTableCell: React.FC<{ plot: ComparisonPlot }> = ({ plot }) => ( +
+ {plot.error ? ( + <> + +
+ +
+
+ + sendMessage({ + payload: plot.revision, + type: MessageFromWebviewType.REFRESH_REVISION + }) + } + /> + + ) : ( +

-

+ )} +
+) + export const ComparisonTableCell: React.FC = ({ path, plot }) => { - const error = plot?.error - const missing = plot?.fetched && !plot?.url + const fetched = plot?.fetched + const loading = !fetched && !plot?.url + const missing = fetched && !plot?.url - if (!plot?.fetched) { + if (loading) { return (

Loading...

@@ -29,29 +54,7 @@ export const ComparisonTableCell: React.FC = ({ } if (missing) { - return ( -
- {error ? ( - <> - -
- -
-
- - sendMessage({ - payload: plot.revision, - type: MessageFromWebviewType.REFRESH_REVISION - }) - } - /> - - ) : ( -

-

- )} -
- ) + return } return ( diff --git a/webview/src/plots/components/customPlots/CustomPlots.tsx b/webview/src/plots/components/customPlots/CustomPlots.tsx index 6b55e4eaf4..d95295c717 100644 --- a/webview/src/plots/components/customPlots/CustomPlots.tsx +++ b/webview/src/plots/components/customPlots/CustomPlots.tsx @@ -23,7 +23,7 @@ interface CustomPlotsProps { export const CustomPlots: React.FC = ({ plotsIds }) => { const [order, setOrder] = useState(plotsIds) - const { nbItemsPerRow, hasData, disabledDragPlotIds } = useSelector( + const { nbItemsPerRow, hasData, hasItems, disabledDragPlotIds } = useSelector( (state: PlotsState) => state.custom ) const [onSection, setOnSection] = useState(false) @@ -46,7 +46,7 @@ export const CustomPlots: React.FC = ({ plotsIds }) => { }) } - if (sectionIsLoading(selectedRevisions)) { + if (sectionIsLoading(selectedRevisions, hasItems)) { return } diff --git a/webview/src/plots/components/customPlots/customPlotsSlice.ts b/webview/src/plots/components/customPlots/customPlotsSlice.ts index 3a2a4f04c9..66140569d1 100644 --- a/webview/src/plots/components/customPlots/customPlotsSlice.ts +++ b/webview/src/plots/components/customPlots/customPlotsSlice.ts @@ -12,6 +12,7 @@ import { addPlotsWithSnapshots, removePlots } from '../plotDataStore' export interface CustomPlotsState extends Omit { isCollapsed: boolean hasData: boolean + hasItems: boolean plotsIds: string[] plotsSnapshots: { [key: string]: string } disabledDragPlotIds: string[] @@ -24,6 +25,7 @@ export const customPlotsInitialState: CustomPlotsState = { disabledDragPlotIds: [], enablePlotCreation: true, hasData: false, + hasItems: false, height: DEFAULT_HEIGHT[PlotsSection.CUSTOM_PLOTS], isCollapsed: DEFAULT_SECTION_COLLAPSED[PlotsSection.CUSTOM_PLOTS], nbItemsPerRow: @@ -58,15 +60,19 @@ export const customPlotsSlice = createSlice({ } const { plots, colors, ...statePayload } = action.payload const plotsIds = plots?.map(plot => plot.id) || [] - const snapShots = addPlotsWithSnapshots(plots, PlotsSection.CUSTOM_PLOTS) + const plotsSnapshots = addPlotsWithSnapshots( + plots, + PlotsSection.CUSTOM_PLOTS + ) removePlots(plotsIds, PlotsSection.CUSTOM_PLOTS) return { ...state, ...statePayload, colors: colors || initialColorsState, hasData: !!action.payload, + hasItems: Object.keys(plotsSnapshots).length > 0, plotsIds: plots?.map(plot => plot.id) || [], - plotsSnapshots: snapShots + plotsSnapshots } } } diff --git a/webview/src/plots/components/templatePlots/TemplatePlots.tsx b/webview/src/plots/components/templatePlots/TemplatePlots.tsx index e3d81042b9..738b0ba4e8 100644 --- a/webview/src/plots/components/templatePlots/TemplatePlots.tsx +++ b/webview/src/plots/components/templatePlots/TemplatePlots.tsx @@ -24,9 +24,10 @@ export enum NewSectionBlock { } export const TemplatePlots: React.FC = () => { - const { nbItemsPerRow, sections } = useSelector( + const { nbItemsPerRow, sections, hasItems } = useSelector( (state: PlotsState) => state.template ) + const draggedOverGroup = useSelector( (state: PlotsState) => state.dragAndDrop.draggedOverGroup ) @@ -98,7 +99,7 @@ export const TemplatePlots: React.FC = () => { [setSections, sections] ) - if (sectionIsLoading(selectedRevisions)) { + if (sectionIsLoading(selectedRevisions, hasItems)) { return } if (!sections || sections.length === 0) { diff --git a/webview/src/plots/components/templatePlots/TemplatePlotsWrapper.tsx b/webview/src/plots/components/templatePlots/TemplatePlotsWrapper.tsx index a32ae16ccc..daf879e6f3 100644 --- a/webview/src/plots/components/templatePlots/TemplatePlotsWrapper.tsx +++ b/webview/src/plots/components/templatePlots/TemplatePlotsWrapper.tsx @@ -7,12 +7,9 @@ import { PlotsContainer } from '../PlotsContainer' import { PlotsState } from '../../store' export const TemplatePlotsWrapper: React.FC = () => { - const { nbItemsPerRow, isCollapsed, height } = useSelector( + const { nbItemsPerRow, isCollapsed, height, hasItems } = useSelector( (state: PlotsState) => state.template ) - const hasItems = useSelector( - (state: PlotsState) => Object.keys(state.template.plotsSnapshots).length > 0 - ) return ( { isCollapsed: boolean hasData: boolean + hasItems: boolean plotsSnapshots: { [key: string]: string } sections: PlotGroup[] disabledDragPlotIds: string[] @@ -22,6 +23,7 @@ export interface TemplatePlotsState extends Omit { export const templatePlotsInitialState: TemplatePlotsState = { disabledDragPlotIds: [], hasData: false, + hasItems: false, height: DEFAULT_HEIGHT[PlotsSection.TEMPLATE_PLOTS], isCollapsed: DEFAULT_SECTION_COLLAPSED[PlotsSection.TEMPLATE_PLOTS], nbItemsPerRow: @@ -62,7 +64,7 @@ export const templatePlotsSlice = createSlice({ const plots = action.payload.plots?.flatMap(section => section.entries) const plotsIds = plots?.map(plot => plot.id) || [] - const snapShots = addPlotsWithSnapshots( + const plotsSnapshots = addPlotsWithSnapshots( plots, PlotsSection.TEMPLATE_PLOTS ) @@ -71,8 +73,9 @@ export const templatePlotsSlice = createSlice({ return { ...state, hasData: !!action.payload, + hasItems: Object.keys(plotsSnapshots).length > 0, nbItemsPerRow: action.payload.nbItemsPerRow, - plotsSnapshots: snapShots, + plotsSnapshots, sections: JSON.stringify(plotSections) === JSON.stringify(state.sections) ? state.sections