Remove running checkpoint experiment workspace race condition code from plots (#2882)

* rip out race condition logic for watching checkpoint experiment plots in the workspace

* remove some more dead code

Author: mattseddon

extension/src/cli/dvc/runner.ts

@@ -20,8 +20,7 @@ import { Disposable } from '../../class/dispose'
 export const autoRegisteredCommands = {
   EXPERIMENT_RESET_AND_RUN: 'runExperimentReset',
   EXPERIMENT_RUN: 'runExperiment',
-  EXPERIMENT_RUN_QUEUED: 'runExperimentQueue',
-  IS_EXPERIMENT_RUNNING: 'isExperimentRunning'
+  EXPERIMENT_RUN_QUEUED: 'runExperimentQueue'
 } as const
 
 export class DvcRunner extends Disposable implements ICli {

extension/src/experiments/model/collect.test.ts

@@ -169,20 +169,6 @@ describe('collectMutableRevisions', () => {
     { label: 'workspace', selected: false, status: ExperimentStatus.FAILED }
   ] as Experiment[]
 
-  it('should not return the workspace when there is a selected running checkpoint experiment (race condition)', () => {
-    const experiments = [
-      {
-        label: 'exp-123',
-        selected: true,
-        status: ExperimentStatus.RUNNING
-      },
-      ...baseExperiments
-    ] as Experiment[]
-
-    const mutableRevisions = collectMutableRevisions(experiments, true)
-    expect(mutableRevisions).toStrictEqual([])
-  })
-
   it('should return the workspace when there is an unselected running checkpoint experiment', () => {
     const experiments = [
       {

extension/src/experiments/model/collect.ts

@@ -13,8 +13,7 @@ import {
   ExperimentFieldsOrError,
   ExperimentFields,
   ExperimentsBranchOutput,
-  ExperimentsOutput,
-  ExperimentStatus
+  ExperimentsOutput
 } from '../../cli/dvc/contract'
 import { addToMapArray } from '../../util/map'
 import { uniqueValues } from '../../util/array'
@@ -328,27 +327,13 @@ export const collectExperiments = (
   return acc
 }
 
-const getDefaultMutableRevision = (hasCheckpoints: boolean): string[] => {
-  if (hasCheckpoints) {
-    return []
-  }
-  return ['workspace']
-}
-
-const noWorkspaceVsSelectedRaceCondition = (
-  hasCheckpoints: boolean,
-  status: ExperimentStatus | undefined,
-  selected: boolean | undefined
-): boolean => !!(hasCheckpoints && isRunning(status) && !selected)
+const getDefaultMutableRevision = (): string[] => ['workspace']
 
 const collectMutableRevision = (
   acc: string[],
-  { label, status, selected }: Experiment,
+  { label, status }: Experiment,
   hasCheckpoints: boolean
 ) => {
-  if (noWorkspaceVsSelectedRaceCondition(hasCheckpoints, status, selected)) {
-    acc.push('workspace')
-  }
   if (isRunning(status) && !hasCheckpoints) {
     acc.push(label)
   }
@@ -358,7 +343,7 @@ export const collectMutableRevisions = (
   experiments: Experiment[],
   hasCheckpoints: boolean
 ): string[] => {
-  const acc = getDefaultMutableRevision(hasCheckpoints)
+  const acc = getDefaultMutableRevision()
 
   for (const experiment of experiments) {
     collectMutableRevision(acc, experiment, hasCheckpoints)

extension/src/plots/data/index.ts

@@ -3,11 +3,7 @@ import { collectFiles } from './collect'
 import { PlotsOutputOrError } from '../../cli/dvc/contract'
 import { AvailableCommands, InternalCommands } from '../../commands/internal'
 import { BaseData } from '../../data'
-import {
-  definedAndNonEmpty,
-  flattenUnique,
-  sameContents
-} from '../../util/array'
+import { flattenUnique, sameContents } from '../../util/array'
 import { PlotsModel } from '../model'
 
 export class PlotsData extends BaseData<{
@@ -43,15 +39,6 @@ export class PlotsData extends BaseData<{
       this.model.getMutableRevisions()
     ])
 
-    if (
-      (await this.internalCommands.executeCommand<boolean>(
-        AvailableCommands.IS_EXPERIMENT_RUNNING
-      )) &&
-      !definedAndNonEmpty(revs)
-    ) {
-      return
-    }
-
     const args = this.getArgs(revs)
     const data = await this.internalCommands.executeCommand<PlotsOutputOrError>(
       AvailableCommands.PLOTS_DIFF,

extension/src/plots/model/collect.test.ts

@@ -1,27 +1,19 @@
 import { join } from 'path'
 import omit from 'lodash.omit'
 import isEmpty from 'lodash.isempty'
-import cloneDeep from 'lodash.clonedeep'
-import merge from 'lodash.merge'
 import {
   collectData,
   collectCheckpointPlotsData,
   collectTemplates,
   collectMetricOrder,
-  collectWorkspaceRunningCheckpoint,
-  collectWorkspaceRaceConditionData,
   collectOverrideRevisionDetails
 } from './collect'
 import plotsDiffFixture from '../../test/fixtures/plotsDiff/output'
 import expShowFixture from '../../test/fixtures/expShow/base/output'
 import modifiedFixture from '../../test/fixtures/expShow/modified/output'
 import checkpointPlotsFixture from '../../test/fixtures/expShow/base/checkpointPlots'
 import { ExperimentsOutput, ExperimentStatus } from '../../cli/dvc/contract'
-import {
-  definedAndNonEmpty,
-  sameContents,
-  uniqueValues
-} from '../../util/array'
+import { definedAndNonEmpty, sameContents } from '../../util/array'
 import { TemplatePlot } from '../webview/contract'
 import { getCLIBranchId } from '../../test/fixtures/plotsDiff/util'
 import { SelectedExperimentWithColor } from '../../experiments/model'
@@ -83,34 +75,6 @@ describe('collectCheckpointPlotsData', () => {
   })
 })
 
-describe('collectWorkspaceRunningCheckpoint', () => {
-  const fixtureCopy = cloneDeep(expShowFixture)
-  const runningCheckpointFixture: ExperimentsOutput = merge(fixtureCopy, {
-    '53c3851f46955fa3e2b8f6e1c52999acc8c9ea77': {
-      '4fb124aebddb2adf1545030907687fa9a4c80e70': {
-        data: {
-          executor: 'workspace'
-        }
-      }
-    }
-  })
-
-  it('should return the expected sha from the test fixture', () => {
-    const checkpointRunningInTheWorkspace = collectWorkspaceRunningCheckpoint(
-      runningCheckpointFixture,
-      true
-    )
-
-    expect(checkpointRunningInTheWorkspace).toStrictEqual('4fb124a')
-  })
-
-  it('should always return undefined when there are no checkpoints', () => {
-    expect(
-      collectWorkspaceRunningCheckpoint(runningCheckpointFixture, false)
-    ).toBeUndefined()
-  })
-})
-
 describe('collectMetricOrder', () => {
   it('should return an empty array if there is no checkpoints data', () => {
     const metricOrder = collectMetricOrder(
@@ -302,66 +266,6 @@ describe('collectTemplates', () => {
   })
 })
 
-describe('collectWorkspaceRaceConditionData', () => {
-  const { comparisonData, revisionData } = collectData(plotsDiffFixture, {
-    '1ba7bcd': '1ba7bcd',
-    '42b8736': '42b8736',
-    '4fb124a': '4fb124a',
-    '53c3851': 'main',
-    workspace: 'workspace'
-  })
-
-  it('should return no overwrite data if there is no selected checkpoint experiment running in the workspace', () => {
-    const { overwriteComparisonData, overwriteRevisionData } =
-      collectWorkspaceRaceConditionData(undefined, comparisonData, revisionData)
-    expect(overwriteComparisonData).toStrictEqual({})
-    expect(overwriteRevisionData).toStrictEqual({})
-  })
-
-  it('should return no overwrite data if there is no data relating to the requested checkpoint', () => {
-    const { overwriteComparisonData, overwriteRevisionData } =
-      collectWorkspaceRaceConditionData('7c500fd', comparisonData, revisionData)
-    expect(overwriteComparisonData).toStrictEqual({})
-    expect(overwriteRevisionData).toStrictEqual({})
-  })
-
-  it('should return the appropriate overwrite data if the revision exists inside of the given data', () => {
-    const { overwriteComparisonData, overwriteRevisionData } =
-      collectWorkspaceRaceConditionData('4fb124a', comparisonData, revisionData)
-
-    expect(overwriteComparisonData.workspace).toStrictEqual(
-      comparisonData['4fb124a']
-    )
-
-    expect(overwriteRevisionData.workspace).not.toStrictEqual(
-      revisionData['4fb124a']
-    )
-
-    const allWorkspaceValues = Object.values(overwriteRevisionData.workspace)
-    const allOverwriteValues = Object.values(revisionData['4fb124a'])
-
-    expect(allWorkspaceValues.length).toBeTruthy()
-    expect(allWorkspaceValues).toHaveLength(allOverwriteValues.length)
-
-    const allWorkspaceRevValues: string[] = []
-    const allWorkspaceNonRevValues: Record<string, unknown>[] = []
-
-    for (const values of allWorkspaceValues) {
-      for (const { rev, ...rest } of values) {
-        allWorkspaceRevValues.push(rev as string)
-        allWorkspaceNonRevValues.push(rest)
-      }
-    }
-
-    expect(uniqueValues(allWorkspaceRevValues)).toStrictEqual(['workspace'])
-    expect(allWorkspaceNonRevValues).toStrictEqual(
-      allOverwriteValues.flatMap(values =>
-        values.map(value => omit(value, 'rev'))
-      )
-    )
-  })
-})
-
 describe('collectOverrideRevisionDetails', () => {
   it('should override the revision details for running checkpoint tips', () => {
     const runningId = 'b'

extension/src/plots/model/collect.ts

@@ -1,4 +1,3 @@
-import cloneDeep from 'lodash.clonedeep'
 import omit from 'lodash.omit'
 import { TopLevelSpec } from 'vega-lite'
 import {
@@ -16,7 +15,6 @@ import {
 } from '../webview/contract'
 import {
   ExperimentFieldsOrError,
-  ExperimentsBranchOutput,
   ExperimentsOutput,
   ExperimentStatus,
   isValueTree,
@@ -241,35 +239,6 @@ export const collectCheckpointPlotsData = (
   return plotsData
 }
 
-const isRunningInWorkspace = (experiment: ExperimentFieldsOrError) =>
-  experiment.data?.executor === 'workspace'
-
-const collectRunningFromBranch = (
-  experimentsObject: ExperimentsBranchOutput
-): string | undefined => {
-  for (const [sha, experiment] of Object.entries(experimentsObject)) {
-    if (isRunningInWorkspace(experiment)) {
-      return shortenForLabel(sha)
-    }
-  }
-}
-
-export const collectWorkspaceRunningCheckpoint = (
-  data: ExperimentsOutput,
-  hasCheckpoints: boolean
-): string | undefined => {
-  if (!hasCheckpoints) {
-    return
-  }
-  for (const experimentsObject of Object.values(omit(data, 'workspace'))) {
-    const checkpointRunningInWorkspace =
-      collectRunningFromBranch(experimentsObject)
-    if (checkpointRunningInWorkspace) {
-      return checkpointRunningInWorkspace
-    }
-  }
-}
-
 type MetricOrderAccumulator = {
   newOrder: string[]
   uncollectedMetrics: string[]
@@ -437,54 +406,6 @@ export const collectData = (
   return acc
 }
 
-const collectWorkspaceRevisionData = (
-  overwriteRevisionData: RevisionPathData
-) => {
-  const acc: RevisionPathData = {}
-
-  for (const [path, values] of Object.entries(overwriteRevisionData)) {
-    acc[path] = []
-    for (const value of values) {
-      acc[path].push({ ...value, rev: 'workspace' })
-    }
-  }
-
-  return acc
-}
-
-export const collectWorkspaceRaceConditionData = (
-  runningSelectedCheckpoint: string | undefined,
-  comparisonData: ComparisonData,
-  revisionData: RevisionData
-): {
-  overwriteComparisonData: ComparisonData
-  overwriteRevisionData: RevisionData
-} => {
-  if (!runningSelectedCheckpoint) {
-    return { overwriteComparisonData: {}, overwriteRevisionData: {} }
-  }
-
-  const overwriteComparisonData = cloneDeep(
-    comparisonData[runningSelectedCheckpoint]
-  )
-  const overwriteRevisionData = cloneDeep(
-    revisionData[runningSelectedCheckpoint]
-  )
-
-  if (!overwriteComparisonData && !overwriteRevisionData) {
-    return { overwriteComparisonData: {}, overwriteRevisionData: {} }
-  }
-
-  const workspaceRevisionData = collectWorkspaceRevisionData(
-    overwriteRevisionData
-  )
-
-  return {
-    overwriteComparisonData: { workspace: overwriteComparisonData },
-    overwriteRevisionData: { workspace: workspaceRevisionData }
-  }
-}
-
 export type TemplateAccumulator = { [path: string]: string }
 
 const collectTemplate = (