Skip to content

Commit 7659a51

Browse files
mattseddonmattseddon
authored
Revert string quoting behaviour in modify and commands (#4116)
1 parent 44b0df4 commit 7659a51

File tree

4 files changed

+40
-54
lines changed

4 files changed

+40
-54
lines changed

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

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,42 +8,34 @@ describe('collectFlatExperimentParams', () => {
88
const params = collectFlatExperimentParams(rowsFixture[0].params)
99
expect(params).toStrictEqual([
1010
{
11-
isString: false,
1211
path: appendColumnToPath('params.yaml', 'code_names'),
1312
value: [0, 1]
1413
},
1514
{
16-
isString: false,
1715
path: appendColumnToPath('params.yaml', 'epochs'),
1816
value: 5
1917
},
2018
{
21-
isString: false,
2219
path: appendColumnToPath('params.yaml', 'learning_rate'),
2320
value: 2.1e-7
2421
},
2522
{
26-
isString: true,
2723
path: appendColumnToPath('params.yaml', 'dvc_logs_dir'),
2824
value: 'dvc_logs'
2925
},
3026
{
31-
isString: true,
3227
path: appendColumnToPath('params.yaml', 'log_file'),
3328
value: 'logs.csv'
3429
},
3530
{
36-
isString: false,
3731
path: appendColumnToPath('params.yaml', 'dropout'),
3832
value: 0.124
3933
},
4034
{
41-
isString: false,
4235
path: appendColumnToPath('params.yaml', 'process', 'threshold'),
4336
value: 0.85
4437
},
4538
{
46-
isString: false,
4739
path: appendColumnToPath(join('nested', 'params.yaml'), 'test'),
4840
value: true
4941
}

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

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

10-
export type ParamWithIsString = Param & { isString: boolean }
11-
1210
const collectFromParamsFile = (
13-
acc: ParamWithIsString[],
11+
acc: Param[],
1412
key: string | undefined,
1513
value: Value | ValueTree,
1614
ancestors: string[] = []
@@ -26,13 +24,13 @@ const collectFromParamsFile = (
2624

2725
const path = appendColumnToPath(...pathArray)
2826

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

3230
export const collectFlatExperimentParams = (
3331
params: MetricOrParamColumns = {}
3432
) => {
35-
const acc: ParamWithIsString[] = []
33+
const acc: Param[] = []
3634
for (const file of Object.keys(params)) {
3735
collectFromParamsFile(acc, undefined, params[file], [file])
3836
}

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

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

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

2323
expect(paramsToQueue).toBeUndefined()
@@ -26,13 +26,12 @@ describe('pickAndModifyParams', () => {
2626

2727
it('should return early if the user exits from the input box', async () => {
2828
const unchanged = {
29-
isString: false,
3029
path: 'params.yaml:learning_rate',
3130
value: 2e-12
3231
}
3332
const initialUserResponse = [
34-
{ isString: false, path: 'params.yaml:dropout', value: 0.15 },
35-
{ isString: false, path: 'params.yaml:process.threshold', value: 0.86 }
33+
{ path: 'params.yaml:dropout', value: 0.15 },
34+
{ path: 'params.yaml:process.threshold', value: 0.86 }
3635
]
3736
mockedQuickPickManyValues.mockResolvedValueOnce(initialUserResponse)
3837
const firstInput = '0.16'
@@ -50,37 +49,38 @@ describe('pickAndModifyParams', () => {
5049

5150
it('should convert any selected params into the required format', async () => {
5251
const unchanged = {
53-
isString: false,
5452
path: 'params.yaml:learning_rate',
5553
value: 2e-12
5654
}
5755

5856
const initialUserResponse = [
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] },
57+
{ path: 'params.yaml:dropout', value: 0.15 },
58+
{ path: 'params.yaml:process.threshold', value: 0.86 },
59+
{ path: 'params.yaml:code_names', value: [0, 1, 2] },
60+
{ path: 'params.yaml:arch', value: 'resnet18' },
6261
{
63-
isString: true,
6462
path: 'params.yaml:transforms',
6563
value: '[Pipeline: PILBase.create, Pipeline: partial -> PILBase.create]'
6664
}
6765
]
6866
mockedQuickPickManyValues.mockResolvedValueOnce(initialUserResponse)
69-
const firstInput = '0.16'
70-
const secondInput = '0.87'
71-
const thirdInput = '[0,1,3]'
72-
const fourthInput = '[Pipeline: PILBase.create]'
73-
mockedGetInput.mockResolvedValueOnce(firstInput)
74-
mockedGetInput.mockResolvedValueOnce(secondInput)
75-
mockedGetInput.mockResolvedValueOnce(thirdInput)
76-
mockedGetInput.mockResolvedValueOnce(fourthInput)
67+
const input1 = '0.16'
68+
const input2 = '0.87,0.88'
69+
const input3 = '[0,1,3]'
70+
const input4 = 'resnet18,shufflenet_v2_x2_0'
71+
const input5 = "'[Pipeline: PILBase.create]'" // user needs to quote
72+
mockedGetInput.mockResolvedValueOnce(input1)
73+
mockedGetInput.mockResolvedValueOnce(input2)
74+
mockedGetInput.mockResolvedValueOnce(input3)
75+
mockedGetInput.mockResolvedValueOnce(input4)
76+
mockedGetInput.mockResolvedValueOnce(input5)
7777

7878
const paramsToQueue = await pickAndModifyParams([
7979
unchanged,
8080
...initialUserResponse
8181
])
8282

83-
expect(mockedGetInput).toHaveBeenCalledTimes(4)
83+
expect(mockedGetInput).toHaveBeenCalledTimes(5)
8484
expect(mockedGetInput).toHaveBeenCalledWith(
8585
'Enter a Value for params.yaml:dropout',
8686
'0.15'
@@ -96,20 +96,27 @@ describe('pickAndModifyParams', () => {
9696
'[0,1,2]'
9797
)
9898

99+
expect(mockedGetInput).toHaveBeenCalledWith(
100+
'Enter a Value for params.yaml:arch',
101+
'resnet18'
102+
)
103+
99104
expect(mockedGetInput).toHaveBeenCalledWith(
100105
'Enter a Value for params.yaml:transforms',
101106
'[Pipeline: PILBase.create, Pipeline: partial -> PILBase.create]'
102107
)
103108

104109
expect(paramsToQueue).toStrictEqual([
105110
'-S',
106-
`params.yaml:dropout=${firstInput}`,
111+
`params.yaml:dropout=${input1}`,
112+
'-S',
113+
`params.yaml:process.threshold=${input2}`,
107114
'-S',
108-
`params.yaml:process.threshold=${secondInput}`,
115+
`params.yaml:code_names=${input3}`,
109116
'-S',
110-
`params.yaml:code_names=${thirdInput}`,
117+
`params.yaml:arch=${input4}`,
111118
'-S',
112-
`params.yaml:transforms='${fourthInput}'`
119+
`params.yaml:transforms=${input5}`
113120
])
114121
})
115122
})

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

Lines changed: 8 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,25 +1,17 @@
1-
import { ParamWithIsString } from './collect'
1+
import { Param } 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 = (
10-
value: Value,
11-
wrapStringParamForCli = false
12-
): string => {
13-
if (wrapStringParamForCli && typeof value === 'string') {
14-
return `'${value}'`
15-
}
9+
const standardizeValue = (value: Value): string => {
1610
return typeof value === 'object' ? JSON.stringify(value) : String(value)
1711
}
1812

19-
const pickParamsToModify = (
20-
params: ParamWithIsString[]
21-
): Thenable<ParamWithIsString[] | undefined> =>
22-
quickPickManyValues<ParamWithIsString>(
13+
const pickParamsToModify = (params: Param[]): Thenable<Param[] | undefined> =>
14+
quickPickManyValues<Param>(
2315
params.map(param => ({
2416
description: standardizeValue(param.value),
2517
label: param.path,
@@ -30,27 +22,24 @@ const pickParamsToModify = (
3022
)
3123

3224
const pickNewParamValues = async (
33-
paramsToModify: ParamWithIsString[]
25+
paramsToModify: Param[]
3426
): Promise<string[] | undefined> => {
3527
const args: string[] = []
36-
for (const { path, value, isString } of paramsToModify) {
28+
for (const { path, value } of paramsToModify) {
3729
const input = await getInput(
3830
getEnterValueTitle(path),
3931
standardizeValue(value)
4032
)
4133
if (input === undefined) {
4234
return
4335
}
44-
args.push(
45-
Flag.SET_PARAM,
46-
[path, standardizeValue(input.trim(), isString)].join('=')
47-
)
36+
args.push(Flag.SET_PARAM, [path, standardizeValue(input.trim())].join('='))
4837
}
4938
return args
5039
}
5140

5241
export const pickAndModifyParams = async (
53-
params: ParamWithIsString[]
42+
params: Param[]
5443
): Promise<string[] | undefined> => {
5544
const paramsToModify = await pickParamsToModify(params)
5645

0 commit comments

Comments
 (0)