From 13cd66baad9ae1079f3730496d217be231e3eb54 Mon Sep 17 00:00:00 2001 From: Matt Seddon Date: Wed, 24 May 2023 11:40:27 +1000 Subject: [PATCH 1/2] rework plotting of running experiments --- extension/src/cli/dvc/contract.ts | 2 +- extension/src/experiments/index.ts | 26 +- extension/src/experiments/model/collect.ts | 3 + extension/src/experiments/model/index.test.ts | 15 +- extension/src/experiments/model/index.ts | 39 +-- .../experiments/model/status/collect.test.ts | 230 +----------------- .../src/experiments/model/status/collect.ts | 184 +------------- extension/src/plots/webview/messages.ts | 4 - .../src/test/fixtures/expShow/base/rows.ts | 8 +- .../src/test/suite/experiments/index.test.ts | 147 ++--------- .../components/table/body/RowContextMenu.tsx | 13 +- 11 files changed, 57 insertions(+), 614 deletions(-) diff --git a/extension/src/cli/dvc/contract.ts b/extension/src/cli/dvc/contract.ts index 3a28344655..1f779f01bb 100644 --- a/extension/src/cli/dvc/contract.ts +++ b/extension/src/cli/dvc/contract.ts @@ -1,6 +1,6 @@ import { Plot } from '../../plots/webview/contract' -export const MIN_CLI_VERSION = '2.57.0' +export const MIN_CLI_VERSION = '2.58.1' export const LATEST_TESTED_CLI_VERSION = '2.58.1' export const MAX_CLI_VERSION = '3' diff --git a/extension/src/experiments/index.ts b/extension/src/experiments/index.ts index 67c3d63f12..581fb2cf2a 100644 --- a/extension/src/experiments/index.ts +++ b/extension/src/experiments/index.ts @@ -21,22 +21,13 @@ 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' import { ColumnsModel } from './columns/model' import { ExperimentsData } from './data' import { stopWorkspaceExperiment } from './processExecution' -import { - Experiment, - ColumnType, - TableData, - isRunning -} from './webview/contract' +import { Experiment, ColumnType, TableData } from './webview/contract' import { WebviewMessages } from './webview/messages' import { DecorationProvider } from './model/decorationProvider' import { starredFilter } from './model/filterBy/constants' @@ -227,17 +218,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() } @@ -345,9 +325,7 @@ export class Experiments extends BaseRepository { } public async selectExperimentsToPlot() { - const experiments = this.experiments - .getWorkspaceCommitsAndExperiments() - .filter(({ status }) => !isRunning(status)) + const experiments = this.experiments.getWorkspaceCommitsAndExperiments() const selected = await pickExperimentsToPlot( experiments, diff --git a/extension/src/experiments/model/collect.ts b/extension/src/experiments/model/collect.ts index 95fa030611..84660ad4b9 100644 --- a/extension/src/experiments/model/collect.ts +++ b/extension/src/experiments/model/collect.ts @@ -300,6 +300,9 @@ const setWorkspaceAsRunning = ( ) { acc.workspace.executor = Executor.WORKSPACE acc.workspace.status = ExperimentStatus.RUNNING + } + + if (dvcLiveOnly) { acc.runningExperiments.unshift({ executor: Executor.WORKSPACE, id: EXPERIMENT_WORKSPACE_ID diff --git a/extension/src/experiments/model/index.test.ts b/extension/src/experiments/model/index.test.ts index b305b2b893..5de1f764fd 100644 --- a/extension/src/experiments/model/index.test.ts +++ b/extension/src/experiments/model/index.test.ts @@ -221,7 +221,7 @@ describe('ExperimentsModel', () => { expect(experimentsModel.getSelectedRevisions()).toHaveLength(7) }) - it('should swap an experiment running in the workspace for the workspace and not select an experiment running in the queue', () => { + it('should not swap an experiment running in the workspace for the workspace and not prevent selection of an experiment running in the queue', () => { const params = { params: { 'params.yaml': { @@ -231,6 +231,9 @@ describe('ExperimentsModel', () => { } } } + const runningExpName = 'selectable-nuffy' + const runningTaskName = 'selectable-task' + const data = generateTestExpShowOutput( {}, { @@ -242,7 +245,7 @@ describe('ExperimentsModel', () => { name: Executor.WORKSPACE, state: ExperimentStatus.RUNNING }, - name: 'unselectable-nuffy', + name: runningExpName, rev: EXPERIMENT_WORKSPACE_ID }, {}, @@ -255,7 +258,7 @@ describe('ExperimentsModel', () => { name: Executor.DVC_TASK, state: ExperimentStatus.RUNNING }, - name: 'unselectable-task', + name: runningTaskName, rev: EXPERIMENT_WORKSPACE_ID } ], @@ -267,7 +270,7 @@ describe('ExperimentsModel', () => { model.transformAndSet(data, false, '') expect(model.getSelectedRevisions().map(({ id }) => id)).toStrictEqual([ - EXPERIMENT_WORKSPACE_ID + runningExpName ]) model.setSelected([]) @@ -277,9 +280,11 @@ describe('ExperimentsModel', () => { expect(model.getSelectedRevisions().map(({ id }) => id)).toStrictEqual([ EXPERIMENT_WORKSPACE_ID, 'testBranch', + runningExpName, 'exp-2', 'exp-3', - 'exp-4' + 'exp-4', + runningTaskName ]) }) diff --git a/extension/src/experiments/model/index.ts b/extension/src/experiments/model/index.ts index 77655aee38..edf6b57b46 100644 --- a/extension/src/experiments/model/index.ts +++ b/extension/src/experiments/model/index.ts @@ -9,7 +9,6 @@ import { } from './collect' import { collectColoredStatus, - collectFinishedRunningExperiments, collectSelectable, collectSelectedColors, collectStartedRunningExperiments @@ -21,7 +20,6 @@ import { Experiment, isQueued, isRunning, - isRunningInQueue, RunningExperiment } from '../webview/contract' import { definedAndNonEmpty, reorderListSubset } from '../../util/array' @@ -146,10 +144,6 @@ export class ExperimentsModel extends ModelWithPersistence { ({ id: expId }) => expId === id ) - if (experiment && isRunning(experiment.status)) { - return this.preventSelectionOfRunningExperiment(experiment) - } - if (isQueued(experiment?.status)) { return UNSELECTED } @@ -270,9 +264,7 @@ export class ExperimentsModel extends ModelWithPersistence { } public setSelected(selectedExperiments: Experiment[]) { - const possibleToSelect = collectSelectable(selectedExperiments, { - ...this.workspace - }) + const possibleToSelect = collectSelectable(selectedExperiments) const { availableColors, coloredStatus } = collectSelectedColors( possibleToSelect, @@ -524,8 +516,7 @@ export class ExperimentsModel extends ModelWithPersistence { this.experimentsByCommit, this.coloredStatus, this.availableColors, - this.startedRunning, - this.finishedRunning + this.startedRunning ) this.startedRunning = new Set() @@ -540,14 +531,6 @@ export class ExperimentsModel extends ModelWithPersistence { stillRunning ) - this.finishedRunning = collectFinishedRunningExperiments( - { ...this.finishedRunning }, - this.getExperimentsAndQueued(), - this.running, - stillRunning, - this.coloredStatus - ) - this.running = stillRunning } @@ -568,24 +551,6 @@ export class ExperimentsModel extends ModelWithPersistence { ) } - private preventSelectionOfRunningExperiment( - experiment: Experiment - ): Color | undefined | typeof UNSELECTED { - if (isRunningInQueue(experiment)) { - return UNSELECTED - } - - const { executor, id } = experiment - if ( - executor === Executor.WORKSPACE && - id !== EXPERIMENT_WORKSPACE_ID && - !this.isSelected(id) && - !this.isSelected(EXPERIMENT_WORKSPACE_ID) - ) { - return this.toggleStatus(EXPERIMENT_WORKSPACE_ID) - } - } - private persistSorts() { return this.persist(PersistenceKey.EXPERIMENTS_SORT_BY, this.currentSorts) } diff --git a/extension/src/experiments/model/status/collect.test.ts b/extension/src/experiments/model/status/collect.test.ts index c0a1e1c571..50d0ca5b97 100644 --- a/extension/src/experiments/model/status/collect.test.ts +++ b/extension/src/experiments/model/status/collect.test.ts @@ -1,14 +1,10 @@ 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 + EXPERIMENT_WORKSPACE_ID } from '../../../cli/dvc/contract' describe('collectColoredStatus', () => { @@ -31,8 +27,7 @@ describe('collectColoredStatus', () => { new Map(), {}, copyOriginalColors(), - new Set(), - {} + new Set() ) expect(availableColors).toStrictEqual(colors) @@ -53,8 +48,7 @@ describe('collectColoredStatus', () => { new Map(), {}, copyOriginalColors(), - new Set(), - {} + new Set() ) expect(availableColors).toStrictEqual(colors) @@ -80,8 +74,7 @@ describe('collectColoredStatus', () => { exp7: colors[6] }, [], - new Set(['exp8']), - {} + new Set(['exp8']) ) expect(availableColors).toStrictEqual([]) @@ -114,8 +107,7 @@ describe('collectColoredStatus', () => { exp8: UNSELECTED }, copyOriginalColors().slice(3), - new Set(['exp1']), - {} + new Set(['exp1']) ) expect(coloredStatus).toStrictEqual({ exp1: colors[0] }) @@ -137,8 +129,7 @@ describe('collectColoredStatus', () => { exp9: colors[1] }, unassignedColors, - new Set(), - {} + new Set() ) expect(coloredStatus).toStrictEqual({ @@ -166,8 +157,7 @@ describe('collectColoredStatus', () => { new Map(), { exp9: colors[0] }, unassignColors, - new Set(), - {} + new Set() ) expect(availableColors).toStrictEqual(unassignColors) @@ -200,8 +190,7 @@ describe('collectColoredStatus', () => { exp9: colors[5] }, copyOriginalColors().slice(6), - new Set(['exp1', 'exp2', 'exp3']), - {} + new Set(['exp1', 'exp2', 'exp3']) ) expect(availableColors).toStrictEqual([]) @@ -218,96 +207,6 @@ describe('collectColoredStatus', () => { }) }) - 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.filter(color => ![colors[2], colors[0]].includes(color)) - ) - }) - it('should prevent colors being available when they are already assigned', () => { const colors = copyOriginalColors() const selectedColor = colors[2] @@ -329,8 +228,7 @@ describe('collectColoredStatus', () => { workspace: UNSELECTED }, colors, - new Set(), - { 'exp-1': EXPERIMENT_WORKSPACE_ID } + new Set() ) expect(coloredStatus).toStrictEqual({ 'exp-1': selectedColor, @@ -343,111 +241,3 @@ describe('collectColoredStatus', () => { ) }) }) - -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 and unqueued (without a status) experiment if there is no longer an experiment running in the workspace', () => { - const latestCreatedId = 'exp-123' - const finishedRunning = collectFinishedRunningExperiments( - {}, - [ - { - Created: '2023-12-01T10:48:24', - id: 'exp-queued', - status: ExperimentStatus.QUEUED - }, - { 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 556540106f..f889585b7b 100644 --- a/extension/src/experiments/model/status/collect.ts +++ b/extension/src/experiments/model/status/collect.ts @@ -7,16 +7,10 @@ import { } from '.' import { Color, copyOriginalColors } from './colors' import { hasKey } from '../../../util/object' -import { - Experiment, - isQueued, - isRunning, - isRunningInQueue, - RunningExperiment -} from '../../webview/contract' +import { Experiment, isQueued, RunningExperiment } from '../../webview/contract' import { definedAndNonEmpty, reorderListSubset } from '../../../util/array' import { flattenMapValues } from '../../../util/map' -import { Executor, EXPERIMENT_WORKSPACE_ID } from '../../../cli/dvc/contract' +import { Executor } from '../../../cli/dvc/contract' const canAssign = ( coloredStatus: ColoredStatus, @@ -69,33 +63,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 - } -} - const unassignColors = ( experiments: Experiment[], current: ColoredStatus, @@ -122,8 +89,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) let availableColors = unassignColors( @@ -150,11 +116,8 @@ export const collectColoredStatus = ( ...flatExperimentsByCommit ]) { collectStatus(coloredStatus, experiment) - removeUnselectedExperimentRunningInWorkspace(coloredStatus, experiment) } - duplicateFinishedWorkspaceExperiment(coloredStatus, finishedRunning) - return { availableColors, coloredStatus @@ -201,29 +164,11 @@ const assignSelected = ( return { availableColors, coloredStatus } } -const cannotSelect = ( - ids: Set, - { executor, id, status }: Experiment -): boolean => - isQueued(status) || isRunningInQueue({ executor, status }) || ids.has(id) - -const shouldSelectWorkspace = ({ executor, status }: Experiment): boolean => - executor === Executor.WORKSPACE && isRunning(status) - -const collectWorkspace = ( - acc: Experiment[], - collectedIds: Set, - workspace: Experiment -) => { - if (!collectedIds.has(EXPERIMENT_WORKSPACE_ID)) { - acc.push(workspace) - } - collectedIds.add(EXPERIMENT_WORKSPACE_ID) -} +const cannotSelect = (ids: Set, { id, status }: Experiment): boolean => + isQueued(status) || ids.has(id) export const collectSelectable = ( - selectedExperiments: Experiment[], - workspace: Experiment + selectedExperiments: Experiment[] ): Experiment[] => { const acc: Experiment[] = [] const collectedIds = new Set() @@ -233,10 +178,6 @@ export const collectSelectable = ( continue } - if (shouldSelectWorkspace(experiment)) { - collectWorkspace(acc, collectedIds, workspace) - continue - } acc.push(experiment) collectedIds.add(experiment.id) } @@ -280,119 +221,8 @@ export const collectStartedRunningExperiments = ( ) { continue } - acc.add( - executor === EXPERIMENT_WORKSPACE_ID ? EXPERIMENT_WORKSPACE_ID : runningId - ) + acc.add(runningId) } return acc } - -const getMostRecentExperiment = ( - experiments: Experiment[], - coloredStatus: ColoredStatus -): Experiment | undefined => - experiments - .filter( - ({ id, status }) => coloredStatus[id] === undefined && !isQueued(status) - ) - .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 - ) - ) { - continue - } - - if (previouslyRunningExecutor === EXPERIMENT_WORKSPACE_ID) { - collectFinishedWorkspaceExperiment( - acc, - getMostRecentExperiment(experiments, coloredStatus) - ) - continue - } - acc[previouslyRunningId] = previouslyRunningId - } - 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 - ) -} diff --git a/extension/src/plots/webview/messages.ts b/extension/src/plots/webview/messages.ts index d6c1a2b5c8..7d02d8f8a4 100644 --- a/extension/src/plots/webview/messages.ts +++ b/extension/src/plots/webview/messages.ts @@ -69,10 +69,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 98e4a65eaf..18862342a3 100644 --- a/extension/src/test/fixtures/expShow/base/rows.ts +++ b/extension/src/test/fixtures/expShow/base/rows.ts @@ -42,7 +42,7 @@ const rowsFixture: Commit[] = [ 'c3961d777cfbd7727f9fde4851896006' ) }, - displayColor: colorsList[0], + displayColor: undefined, executor: Executor.WORKSPACE, id: EXPERIMENT_WORKSPACE_ID, label: EXPERIMENT_WORKSPACE_ID, @@ -69,7 +69,7 @@ const rowsFixture: Commit[] = [ } }, status: ExperimentStatus.RUNNING, - selected: true, + selected: false, starred: false }, { @@ -269,7 +269,7 @@ const rowsFixture: Commit[] = [ 'c3961d777cfbd7727f9fde4851896006' ) }, - displayColor: undefined, + displayColor: colorsList[0], description: '[exp-83425]', executor: Executor.WORKSPACE, id: 'exp-83425', @@ -296,7 +296,7 @@ const rowsFixture: 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 da7d0e272f..4027bb5b68 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 } from '../../../cli/dvc/contract' import * as Time from '../../../util/time' import { AvailableCommands } from '../../../commands/internal' import { Setup } from '../../../setup' @@ -931,8 +927,8 @@ suite('Experiments Test Suite', () => { expect( isExperimentSelected(runningInQueueId), - 'experiment running the queue cannot be selected' - ).to.be.false + 'experiment running in the queue can be selected' + ).to.be.true mockMessageReceived.fire({ payload: queuedId, @@ -1159,8 +1155,8 @@ suite('Experiments Test Suite', () => { const mockMessageReceived = getMessageReceivedEmitter(webview) const queuedId = '90aea7f' const runningInQueueId = 'exp-e7a67' - const expectedIds = ['main', 'test-branch'] - const mockExperimentIds = [...expectedIds, queuedId, runningInQueueId] + const expectedIds = ['main', 'test-branch', runningInQueueId] + const mockExperimentIds = [...expectedIds, queuedId] stubWorkspaceExperimentsGetters(dvcDemoPath, experiments) @@ -1177,7 +1173,7 @@ suite('Experiments Test Suite', () => { .getSelectedRevisions() .map(({ id }) => id) .sort() - mockExperimentIds.sort() + expectedIds.sort() expect( selectExperimentIds, 'should exclude queued experiments and experiments running in the queue from selection' @@ -1204,14 +1200,14 @@ suite('Experiments Test Suite', () => { const webview = await experiments.showWebview() const mockMessageReceived = getMessageReceivedEmitter(webview) const runningInQueueId = 'exp-e7a67' - const mockExperimentIds = ['main', 'test-branch'] + const mockExperimentIds = ['main', 'test-branch', runningInQueueId] stubWorkspaceExperimentsGetters(dvcDemoPath, experiments) const tableChangePromise = experimentsUpdatedEvent(experiments) mockMessageReceived.fire({ - payload: [...mockExperimentIds, runningInQueueId], + payload: mockExperimentIds, type: MessageFromWebviewType.SET_EXPERIMENTS_AND_OPEN_PLOTS }) @@ -1692,11 +1688,12 @@ suite('Experiments Test Suite', () => { ).to.deep.equal({ '489fd8b': 0, '55d492c': 0, + 'exp-83425': colors[0], 'exp-e7a67': 0, 'exp-f13bca': 0, main: 0, 'test-branch': 0, - [EXPERIMENT_WORKSPACE_ID]: colors[0] + [EXPERIMENT_WORKSPACE_ID]: 0 }) expect( @@ -1787,11 +1784,12 @@ suite('Experiments Test Suite', () => { ).to.deep.equal({ '489fd8b': 0, '55d492c': 0, + 'exp-83425': colors[0], 'exp-e7a67': 0, 'exp-f13bca': colors[1], main: 0, 'test-branch': 0, - [EXPERIMENT_WORKSPACE_ID]: colors[0] + [EXPERIMENT_WORKSPACE_ID]: 0 }) }) @@ -1802,7 +1800,7 @@ suite('Experiments Test Suite', () => { 'experimentsFilterBy:test': filterMapEntries, 'experimentsSortBy:test': sortDefinitions, 'experimentsStatus:test': { - [EXPERIMENT_WORKSPACE_ID]: colors[0], + 'exp-83425': colors[0], 'exp-e7a67': colors[5], 'exp-f13bca': 0, 'test-branch': colors[1] @@ -1841,7 +1839,7 @@ suite('Experiments Test Suite', () => { ).to.deep.equal([ { displayColor: colors[0], - id: EXPERIMENT_WORKSPACE_ID + id: 'exp-83425' }, { displayColor: colors[1], @@ -2007,121 +2005,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.hasRunningWorkspaceExperiment(), - '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/webview/src/experiments/components/table/body/RowContextMenu.tsx b/webview/src/experiments/components/table/body/RowContextMenu.tsx index fae29c143d..4cfaab6750 100644 --- a/webview/src/experiments/components/table/body/RowContextMenu.tsx +++ b/webview/src/experiments/components/table/body/RowContextMenu.tsx @@ -56,7 +56,6 @@ const collectDisabledOptions = ( const starredExperimentIds: string[] = [] const unstarredExperimentIds: string[] = [] let disableExperimentOnlyOption = false - let disablePlotOption = false let disableStopOption = false for (const { row } of selectedRowsList) { @@ -78,16 +77,13 @@ const collectDisabledOptions = ( disableExperimentOnlyOption = true } - if (isRunning(status)) { - disablePlotOption = true - continue + if (!isRunning(status)) { + disableStopOption = true } - disableStopOption = true } return { disableExperimentOnlyOption, - disablePlotOption, disableStopOption, selectedIds, starredExperimentIds, @@ -101,7 +97,6 @@ const getMultiSelectMenuOptions = ( ) => { const { disableExperimentOnlyOption, - disablePlotOption, disableStopOption, selectedIds, starredExperimentIds, @@ -126,14 +121,14 @@ const getMultiSelectMenuOptions = ( ids, 'Plot', MessageFromWebviewType.SET_EXPERIMENTS_FOR_PLOTS, - disablePlotOption, + false, true ), experimentMenuOption( ids, 'Plot and Show', MessageFromWebviewType.SET_EXPERIMENTS_AND_OPEN_PLOTS, - disablePlotOption, + false, false ), experimentMenuOption( From 18e6e37ff3c11e47198c8b4dd7b01a967ae7c2fd Mon Sep 17 00:00:00 2001 From: Matt Seddon Date: Thu, 25 May 2023 13:57:45 +1000 Subject: [PATCH 2/2] remove more dead code --- extension/src/experiments/index.ts | 8 -------- extension/src/experiments/model/index.ts | 20 -------------------- extension/src/plots/model/index.ts | 2 -- 3 files changed, 30 deletions(-) diff --git a/extension/src/experiments/index.ts b/extension/src/experiments/index.ts index 581fb2cf2a..8214f904e1 100644 --- a/extension/src/experiments/index.ts +++ b/extension/src/experiments/index.ts @@ -417,14 +417,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 edf6b57b46..2fa89c9226 100644 --- a/extension/src/experiments/model/index.ts +++ b/extension/src/experiments/model/index.ts @@ -24,7 +24,6 @@ import { } from '../webview/contract' import { definedAndNonEmpty, reorderListSubset } from '../../util/array' import { - EXPERIMENT_WORKSPACE_ID, Executor, ExpShowOutput, ExperimentStatus @@ -66,7 +65,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) { @@ -160,10 +158,6 @@ export class ExperimentsModel extends ModelWithPersistence { return this.coloredStatus[id] } - public unselectWorkspace() { - this.coloredStatus[EXPERIMENT_WORKSPACE_ID] = UNSELECTED - } - public hasRunningExperiment() { return this.running.length > 0 } @@ -176,16 +170,6 @@ export class ExperimentsModel extends ModelWithPersistence { 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) } @@ -424,10 +408,6 @@ export class ExperimentsModel extends ModelWithPersistence { })) } - public getFinishedExperiments() { - return this.finishedRunning - } - public setNbfCommitsToShow(numberOfCommitsToShow: number, branch: string) { this.numberOfCommitsToShow[branch] = numberOfCommitsToShow this.persistNbOfCommitsToShow() diff --git a/extension/src/plots/model/index.ts b/extension/src/plots/model/index.ts index a739eefaec..6e564ecb81 100644 --- a/extension/src/plots/model/index.ts +++ b/extension/src/plots/model/index.ts @@ -113,8 +113,6 @@ export class PlotsModel extends ModelWithPersistence { this.fetchedRevs = new Set(revs) - this.experiments.setRevisionCollected(revs) - this.deferred.resolve() }