Skip to content
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

Highlight experiments with errors #2072

Merged
merged 12 commits into from
Jul 21, 2022
1 change: 1 addition & 0 deletions .vscode/settings.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
"conda",
"datapoint",
"datapoints",
"Decoratable",
"dvcignore",
"DVCLIVE",
"DVCPATH",
Expand Down
21 changes: 21 additions & 0 deletions extension/src/experiments/columns/extract.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import { extractColumns } from './extract'

describe('extractColumns', () => {
it('should handle concatenating errors', () => {
const data = {
metrics: {
'summary.json': {
error: { msg: 'metrics file is busted', type: 'fatal' }
}
},
params: {
'params.yaml': {
error: { msg: 'this is also broken', type: 'impossible' }
}
}
}

const { error } = extractColumns(data)
expect(error).toStrictEqual('metrics file is busted\nthis is also broken')
})
})
79 changes: 59 additions & 20 deletions extension/src/experiments/columns/extract.ts
Original file line number Diff line number Diff line change
@@ -1,25 +1,45 @@
import { Deps, ExperimentFields, ValueTreeRoot } from '../../cli/reader'
import {
Deps,
ExperimentFields,
ValueTreeOrError,
ValueTreeRoot
} from '../../cli/reader'
import { shortenForLabel } from '../../util/string'
import {
DepColumns,
Experiment,
MetricOrParamColumns
} from '../webview/contract'

const extractFileMetricsOrParams = (
acc: { columns: MetricOrParamColumns; errors: string[] },
file: string,
dataOrError: ValueTreeOrError
) => {
const data = dataOrError?.data
const error = dataOrError?.error?.msg
if (error) {
acc.errors.push(error)
}
if (!data) {
return
}
acc.columns[file] = data
}

const extractMetricsOrParams = (
columns?: ValueTreeRoot
): MetricOrParamColumns | undefined => {
if (!columns) {
valueTreeRoot?: ValueTreeRoot
): { columns: MetricOrParamColumns; errors: string[] } | undefined => {
if (!valueTreeRoot) {
return
}
const acc: MetricOrParamColumns = {}
const acc: { columns: MetricOrParamColumns; errors: string[] } = {
columns: {},
errors: []
}

for (const [file, dataOrError] of Object.entries(columns)) {
const data = dataOrError?.data
if (!data) {
continue
}
acc[file] = data
for (const [file, dataOrError] of Object.entries(valueTreeRoot)) {
extractFileMetricsOrParams(acc, file, dataOrError)
}

return acc
Expand All @@ -46,15 +66,34 @@ const extractDeps = (
return acc
}

export const extractColumns = (
experiment: ExperimentFields,
branch?: Experiment
): {
type Columns = {
error?: string
deps: DepColumns | undefined
metrics: MetricOrParamColumns | undefined
params: MetricOrParamColumns | undefined
} => ({
deps: extractDeps(experiment.deps, branch),
metrics: extractMetricsOrParams(experiment.metrics),
params: extractMetricsOrParams(experiment.params)
})
}

export const extractColumns = (
experiment: ExperimentFields,
branch?: Experiment
): Columns => {
const metricsData = extractMetricsOrParams(experiment.metrics)
const paramsData = extractMetricsOrParams(experiment.params)

const error = [
...(metricsData?.errors || []),
...(paramsData?.errors || [])
].join('\n')
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[I] Test this bit and the tooltip showing up in the UI as markdown.


const columns: Columns = {
deps: extractDeps(experiment.deps, branch),
metrics: metricsData?.columns,
params: paramsData?.columns
}

if (error) {
columns.error = error
}

return columns
}
5 changes: 3 additions & 2 deletions extension/src/experiments/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import { ExperimentsData } from './data'
import { askToDisableAutoApplyFilters } from './toast'
import { Experiment, ColumnType, TableData } from './webview/contract'
import { WebviewMessages } from './webview/messages'
import { DecorationProvider } from './model/filterBy/decorationProvider'
import { DecorationProvider } from './model/decorationProvider'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[F] Move to a more central location as the decorations are now for both errors and filtered experiments

import { ResourceLocator } from '../resourceLocator'
import { AvailableCommands, InternalCommands } from '../commands/internal'
import { ExperimentsOutput } from '../cli/reader'
Expand Down Expand Up @@ -425,7 +425,8 @@ export class Experiments extends BaseRepository<TableData> {
private notifyChanged(data?: ExperimentsOutput) {
this.decorationProvider.setState(
this.experiments.getLabels(),
this.experiments.getLabelsToDecorate()
this.experiments.getLabelsToDecorate(),
this.experiments.getErrors()
)
this.experimentsChanged.fire(data)
this.notifyColumnsChanged()
Expand Down
60 changes: 40 additions & 20 deletions extension/src/experiments/model/collect.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
import { ThemeIcon, TreeItemCollapsibleState, Uri } from 'vscode'
import {
MarkdownString,
ThemeIcon,
TreeItemCollapsibleState,
Uri
} from 'vscode'
import omit from 'lodash.omit'
import { ExperimentType } from '.'
import { ExperimentsAccumulator } from './accumulator'
Expand Down Expand Up @@ -29,6 +34,7 @@ export type ExperimentItem = {
collapsibleState: TreeItemCollapsibleState
type: ExperimentType
iconPath: ThemeIcon | Uri | Resource
tooltip: MarkdownString | undefined
}

type ExperimentsObject = { [sha: string]: ExperimentFieldsOrError }
Expand All @@ -40,8 +46,12 @@ export const isCheckpoint = (

const getExperimentId = (
sha: string,
experimentsFields: ExperimentFields
experimentsFields?: ExperimentFields
): string => {
if (!experimentsFields) {
return sha
}

const { name, checkpoint_tip } = experimentsFields

if (isCheckpoint(checkpoint_tip, sha)) {
Expand Down Expand Up @@ -115,7 +125,10 @@ const transformColumns = (
experimentFields: ExperimentFields,
branch?: Experiment
) => {
const { metrics, params, deps } = extractColumns(experimentFields, branch)
const { error, metrics, params, deps } = extractColumns(
experimentFields,
branch
)

if (metrics) {
experiment.metrics = metrics
Expand All @@ -126,21 +139,28 @@ const transformColumns = (
if (deps) {
experiment.deps = deps
}
if (error) {
experiment.error = error
}
}

const transformExperimentData = (
id: string,
experimentFields: ExperimentFields,
experimentFieldsOrError: ExperimentFieldsOrError,
label: string | undefined,
sha?: string,
displayNameOrParent?: string,
logicalGroupName?: string,
branch?: Experiment
): Experiment => {
const data = experimentFieldsOrError.data || {}

const error = experimentFieldsOrError.error

const experiment = {
id,
label,
...omit(experimentFields, Object.values(ColumnType))
...omit(data, Object.values(ColumnType))
} as Experiment

if (displayNameOrParent) {
Expand All @@ -155,7 +175,11 @@ const transformExperimentData = (
experiment.sha = sha
}

transformColumns(experiment, experimentFields, branch)
if (error) {
experiment.error = error.msg
}

transformColumns(experiment, data, branch)

return experiment
}
Expand All @@ -172,7 +196,8 @@ const transformExperimentOrCheckpointData = (
} => {
const experimentFields = experimentData.data
if (!experimentFields) {
return { experiment: undefined }
const error = experimentData?.error?.msg
return { experiment: { error, id: sha, label: shortenForLabel(sha) } }
}

const checkpointTipId = getCheckpointTipId(
Expand All @@ -186,7 +211,7 @@ const transformExperimentOrCheckpointData = (
checkpointTipId,
experiment: transformExperimentData(
id,
experimentFields,
experimentData,
shortenForLabel(sha),
sha,
getDisplayNameOrParent(sha, branchSha, experimentsObject),
Expand Down Expand Up @@ -254,14 +279,8 @@ const collectFromBranchesObject = (
for (const [sha, { baseline, ...experimentsObject }] of Object.entries(
branchesObject
)) {
const experimentFields = baseline.data
if (!experimentFields) {
continue
}

const name = experimentFields.name as string

const branch = transformExperimentData(name, experimentFields, name, sha)
const name = baseline.data?.name || sha
const branch = transformExperimentData(name, baseline, name, sha)

if (branch) {
collectFromExperimentsObject(acc, experimentsObject, sha, branch)
Expand All @@ -278,10 +297,11 @@ export const collectExperiments = (
const { workspace, ...branchesObject } = data
const workspaceId = 'workspace'

const workspaceFields = workspace.baseline.data
const workspaceBaseline = workspaceFields
? transformExperimentData(workspaceId, workspaceFields, workspaceId)
: undefined
const workspaceBaseline = transformExperimentData(
workspaceId,
workspace.baseline,
workspaceId
)

const acc = new ExperimentsAccumulator(workspaceBaseline)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import {
Uri,
window
} from 'vscode'
import { Disposable } from '../../../class/dispose'
import { Disposable } from '../../class/dispose'

export const getDecoratableUri = (label: string): Uri =>
Uri.from({ path: label, scheme: 'dvc.experiments' })
Expand All @@ -16,6 +16,10 @@ export class DecorationProvider
extends Disposable
implements FileDecorationProvider
{
private static DecorationError: FileDecoration = {
color: new ThemeColor('errorForeground')
}

private static DecorationFiltered: FileDecoration = {
color: new ThemeColor('gitDecoration.ignoredResourceForeground'),
tooltip: 'Filtered'
Expand All @@ -24,6 +28,7 @@ export class DecorationProvider
public readonly onDidChangeFileDecorations: Event<Uri[]>
private readonly decorationsChanged: EventEmitter<Uri[]>

private errors = new Set<string>()
private filtered = new Set<string>()

constructor(decorationsChanged?: EventEmitter<Uri[]>) {
Expand All @@ -38,19 +43,27 @@ export class DecorationProvider
}

public provideFileDecoration(uri: Uri): FileDecoration | undefined {
if (this.errors.has(uri.fsPath)) {
return DecorationProvider.DecorationError
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[F] Take priority but under the current implementation, we can't have both.

}
if (this.filtered.has(uri.fsPath)) {
return DecorationProvider.DecorationFiltered
}
}

public setState(labels: string[], filtered: Set<string>) {
public setState(
labels: string[],
filtered: Set<string>,
errors: Set<string>
) {
const urisToUpdate: Uri[] = []

for (const label of labels) {
urisToUpdate.push(getDecoratableUri(label))
}

this.filtered = filtered
this.errors = errors
this.decorationsChanged.fire(urisToUpdate)
}
}
8 changes: 8 additions & 0 deletions extension/src/experiments/model/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,14 @@ export class ExperimentsModel extends ModelWithPersistence {
]
}

public getErrors() {
return new Set(
this.getCombinedList()
.filter(({ error }) => error)
.map(({ label }) => label)
)
}

public getExperimentsWithCheckpoints(): ExperimentWithCheckpoints[] {
return this.getExperiments().map(experiment => {
const checkpoints = this.checkpointsByTip
Expand Down
Loading