Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to stop experiments running in the queue into stop experiments command #3157

Merged
merged 3 commits into from
Jan 25, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions extension/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -412,8 +412,8 @@
"category": "DVC"
},
{
"title": "%command.stopRunningExperiment%",
"command": "dvc.stopRunningExperiment",
"title": "%command.stopAllRunningExperiments%",
"command": "dvc.stopAllRunningExperiments",
"category": "DVC",
"icon": "$(debug-stop)"
},
Expand Down Expand Up @@ -828,7 +828,7 @@
"when": "dvc.commands.available && dvc.project.available"
},
{
"command": "dvc.stopRunningExperiment",
"command": "dvc.stopAllRunningExperiments",
"when": "dvc.commands.available && dvc.project.available && dvc.experiment.stoppable"
},
{
Expand Down Expand Up @@ -1000,7 +1000,7 @@
"when": "dvc.experiments.webview.active && !dvc.experiment.running && dvc.commands.available && !dvc.experiment.checkpoints"
},
{
"command": "dvc.stopRunningExperiment",
"command": "dvc.stopAllRunningExperiments",
"group": "navigation@0",
"when": "dvc.experiment.stoppable && dvc.commands.available"
},
Expand Down Expand Up @@ -1219,12 +1219,12 @@
"group": "navigation@2"
},
{
"command": "dvc.stopRunningExperiment",
"command": "dvc.stopAllRunningExperiments",
"when": "view == dvc.views.experimentsTree && !dvc.experiments.webview.active && dvc.experiment.stoppable",
"group": "1_run@1"
},
{
"command": "dvc.stopRunningExperiment",
"command": "dvc.stopAllRunningExperiments",
"when": "view == dvc.views.experimentsTree && dvc.experiments.webview.active && dvc.experiment.stoppable",
"group": "navigation@1"
},
Expand Down
2 changes: 1 addition & 1 deletion extension/package.nls.json
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,9 @@
"command.showOutput": "Show DVC Output",
"command.showPlots": "Show Plots",
"command.startExperimentsQueue": "Start Processing Queued Experiments",
"command.stopAllRunningExperiments": "Stop All Running Experiments",
"command.stopExperimentsQueue": "Stop Processing Queued Experiments Gracefully (Complete Running)",
"command.stopQueuedExperiments": "Stop Running Queued Experiment(s)",
"command.stopRunningExperiment": "Stop Running Experiment",
"command.views.experimentsColumnsTree.selectColumns": "Select Columns to Display in the Experiments Table",
"command.views.experimentsFilterByTree.removeAllFilters": "Remove All Filters From Experiments Table",
"command.views.experimentsFilterByTree.removeFilter": "Remove Filter From Experiments Table",
Expand Down
1 change: 1 addition & 0 deletions extension/src/cli/dvc/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ export enum Flag {
GRANULAR = '--granular',
JOBS = '-j',
JSON = '--json',
KILL = '--kill',
NUM_COMMIT = '-n',
OUTPUT_PATH = '-o',
SUBDIRECTORY = '--subdir',
Expand Down
9 changes: 7 additions & 2 deletions extension/src/cli/dvc/executor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -149,8 +149,13 @@ export class DvcExecutor extends DvcCli {
)
}

public queueStop(cwd: string) {
return this.executeDvcProcess(cwd, Command.QUEUE, QueueSubCommand.STOP)
public queueStop(cwd: string, ...args: Args) {
return this.executeDvcProcess(
cwd,
Command.QUEUE,
QueueSubCommand.STOP,
...args
)
}

public remove(cwd: string, ...args: Args) {
Expand Down
2 changes: 1 addition & 1 deletion extension/src/commands/external.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ export enum RegisteredCommands {
EXPERIMENT_SORTS_REMOVE = 'dvc.removeExperimentsTableSorts',
EXPERIMENT_SORTS_REMOVE_ALL = 'dvc.views.experimentsSortByTree.removeAllSorts',
EXPERIMENT_TOGGLE = 'dvc.views.experiments.toggleStatus',
STOP_EXPERIMENT = 'dvc.stopRunningExperiment',
STOP_EXPERIMENTS = 'dvc.stopAllRunningExperiments',

PLOTS_PATH_TOGGLE = 'dvc.views.plotsPathsTree.toggleStatus',
PLOTS_SHOW = 'dvc.showPlots',
Expand Down
18 changes: 10 additions & 8 deletions extension/src/context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,14 @@ export class Context extends Disposable {
(experiments: Experiments) => experiments.hasRunningExperiment()
)

const hasRunningQueuedExperiment = repositories.some(experiments =>
experiments.hasRunningQueuedExperiment()
)

void setContextValue(
ContextKey.EXPERIMENT_STOPPABLE,
await this.experiments.hasDvcLiveOnlyExperimentRunning()
hasRunningQueuedExperiment ||
(await this.experiments.hasDvcLiveOnlyExperimentRunning())
)
}

Expand All @@ -78,12 +83,9 @@ export class Context extends Disposable {
repositories: Experiments[],
hasRequirement: (experiments: Experiments) => boolean
) {
for (const experiments of repositories) {
if (hasRequirement(experiments)) {
void setContextValue(contextKey, true)
return
}
}
void setContextValue(contextKey, false)
void setContextValue(
contextKey,
repositories.some(experiments => hasRequirement(experiments))
)
}
}
4 changes: 4 additions & 0 deletions extension/src/experiments/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -528,6 +528,10 @@ export class Experiments extends BaseRepository<TableData> {
return this.experiments.hasRunningExperiment()
}

public hasRunningQueuedExperiment() {
return this.experiments.getRunningQueueTasks().length > 0
}

public getFirstThreeColumnOrder() {
return this.columns.getFirstThreeColumnOrder()
}
Expand Down
6 changes: 6 additions & 0 deletions extension/src/experiments/workspace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -431,6 +431,12 @@ export class WorkspaceExperiments extends BaseWorkspaceWebviews<
return pids
}

public hasQueuedExperimentsRunning() {
return Object.values(this.repositories).some(experiments =>
experiments.hasRunningQueuedExperiment()
)
}

private async pickExpThenRun(
commandId: CommandId,
pickFunc: (
Expand Down
64 changes: 36 additions & 28 deletions extension/src/extension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ import { GitReader } from './cli/git/reader'
import { Setup } from './setup'
import { definedAndNonEmpty } from './util/array'
import { stopProcesses } from './processExecution'
import { Flag } from './cli/dvc/constants'

export class Extension extends Disposable {
protected readonly internalCommands: InternalCommands
Expand Down Expand Up @@ -194,35 +195,42 @@ export class Extension extends Disposable {
)

this.dispose.track(
commands.registerCommand(RegisteredCommands.STOP_EXPERIMENT, async () => {
const stopWatch = new StopWatch()
const dvcLiveOnlyPids = await this.experiments.getDvcLiveOnlyPids()
const wasRunning =
this.dvcRunner.isExperimentRunning() ||
definedAndNonEmpty(dvcLiveOnlyPids)
try {
const [dvcLiveOnlyStopped, runnerStopped] = await Promise.all([
stopProcesses(dvcLiveOnlyPids),
this.dvcRunner.stop()
])

const stopped = dvcLiveOnlyStopped && runnerStopped
sendTelemetryEvent(
RegisteredCommands.STOP_EXPERIMENT,
{ stopped, wasRunning },
{
duration: stopWatch.getElapsedTime()
}
)
return stopped
} catch (error: unknown) {
return sendTelemetryEventAndThrow(
RegisteredCommands.STOP_EXPERIMENT,
error as Error,
stopWatch.getElapsedTime()
)
commands.registerCommand(
RegisteredCommands.STOP_EXPERIMENTS,
async () => {
const stopWatch = new StopWatch()
const dvcLiveOnlyPids = await this.experiments.getDvcLiveOnlyPids()
const wasRunning =
this.dvcRunner.isExperimentRunning() ||
definedAndNonEmpty(dvcLiveOnlyPids) ||
this.experiments.hasQueuedExperimentsRunning()
try {
const allStopped = await Promise.all([
stopProcesses(dvcLiveOnlyPids),
this.dvcRunner.stop(),
...this.getRoots().map(dvcRoot =>
this.dvcExecutor.queueStop(dvcRoot, Flag.KILL)
)
])

const stopped = allStopped.every(Boolean)
sendTelemetryEvent(
RegisteredCommands.STOP_EXPERIMENTS,
{ stopped, wasRunning },
{
duration: stopWatch.getElapsedTime()
}
)
return stopped
} catch (error: unknown) {
return sendTelemetryEventAndThrow(
RegisteredCommands.STOP_EXPERIMENTS,
error as Error,
stopWatch.getElapsedTime()
)
}
}
})
)
)

this.internalCommands.registerExternalCommand(
Expand Down
2 changes: 1 addition & 1 deletion extension/src/telemetry/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ export interface IEventNamePropertyMapping {
[EventName.MODIFY_EXPERIMENT_PARAMS_AND_RESUME]: undefined
[EventName.MODIFY_EXPERIMENT_PARAMS_AND_RUN]: undefined
[EventName.MODIFY_EXPERIMENT_PARAMS_RESET_AND_RUN]: undefined
[EventName.STOP_EXPERIMENT]: { stopped: boolean; wasRunning: boolean }
[EventName.STOP_EXPERIMENTS]: { stopped: boolean; wasRunning: boolean }

[EventName.PLOTS_PATH_TOGGLE]: undefined
[EventName.PLOTS_SHOW]: undefined
Expand Down
55 changes: 52 additions & 3 deletions extension/src/test/suite/context.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,12 +67,14 @@ suite('Context Test Suite', () => {
const buildMockExperiments = (
filters: FilterDefinition[] = [],
sorts: SortDefinition[] = [],
experimentRunning = false
experimentRunning = false,
queuedExperimentRunning = false
) => {
return {
getFilters: () => filters,
getSorts: () => sorts,
hasRunningExperiment: () => experimentRunning
hasRunningExperiment: () => experimentRunning,
hasRunningQueuedExperiment: () => queuedExperimentRunning
}
}

Expand Down Expand Up @@ -173,7 +175,7 @@ suite('Context Test Suite', () => {
)
})

it('should set the dvc.experiment.stoppable context to whether or not there is a DVCLive only experiment running if an experiment is not running in the runner', async () => {
it('should set the dvc.experiment.stoppable context to whether or not there is a DVCLive only experiment running if an experiment is not running in the runner or queue', async () => {
const {
executeCommandSpy,
experimentsChanged,
Expand Down Expand Up @@ -220,6 +222,53 @@ suite('Context Test Suite', () => {
)
})

it('should set the dvc.experiment.stoppable context to true if an experiment is running in the queue', async () => {
const {
executeCommandSpy,
experimentsChanged,
mockGetDvcRoots,
mockGetRepository,
mockHasDvcLiveOnlyExperimentRunning,
onDidChangeExperiments
} = buildContext(false)

const queuedExperimentRunningEvent = new Promise(resolve =>
disposable.track(onDidChangeExperiments(() => resolve(undefined)))
)

const mockDvcRoot = resolve('first', 'root')
mockGetDvcRoots.returns([mockDvcRoot])
let hasRunningQueuedExperiment = true
mockGetRepository.callsFake(() =>
buildMockExperiments([], [], false, hasRunningQueuedExperiment)
)

mockHasDvcLiveOnlyExperimentRunning.resolves(false)

experimentsChanged.fire()
await queuedExperimentRunningEvent

expect(executeCommandSpy).to.be.calledWith(
'setContext',
'dvc.experiment.stoppable',
true
)
executeCommandSpy.resetHistory()

const queuedExperimentStoppedEvent = new Promise(resolve =>
disposable.track(onDidChangeExperiments(() => resolve(undefined)))
)
hasRunningQueuedExperiment = false
experimentsChanged.fire()
await queuedExperimentStoppedEvent

expect(executeCommandSpy).to.be.calledWith(
'setContext',
'dvc.experiment.stoppable',
false
)
})

it('should set the dvc.experiments.filtered context correctly depending on whether there are filters applied', async () => {
const {
executeCommandSpy,
Expand Down
19 changes: 16 additions & 3 deletions extension/src/test/suite/extension.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ import { WorkspaceExperiments } from '../../experiments/workspace'
import { GitReader } from '../../cli/git/reader'
import { MIN_CLI_VERSION } from '../../cli/dvc/contract'
import { ConfigKey, setConfigValue } from '../../vscode/config'
import { DvcExecutor } from '../../cli/dvc/executor'
import { dvcDemoPath } from '../util'
import { Setup } from '../../setup'
import { Flag } from '../../cli/dvc/constants'

suite('Extension Test Suite', () => {
const disposable = Disposable.fn()
Expand Down Expand Up @@ -240,17 +244,23 @@ suite('Extension Test Suite', () => {
}).timeout(25000)
})

describe('dvc.stopRunningExperiment', () => {
describe('dvc.stopAllRunningExperiments', () => {
it('should send a telemetry event containing properties relating to the event', async () => {
const duration = 1234
const otherRoot = resolve('other', 'root')
mockDuration(duration)

const mockSendTelemetryEvent = stub(Telemetry, 'sendTelemetryEvent')
const mockQueueStop = stub(DvcExecutor.prototype, 'queueStop').resolves(
undefined
)

stub(Setup.prototype, 'getRoots').returns([dvcDemoPath, otherRoot])

await commands.executeCommand(RegisteredCommands.STOP_EXPERIMENT)
await commands.executeCommand(RegisteredCommands.STOP_EXPERIMENTS)

expect(mockSendTelemetryEvent).to.be.calledWith(
RegisteredCommands.STOP_EXPERIMENT,
RegisteredCommands.STOP_EXPERIMENTS,
{
stopped: false,
wasRunning: false
Expand All @@ -259,6 +269,9 @@ suite('Extension Test Suite', () => {
duration
}
)

expect(mockQueueStop).to.be.calledWith(dvcDemoPath, Flag.KILL)
expect(mockQueueStop).to.be.calledWith(otherRoot, Flag.KILL)
})
})

Expand Down