Skip to content

Commit e15dd7b

Browse files
mattseddonmattseddon
authored
Correctly handle string arrays in modify and commands (#4092)
1 parent f8d64d0 commit e15dd7b

File tree

4 files changed

+137
-29
lines changed

4 files changed

+137
-29
lines changed

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

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,29 +7,43 @@ describe('collectFlatExperimentParams', () => {
77
it('should flatten the params into an array', () => {
88
const params = collectFlatExperimentParams(rowsFixture[0].params)
99
expect(params).toStrictEqual([
10-
{ path: appendColumnToPath('params.yaml', 'code_names'), value: [0, 1] },
11-
{ path: appendColumnToPath('params.yaml', 'epochs'), value: 5 },
1210
{
11+
isString: false,
12+
path: appendColumnToPath('params.yaml', 'code_names'),
13+
value: [0, 1]
14+
},
15+
{
16+
isString: false,
17+
path: appendColumnToPath('params.yaml', 'epochs'),
18+
value: 5
19+
},
20+
{
21+
isString: false,
1322
path: appendColumnToPath('params.yaml', 'learning_rate'),
1423
value: 2.1e-7
1524
},
1625
{
26+
isString: true,
1727
path: appendColumnToPath('params.yaml', 'dvc_logs_dir'),
1828
value: 'dvc_logs'
1929
},
2030
{
31+
isString: true,
2132
path: appendColumnToPath('params.yaml', 'log_file'),
2233
value: 'logs.csv'
2334
},
2435
{
36+
isString: false,
2537
path: appendColumnToPath('params.yaml', 'dropout'),
2638
value: 0.124
2739
},
2840
{
41+
isString: false,
2942
path: appendColumnToPath('params.yaml', 'process', 'threshold'),
3043
value: 0.85
3144
},
3245
{
46+
isString: false,
3347
path: appendColumnToPath(join('nested', 'params.yaml'), 'test'),
3448
value: true
3549
}

‎extension/src/experiments/model/modify/collect.ts‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,10 @@ export type Param = {
77
value: Value
88
}
99

10+
export type ParamWithIsString = Param & { isString: boolean }
11+
1012
const collectFromParamsFile = (
11-
acc: { path: string; value: Value }[],
13+
acc: ParamWithIsString[],
1214
key: string | undefined,
1315
value: Value | ValueTree,
1416
ancestors: string[] = []
@@ -24,13 +26,13 @@ const collectFromParamsFile = (
2426

2527
const path = appendColumnToPath(...pathArray)
2628

27-
acc.push({ path, value })
29+
acc.push({ isString: typeof value === 'string', path, value })
2830
}
2931

3032
export const collectFlatExperimentParams = (
3133
params: MetricOrParamColumns = {}
3234
) => {
33-
const acc: { path: string; value: string | number | boolean }[] = []
35+
const acc: ParamWithIsString[] = []
3436
for (const file of Object.keys(params)) {
3537
collectFromParamsFile(acc, undefined, params[file], [file])
3638
}

‎extension/src/experiments/model/modify/quickPick.test.ts‎

Lines changed: 90 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -17,18 +17,22 @@ describe('pickAndModifyParams', () => {
1717
mockedQuickPickManyValues.mockResolvedValueOnce(undefined)
1818

1919
const paramsToQueue = await pickAndModifyParams([
20-
{ path: 'params.yaml:learning_rate', value: 2e-12 }
20+
{ isString: false, path: 'params.yaml:learning_rate', value: 2e-12 }
2121
])
2222

2323
expect(paramsToQueue).toBeUndefined()
2424
expect(mockedGetInput).not.toHaveBeenCalled()
2525
})
2626

2727
it('should return early if the user exits from the input box', async () => {
28-
const unchanged = { path: 'params.yaml:learning_rate', value: 2e-12 }
28+
const unchanged = {
29+
isString: false,
30+
path: 'params.yaml:learning_rate',
31+
value: 2e-12
32+
}
2933
const initialUserResponse = [
30-
{ path: 'params.yaml:dropout', value: 0.15 },
31-
{ path: 'params.yaml:process.threshold', value: 0.86 }
34+
{ isString: false, path: 'params.yaml:dropout', value: 0.15 },
35+
{ isString: false, path: 'params.yaml:process.threshold', value: 0.86 }
3236
]
3337
mockedQuickPickManyValues.mockResolvedValueOnce(initialUserResponse)
3438
const firstInput = '0.16'
@@ -45,31 +49,58 @@ describe('pickAndModifyParams', () => {
4549
})
4650

4751
it('should convert any selected params into the required format', async () => {
48-
const unchanged = { path: 'params.yaml:learning_rate', value: 2e-12 }
52+
const unchanged = {
53+
isString: false,
54+
path: 'params.yaml:learning_rate',
55+
value: 2e-12
56+
}
57+
4958
const initialUserResponse = [
50-
{ path: 'params.yaml:dropout', value: 0.15 },
51-
{ path: 'params.yaml:process.threshold', value: 0.86 },
52-
{ path: 'params.yaml:code_names', value: [0, 1, 2] }
59+
{ isString: false, path: 'params.yaml:dropout', value: 0.15 },
60+
{ isString: false, path: 'params.yaml:process.threshold', value: 0.86 },
61+
{ isString: false, path: 'params.yaml:code_names', value: [0, 1, 2] },
62+
{
63+
isString: true,
64+
path: 'params.yaml:transforms',
65+
value: '[Pipeline: PILBase.create, Pipeline: partial -> PILBase.create]'
66+
}
5367
]
5468
mockedQuickPickManyValues.mockResolvedValueOnce(initialUserResponse)
5569
const firstInput = '0.16'
5670
const secondInput = '0.87'
5771
const thirdInput = '[0,1,3]'
72+
const fourthInput = '[Pipeline: PILBase.create]'
5873
mockedGetInput.mockResolvedValueOnce(firstInput)
5974
mockedGetInput.mockResolvedValueOnce(secondInput)
6075
mockedGetInput.mockResolvedValueOnce(thirdInput)
76+
mockedGetInput.mockResolvedValueOnce(fourthInput)
6177

6278
const paramsToQueue = await pickAndModifyParams([
6379
unchanged,
6480
...initialUserResponse
6581
])
6682

67-
expect(mockedGetInput).toHaveBeenCalledTimes(3)
83+
expect(mockedGetInput).toHaveBeenCalledTimes(4)
84+
expect(mockedGetInput).toHaveBeenCalledWith(
85+
'Enter a Value for params.yaml:dropout',
86+
'0.15'
87+
)
88+
89+
expect(mockedGetInput).toHaveBeenCalledWith(
90+
'Enter a Value for params.yaml:process.threshold',
91+
'0.86'
92+
)
93+
6894
expect(mockedGetInput).toHaveBeenCalledWith(
6995
'Enter a Value for params.yaml:code_names',
7096
'[0,1,2]'
7197
)
7298

99+
expect(mockedGetInput).toHaveBeenCalledWith(
100+
'Enter a Value for params.yaml:transforms',
101+
'[Pipeline: PILBase.create, Pipeline: partial -> PILBase.create]'
102+
)
103+
73104
expect(paramsToQueue).toStrictEqual([
74105
'-S',
75106
`params.yaml:dropout=${firstInput}`,
@@ -78,8 +109,57 @@ describe('pickAndModifyParams', () => {
78109
'-S',
79110
`params.yaml:code_names=${thirdInput}`,
80111
'-S',
112+
`params.yaml:transforms='${fourthInput}'`,
113+
'-S',
81114
[unchanged.path, unchanged.value].join('=')
82115
])
83-
expect(mockedGetInput).toHaveBeenCalledTimes(3)
116+
})
117+
118+
it('should convert any unselected params into the required format', async () => {
119+
const numericValue = {
120+
isString: false,
121+
path: 'params.yaml:learning_rate',
122+
value: 2e-12
123+
}
124+
const stringArrayValue = {
125+
isString: true,
126+
path: 'params.yaml:transforms',
127+
value: '[Pipeline: PILBase.create, Pipeline: partial -> PILBase.create]'
128+
}
129+
130+
const actualArrayValue = {
131+
isString: false,
132+
path: 'params.yaml:code_names',
133+
value: [0, 1, 2]
134+
}
135+
136+
const unchanged = [numericValue, stringArrayValue, actualArrayValue]
137+
const initialUserResponse = {
138+
isString: false,
139+
path: 'params.yaml:dropout',
140+
value: 0.15
141+
}
142+
143+
mockedQuickPickManyValues.mockResolvedValueOnce([initialUserResponse])
144+
const firstInput = '0.16'
145+
mockedGetInput.mockResolvedValueOnce(firstInput)
146+
147+
const paramsToQueue = await pickAndModifyParams([
148+
...unchanged,
149+
initialUserResponse
150+
])
151+
152+
expect(mockedGetInput).toHaveBeenCalledTimes(1)
153+
154+
expect(paramsToQueue).toStrictEqual([
155+
'-S',
156+
`params.yaml:dropout=${firstInput}`,
157+
'-S',
158+
[numericValue.path, numericValue.value].join('='),
159+
'-S',
160+
[stringArrayValue.path, `'${stringArrayValue.value}'`].join('='),
161+
'-S',
162+
[actualArrayValue.path, JSON.stringify(actualArrayValue.value)].join('=')
163+
])
84164
})
85165
})

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

Lines changed: 26 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,24 @@
1-
import { Param } from './collect'
1+
import { ParamWithIsString } from './collect'
22
import { quickPickManyValues } from '../../../vscode/quickPick'
33
import { getInput } from '../../../vscode/inputBox'
44
import { Flag } from '../../../cli/dvc/constants'
55
import { definedAndNonEmpty } from '../../../util/array'
66
import { getEnterValueTitle, Title } from '../../../vscode/title'
77
import { Value } from '../../../cli/dvc/contract'
88

9-
const standardizeValue = (value: Value): string =>
10-
typeof value === 'object' ? JSON.stringify(value) : String(value)
9+
const standardizeValue = (value: Value, isString: boolean): string => {
10+
if (isString && typeof value === 'string') {
11+
return `'${value}'`
12+
}
13+
return typeof value === 'object' ? JSON.stringify(value) : String(value)
14+
}
1115

12-
const pickParamsToModify = (params: Param[]): Thenable<Param[] | undefined> =>
13-
quickPickManyValues<Param>(
16+
const pickParamsToModify = (
17+
params: ParamWithIsString[]
18+
): Thenable<ParamWithIsString[] | undefined> =>
19+
quickPickManyValues<ParamWithIsString>(
1420
params.map(param => ({
15-
description: standardizeValue(param.value),
21+
description: standardizeValue(param.value, false),
1622
label: param.path,
1723
picked: false,
1824
value: param
@@ -21,32 +27,38 @@ const pickParamsToModify = (params: Param[]): Thenable<Param[] | undefined> =>
2127
)
2228

2329
const pickNewParamValues = async (
24-
paramsToModify: Param[]
30+
paramsToModify: ParamWithIsString[]
2531
): Promise<string[] | undefined> => {
2632
const args: string[] = []
27-
for (const { path, value } of paramsToModify) {
33+
for (const { path, value, isString } of paramsToModify) {
2834
const input = await getInput(
2935
getEnterValueTitle(path),
30-
standardizeValue(value)
36+
standardizeValue(value, false)
3137
)
3238
if (input === undefined) {
3339
return
3440
}
35-
args.push(Flag.SET_PARAM, [path, standardizeValue(input.trim())].join('='))
41+
args.push(
42+
Flag.SET_PARAM,
43+
[path, standardizeValue(input.trim(), isString)].join('=')
44+
)
3645
}
3746
return args
3847
}
3948

40-
const addUnchanged = (args: string[], unchanged: Param[]) => {
41-
for (const { path, value } of unchanged) {
42-
args.push(Flag.SET_PARAM, [path, standardizeValue(value)].join('='))
49+
const addUnchanged = (args: string[], unchanged: ParamWithIsString[]) => {
50+
for (const { path, value, isString } of unchanged) {
51+
args.push(
52+
Flag.SET_PARAM,
53+
[path, standardizeValue(value, isString)].join('=')
54+
)
4355
}
4456

4557
return args
4658
}
4759

4860
export const pickAndModifyParams = async (
49-
params: Param[]
61+
params: ParamWithIsString[]
5062
): Promise<string[] | undefined> => {
5163
const paramsToModify = await pickParamsToModify(params)
5264

0 commit comments

Comments
 (0)