Skip to content

Show CLI errors generated by exp show #4062

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion extension/src/experiments/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,10 @@ export class Experiments extends BaseRepository<TableData> {
}

public getWorkspaceAndCommits() {
if (!this.columns.hasNonDefaultColumns()) {
if (
!this.experiments.getCliError() &&
!this.columns.hasNonDefaultColumns()
) {
return []
}

Expand Down Expand Up @@ -499,6 +502,10 @@ export class Experiments extends BaseRepository<TableData> {
return this.experiments.hasRunningWorkspaceExperiment()
}

public getCliError() {
return this.experiments.getCliError()
}

public getFirstThreeColumnOrder() {
return this.columns.getFirstThreeColumnOrder()
}
Expand Down
13 changes: 13 additions & 0 deletions extension/src/experiments/model/collect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ export type ExperimentItem = {
}

type ExperimentsAccumulator = {
cliError: string | undefined
commits: Experiment[]
experimentsByCommit: Map<string, Experiment[]>
hasCheckpoints: boolean
Expand Down Expand Up @@ -330,12 +331,22 @@ const hasCheckpoints = (data: ExpShowOutput) => {
return !!workspace.data.meta.has_checkpoints
}

const collectCliError = (
acc: ExperimentsAccumulator,
expShow: ExpShowOutput
) => {
if (expShow.length === 1 && acc.workspace.error) {
acc.cliError = acc.workspace.error
}
}

export const collectExperiments = (
expShow: ExpShowOutput,
gitLog: string,
dvcLiveOnly: boolean
): ExperimentsAccumulator => {
const acc: ExperimentsAccumulator = {
cliError: undefined,
commits: [],
experimentsByCommit: new Map(),
hasCheckpoints: hasCheckpoints(expShow),
Expand Down Expand Up @@ -363,6 +374,8 @@ export const collectExperiments = (

setWorkspaceAsRunning(acc, dvcLiveOnly)

collectCliError(acc, expShow)

return acc
}

Expand Down
28 changes: 28 additions & 0 deletions extension/src/experiments/model/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -522,4 +522,32 @@ describe('ExperimentsModel', () => {
expect(getSelectedRevisions(model)).toStrictEqual([])
expect(model.hasRunningExperiment()).toBe(false)
})

it('should capture a Cli error when exp show fails', () => {
const model = new ExperimentsModel('', buildMockMemento())

const errorMsg = 'a very unexpected error - (╯°□°)╯︵ ┻━┻'

const data = [
{
rev: EXPERIMENT_WORKSPACE_ID,
error: { msg: errorMsg, type: 'caught error' }
}
]
model.transformAndSet(data, gitLogFixture, false, [], {
main: 6
})

expect(model.getCliError()).toStrictEqual(errorMsg)

model.transformAndSet(
outputFixture,
gitLogFixture,
false,
rowOrderFixture,
{ main: 6 }
)

expect(model.getCliError()).toBe(undefined)
})
})
28 changes: 21 additions & 7 deletions extension/src/experiments/model/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ export enum ExperimentType {

export class ExperimentsModel extends ModelWithPersistence {
private workspace = {} as Experiment
private cliError: undefined | string
private commits: Experiment[] = []
private experimentsByCommit: Map<string, Experiment[]> = new Map()
private rowOrder: { branch: string; sha: string }[] = []
Expand Down Expand Up @@ -122,11 +123,12 @@ export class ExperimentsModel extends ModelWithPersistence {
availableNbCommits: { [branch: string]: number }
) {
const {
workspace,
cliError,
commits,
experimentsByCommit,
hasCheckpoints,
runningExperiments,
hasCheckpoints
workspace
} = collectExperiments(expShow, gitLog, dvcLiveOnly)

const { hasMoreCommits, isShowingMoreCommits } =
Expand All @@ -141,6 +143,7 @@ export class ExperimentsModel extends ModelWithPersistence {

this.rowOrder = rowOrder
this.workspace = workspace
this.cliError = cliError
this.commits = commits
this.experimentsByCommit = experimentsByCommit
this.checkpoints = hasCheckpoints
Expand Down Expand Up @@ -192,6 +195,10 @@ export class ExperimentsModel extends ModelWithPersistence {
return this.checkpoints
}

public getCliError() {
return this.cliError
}

public canSelect() {
return canSelect(this.coloredStatus)
}
Expand Down Expand Up @@ -323,11 +330,18 @@ export class ExperimentsModel extends ModelWithPersistence {
}

public getErrors() {
return new Set(
this.getCombinedList()
.filter(({ error }) => error)
.map(({ label }) => label)
)
const errors = new Set<string>()
for (const { error, label } of this.getCombinedList()) {
if (!error) {
continue
}
errors.add(label)
}
if (this.cliError) {
errors.add(this.cliError)
}

return errors
}

public getExperimentParams(id: string) {
Expand Down
32 changes: 32 additions & 0 deletions extension/src/experiments/model/tree.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ const mockedGetMarkdownString = jest.mocked(getMarkdownString)
const {
mockedExperiments,
mockedGetCommitExperiments,
mockedGetCliError,
mockedGetDvcRoots,
mockedGetFirstThreeColumnOrder,
mockedGetWorkspaceAndCommits
Expand Down Expand Up @@ -450,6 +451,37 @@ describe('ExperimentsTree', () => {
}
])
})

it('should return an error item if the repository has a CLI error', async () => {
const errorMsg = 'the CLI is broken :('
mockedGetCliError.mockReturnValueOnce(errorMsg)
const experiments = [
{
displayColor: undefined,
error: errorMsg,
hasChildren: false,
id: EXPERIMENT_WORKSPACE_ID,
label: EXPERIMENT_WORKSPACE_ID,
selected: false,
type: ExperimentType.WORKSPACE
}
]

mockedGetWorkspaceAndCommits
.mockReturnValueOnce(experiments)
.mockReturnValueOnce(experiments)
mockedGetDvcRoots.mockReturnValueOnce(['repo'])
mockedGetFirstThreeColumnOrder.mockReturnValue([])

const experimentsTree = new ExperimentsTree(
mockedExperiments,
mockedResourceLocator
)

const children = await experimentsTree.getChildren()

expect(children).toStrictEqual([{ error: errorMsg }])
})
})

describe('getTreeItem', () => {
Expand Down
38 changes: 31 additions & 7 deletions extension/src/experiments/model/tree.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,12 @@ import { definedAndNonEmpty, sortCollectedArray } from '../../util/array'
import {
createTreeView,
DecoratableTreeItemScheme,
ErrorItem,
getCliErrorTreeItem,
getDecoratableTreeItem,
getErrorTooltip,
getRootItem,
isErrorItem,
isRoot
} from '../../tree'
import { IconName, Resource, ResourceLocator } from '../../resourceLocator'
Expand All @@ -44,14 +47,14 @@ type ExperimentAugmented = Experiment & {

export class ExperimentsTree
extends Disposable
implements TreeDataProvider<string | ExperimentItem>
implements TreeDataProvider<string | ExperimentItem | ErrorItem>
{
public readonly onDidChangeTreeData: Event<string | void>

private readonly experiments: WorkspaceExperiments
private readonly resourceLocator: ResourceLocator

private readonly view: TreeView<string | ExperimentItem>
private readonly view: TreeView<string | ExperimentItem | ErrorItem>
private viewed = false

constructor(
Expand All @@ -63,7 +66,11 @@ export class ExperimentsTree
this.onDidChangeTreeData = experiments.experimentsChanged.event

this.view = this.dispose.track(
createTreeView<ExperimentItem>('dvc.views.experimentsTree', this, true)
createTreeView<ExperimentItem | ErrorItem>(
'dvc.views.experimentsTree',
this,
true
)
)

this.experiments = experiments
Expand All @@ -79,6 +86,15 @@ export class ExperimentsTree
return getRootItem(element)
}

if (isErrorItem(element)) {
const { error } = element
return getCliErrorTreeItem(
error,
error,
DecoratableTreeItemScheme.EXPERIMENTS
)
}

const {
label,
collapsibleState,
Expand Down Expand Up @@ -109,7 +125,7 @@ export class ExperimentsTree

public getChildren(
element?: string | ExperimentItem
): Promise<string[] | ExperimentItem[]> {
): Promise<string[] | ExperimentItem[] | ErrorItem[]> {
if (!element) {
return this.getRootElements()
}
Expand Down Expand Up @@ -226,9 +242,17 @@ export class ExperimentsTree
}
}

private getWorkspaceAndCommits(dvcRoot: string): ExperimentItem[] {
return this.experiments
.getRepository(dvcRoot)
private getWorkspaceAndCommits(
dvcRoot: string
): ExperimentItem[] | ErrorItem[] {
const repository = this.experiments.getRepository(dvcRoot)

const cliError = repository.getCliError()
if (cliError) {
return [{ error: cliError }]
}

return repository
.getWorkspaceAndCommits()
.map(experiment => this.formatExperiment(experiment, dvcRoot))
}
Expand Down
1 change: 1 addition & 0 deletions extension/src/experiments/webview/contract.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ export interface Column extends ColumnAggregateData {

export type TableData = {
changes: string[]
cliError: string | undefined
columnOrder: string[]
columns: Column[]
columnWidths: Record<string, number>
Expand Down
14 changes: 14 additions & 0 deletions extension/src/experiments/webview/messages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,9 @@ export class WebviewMessages {
case MessageFromWebviewType.SELECT_BRANCHES:
return this.addAndRemoveBranches()

case MessageFromWebviewType.REFRESH_EXP_DATA:
return this.refreshData()

default:
Logger.error(`Unexpected message: ${JSON.stringify(message)}`)
}
Expand Down Expand Up @@ -242,9 +245,20 @@ export class WebviewMessages {
await this.update()
}

private refreshData() {
sendTelemetryEvent(
EventName.VIEWS_EXPERIMENTS_TABLE_REFRESH,
undefined,
undefined
)

return this.update()
}

private getWebviewData() {
return {
changes: this.columns.getChanges(),
cliError: this.experiments.getCliError(),
columnOrder: this.columns.getColumnOrder(),
columnWidths: this.columns.getColumnWidths(),
columns: this.columns.getSelected(),
Expand Down
13 changes: 13 additions & 0 deletions extension/src/experiments/workspace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -386,6 +386,19 @@ export class WorkspaceExperiments extends BaseWorkspaceWebviews<
return allLoading
}

public getCliError() {
const repositories = Object.values(this.repositories)
const errors = []
for (const repository of repositories) {
const cliError = repository.getCliError()
if (!cliError) {
continue
}
errors.push(cliError)
}
return errors.length > 0 ? errors.join('\n') : undefined
}

public hasRunningExperiment() {
return Object.values(this.repositories).some(experiments =>
experiments.hasRunningExperiment()
Expand Down
4 changes: 3 additions & 1 deletion extension/src/setup/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ export class Setup

private readonly webviewMessages: WebviewMessages
private readonly getHasData: () => boolean | undefined
private readonly getExpShowError: () => string | undefined
private readonly collectWorkspaceScale: () => Promise<WorkspaceScale>

private readonly workspaceChanged: EventEmitter<void> = this.dispose.track(
Expand Down Expand Up @@ -134,6 +135,7 @@ export class Setup
}

this.getHasData = () => experiments.getHasData()
this.getExpShowError = () => experiments.getCliError()
const onDidChangeHasData = experiments.columnsChanged.event
this.dispose.track(
onDidChangeHasData(() =>
Expand Down Expand Up @@ -237,7 +239,7 @@ export class Setup
public shouldBeShown(): { dvc: boolean; experiments: boolean } {
return {
dvc: !!this.getCliCompatible() && this.hasRoots(),
experiments: !!this.getHasData()
experiments: !!(this.getExpShowError() || this.getHasData())
}
}

Expand Down
Loading