Skip to content

Commit 82628c9

Browse files
mattseddonmattseddon
authored
Fix sorts and filters for dep columns (#4432)
1 parent 65ea5fb commit 82628c9

File tree

13 files changed

+138
-27
lines changed

13 files changed

+138
-27
lines changed

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

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
import { join } from 'path'
2-
import get from 'lodash.get'
32
import isEqual from 'lodash.isequal'
43
import { ColumnAccumulator } from './util'
54
import { collectDepChanges, collectDeps } from './deps'
@@ -8,13 +7,13 @@ import {
87
collectMetricsAndParams
98
} from './metricsAndParams'
109
import { Column, Commit, Experiment } from '../../webview/contract'
10+
import { getValue } from '../util'
1111
import {
1212
ExpRange,
1313
ExpShowOutput,
1414
ExpState,
1515
ExpData,
1616
experimentHasError,
17-
Value,
1817
EXPERIMENT_WORKSPACE_ID
1918
} from '../../../cli/dvc/contract'
2019
import { standardizePath } from '../../../fileSystem/path'
@@ -141,11 +140,6 @@ export const collectRelativeMetricsFiles = (
141140
return uniqueValues(files)
142141
}
143142

144-
const getValue = (
145-
experiment: Commit | Experiment,
146-
pathArray: string[]
147-
): Value => get(experiment, pathArray) as Value
148-
149143
const collectChangedPath = (
150144
acc: string[],
151145
path: string,

‎extension/src/experiments/columns/like.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,11 @@ export const addStarredToColumns = (
2020
}
2121

2222
return [
23+
starredColumnLike,
2324
...columns.map(({ label, path, firstValueType }) => ({
2425
firstValueType,
2526
label,
2627
path
27-
})),
28-
starredColumnLike
28+
}))
2929
]
3030
}

‎extension/src/experiments/columns/util.test.ts‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
import { getValue } from './util'
2+
import rowsFixture from '../../test/fixtures/expShow/base/rows'
3+
import columnsFixture from '../../test/fixtures/expShow/base/columns'
4+
import { ColumnType } from '../webview/contract'
5+
6+
describe('getValue', () => {
7+
const experiment = rowsFixture[0]
8+
9+
const getPathArrayFromType = (columnType: ColumnType) => {
10+
const column = columnsFixture.find(
11+
({ type, hasChildren }) => type === columnType && !hasChildren
12+
)
13+
14+
if (!column) {
15+
throw new Error('column not defined')
16+
}
17+
const { pathArray } = column
18+
expect(pathArray).toBeDefined()
19+
if (!pathArray) {
20+
throw new Error('pathArray not defined')
21+
}
22+
return pathArray
23+
}
24+
25+
it('should return the expected value for an experiment given a metric', () => {
26+
const pathArray = getPathArrayFromType(ColumnType.METRICS)
27+
expect(getValue(experiment, pathArray)).toStrictEqual(1.775016188621521)
28+
})
29+
30+
it('should return the expected value for an experiment given a param', () => {
31+
const pathArray = getPathArrayFromType(ColumnType.PARAMS)
32+
expect(getValue(experiment, pathArray)).toStrictEqual([0, 1])
33+
})
34+
35+
it('should return the expected value for an experiment given a dep', () => {
36+
const pathArray = getPathArrayFromType(ColumnType.DEPS)
37+
expect(getValue(experiment, pathArray)).toStrictEqual('22a1a29')
38+
})
39+
40+
it('should not mutate the original array', () => {
41+
const pathArray = getPathArrayFromType(ColumnType.DEPS)
42+
const copy = [...pathArray]
43+
getValue(experiment, pathArray)
44+
expect(pathArray).toStrictEqual(copy)
45+
})
46+
})

‎extension/src/experiments/columns/util.ts‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
1-
import { ColumnType } from '../webview/contract'
1+
import get from 'lodash.get'
2+
import { ColumnType, Experiment } from '../webview/contract'
23

34
export const MAX_SUMMARY_ORDER_LENGTH = 3
45

@@ -27,3 +28,11 @@ export const limitSummaryOrder = (acc: SummaryAcc): string[] => [
2728
...acc.params.slice(0, MAX_SUMMARY_ORDER_LENGTH),
2829
...acc.metrics.slice(0, MAX_SUMMARY_ORDER_LENGTH)
2930
]
31+
32+
export const getValue = (experiment: Experiment, pathArray: string[]) => {
33+
const copy = [...pathArray]
34+
if (pathArray[0] === String(ColumnType.DEPS)) {
35+
copy.push('value')
36+
}
37+
return get(experiment, copy) as string | number | boolean
38+
}

‎extension/src/experiments/model/filterBy/index.test.ts‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
1+
import { join } from 'path'
12
import { FilterDefinition, filterExperiment, Operator } from '.'
2-
import { buildMetricOrParamPath } from '../../columns/paths'
3+
import rowsFixture from '../../../test/fixtures/expShow/base/rows'
4+
import { buildDepPath, buildMetricOrParamPath } from '../../columns/paths'
35
import { Experiment, ColumnType } from '../../webview/contract'
46

57
describe('filterExperiment', () => {
@@ -268,4 +270,26 @@ describe('filterExperiment', () => {
268270

269271
expect(unfiltered.map(experiment => experiment.id)).toStrictEqual([])
270272
})
273+
274+
it('should correctly filter using a dep', () => {
275+
const path = join('data', 'data.xml')
276+
const depPath = buildDepPath(path)
277+
278+
const experiment = rowsFixture[0]
279+
const value = experiment.deps?.[join('data', 'data.xml')]?.value
280+
expect(value).toBeDefined()
281+
282+
const unfiltered = filterExperiment(
283+
[{ operator: Operator.EQUAL, path: depPath, value }],
284+
experiment
285+
)
286+
expect(unfiltered).toStrictEqual(experiment)
287+
288+
expect(
289+
filterExperiment(
290+
[{ operator: Operator.NOT_EQUAL, path: depPath, value }],
291+
experiment
292+
)
293+
).toBeUndefined()
294+
})
271295
})

‎extension/src/experiments/model/filterBy/index.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
1-
import get from 'lodash.get'
21
import { compareDateStrings } from './date'
32
import { Experiment } from '../../webview/contract'
43
import { definedAndNonEmpty } from '../../../util/array'
54
import { splitColumnPath } from '../../columns/paths'
5+
import { getValue } from '../../columns/util'
66

77
export enum Operator {
88
EQUAL = '=',
@@ -112,7 +112,7 @@ const buildFilter =
112112
experiment => {
113113
const firstFailure = filterDefinitions.find(filter => {
114114
const pathArray = splitColumnPath(filter.path)
115-
const value = get(experiment, pathArray) as string | number | boolean
115+
const value = getValue(experiment, pathArray)
116116

117117
return !evaluate<typeof value>(
118118
value,

‎extension/src/experiments/model/filterBy/quickPick.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ export const pickColumnToFilter = (
119119
columns: ColumnLike[] | undefined
120120
): Thenable<ColumnLike | undefined> =>
121121
pickFromColumnLikes(columns, {
122-
title: Title.SELECT_PARAM_OR_METRIC_FILTER
122+
title: Title.SELECT_COLUMN_FILTER
123123
})
124124

125125
export const pickFilterToAdd = async ({

‎extension/src/experiments/model/sortBy/index.test.ts‎

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import get from 'lodash.get'
22
import { sortExperiments } from '.'
3-
import { buildMetricOrParamPath } from '../../columns/paths'
3+
import { buildDepPath, buildMetricOrParamPath } from '../../columns/paths'
44
import { Experiment, ColumnType } from '../../webview/contract'
55

66
describe('sortExperiments', () => {
@@ -253,4 +253,44 @@ describe('sortExperiments', () => {
253253
])
254254
})
255255
})
256+
257+
it('should reorder experiments when provided a dep', () => {
258+
const testData = [
259+
{
260+
...irrelevantExperimentData,
261+
deps: {
262+
'train.py': {
263+
changes: true,
264+
value: 'e804f48'
265+
}
266+
}
267+
},
268+
{
269+
...irrelevantExperimentData,
270+
deps: {
271+
'train.py': {
272+
changes: true,
273+
value: 'e804f41'
274+
}
275+
}
276+
},
277+
{
278+
...irrelevantExperimentData,
279+
deps: {
280+
'train.py': {
281+
changes: true,
282+
value: 'fd5998e'
283+
}
284+
}
285+
}
286+
]
287+
288+
const result = sortExperiments(
289+
[{ descending: true, path: buildDepPath('train.py') }],
290+
testData
291+
)
292+
expect(
293+
result.map(experiment => get(experiment, ['deps', 'train.py', 'value']))
294+
).toStrictEqual(['fd5998e', 'e804f48', 'e804f41'])
295+
})
256296
})

‎extension/src/experiments/model/sortBy/index.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
1-
import get from 'lodash.get'
21
import { splitColumnPath } from '../../columns/paths'
32
import { Experiment } from '../../webview/contract'
3+
import { getValue } from '../../columns/util'
44

55
export interface SortDefinition {
66
descending: boolean
@@ -10,12 +10,12 @@ export interface SortDefinition {
1010
type SortFunction = (a: Experiment, b: Experiment) => number
1111

1212
const compareExperimentsByPath = (
13-
path: string[],
13+
pathArray: string[],
1414
a: Experiment,
1515
b: Experiment
1616
): number => {
17-
const valueA = get(a, path) as string | number
18-
const valueB = get(b, path) as string | number
17+
const valueA = getValue(a, pathArray) as string | number
18+
const valueB = getValue(b, pathArray) as string | number
1919
if (valueA === valueB) {
2020
return 0
2121
}

‎extension/src/experiments/model/sortBy/quickPick.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import { pickFromColumnLikes } from '../../columns/quickPick'
88

99
export const pickSortToAdd = async (columns: ColumnLike[] | undefined) => {
1010
const picked = await pickFromColumnLikes(columns, {
11-
title: Title.SELECT_PARAM_OR_METRIC_SORT
11+
title: Title.SELECT_COLUMN_SORT
1212
})
1313
if (picked === undefined) {
1414
return

0 commit comments

Comments
 (0)