Merge branch 'main' into update-colors

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))
   }
 }

extension/src/experiments/webview/messages.ts

@@ -108,6 +108,13 @@ export class WebviewMessages {
           RegisteredCliCommands.EXPERIMENT_VIEW_REMOVE,
           { dvcRoot: this.dvcRoot, ids: [message.payload].flat() }
         )
+
+      case MessageFromWebviewType.ADD_STARRED_EXPERIMENT_FILTER:
+        return commands.executeCommand(
+          RegisteredCommands.EXPERIMENT_FILTER_ADD_STARRED,
+          this.dvcRoot
+        )
+
       case MessageFromWebviewType.SELECT_COLUMNS:
         return this.setColumnsStatus()
 

extension/src/fileSystem/index.ts

@@ -10,6 +10,7 @@ import {
 } from 'fs-extra'
 import { load } from 'js-yaml'
 import { Uri } from 'vscode'
+import { standardizePath } from './path'
 import { definedAndNonEmpty } from '../util/array'
 import { Logger } from '../common/logger'
 
@@ -36,7 +37,7 @@ export const findDvcSubRootPaths = async (
 
   return children
     .filter(child => isDirectory(join(cwd, child, '.dvc')))
-    .map(child => join(cwd, child))
+    .map(child => standardizePath(join(cwd, child)) as string)
 }
 
 export const findDvcRootPaths = async (cwd: string): Promise<string[]> => {
@@ -57,7 +58,7 @@ export const findAbsoluteDvcRootPath = async (
     return []
   }
 
-  const absoluteRoot = resolve(cwd, relativePath)
+  const absoluteRoot = standardizePath(resolve(cwd, relativePath)) as string
 
   return [absoluteRoot]
 }

extension/src/test/e2e/wdio.conf.ts

@@ -33,7 +33,7 @@ export const config: Options.Testrunner = {
   capabilities: [
     {
       browserName: 'vscode',
-      browserVersion: 'insiders',
+      browserVersion: 'stable',
       'wdio:vscodeOptions': {
         extensionPath,
         userSettings: {

extension/src/test/suite/experiments/index.test.ts

@@ -66,6 +66,7 @@ import { DvcExecutor } from '../../../cli/dvc/executor'
 import { shortenForLabel } from '../../../util/string'
 import { GitExecutor } from '../../../cli/git/executor'
 import { WorkspacePlots } from '../../../plots/workspace'
+import { RegisteredCommands } from '../../../commands/external'
 
 suite('Experiments Test Suite', () => {
   const disposable = Disposable.fn()
@@ -949,6 +950,29 @@ suite('Experiments Test Suite', () => {
       ).to.be.true
     }).timeout(WEBVIEW_TEST_TIMEOUT)
 
+    it('should be able to handle a message to filter to starred experiments', async () => {
+      const { experiments } = setupExperimentsAndMockCommands()
+
+      const mockExecuteCommand = stub(commands, 'executeCommand')
+
+      const webview = await experiments.showWebview()
+      const mockMessageReceived = getMessageReceivedEmitter(webview)
+      const messageReceived = new Promise(resolve =>
+        disposable.track(mockMessageReceived.event(() => resolve(undefined)))
+      )
+
+      mockMessageReceived.fire({
+        type: MessageFromWebviewType.ADD_STARRED_EXPERIMENT_FILTER
+      })
+
+      await messageReceived
+
+      expect(mockExecuteCommand).to.be.calledWithExactly(
+        RegisteredCommands.EXPERIMENT_FILTER_ADD_STARRED,
+        dvcDemoPath
+      )
+    }).timeout(WEBVIEW_TEST_TIMEOUT)
+
     it('should be able to handle a message to select experiments for plotting', async () => {
       const { experiments, experimentsModel } = buildExperiments(disposable)
       await experiments.isReady()