Fix experiment table changes collection bug

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

@@ -1,3 +1,4 @@
+/* eslint-disable sort-keys-fix/sort-keys-fix */
 import { join } from 'path'
 import { collectChanges, collectColumns } from '.'
 import { timestampColumn } from '../constants'
@@ -7,7 +8,11 @@ import outputFixture from '../../../test/fixtures/expShow/output'
 import columnsFixture from '../../../test/fixtures/expShow/columns'
 import workspaceChangesFixture from '../../../test/fixtures/expShow/workspaceChanges'
 import uncommittedDepsFixture from '../../../test/fixtures/expShow/uncommittedDeps'
-import { ExperimentsOutput } from '../../../cli/dvc/contract'
+import {
+  ExperimentsOutput,
+  ExperimentStatus,
+  ValueTree
+} from '../../../cli/dvc/contract'
 import { getConfigValue } from '../../../vscode/config'
 
 jest.mock('../../../vscode/config')
@@ -487,15 +492,6 @@ describe('collectColumns', () => {
       ]
     )
   })
-
-  it('should mark new dep files as changes', () => {
-    const changes = collectChanges(uncommittedDepsFixture)
-    expect(changes).toStrictEqual(
-      Object.keys(uncommittedDepsFixture.workspace.baseline.data?.deps || {})
-        .map(dep => `deps:${dep}`)
-        .sort()
-    )
-  })
 })
 
 describe('collectChanges', () => {
@@ -516,6 +512,15 @@ describe('collectChanges', () => {
     }
   }
 
+  it('should mark new dep files as changes', () => {
+    const changes = collectChanges(uncommittedDepsFixture)
+    expect(changes).toStrictEqual(
+      Object.keys(uncommittedDepsFixture.workspace.baseline.data?.deps || {})
+        .map(dep => `deps:${dep}`)
+        .sort()
+    )
+  })
+
   it('should return the expected data from the output fixture', () => {
     const changes = collectChanges(outputFixture)
     expect(changes).toStrictEqual(workspaceChangesFixture)
@@ -532,12 +537,12 @@ describe('collectChanges', () => {
 
   it('should collect the changes between the current commit and the workspace', () => {
     const data: ExperimentsOutput = {
+      workspace: mockedExperimentData,
       f8a6ee1997b193ebc774837a284081ff9e8dc2d5: {
         baseline: {
           data: {}
         }
-      },
-      workspace: mockedExperimentData
+      }
     }
 
     expect(collectChanges(data)).toStrictEqual([
@@ -550,6 +555,175 @@ describe('collectChanges', () => {
     ])
   })
 
+  it('should not fail when the workspace does not have metrics but a previous commit does', () => {
+    const data: ExperimentsOutput = {
+      workspace: {
+        baseline: {
+          data: {
+            params: mockedExperimentData.baseline.data.params
+          }
+        }
+      },
+      f8a6ee1997b193ebc774837a284081ff9e8dc2d5: mockedExperimentData
+    }
+
+    expect(collectChanges(data)).toStrictEqual([])
+  })
+
+  const updateParams = (data: ValueTree) => ({
+    baseline: {
+      data: {
+        timestamp: null,
+        params: {
+          'params.yaml': {
+            data
+          }
+        },
+        status: ExperimentStatus.SUCCESS,
+        executor: null
+      }
+    }
+  })
+
+  it('should work for objects', () => {
+    expect(
+      collectChanges({
+        workspace: updateParams({
+          a: { b: 1, d: { e: 100 } }
+        }),
+        '31c419826c6961bc0ec1d3900ac18bf904dcf82e': updateParams({
+          a: { b: 'c', d: { e: 'f' } }
+        })
+      })
+    ).toStrictEqual(['params:params.yaml:a.b', 'params:params.yaml:a.d.e'])
+
+    expect(
+      collectChanges({
+        workspace: updateParams({
+          a: { b: 'c', d: { e: 'f' } }
+        }),
+        '31c419826c6961bc0ec1d3900ac18bf904dcf82e': updateParams({
+          a: { b: 'c', d: { e: 'f' } }
+        })
+      })
+    ).toStrictEqual([])
+  })
+
+  it('should work for arrays', () => {
+    expect(
+      collectChanges({
+        workspace: updateParams({
+          a: [1, 1]
+        }),
+        '31c419826c6961bc0ec1d3900ac18bf904dcf82e': updateParams({
+          a: [1, 0]
+        })
+      })
+    ).toStrictEqual(['params:params.yaml:a'])
+
+    expect(
+      collectChanges({
+        workspace: updateParams({
+          a: [1, 0]
+        }),
+        '31c419826c6961bc0ec1d3900ac18bf904dcf82e': updateParams({
+          a: [1, 0]
+        })
+      })
+    ).toStrictEqual([])
+  })
+
+  it('should work for nested arrays', () => {
+    expect(
+      collectChanges({
+        workspace: updateParams({
+          a: { b: [1, 1] }
+        }),
+        '31c419826c6961bc0ec1d3900ac18bf904dcf82e': updateParams({
+          a: { b: [1, 0] }
+        })
+      })
+    ).toStrictEqual(['params:params.yaml:a.b'])
+
+    expect(
+      collectChanges({
+        workspace: updateParams({
+          a: { b: [1, 0] }
+        }),
+        '31c419826c6961bc0ec1d3900ac18bf904dcf82e': updateParams({
+          a: { b: [1, 0] }
+        })
+      })
+    ).toStrictEqual([])
+  })
+
+  it('should work for missing nested arrays', () => {
+    expect(
+      collectChanges({
+        workspace: {
+          baseline: {
+            data: {
+              timestamp: null,
+              status: ExperimentStatus.SUCCESS,
+              executor: null
+            }
+          }
+        },
+        '31c419826c6961bc0ec1d3900ac18bf904dcf82e': updateParams({
+          a: { b: [1, 0] }
+        })
+      })
+    ).toStrictEqual([])
+
+    expect(
+      collectChanges({
+        workspace: updateParams({
+          a: { b: [1, 0] }
+        }),
+        '31c419826c6961bc0ec1d3900ac18bf904dcf82e': {
+          baseline: {
+            data: {
+              timestamp: null,
+              status: ExperimentStatus.SUCCESS,
+              executor: null
+            }
+          }
+        }
+      })
+    ).toStrictEqual(['params:params.yaml:a.b'])
+  })
+
+  it('should compare against the most recent commit', () => {
+    const matchingParams = {
+      lr: 0.1
+    }
+    const differingParams = {
+      lr: 10000000
+    }
+
+    const data = {
+      workspace: updateParams(matchingParams),
+      '31c419826c6961bc0ec1d3900ac18bf904dcf82e': updateParams(matchingParams),
+      '1987d9de990090d73cf2afd73e6889d182585bf3': updateParams(differingParams),
+      '3d7fcb87062d136a2025f8c302312abe9593edf8': updateParams(differingParams)
+    }
+    expect(collectChanges(data)).toStrictEqual([])
+  })
+
+  it('should not fail when there is no commit data', () => {
+    const data: ExperimentsOutput = {
+      workspace: {
+        baseline: {
+          data: {
+            params: mockedExperimentData.baseline.data.params
+          }
+        }
+      }
+    }
+
+    expect(collectChanges(data)).toStrictEqual([])
+  })
+
   it('should collect the changes between the current commit and the workspace when the values are nested', () => {
     const mockedCommitDropoutData = {
       baseline: {

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

@@ -7,6 +7,7 @@ import {
 } from './metricsAndParams'
 import { Column } from '../../webview/contract'
 import {
+  ExperimentFields,
   ExperimentFieldsOrError,
   ExperimentsBranchOutput,
   ExperimentsOutput
@@ -49,22 +50,16 @@ export const collectColumns = (data: ExperimentsOutput): Column[] => {
   return Object.values(acc)
 }
 
-const getData = (value: { baseline: ExperimentFieldsOrError }) =>
-  value.baseline.data || {}
+const getData = (value?: {
+  baseline?: ExperimentFieldsOrError
+}): ExperimentFields | undefined => value?.baseline?.data
 
 export const collectChanges = (data: ExperimentsOutput): string[] => {
   const changes: string[] = []
 
-  let workspace
-  let currentCommit
-
-  for (const [key, value] of Object.entries(data)) {
-    if (key === 'workspace') {
-      workspace = getData(value)
-      continue
-    }
-    currentCommit = getData(value)
-  }
+  const [workspaceData, currentCommitData] = Object.values(data)
+  const workspace = getData(workspaceData)
+  const currentCommit = getData(currentCommitData)
 
   if (!(workspace && currentCommit)) {
     return changes

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

@@ -1,4 +1,5 @@
 import get from 'lodash.get'
+import isEqual from 'lodash.isequal'
 import {
   ColumnAccumulator,
   limitAncestorDepth,
@@ -109,7 +110,7 @@ const collectChange = (
   commitData: ExperimentFields,
   ancestors: string[] = []
 ) => {
-  if (typeof value === 'object') {
+  if (!Array.isArray(value) && typeof value === 'object') {
     for (const [childKey, childValue] of Object.entries(value as ValueTree)) {
       collectChange(changes, type, file, childKey, childValue, commitData, [
         ...ancestors,
@@ -119,7 +120,9 @@ const collectChange = (
     return
   }
 
-  if (get(commitData?.[type], [file, 'data', ...ancestors, key]) !== value) {
+  if (
+    !isEqual(get(commitData?.[type], [file, 'data', ...ancestors, key]), value)
+  ) {
     changes.push(buildMetricOrParamPath(type, file, ...ancestors, key))
   }
 }