Skip to content

Commit

Permalink
remove exp show and plots diff retry when git repo has no commits
Browse files Browse the repository at this point in the history
  • Loading branch information
mattseddon committed Aug 16, 2022
1 parent 4775826 commit 36571f5
Show file tree
Hide file tree
Showing 8 changed files with 109 additions and 21 deletions.
2 changes: 2 additions & 0 deletions extension/src/cli/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ export const MIN_CLI_VERSION = '2.11.0'
export const LATEST_TESTED_CLI_VERSION = '2.13.0'
export const MAX_CLI_VERSION = '3'

export const EMPTY_REPO_ERROR = 'unexpected error - Empty git repo'

export enum Command {
ADD = 'add',
CHECKOUT = 'checkout',
Expand Down
34 changes: 33 additions & 1 deletion extension/src/cli/reader.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@ import { join } from 'path'
import { EventEmitter } from 'vscode'
import { Disposable, Disposer } from '@hediet/std/disposable'
import { CliResult, CliStarted } from '.'
import { CliReader } from './reader'
import { EMPTY_REPO_ERROR } from './constants'
import { CliReader, ExperimentsOutput } from './reader'
import { createProcess } from '../processExecution'
import { getFailingMockedProcess, getMockedProcess } from '../test/util/jest'
import { getProcessEnv } from '../env'
Expand Down Expand Up @@ -76,6 +77,37 @@ describe('CliReader', () => {
executable: 'dvc'
})
})

it('should return the default output if the cli returns an empty repo error', async () => {
const cwd = __dirname
mockedCreateProcess.mockImplementationOnce(() => {
throw new Error(EMPTY_REPO_ERROR)
})

const cliOutput = await cliReader.expShow(cwd)
expect(cliOutput).toStrictEqual({ workspace: { baseline: {} } })
})

it('should retry the cli given any other type of error', async () => {
const cwd = __dirname
const mockOutput: ExperimentsOutput = {
workspace: {
baseline: {
data: { params: { 'params.yaml': { data: { epochs: 100000000 } } } }
}
}
}
mockedCreateProcess.mockImplementationOnce(() => {
throw new Error('error that should be retried')
})
mockedCreateProcess.mockReturnValueOnce(
getMockedProcess(JSON.stringify(mockOutput))
)

const cliOutput = await cliReader.expShow(cwd)
expect(cliOutput).toStrictEqual(mockOutput)
expect(mockedCreateProcess).toBeCalledTimes(2)
})
})

describe('diff', () => {
Expand Down
42 changes: 37 additions & 5 deletions extension/src/cli/reader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { Cli, typeCheckCommands } from '.'
import {
Args,
Command,
EMPTY_REPO_ERROR,
ExperimentFlag,
Flag,
ListFlag,
Expand Down Expand Up @@ -108,6 +109,10 @@ export interface ExperimentsOutput {
}
}

const defaultExperimentsOutput: ExperimentsOutput = {
workspace: { baseline: {} }
}

export interface PlotsOutput {
[path: string]: Plot[]
}
Expand All @@ -132,9 +137,11 @@ export class CliReader extends Cli {
cwd: string,
...flags: ExperimentFlag[]
): Promise<ExperimentsOutput> {
return this.readProcessJson<ExperimentsOutput>(
return this.readProcessJsonWithKnownErrors<ExperimentsOutput>(
cwd,
Command.EXPERIMENT,
defaultExperimentsOutput,
[EMPTY_REPO_ERROR],
SubCommand.SHOW,
...flags
)
Expand All @@ -155,10 +162,12 @@ export class CliReader extends Cli {
}

public plotsDiff(cwd: string, ...revisions: string[]): Promise<PlotsOutput> {
return this.readProcessJson<PlotsOutput>(
return this.readProcessJsonWithKnownErrors<PlotsOutput>(
cwd,
Command.PLOTS,
'diff',
{},
[EMPTY_REPO_ERROR],
Command.DIFF,
...revisions,
Flag.OUTPUT_PATH,
TEMP_PLOTS_DIR,
Expand All @@ -184,11 +193,15 @@ export class CliReader extends Cli {
cwd: string,
formatter: typeof trimAndSplit | typeof JSON.parse | typeof trim,
defaultValue: string,
nonRetryErrors: string[],
...args: Args
): Promise<T> {
const output =
(await retry(() => this.executeProcess(cwd, ...args), args.join(' '))) ||
defaultValue
(await retry(
() => this.executeProcess(cwd, ...args),
args.join(' '),
nonRetryErrors
)) || defaultValue
if (!formatter) {
return output as unknown as T
}
Expand All @@ -200,6 +213,25 @@ export class CliReader extends Cli {
cwd,
JSON.parse,
'{}',
[],
command,
...args,
Flag.SHOW_JSON
)
}

private readProcessJsonWithKnownErrors<T>(
cwd: string,
command: Command,
defaultValue: T,
nonRetryErrors: string[],
...args: Args
) {
return this.readProcess<T>(
cwd,
JSON.parse,
JSON.stringify(defaultValue),
nonRetryErrors,
command,
...args,
Flag.SHOW_JSON
Expand Down
4 changes: 2 additions & 2 deletions extension/src/cli/retry.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ describe('retry', () => {

const promiseRefresher = jest.fn().mockImplementation(() => promise())

const output = await retry<string>(promiseRefresher, 'Definitely did not')
const output = await retry(promiseRefresher, 'Definitely did not', [])

expect(output).toStrictEqual(returnValue)

Expand Down Expand Up @@ -45,7 +45,7 @@ describe('retry', () => {
.fn()
.mockImplementation(() => unreliablePromise())

await retry<string>(promiseRefresher, 'Data update')
await retry(promiseRefresher, 'Data update', [])

expect(promiseRefresher).toBeCalledTimes(4)
expect(mockedDelay).toBeCalledTimes(3)
Expand Down
25 changes: 21 additions & 4 deletions extension/src/cli/retry.ts
Original file line number Diff line number Diff line change
@@ -1,18 +1,35 @@
import { delay } from '../util/time'
import { Logger } from '../common/logger'

export const retry = async <T>(
getNewPromise: () => Promise<T>,
const isNonRetryError = (
errorMessage: string,
nonRetryErrors: string[]
): boolean => {
for (const partialErrorMessage of nonRetryErrors) {
if (errorMessage.includes(partialErrorMessage)) {
return true
}
}
return false
}

export const retry = async (
getNewPromise: () => Promise<string>,
args: string,
nonRetryErrors: string[],
waitBeforeRetry = 500
): Promise<T> => {
): Promise<string> => {
try {
return await getNewPromise()
} catch (error: unknown) {
const errorMessage = (error as Error).message
Logger.error(`${args} failed with ${errorMessage} retrying...`)

if (isNonRetryError(errorMessage, nonRetryErrors)) {
return ''
}

await delay(waitBeforeRetry)
return retry(getNewPromise, args, waitBeforeRetry * 2)
return retry(getNewPromise, args, nonRetryErrors, waitBeforeRetry * 2)
}
}
4 changes: 2 additions & 2 deletions extension/src/experiments/data/collect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ import { ExperimentsOutput } from '../../cli/reader'
export const collectFiles = (data: ExperimentsOutput): string[] => {
const files = new Set<string>(
Object.keys({
...data?.workspace.baseline?.data?.params,
...data?.workspace.baseline?.data?.metrics
...data?.workspace?.baseline?.data?.params,
...data?.workspace?.baseline?.data?.metrics
}).filter(Boolean)
)

Expand Down
4 changes: 4 additions & 0 deletions extension/src/experiments/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,10 @@ export class Experiments extends BaseRepository<TableData> {
}

public getExperimentCount() {
if (!this.columns.hasColumns()) {
return 0
}

return this.experiments.getExperimentCount()
}

Expand Down
15 changes: 8 additions & 7 deletions extension/src/experiments/model/tree.ts
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,14 @@ export class ExperimentsTree

private getDescription() {
const dvcRoots = this.experiments.getDvcRoots()
if (!definedAndNonEmpty(dvcRoots)) {

const total = sum(
dvcRoots.map(dvcRoot =>
this.experiments.getRepository(dvcRoot).getExperimentCount()
)
)

if (!total) {
return
}

Expand All @@ -279,12 +286,6 @@ export class ExperimentsTree
)
)

const total = sum(
dvcRoots.map(dvcRoot =>
this.experiments.getRepository(dvcRoot).getExperimentCount()
)
)

return `${selected} of ${total} (max ${MAX_SELECTED_EXPERIMENTS})`
}

Expand Down

0 comments on commit 36571f5

Please sign in to comment.