diff --git a/extension/src/experiments/columns/collect/index.test.ts b/extension/src/experiments/columns/collect/index.test.ts index 445b334b35..81b57eaa4b 100644 --- a/extension/src/experiments/columns/collect/index.test.ts +++ b/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: { diff --git a/extension/src/experiments/columns/collect/index.ts b/extension/src/experiments/columns/collect/index.ts index f988ab01f9..182430bb28 100644 --- a/extension/src/experiments/columns/collect/index.ts +++ b/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 diff --git a/extension/src/experiments/columns/collect/metricsAndParams.ts b/extension/src/experiments/columns/collect/metricsAndParams.ts index 54226d5af3..fe208be392 100644 --- a/extension/src/experiments/columns/collect/metricsAndParams.ts +++ b/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)) } }