Remove checkpoints from experiment collection (#3639)

Author: mattseddon

extension/src/experiments/columns/collect/index.ts

@@ -16,6 +16,7 @@ import {
 import { standardizePath } from '../../../fileSystem/path'
 import { timestampColumn } from '../constants'
 import { sortCollectedArray } from '../../../util/array'
+import { isCheckpoint } from '../../model/collect'
 
 const collectFromExperiment = (
   acc: ColumnAccumulator,
@@ -34,7 +35,10 @@ const collectFromCommit = (
 ) => {
   const { baseline, ...rest } = commit
   collectFromExperiment(acc, baseline)
-  for (const experiment of Object.values(rest)) {
+  for (const [sha, experiment] of Object.entries(rest)) {
+    if (isCheckpoint(experiment.data?.checkpoint_tip, sha)) {
+      continue
+    }
     collectFromExperiment(acc, experiment)
   }
 }

extension/src/experiments/model/accumulator.ts

@@ -4,7 +4,6 @@ import { Experiment, isRunning, RunningExperiment } from '../webview/contract'
 export class ExperimentsAccumulator {
   public workspace = {} as Experiment
   public commits: Experiment[] = []
-  public checkpointsByTip: Map<string, Experiment[]> = new Map()
   public experimentsByCommit: Map<string, Experiment[]> = new Map()
   public runningExperiments: RunningExperiment[]
 

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

@@ -1,6 +1,4 @@
 import { collectExperiments } from './collect'
-import { Experiment } from '../webview/contract'
-import modifiedFixture from '../../test/fixtures/expShow/modified/output'
 import { EXPERIMENT_WORKSPACE_ID } from '../../cli/dvc/contract'
 import { COMMITS_SEPARATOR } from '../../cli/git/constants'
 
@@ -92,10 +90,8 @@ describe('collectExperiments', () => {
       `a123\nJohn Smith\n3 days ago\nrefNames:tag: v.1.1\nmessage:add new feature${COMMITS_SEPARATOR}b123\nrenovate[bot]\n5 weeks ago\nrefNames:\nmessage:update various dependencies\n* update dvc\n* update dvclive`
     )
     const [branch1, branch2] = commits
-    expect(branch1.displayNameOrParent).toStrictEqual('add new feature')
-    expect(branch2.displayNameOrParent).toStrictEqual(
-      'update various dependencies ...'
-    )
+    expect(branch1.displayName).toStrictEqual('add new feature')
+    expect(branch2.displayName).toStrictEqual('update various dependencies ...')
     expect(branch1.commit).toStrictEqual({
       author: 'John Smith',
       date: '3 days ago',
@@ -137,96 +133,4 @@ describe('collectExperiments', () => {
     )
     expect(experimentsByCommit.size).toStrictEqual(1)
   })
-
-  it('should find three checkpoints on the tip', () => {
-    const { checkpointsByTip } = collectExperiments(
-      repoWithNestedCheckpoints,
-      false,
-      ''
-    )
-    const checkpoints = checkpointsByTip.get('tip1') as Experiment[]
-
-    expect(checkpoints?.length).toStrictEqual(3)
-  })
-
-  it('should find checkpoints in the correct order', () => {
-    const { checkpointsByTip } = collectExperiments(
-      repoWithNestedCheckpoints,
-      false,
-      ''
-    )
-    const checkpoints = checkpointsByTip.get('tip1') as Experiment[]
-    const [tip1cp1, tip1cp2, tip1cp3] = checkpoints
-    expect(tip1cp1.id).toStrictEqual('tip1cp1')
-    expect(tip1cp2.id).toStrictEqual('tip1cp2')
-    expect(tip1cp3.id).toStrictEqual('tip1cp3')
-  })
-
-  it('should handle the continuation of a modified checkpoint', () => {
-    const { checkpointsByTip } = collectExperiments(modifiedFixture, false, '')
-
-    const modifiedCheckpointTip = checkpointsByTip
-      .get('exp-01b3a')
-      ?.filter(checkpoint => checkpoint.displayNameOrParent?.includes('('))
-
-    expect(modifiedCheckpointTip).toHaveLength(1)
-    expect(modifiedCheckpointTip?.[0].displayNameOrParent).toStrictEqual(
-      '(3b0c6ac)'
-    )
-    expect(modifiedCheckpointTip?.[0].label).toStrictEqual('7e3cb21')
-
-    const modifiedCheckpoint = checkpointsByTip
-      .get('exp-9bc1b')
-      ?.filter(checkpoint => checkpoint.displayNameOrParent?.includes('('))
-
-    expect(modifiedCheckpoint).toHaveLength(1)
-    expect(modifiedCheckpoint?.[0].displayNameOrParent).toStrictEqual(
-      '(df39067)'
-    )
-    expect(modifiedCheckpoint?.[0].label).toStrictEqual('98cb38c')
-
-    for (const checkpoints of checkpointsByTip.values()) {
-      const continuationCheckpoints = checkpoints.filter(checkpoint => {
-        const { label, displayNameOrParent } = checkpoint
-        return (
-          displayNameOrParent?.includes('(') &&
-          !(label === '7e3cb21' && displayNameOrParent === '(3b0c6ac)') &&
-          !(label === '98cb38c' && displayNameOrParent === '(df39067)')
-        )
-      })
-      expect(continuationCheckpoints).toHaveLength(0)
-    }
-  })
-
-  it('should handle a checkpoint tip not having a name', () => {
-    const checkpointTipWithoutAName = '3fceabdcef3c7b97c7779f8ae0c69a5542eefaf5'
-
-    const repoWithNestedCheckpoints = {
-      [EXPERIMENT_WORKSPACE_ID]: { baseline: {} },
-      branchA: {
-        baseline: { data: {} },
-        [checkpointTipWithoutAName]: {
-          data: { checkpoint_tip: checkpointTipWithoutAName }
-        },
-        tip1cp1: {
-          data: { checkpoint_tip: checkpointTipWithoutAName }
-        },
-        tip1cp2: {
-          data: { checkpoint_tip: checkpointTipWithoutAName }
-        },
-        tip1cp3: {
-          data: { checkpoint_tip: checkpointTipWithoutAName }
-        }
-      }
-    }
-    const acc = collectExperiments(repoWithNestedCheckpoints, false, '')
-
-    const { experimentsByCommit, checkpointsByTip } = acc
-    const [experiment] = experimentsByCommit.get('branchA') || []
-
-    expect(experiment.id).toStrictEqual(checkpointTipWithoutAName)
-    expect(
-      checkpointsByTip.get(checkpointTipWithoutAName)?.length
-    ).toStrictEqual(3)
-  })
 })

extension/src/experiments/model/collect.ts

@@ -68,68 +68,21 @@ const getExperimentId = (
   return name || sha
 }
 
-const getDisplayNameOrParent = (
+const getDisplayName = (
   sha: string,
-  commitSha: string,
   experimentsObject: { [sha: string]: ExperimentFieldsOrError }
 ): string | undefined => {
   const experiment = experimentsObject[sha]?.data
   if (!experiment) {
     return
   }
 
-  const {
-    checkpoint_parent: checkpointParent,
-    checkpoint_tip: checkpointTip,
-    name
-  } = experiment
-  if (
-    checkpointParent &&
-    commitSha !== checkpointParent &&
-    experimentsObject[checkpointParent]?.data?.checkpoint_tip !== checkpointTip
-  ) {
-    return `(${shortenForLabel(checkpointParent)})`
-  }
+  const { name } = experiment
   if (name) {
     return `[${name}]`
   }
 }
 
-const getLogicalGroupName = (
-  sha: string,
-  commitSha: string,
-  experimentsObject: { [sha: string]: ExperimentFieldsOrError }
-): string | undefined => {
-  const experiment = experimentsObject[sha]?.data
-
-  const { checkpoint_tip: checkpointTip = undefined, name = undefined } =
-    experiment || {}
-
-  if (name) {
-    return `[${name}]`
-  }
-
-  return (
-    getDisplayNameOrParent(sha, commitSha, experimentsObject) ||
-    (checkpointTip && checkpointTip !== sha
-      ? getLogicalGroupName(checkpointTip, commitSha, experimentsObject)
-      : undefined)
-  )
-}
-
-const getCheckpointTipId = (
-  checkpointTip: string | undefined,
-  experimentsObject: ExperimentsObject
-): string | undefined => {
-  if (!checkpointTip) {
-    return
-  }
-
-  const tip = experimentsObject[checkpointTip]?.data
-
-  return tip?.name || checkpointTip
-}
-
 const transformColumns = (
   experiment: Experiment,
   experimentFields: ExperimentFields,
@@ -167,8 +120,7 @@ const transformExperimentData = (
   experimentFieldsOrError: ExperimentFieldsOrError,
   label: string | undefined,
   sha?: string,
-  displayNameOrParent?: string,
-  logicalGroupName?: string,
+  displayName?: string,
   commit?: Experiment
 ): Experiment => {
   const data = experimentFieldsOrError.data || {}
@@ -181,12 +133,12 @@ const transformExperimentData = (
     ...omit(data, Object.values(ColumnType))
   } as Experiment
 
-  if (displayNameOrParent) {
-    experiment.displayNameOrParent = displayNameOrParent
+  if (displayName) {
+    experiment.displayName = displayName
   }
 
-  if (logicalGroupName) {
-    experiment.logicalGroupName = logicalGroupName
+  if (experiment.name && displayName) {
+    experiment.logicalGroupName = displayName
   }
 
   if (sha) {
@@ -206,37 +158,28 @@ const transformExperimentOrCheckpointData = (
   sha: string,
   experimentData: ExperimentFieldsOrError,
   experimentsObject: ExperimentsObject,
-  commitSha: string,
   commit: Experiment
-): {
-  checkpointTipId?: string
-  experiment: Experiment | undefined
-} => {
+): Experiment | undefined => {
   const experimentFields = experimentData.data
   if (!experimentFields) {
     const error = experimentData?.error?.msg
-    return { experiment: { error, id: sha, label: shortenForLabel(sha) } }
+    return { error, id: sha, label: shortenForLabel(sha) }
   }
 
-  const checkpointTipId = getCheckpointTipId(
-    experimentFields.checkpoint_tip,
-    experimentsObject
-  )
+  if (isCheckpoint(experimentFields.checkpoint_tip, sha)) {
+    return
+  }
 
   const id = getExperimentId(sha, experimentFields)
 
-  return {
-    checkpointTipId,
-    experiment: transformExperimentData(
-      id,
-      experimentData,
-      shortenForLabel(sha),
-      sha,
-      getDisplayNameOrParent(sha, commitSha, experimentsObject),
-      getLogicalGroupName(sha, commitSha, experimentsObject),
-      commit
-    )
-  }
+  return transformExperimentData(
+    id,
+    experimentData,
+    shortenForLabel(sha),
+    sha,
+    getDisplayName(sha, experimentsObject),
+    commit
+  )
 }
 
 const collectHasRunningExperiment = (
@@ -249,44 +192,25 @@ const collectHasRunningExperiment = (
   }
 }
 
-const collectExperimentOrCheckpoint = (
-  acc: ExperimentsAccumulator,
-  experiment: Experiment,
-  commitName: string,
-  checkpointTipId: string | undefined
-) => {
-  const { checkpoint_tip: checkpointTip, sha } = experiment
-  if (isCheckpoint(checkpointTip, sha as string)) {
-    if (!checkpointTipId) {
-      return
-    }
-    addToMapArray(acc.checkpointsByTip, checkpointTipId, experiment)
-    return
-  }
-  addToMapArray(acc.experimentsByCommit, commitName, experiment)
-}
-
 const collectFromExperimentsObject = (
   acc: ExperimentsAccumulator,
   experimentsObject: ExperimentsObject,
-  commitSha: string,
   commit: Experiment
 ) => {
   const commitName = commit.label
 
   for (const [sha, experimentData] of Object.entries(experimentsObject)) {
-    const { checkpointTipId, experiment } = transformExperimentOrCheckpointData(
+    const experiment = transformExperimentOrCheckpointData(
       sha,
       experimentData,
       experimentsObject,
-      commitSha,
       commit
     )
     if (!experiment) {
       continue
     }
 
-    collectExperimentOrCheckpoint(acc, experiment, commitName, checkpointTipId)
+    addToMapArray(acc.experimentsByCommit, commitName, experiment)
     collectHasRunningExperiment(acc, experiment)
   }
 }
@@ -308,7 +232,7 @@ const collectFromCommits = (
     const commit = transformExperimentData(name, baseline, label, sha)
 
     if (commit) {
-      collectFromExperimentsObject(acc, experimentsObject, sha, commit)
+      collectFromExperimentsObject(acc, experimentsObject, commit)
       collectHasRunningExperiment(acc, commit)
 
       acc.commits.push(commit)
@@ -371,9 +295,7 @@ const addDataToCommits = (
   return commits.map(commit => {
     const { sha } = commit
     if (sha && commitMessages[sha]) {
-      commit.displayNameOrParent = formatCommitMessage(
-        commitMessages[sha].message
-      )
+      commit.displayName = formatCommitMessage(commitMessages[sha].message)
       commit.commit = commitMessages[sha]
     }
     return commit