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.

Sign up for GitHub

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

Fix auto-selection of running experiments #3705

Closed
wants to merge 1 commit into from
Closed

Fix auto-selection of running experiments #3705

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
34 changes: 1 addition & 33 deletions extension/src/experiments/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand All @@ -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'
Expand Down Expand Up @@ -217,15 +213,6 @@ export class Experiments extends BaseRepository<TableData> {
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
Expand All @@ -236,17 +223,6 @@ export class Experiments extends BaseRepository<TableData> {
return status
}

public checkForFinishedWorkspaceExperiment(
fetchedExperiments: FetchedExperiment[]
) {
if (!hasFinishedWorkspaceExperiment(fetchedExperiments)) {
return
}

this.experiments.unselectWorkspace()
this.notifyChanged()
}

public getSorts() {
return this.experiments.getSorts()
}
Expand Down Expand Up @@ -438,14 +414,6 @@ export class Experiments extends BaseRepository<TableData> {
return this.experiments.getSelectedRevisions()
}

public setRevisionCollected(revisions: string[]) {
this.experiments.setRevisionCollected(revisions)
}

public getFinishedExperiments() {
return this.experiments.getFinishedExperiments()
}

public getExperiments() {
return this.experiments.getExperiments()
}
Expand Down
64 changes: 13 additions & 51 deletions extension/src/experiments/model/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import { FilterDefinition, filterExperiment, getFilterId } from './filterBy'
import { collectExperiments } from './collect'
import {
collectColoredStatus,
collectFinishedRunningExperiments,
collectSelected,
collectStartedRunningExperiments
} from './status/collect'
Expand All @@ -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'
Expand Down Expand Up @@ -62,7 +61,6 @@ export class ExperimentsModel extends ModelWithPersistence {

private currentSorts: SortDefinition[]
private running: RunningExperiment[] = []
private finishedRunning: { [id: string]: string } = {}
private startedRunning: Set<string> = new Set()

constructor(dvcRoot: string, workspaceState: Memento) {
Expand Down Expand Up @@ -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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While nothing happens, I can still select an experiment running in the queue when I run the Select Experiments to Display in Plots command

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is in the demo and expected as it is a patch for upcoming behaviour. As I have to redo most of this PR I will remove them from selection.

) {
return
return UNSELECTED
}

const current = this.coloredStatus[id]
Expand All @@ -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)
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -389,10 +366,6 @@ export class ExperimentsModel extends ModelWithPersistence {
}))
}

public getFinishedExperiments() {
return this.finishedRunning
}

public setNbfCommitsToShow(numberOfCommitsToShow: number) {
this.numberOfCommitsToShow = numberOfCommitsToShow
this.persistNbOfCommitsToShow()
Expand Down Expand Up @@ -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()
Expand All @@ -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
}

Expand Down
Loading