From 3bf6eabb2c58a23ef101a525f68c7f732d25f10d Mon Sep 17 00:00:00 2001 From: Matt Seddon Date: Tue, 18 Apr 2023 16:23:50 +1000 Subject: [PATCH] Fix auto-selection of experiments --- extension/src/experiments/index.ts | 34 +-- extension/src/experiments/model/index.ts | 64 +---- .../experiments/model/status/collect.test.ts | 223 +----------------- .../src/experiments/model/status/collect.ts | 157 +----------- extension/src/plots/model/index.ts | 2 - extension/src/plots/webview/messages.ts | 4 - .../src/test/fixtures/expShow/base/rows.ts | 12 +- .../src/test/suite/experiments/index.test.ts | 192 ++++----------- .../test/suite/experiments/model/tree.test.ts | 48 ++-- extension/src/test/suite/plots/util.ts | 1 + 10 files changed, 115 insertions(+), 622 deletions(-) diff --git a/extension/src/experiments/index.ts b/extension/src/experiments/index.ts index 0e428a0a4d..b0d52766ed 100644 --- a/extension/src/experiments/index.ts +++ b/extension/src/experiments/index.ts @@ -21,10 +21,6 @@ import { pickFiltersToRemove } from './model/filterBy/quickPick' import { Color } from './model/status/colors' -import { - FetchedExperiment, - hasFinishedWorkspaceExperiment -} from './model/status/collect' import { UNSELECTED } from './model/status' import { starredSort } from './model/sortBy/constants' import { pickSortsToRemove, pickSortToAdd } from './model/sortBy/quickPick' @@ -36,7 +32,7 @@ import { DecorationProvider } from './model/decorationProvider' import { starredFilter } from './model/filterBy/constants' import { ResourceLocator } from '../resourceLocator' import { AvailableCommands, InternalCommands } from '../commands/internal' -import { EXPERIMENT_WORKSPACE_ID, ExpShowOutput } from '../cli/dvc/contract' +import { ExpShowOutput } from '../cli/dvc/contract' import { ViewKey } from '../webview/constants' import { BaseRepository } from '../webview/repository' import { Title } from '../vscode/title' @@ -217,15 +213,6 @@ export class Experiments extends BaseRepository { public toggleExperimentStatus( id: string ): Color | typeof UNSELECTED | undefined { - if ( - this.experiments.isRunningInWorkspace(id) && - !this.experiments.isSelected(id) - ) { - return this.experiments.isSelected(EXPERIMENT_WORKSPACE_ID) - ? undefined - : this.toggleExperimentStatus(EXPERIMENT_WORKSPACE_ID) - } - const selected = this.experiments.isSelected(id) if (!selected && !this.experiments.canSelect()) { return @@ -236,17 +223,6 @@ export class Experiments extends BaseRepository { return status } - public checkForFinishedWorkspaceExperiment( - fetchedExperiments: FetchedExperiment[] - ) { - if (!hasFinishedWorkspaceExperiment(fetchedExperiments)) { - return - } - - this.experiments.unselectWorkspace() - this.notifyChanged() - } - public getSorts() { return this.experiments.getSorts() } @@ -438,14 +414,6 @@ export class Experiments extends BaseRepository { return this.experiments.getSelectedRevisions() } - public setRevisionCollected(revisions: string[]) { - this.experiments.setRevisionCollected(revisions) - } - - public getFinishedExperiments() { - return this.experiments.getFinishedExperiments() - } - public getExperiments() { return this.experiments.getExperiments() } diff --git a/extension/src/experiments/model/index.ts b/extension/src/experiments/model/index.ts index daeb8bb26f..e2ef977217 100644 --- a/extension/src/experiments/model/index.ts +++ b/extension/src/experiments/model/index.ts @@ -4,7 +4,6 @@ import { FilterDefinition, filterExperiment, getFilterId } from './filterBy' import { collectExperiments } from './collect' import { collectColoredStatus, - collectFinishedRunningExperiments, collectSelected, collectStartedRunningExperiments } from './status/collect' @@ -24,7 +23,7 @@ import { RunningExperiment } from '../webview/contract' import { definedAndNonEmpty, reorderListSubset } from '../../util/array' -import { EXPERIMENT_WORKSPACE_ID, ExpShowOutput } from '../../cli/dvc/contract' +import { ExpShowOutput } from '../../cli/dvc/contract' import { flattenMapValues } from '../../util/map' import { ModelWithPersistence } from '../../persistence/model' import { PersistenceKey } from '../../persistence/constants' @@ -62,7 +61,6 @@ export class ExperimentsModel extends ModelWithPersistence { private currentSorts: SortDefinition[] private running: RunningExperiment[] = [] - private finishedRunning: { [id: string]: string } = {} private startedRunning: Set = new Set() constructor(dvcRoot: string, workspaceState: Memento) { @@ -136,14 +134,15 @@ export class ExperimentsModel extends ModelWithPersistence { } public toggleStatus(id: string) { + const experiment = this.getExperimentsAndQueued().find( + ({ id: queuedId }) => queuedId === id + ) + if ( - isQueued( - this.getExperimentsAndQueued().find( - ({ id: queuedId }) => queuedId === id - )?.status - ) + experiment && + (isQueued(experiment?.status) || isRunningInQueue(experiment)) ) { - return + return UNSELECTED } const current = this.coloredStatus[id] @@ -158,39 +157,14 @@ export class ExperimentsModel extends ModelWithPersistence { return this.coloredStatus[id] } - public unselectWorkspace() { - this.coloredStatus[EXPERIMENT_WORKSPACE_ID] = UNSELECTED - } - public hasRunningExperiment() { return this.running.length > 0 } - public isRunningInWorkspace(id: string) { - if (id === EXPERIMENT_WORKSPACE_ID) { - return false - } - - return this.running.some( - ({ id: runningId, executor }) => - executor === EXPERIMENT_WORKSPACE_ID && runningId === id - ) - } - public hasCheckpoints() { return this.checkpoints } - public setRevisionCollected(revisions: string[]) { - for (const { id } of this.getExperimentsAndQueued().filter(({ label }) => - revisions.includes(label) - )) { - if (this.finishedRunning[id]) { - delete this.finishedRunning[id] - } - } - } - public canSelect() { return canSelect(this.coloredStatus) } @@ -260,7 +234,10 @@ export class ExperimentsModel extends ModelWithPersistence { } const { availableColors, coloredStatus } = collectSelected( - selectedExperiments.filter(({ status }) => !isQueued(status)), + selectedExperiments.filter( + ({ executor, status }) => + !isQueued(status) && !isRunningInQueue({ executor, status }) + ), this.getWorkspaceCommitsAndExperiments(), this.coloredStatus, this.availableColors @@ -389,10 +366,6 @@ export class ExperimentsModel extends ModelWithPersistence { })) } - public getFinishedExperiments() { - return this.finishedRunning - } - public setNbfCommitsToShow(numberOfCommitsToShow: number) { this.numberOfCommitsToShow = numberOfCommitsToShow this.persistNbOfCommitsToShow() @@ -455,17 +428,14 @@ export class ExperimentsModel extends ModelWithPersistence { private setColoredStatus(runningExperiments: RunningExperiment[]) { this.setRunning(runningExperiments) - const { coloredStatus, availableColors } = collectColoredStatus( this.getWorkspaceAndCommits(), this.experimentsByCommit, this.coloredStatus, this.availableColors, - this.startedRunning, - this.finishedRunning + this.startedRunning ) this.startedRunning = new Set() - this.setColors(coloredStatus, availableColors) this.persistStatus() @@ -477,14 +447,6 @@ export class ExperimentsModel extends ModelWithPersistence { stillRunning ) - this.finishedRunning = collectFinishedRunningExperiments( - { ...this.finishedRunning }, - this.getExperimentsAndQueued(), - this.running, - stillRunning, - this.coloredStatus - ) - this.running = stillRunning } diff --git a/extension/src/experiments/model/status/collect.test.ts b/extension/src/experiments/model/status/collect.test.ts index bb4a5e1aa6..16cf3f36b8 100644 --- a/extension/src/experiments/model/status/collect.test.ts +++ b/extension/src/experiments/model/status/collect.test.ts @@ -1,15 +1,8 @@ import { UNSELECTED } from '.' -import { - collectColoredStatus, - collectFinishedRunningExperiments -} from './collect' +import { collectColoredStatus } from './collect' import { copyOriginalColors } from './colors' import { Experiment } from '../../webview/contract' -import { - ExperimentStatus, - EXPERIMENT_WORKSPACE_ID, - Executor -} from '../../../cli/dvc/contract' +import { ExperimentStatus } from '../../../cli/dvc/contract' describe('collectColoredStatus', () => { const buildMockExperiments = (n: number, prefix = 'exp') => { @@ -31,8 +24,7 @@ describe('collectColoredStatus', () => { new Map(), {}, copyOriginalColors(), - new Set(), - {} + new Set() ) expect(availableColors).toStrictEqual(colors) @@ -53,8 +45,7 @@ describe('collectColoredStatus', () => { new Map(), {}, copyOriginalColors(), - new Set(), - {} + new Set() ) expect(availableColors).toStrictEqual(colors) @@ -80,8 +71,7 @@ describe('collectColoredStatus', () => { exp7: colors[6] }, [], - new Set(['exp8']), - {} + new Set(['exp8']) ) expect(availableColors).toStrictEqual([]) @@ -114,8 +104,7 @@ describe('collectColoredStatus', () => { exp8: UNSELECTED }, copyOriginalColors().slice(3), - new Set(['exp1']), - {} + new Set(['exp1']) ) expect(coloredStatus).toStrictEqual({ exp1: colors[0] }) @@ -137,8 +126,7 @@ describe('collectColoredStatus', () => { exp9: colors[1] }, unassignedColors, - new Set(), - {} + new Set() ) expect(coloredStatus).toStrictEqual({ @@ -166,8 +154,7 @@ describe('collectColoredStatus', () => { new Map(), { exp9: colors[0] }, unassignColors, - new Set(), - {} + new Set() ) expect(availableColors).toStrictEqual(unassignColors) @@ -200,8 +187,7 @@ describe('collectColoredStatus', () => { exp9: colors[5] }, copyOriginalColors().slice(6), - new Set(['exp1', 'exp2', 'exp3']), - {} + new Set(['exp1', 'exp2', 'exp3']) ) expect(availableColors).toStrictEqual([]) @@ -217,195 +203,4 @@ describe('collectColoredStatus', () => { exp9: colors[5] }) }) - - it('should remove the unselected status of experiments running in the workspace (for getMostRecentExperiment)', () => { - const colors = copyOriginalColors() - const { availableColors, coloredStatus } = collectColoredStatus( - [ - { - executor: Executor.WORKSPACE, - id: 'exp-1', - status: ExperimentStatus.RUNNING - }, - { - executor: Executor.WORKSPACE, - id: 'exp-2', - status: ExperimentStatus.RUNNING - } - ] as Experiment[], - new Map(), - { - 'exp-1': UNSELECTED, - 'exp-2': colors[0] - }, - colors.slice(1), - new Set(), - {} - ) - expect(coloredStatus['exp-1']).toBeUndefined() - expect(coloredStatus['exp-2']).toStrictEqual(colors[0]) - expect(availableColors).toStrictEqual(colors.slice(1)) - }) - - it("should duplicate the workspace's color when an experiment finishes running in the workspace", () => { - const colors = copyOriginalColors() - const { availableColors, coloredStatus } = collectColoredStatus( - [ - { - executor: null, - id: 'exp-1', - status: ExperimentStatus.SUCCESS - }, - { - id: EXPERIMENT_WORKSPACE_ID - } - ] as Experiment[], - new Map(), - { - workspace: colors[0] - }, - colors.slice(1), - new Set(), - { 'exp-1': EXPERIMENT_WORKSPACE_ID } - ) - expect(coloredStatus).toStrictEqual({ - 'exp-1': colors[0], - workspace: colors[0] - }) - - expect(availableColors).toStrictEqual(colors.slice(1)) - }) - - it('should not overwrite an experiments color if it has one and finishes running in the workspace', () => { - const colors = copyOriginalColors() - const { availableColors, coloredStatus } = collectColoredStatus( - [ - { - executor: null, - id: 'exp-1', - status: ExperimentStatus.SUCCESS - }, - { - id: EXPERIMENT_WORKSPACE_ID - } - ] as Experiment[], - new Map(), - { - 'exp-1': colors[2], - workspace: colors[0] - }, - colors.slice(1), - new Set(), - { 'exp-1': EXPERIMENT_WORKSPACE_ID } - ) - expect(coloredStatus).toStrictEqual({ - 'exp-1': colors[2], - workspace: colors[0] - }) - - expect(availableColors).toStrictEqual(colors.slice(1)) - }) -}) - -describe('collectFinishedRunningExperiments', () => { - it('should return an empty object when an experiment is still running', () => { - const finishedRunning = collectFinishedRunningExperiments( - {}, - [{ Created: '2022-12-02T07:48:24', id: 'exp-1234' }] as Experiment[], - [{ executor: Executor.WORKSPACE, id: 'exp-1234' }], - [{ executor: Executor.WORKSPACE, id: 'exp-1234' }], - {} - ) - expect(finishedRunning).toStrictEqual({}) - }) - - it('should return the most recently created and unseen (without a status) experiment if there is no longer an experiment running in the workspace', () => { - const latestCreatedId = 'exp-123' - const finishedRunning = collectFinishedRunningExperiments( - {}, - [ - { Created: '2022-12-02T10:48:24', id: 'exp-456' }, - { Created: '2022-10-02T07:48:24', id: 'exp-789' }, - { Created: '2022-12-02T07:48:25', id: latestCreatedId }, - { Created: null, id: 'exp-null' } - ] as Experiment[], - [{ executor: Executor.WORKSPACE, id: EXPERIMENT_WORKSPACE_ID }], - [], - { 'exp-456': UNSELECTED } - ) - expect(finishedRunning).toStrictEqual({ - [latestCreatedId]: EXPERIMENT_WORKSPACE_ID - }) - }) - - it('should return the most recently created experiment if there is no longer a checkpoint experiment running in the workspace', () => { - const latestCreatedId = 'exp-123' - const finishedRunning = collectFinishedRunningExperiments( - {}, - [ - { Created: '2022-12-02T07:48:24', id: 'exp-456' }, - { Created: '2022-10-02T07:48:24', id: 'exp-789' }, - { Created: '2022-12-02T07:48:25', id: latestCreatedId }, - { Created: null, id: 'exp-null' } - ] as Experiment[], - [{ executor: Executor.WORKSPACE, id: latestCreatedId }], - [], - {} - ) - expect(finishedRunning).toStrictEqual({ - [latestCreatedId]: EXPERIMENT_WORKSPACE_ID - }) - }) - - it('should not return an experiment if all of the experiments can be found in the status object', () => { - const previouslyCreatedId = 'exp-123' - const finishedRunning = collectFinishedRunningExperiments( - {}, - [ - { Created: '2022-12-02T07:48:25', id: previouslyCreatedId } - ] as Experiment[], - [{ executor: Executor.WORKSPACE, id: previouslyCreatedId }], - [], - { [previouslyCreatedId]: UNSELECTED } - ) - expect(finishedRunning).toStrictEqual({}) - }) - - it('should return the most recently created experiment if there is no longer an experiment running in the queue', () => { - const latestCreatedId = 'exp-123' - const finishedRunning = collectFinishedRunningExperiments( - {}, - [ - { Created: '2022-12-02T07:48:24', id: 'exp-456' }, - { Created: '2022-10-02T07:48:24', id: 'exp-789' }, - { Created: '2022-12-02T07:48:25', id: latestCreatedId }, - { Created: null, id: 'exp-null' } - ] as Experiment[], - [{ executor: Executor.DVC_TASK, id: latestCreatedId }], - [], - {} - ) - expect(finishedRunning).toStrictEqual({ - [latestCreatedId]: latestCreatedId - }) - }) - - it('should remove the id that was run in the workspace if a new one is found', () => { - const latestCreatedId = 'exp-123' - const finishedRunning = collectFinishedRunningExperiments( - { 'exp-previous': EXPERIMENT_WORKSPACE_ID }, - [ - { Created: '2022-12-02T07:48:24', id: 'exp-456' }, - { Created: '2022-10-02T07:48:24', id: 'exp-789' }, - { Created: '2022-12-02T07:48:25', id: latestCreatedId }, - { Created: null, id: 'exp-null' } - ] as Experiment[], - [{ executor: Executor.WORKSPACE, id: latestCreatedId }], - [], - {} - ) - expect(finishedRunning).toStrictEqual({ - [latestCreatedId]: EXPERIMENT_WORKSPACE_ID - }) - }) }) diff --git a/extension/src/experiments/model/status/collect.ts b/extension/src/experiments/model/status/collect.ts index 54377201b2..33c85cd22c 100644 --- a/extension/src/experiments/model/status/collect.ts +++ b/extension/src/experiments/model/status/collect.ts @@ -1,15 +1,10 @@ import { canSelect, ColoredStatus, UNSELECTED } from '.' import { Color, copyOriginalColors } from './colors' import { hasKey } from '../../../util/object' -import { - Experiment, - isQueued, - isRunning, - RunningExperiment -} from '../../webview/contract' +import { Experiment, isQueued, RunningExperiment } from '../../webview/contract' import { definedAndNonEmpty, reorderListSubset } from '../../../util/array' import { flattenMapValues } from '../../../util/map' -import { EXPERIMENT_WORKSPACE_ID } from '../../../cli/dvc/contract' +import { Executor, EXPERIMENT_WORKSPACE_ID } from '../../../cli/dvc/contract' const canAssign = ( coloredStatus: ColoredStatus, @@ -63,33 +58,6 @@ const collectStartedRunningColors = ( } } -const removeUnselectedExperimentRunningInWorkspace = ( - coloredStatus: ColoredStatus, - { status, executor, id }: Experiment -): void => { - if ( - isRunning(status) && - executor === EXPERIMENT_WORKSPACE_ID && - id !== EXPERIMENT_WORKSPACE_ID && - !coloredStatus[id] - ) { - delete coloredStatus[id] - } -} - -const duplicateFinishedWorkspaceExperiment = ( - coloredStatus: ColoredStatus, - finishedRunning: { [id: string]: string } -): void => { - for (const [id, previousId] of Object.entries(finishedRunning)) { - if (previousId !== EXPERIMENT_WORKSPACE_ID || coloredStatus[id]) { - continue - } - - coloredStatus[id] = coloredStatus.workspace - } -} - export const unassignColors = ( experiments: Experiment[], current: ColoredStatus, @@ -114,8 +82,7 @@ export const collectColoredStatus = ( experimentsByCommit: Map, previousStatus: ColoredStatus, unassignedColors: Color[], - startedRunning: Set, - finishedRunning: { [id: string]: string } + startedRunning: Set ): { coloredStatus: ColoredStatus; availableColors: Color[] } => { const flatExperimentsByCommit = flattenMapValues(experimentsByCommit) const availableColors = unassignColors( @@ -139,11 +106,8 @@ export const collectColoredStatus = ( for (const experiment of [...experiments, ...flatExperimentsByCommit]) { collectStatus(coloredStatus, experiment) - removeUnselectedExperimentRunningInWorkspace(coloredStatus, experiment) } - duplicateFinishedWorkspaceExperiment(coloredStatus, finishedRunning) - return { availableColors, coloredStatus } } @@ -217,120 +181,15 @@ export const collectStartedRunningExperiments = ( const acc = new Set() for (const { id: runningId, executor } of nowRunning) { - if (previouslyRunning.some(({ id }) => id === runningId)) { - continue - } - acc.add( - executor === EXPERIMENT_WORKSPACE_ID ? EXPERIMENT_WORKSPACE_ID : runningId - ) - } - - return acc -} - -const getMostRecentExperiment = ( - experiments: Experiment[], - coloredStatus: ColoredStatus -): Experiment | undefined => - experiments - .filter(({ id }) => coloredStatus[id] === undefined) - .sort(({ Created: aCreated }, { Created: bCreated }) => { - if (!aCreated) { - return 1 - } - if (!bCreated) { - return -1 - } - return bCreated.localeCompare(aCreated) - }) - .slice(0, 1)[0] - -const collectFinishedWorkspaceExperiment = ( - acc: { [id: string]: string }, - mostRecentExperiment: Experiment | undefined -): void => { - const newId = mostRecentExperiment?.id - - if (!newId) { - return - } - - for (const [id, previousId] of Object.entries(acc)) { - if (previousId === EXPERIMENT_WORKSPACE_ID) { - delete acc[id] - } - } - acc[newId] = EXPERIMENT_WORKSPACE_ID -} - -const isStillRunning = ( - stillExecutingInWorkspace: boolean, - previouslyRunningId: string, - previouslyRunningExecutor: string, - stillRunning: RunningExperiment[] -): boolean => - (stillExecutingInWorkspace && - previouslyRunningExecutor === EXPERIMENT_WORKSPACE_ID) || - stillRunning.some(({ id }) => id === previouslyRunningId) - -export const collectFinishedRunningExperiments = ( - acc: { [id: string]: string }, - experiments: Experiment[], - previouslyRunning: RunningExperiment[], - stillRunning: RunningExperiment[], - coloredStatus: ColoredStatus -): { [id: string]: string } => { - const stillExecutingInWorkspace = stillRunning.some( - ({ executor }) => executor === EXPERIMENT_WORKSPACE_ID - ) - for (const { - id: previouslyRunningId, - executor: previouslyRunningExecutor - } of previouslyRunning) { if ( - isStillRunning( - stillExecutingInWorkspace, - previouslyRunningId, - previouslyRunningExecutor, - stillRunning - ) + runningId === EXPERIMENT_WORKSPACE_ID || + executor === Executor.DVC_TASK || + previouslyRunning.some(({ id }) => id === runningId) ) { continue } - - if (previouslyRunningExecutor === EXPERIMENT_WORKSPACE_ID) { - collectFinishedWorkspaceExperiment( - acc, - getMostRecentExperiment(experiments, coloredStatus) - ) - continue - } - acc[previouslyRunningId] = previouslyRunningId + acc.add(runningId) } - return acc -} - -export type FetchedExperiment = { id?: string; displayColor: Color } -export const hasFinishedWorkspaceExperiment = ( - fetchedExperiments: FetchedExperiment[] -): boolean => { - let workspace: FetchedExperiment | undefined - const nonWorkspace: FetchedExperiment[] = [] - - for (const revision of fetchedExperiments) { - if (revision.id === EXPERIMENT_WORKSPACE_ID) { - workspace = revision - continue - } - nonWorkspace.push(revision) - } - - if (!workspace) { - return false - } - - return nonWorkspace.some( - ({ displayColor }) => displayColor === workspace?.displayColor - ) + return acc } diff --git a/extension/src/plots/model/index.ts b/extension/src/plots/model/index.ts index 2901aedb6e..1b33205d59 100644 --- a/extension/src/plots/model/index.ts +++ b/extension/src/plots/model/index.ts @@ -112,8 +112,6 @@ export class PlotsModel extends ModelWithPersistence { this.fetchedRevs = new Set(revs) - this.experiments.setRevisionCollected(revs) - this.deferred.resolve() } diff --git a/extension/src/plots/webview/messages.ts b/extension/src/plots/webview/messages.ts index c4d513ba16..b6847c570e 100644 --- a/extension/src/plots/webview/messages.ts +++ b/extension/src/plots/webview/messages.ts @@ -70,10 +70,6 @@ export class WebviewMessages { selectedRevisions, template: this.getTemplatePlots(selectedRevisions) }) - - this.experiments.checkForFinishedWorkspaceExperiment( - selectedRevisions.filter(({ fetched }) => fetched) - ) } public handleMessageFromWebview(message: MessageFromWebview) { diff --git a/extension/src/test/fixtures/expShow/base/rows.ts b/extension/src/test/fixtures/expShow/base/rows.ts index 0b1a806b98..a78131367b 100644 --- a/extension/src/test/fixtures/expShow/base/rows.ts +++ b/extension/src/test/fixtures/expShow/base/rows.ts @@ -41,7 +41,7 @@ const data: Commit[] = [ 'c3961d777cfbd7727f9fde4851896006' ) }, - displayColor: colorsList[0], + displayColor: undefined, executor: Executor.WORKSPACE, id: EXPERIMENT_WORKSPACE_ID, label: EXPERIMENT_WORKSPACE_ID, @@ -68,7 +68,7 @@ const data: Commit[] = [ } }, status: ExperimentStatus.RUNNING, - selected: true, + selected: false, starred: false }, { @@ -150,7 +150,7 @@ const data: Commit[] = [ 'c3961d777cfbd7727f9fde4851896006' ) }, - displayColor: colorsList[1], + displayColor: undefined, description: '[exp-e7a67]', executor: Executor.DVC_TASK, id: 'exp-e7a67', @@ -179,7 +179,7 @@ const data: Commit[] = [ } }, status: ExperimentStatus.RUNNING, - selected: true, + selected: false, sha: '4fb124aebddb2adf1545030907687fa9a4c80e70', starred: false, Created: '2020-12-29T15:31:52' @@ -266,7 +266,7 @@ const data: Commit[] = [ 'c3961d777cfbd7727f9fde4851896006' ) }, - displayColor: undefined, + displayColor: colorsList[0], description: '[exp-83425]', executor: Executor.WORKSPACE, id: 'exp-83425', @@ -294,7 +294,7 @@ const data: Commit[] = [ test: true } }, - selected: false, + selected: true, starred: false, status: ExperimentStatus.RUNNING, Created: '2020-12-29T15:27:02' diff --git a/extension/src/test/suite/experiments/index.test.ts b/extension/src/test/suite/experiments/index.test.ts index 3a697ecebe..951c260396 100644 --- a/extension/src/test/suite/experiments/index.test.ts +++ b/extension/src/test/suite/experiments/index.test.ts @@ -71,11 +71,7 @@ import { RegisteredCommands } from '../../../commands/external' import { ConfigKey } from '../../../vscode/config' -import { - EXPERIMENT_WORKSPACE_ID, - Executor, - ExperimentStatus -} from '../../../cli/dvc/contract' +import { EXPERIMENT_WORKSPACE_ID, Executor } from '../../../cli/dvc/contract' import * as Time from '../../../util/time' import { AvailableCommands } from '../../../commands/internal' import { Setup } from '../../../setup' @@ -924,17 +920,22 @@ suite('Experiments Test Suite', () => { await experiments.isReady() - const experimentToToggle = 'exp-e7a67' - const queuedExperiment = '90aea7f2482117a55dfcadcdb901aaa6610fbbc9' + const idToToggle = 'exp-83425' + const runningInQueueId = 'exp-e7a67' + const queuedId = '90aea7f' const isExperimentSelected = (expId: string): boolean => !!experimentsModel.getCombinedList().find(({ id }) => id === expId) ?.selected - expect(isExperimentSelected(experimentToToggle), 'experiment is selected') - .to.be.true + expect(isExperimentSelected(idToToggle), 'experiment is selected').to.be + .true + expect( + isExperimentSelected(runningInQueueId), + 'experiment running in the queue cannot be selected' + ).to.be.false expect( - isExperimentSelected(queuedExperiment), + isExperimentSelected(queuedId), 'queued experiment cannot be selected' ).to.be.false @@ -945,27 +946,39 @@ suite('Experiments Test Suite', () => { const toggleSpy = spy(experimentsModel, 'toggleStatus') mockMessageReceived.fire({ - payload: experimentToToggle, + payload: idToToggle, type: MessageFromWebviewType.TOGGLE_EXPERIMENT }) - expect(toggleSpy).to.be.calledWith(experimentToToggle) + expect(toggleSpy).to.be.calledWith(idToToggle) toggleSpy.resetHistory() expect( - isExperimentSelected(experimentToToggle), + isExperimentSelected(idToToggle), 'experiment has been toggled to unselected' ).to.be.false mockMessageReceived.fire({ - payload: queuedExperiment, + payload: runningInQueueId, type: MessageFromWebviewType.TOGGLE_EXPERIMENT }) - expect(toggleSpy).to.be.calledWith(queuedExperiment) + expect(toggleSpy).to.be.calledWith(runningInQueueId) expect( - isExperimentSelected(queuedExperiment), + isExperimentSelected(runningInQueueId), + 'experiment running the queue cannot be selected' + ).to.be.false + + mockMessageReceived.fire({ + payload: queuedId, + type: MessageFromWebviewType.TOGGLE_EXPERIMENT + }) + + expect(toggleSpy).to.be.calledWith(queuedId) + + expect( + isExperimentSelected(queuedId), 'queued experiment cannot be selected' ).to.be.false }).timeout(WEBVIEW_TEST_TIMEOUT) @@ -1180,9 +1193,10 @@ suite('Experiments Test Suite', () => { const webview = await experiments.showWebview() const mockMessageReceived = getMessageReceivedEmitter(webview) - const queuedId = '90aea7f2482117a55dfcadcdb901aaa6610fbbc9' - const expectedIds = ['exp-e7a67', 'test-branch'] - const mockExperimentIds = [...expectedIds, queuedId] + const queuedId = '90aea7f' + const runningInQueueId = 'exp-e7a67' + const expectedIds = ['main', 'test-branch'] + const mockExperimentIds = [...expectedIds, queuedId, runningInQueueId] stubWorkspaceExperimentsGetters(dvcDemoPath, experiments) @@ -1202,7 +1216,7 @@ suite('Experiments Test Suite', () => { mockExperimentIds.sort() expect( selectExperimentIds, - 'should exclude queued experiments from selection' + 'should exclude queued experiments and experiments running in the queue from selection' ).to.deep.equal(expectedIds) }).timeout(WEBVIEW_TEST_TIMEOUT) @@ -1223,14 +1237,15 @@ suite('Experiments Test Suite', () => { const webview = await experiments.showWebview() const mockMessageReceived = getMessageReceivedEmitter(webview) - const mockExperimentIds = ['exp-e7a67', 'test-branch'] + const runningInQueueId = 'exp-e7a67' + const mockExperimentIds = ['main', 'test-branch'] stubWorkspaceExperimentsGetters(dvcDemoPath, experiments) const tableChangePromise = experimentsUpdatedEvent(experiments) mockMessageReceived.fire({ - payload: mockExperimentIds, + payload: [...mockExperimentIds, runningInQueueId], type: MessageFromWebviewType.SET_EXPERIMENTS_AND_OPEN_PLOTS }) @@ -1752,11 +1767,12 @@ suite('Experiments Test Suite', () => { ).to.deep.equal({ '489fd8b': 0, '55d492c': 0, - 'exp-e7a67': colors[1], + 'exp-83425': colors[0], + 'exp-e7a67': 0, 'exp-f13bca': 0, main: 0, 'test-branch': 0, - workspace: colors[0] + workspace: 0 }) expect( @@ -1840,18 +1856,19 @@ suite('Experiments Test Suite', () => { 'both filters should be removed from memento after removeFilters is run against them' ).to.deep.equal([]) - testRepository.toggleExperimentStatus('exp-e7a67') + testRepository.toggleExperimentStatus('exp-f13bca') expect( mockMemento.get('experimentsStatus:test'), 'the correct statuses have been recorded in the memento' ).to.deep.equal({ '489fd8b': 0, '55d492c': 0, + 'exp-83425': colors[0], 'exp-e7a67': 0, - 'exp-f13bca': 0, + 'exp-f13bca': colors[1], main: 0, 'test-branch': 0, - workspace: colors[0] + workspace: 0 }) }) @@ -1908,10 +1925,6 @@ suite('Experiments Test Suite', () => { displayColor: colors[1], id: 'test-branch' }, - { - displayColor: colors[2], - id: EXPERIMENT_WORKSPACE_ID - }, { displayColor: colors[5], id: 'exp-e7a67' @@ -2072,121 +2085,4 @@ suite('Experiments Test Suite', () => { expect(mockUpdateExperimentsData).to.be.calledOnce }) }) - - describe('checkForFinishedWorkspaceExperiment', () => { - it('should unselect the workspace record if it has the same color as an experiment', async () => { - const colors = copyOriginalColors() - const [color] = colors - const commit = 'df3f8647a47e403c9c4aa6562cad0b74afbe900b' - const name = 'fizzy-dilemma' - const params = { 'params.yaml': { data: { lr: 1 } } } - const runningOutput = generateTestExpShowOutput( - { params }, - { - rev: commit, - experiments: [ - { - rev: EXPERIMENT_WORKSPACE_ID, - data: { params }, - name, - executor: { - local: null, - state: ExperimentStatus.RUNNING, - name: Executor.WORKSPACE - } - } - ] - } - ) - - const getSelectedIdsWithColor = ( - experiments: Experiments - ): { id: string; displayColor: string }[] => - experiments - .getSelectedRevisions() - .map(({ id, displayColor }) => ({ id, displayColor })) - - const selectedWorkspace = { - id: EXPERIMENT_WORKSPACE_ID, - displayColor: color - } - const selectedExperiment = { - id: name, - displayColor: color - } - const bothSelected = [selectedWorkspace, selectedExperiment] - - const { experiments, experimentsModel } = buildExperiments( - disposable, - runningOutput - ) - - await experiments.isReady() - - expect( - experimentsModel.hasRunningExperiment(), - 'should have a running experiment' - ).to.be.true - - expect( - getSelectedIdsWithColor(experiments), - 'should automatically select the running experiment' - ).to.deep.equal([selectedWorkspace]) - - const experimentsChanged = new Promise(resolve => - experiments.onDidChangeExperiments(() => resolve(undefined)) - ) - - await experiments.setState( - generateTestExpShowOutput( - { params }, - { - rev: commit, - experiments: [ - { - rev: '679c7ce7469020332154717126f88ce157ebc612', - data: { params }, - name, - executor: null - } - ] - } - ) - ) - - await experimentsChanged - - expect( - getSelectedIdsWithColor(experiments), - "should apply the workspace's color to a newly created experiment" - ).to.deep.equal(bothSelected) - - experiments.checkForFinishedWorkspaceExperiment([selectedWorkspace]) - - expect( - getSelectedIdsWithColor(experiments), - "should not remove the workspace's color if the experiment's data has not been fetched" - ).to.deep.equal(bothSelected) - - experiments.checkForFinishedWorkspaceExperiment([ - selectedExperiment, - selectedWorkspace - ]) - - expect( - getSelectedIdsWithColor(experiments), - "should remove the workspace's color once the experiment's data has been shown to be fetched" - ).to.deep.equal([selectedExperiment]) - - experiments.toggleExperimentStatus(EXPERIMENT_WORKSPACE_ID) - - expect( - getSelectedIdsWithColor(experiments), - 'should not duplicate the color when toggling another experiment' - ).to.deep.equal([ - selectedExperiment, - { id: EXPERIMENT_WORKSPACE_ID, displayColor: colors[1] } - ]) - }) - }) }) diff --git a/extension/src/test/suite/experiments/model/tree.test.ts b/extension/src/test/suite/experiments/model/tree.test.ts index b8692f7592..6dca97c3af 100644 --- a/extension/src/test/suite/experiments/model/tree.test.ts +++ b/extension/src/test/suite/experiments/model/tree.test.ts @@ -38,6 +38,8 @@ import { EXPERIMENT_WORKSPACE_ID } from '../../../../cli/dvc/contract' import { DvcReader } from '../../../../cli/dvc/reader' import { copyOriginalColors } from '../../../../experiments/model/status/colors' import { Revision } from '../../../../plots/webview/contract' +import { BaseWebview } from '../../../../webview' +import { Experiment } from '../../../../experiments/webview/contract' suite('Experiments Tree Test Suite', () => { const disposable = getTimeSafeDisposer() @@ -61,17 +63,22 @@ 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, plotsModel, experiments } = await buildPlots( - disposable - ) + const { plots, messageSpy, plotsModel, experimentsModel } = + await buildPlots(disposable) + messageSpy.restore() + const mockShow = stub(BaseWebview.prototype, 'show') + + experimentsModel.setSelected([ + { id: EXPERIMENT_WORKSPACE_ID }, + { id: 'main' }, + { id: 'test-branch' }, + { id: 'exp-83425' }, + { id: 'exp-f13bca' } + ] as Experiment[]) const expectedRevisions: { displayColor: string; id: string }[] = [] const [{ id }] = plotsModel.getSelectedRevisionDetails() - const mockCheckForFinishedWorkspaceExperiment = stub( - experiments, - 'checkForFinishedWorkspaceExperiment' - ) for (const { id, @@ -86,7 +93,7 @@ suite('Experiments Tree Test Suite', () => { let updateCall = 1 while (expectedRevisions.length > 0) { - const { selectedRevisions } = getFirstArgOfLastCall(messageSpy) + const { selectedRevisions } = mockShow.lastCall.firstArg expect( (selectedRevisions as Revision[]).map(({ displayColor, id }) => ({ @@ -95,16 +102,19 @@ suite('Experiments Tree Test Suite', () => { })), 'a message is sent with colors for the currently selected experiments' ).to.deep.equal(expectedRevisions) - messageSpy.resetHistory() + mockShow.resetHistory() + mockShow.resetBehavior() const { id } = expectedRevisions.pop() as { id: string } - bypassProcessManagerDebounce(mockNow, updateCall) const messageSent = new Promise(resolve => - mockCheckForFinishedWorkspaceExperiment.callsFake(() => + mockShow.callsFake(() => { resolve(undefined) - ) + return Promise.resolve(true) + }) ) + + bypassProcessManagerDebounce(mockNow, updateCall) const unSelected = await commands.executeCommand( RegisteredCommands.EXPERIMENT_TOGGLE, { @@ -116,16 +126,22 @@ suite('Experiments Tree Test Suite', () => { expect(unSelected).to.equal(UNSELECTED) await messageSent - mockCheckForFinishedWorkspaceExperiment.resetBehavior() } expect( - messageSpy, + mockShow, "when there are no experiments selected we don't send any template plots" ).to.be.calledWithMatch({ template: null }) - messageSpy.resetHistory() + mockShow.resetHistory() + + const messageSent = new Promise(resolve => + mockShow.callsFake(() => { + resolve(undefined) + return Promise.resolve(true) + }) + ) bypassProcessManagerDebounce(mockNow, updateCall) const selected = await commands.executeCommand( @@ -136,6 +152,8 @@ suite('Experiments Tree Test Suite', () => { } ) + await messageSent + expect(selected, 'the experiment is now selected').to.equal( copyOriginalColors()[0] ) diff --git a/extension/src/test/suite/plots/util.ts b/extension/src/test/suite/plots/util.ts index 5079aeec75..29765707b5 100644 --- a/extension/src/test/suite/plots/util.ts +++ b/extension/src/test/suite/plots/util.ts @@ -100,6 +100,7 @@ export const buildPlots = async ( data, errorsModel, experiments, + experimentsModel, messageSpy, mockGetModifiedTime, mockPlotsDiff,