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

Bump min DVC version to 3.33.0 (standardise plots across products) #4734

Merged
merged 31 commits into from
Dec 6, 2023
Merged
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
c0f2680
update test fixtures, unit tests and jam new functionality in
mattseddon Sep 28, 2023
07e17e5
use new approach for custom plots (reuse code)
mattseddon Sep 29, 2023
4f95632
fix zoomable vs zoomed in plot
mattseddon Sep 29, 2023
c260478
refactor extending vega lite
mattseddon Oct 2, 2023
75cf24d
add zoom and pan anchor
mattseddon Oct 2, 2023
1684130
fill new tooltip anchor
mattseddon Oct 5, 2023
654db11
use new width and height anchors
mattseddon Oct 9, 2023
4a6d460
switch empty string to empty object from zoom and pan
mattseddon Oct 12, 2023
1266f3d
Merge branch 'main' into use-new-templates
mattseddon Nov 2, 2023
e617009
Merge branch 'main' into use-new-templates
mattseddon Nov 2, 2023
d89b179
fix svg export issue
mattseddon Nov 2, 2023
2feebf6
Merge branch 'main' into use-new-templates
mattseddon Nov 6, 2023
30bf918
pull out constants into contract
mattseddon Nov 6, 2023
bc452e4
Merge branch 'main' into use-new-templates
mattseddon Nov 21, 2023
7cef8c5
unmangle types
mattseddon Nov 22, 2023
28677b8
patch tests
mattseddon Nov 22, 2023
d5e9bd5
clean up test fixtures
mattseddon Nov 22, 2023
5eb85b7
make multi source bar horizontal plots multi view
mattseddon Nov 22, 2023
8859f8f
update demo project with test version of dvc
mattseddon Nov 22, 2023
ab08fc2
add tests for fill template
mattseddon Nov 23, 2023
aa4facc
patch titles behaviour
mattseddon Nov 23, 2023
cc8ca1f
ensure that there is always a data array for collection
mattseddon Nov 23, 2023
1ac3067
split use get plot and use get titles
mattseddon Nov 26, 2023
ee96b29
fix vega package json entries
mattseddon Nov 26, 2023
c57c33e
refactor fill template
mattseddon Nov 27, 2023
38abac9
Merge branch 'main' into use-new-templates
mattseddon Nov 30, 2023
d59d594
use parent ref to appropriately size plot titles
mattseddon Nov 30, 2023
b63f30c
Merge branch 'main' into use-new-templates
mattseddon Dec 5, 2023
faa1d34
create enum from plot anchors
mattseddon Dec 5, 2023
fb24a3e
Merge branch 'main' into use-new-templates
mattseddon Dec 5, 2023
7bde398
bump min version to 3.33.0 (breaking change plots diff)
mattseddon Dec 6, 2023
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
2 changes: 1 addition & 1 deletion demo
Submodule demo updated 1 files
+2 −2 requirements.txt
3 changes: 1 addition & 2 deletions extension/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -1727,7 +1727,7 @@
"process-exists": "4.1.0",
"tree-kill": "1.2.2",
"uuid": "9.0.1",
"vega-util": "1.17.2",
"vega-lite": "5.14.1",
"vscode-languageclient": "9.0.1",
"yaml": "2.3.4"
},
Expand All @@ -1748,7 +1748,6 @@
"@types/mocha": "10.0.6",
"@types/mock-require": "2.0.3",
"@types/node": "16.x",
"@types/react-vega": "7.0.0",
"@types/sinon-chai": "3.2.12",
"@types/uuid": "9.0.7",
"@types/vega": "3.2.0",
Expand Down
120 changes: 116 additions & 4 deletions extension/src/cli/dvc/contract.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Plot } from '../../plots/webview/contract'
import type { TopLevelSpec } from 'vega-lite'

export const MIN_CLI_VERSION = '2.58.1'
export const LATEST_TESTED_CLI_VERSION = '3.32.0'
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] Will need bumped

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't forget to remove the code that checks the CLI version when running exp rename

Copy link
Contributor

Choose a reason for hiding this comment

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

Are these things that need to be done before merging?

Copy link
Member Author

Choose a reason for hiding this comment

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

yep

Expand Down Expand Up @@ -129,8 +129,120 @@ export const experimentHasError = (

export type ExpShowOutput = (ExpState & { experiments?: ExpRange[] | null })[]

export enum PlotsType {
VEGA = 'vega',
IMAGE = 'image'
}
export const isImagePlotOutput = (plot: {
type: PlotsType
}): plot is ImagePlotOutput => plot.type === PlotsType.IMAGE

export const PLOT_REV_FIELD = 'rev' as const

export enum PLOT_ANCHORS {
COLOR = '<DVC_METRIC_COLOR>',
COLUMN = '<DVC_METRIC_COLUMN>',
DATA = '<DVC_METRIC_DATA>',
HEIGHT = '<DVC_METRIC_PLOT_HEIGHT>',
METRIC_TYPE = '<DVC_METRIC_TYPE>',
PARAM_TYPE = '<DVC_PARAM_TYPE>',
SHAPE = '<DVC_METRIC_SHAPE>',
STROKE_DASH = '<DVC_METRIC_STROKE_DASH>',
TITLE = '<DVC_METRIC_TITLE>',
WIDTH = '<DVC_METRIC_PLOT_WIDTH>',
X = '<DVC_METRIC_X>',
X_LABEL = '<DVC_METRIC_X_LABEL>',
Y = '<DVC_METRIC_Y>',
Y_LABEL = '<DVC_METRIC_Y_LABEL>',
ZOOM_AND_PAN = '<DVC_METRIC_ZOOM_AND_PAN>'
}

export const ZOOM_AND_PAN_PROP = {
bind: 'scales',
name: 'grid',
select: 'interval'
} as const

type FieldDef = { field: string }

type EmptyObject = Record<string, never>

export const PLOT_STROKE_DASH = [
[1, 0],
[8, 8],
[8, 4],
[4, 4],
[4, 2],
[2, 1],
[1, 1]
] as const
export type StrokeDashValue = (typeof PLOT_STROKE_DASH)[number]

export const PLOT_SHAPE = ['square', 'circle', 'triangle', 'diamond'] as const

export type ShapeValue = (typeof PLOT_SHAPE)[number]

type Scale<T extends StrokeDashValue | string> = {
domain: string[]
range: T[]
}

export type StrokeDashScale = Scale<StrokeDashValue>

export type ShapeScale = Scale<ShapeValue>

type Encoding<T extends StrokeDashValue | string> = FieldDef & {
scale: Scale<T>
legend?: null | {
symbolType?: string
symbolFillColor?: string
symbolStrokeColor?: string
}
}

type MultiSourceEncoding<T extends StrokeDashValue | ShapeValue> =
| Encoding<T>
| EmptyObject

export type StrokeDashEncoding = MultiSourceEncoding<StrokeDashValue>

export type ShapeEncoding = MultiSourceEncoding<ShapeValue>

export type AnchorDefinitions = {
[PLOT_ANCHORS.COLOR]?: Encoding<string>
[PLOT_ANCHORS.COLUMN]?: EmptyObject | { sort: never[]; field: string }
[PLOT_ANCHORS.DATA]?: Array<Record<string, unknown>>
[PLOT_ANCHORS.HEIGHT]?: number | 'container'
[PLOT_ANCHORS.METRIC_TYPE]?: 'quantitative' | 'nominal'
[PLOT_ANCHORS.PARAM_TYPE]?: 'quantitative' | 'nominal'
[PLOT_ANCHORS.SHAPE]?: MultiSourceEncoding<ShapeValue>
[PLOT_ANCHORS.STROKE_DASH]?: MultiSourceEncoding<StrokeDashValue>
[PLOT_ANCHORS.TITLE]?: string
[PLOT_ANCHORS.WIDTH]?: number | 'container'
[PLOT_ANCHORS.X]?: string
[PLOT_ANCHORS.X_LABEL]?: string
[PLOT_ANCHORS.Y]?: string
[PLOT_ANCHORS.Y_LABEL]?: string
[PLOT_ANCHORS.ZOOM_AND_PAN]?: typeof ZOOM_AND_PAN_PROP
}

export type TemplatePlotOutput = {
anchor_definitions: AnchorDefinitions
content: TopLevelSpec
revisions: string[]
type: PlotsType
}

export type ImagePlotOutput = {
revisions: string[]
type: PlotsType
url: string
}

export type PlotOutput = TemplatePlotOutput | ImagePlotOutput

export interface PlotsData {
[path: string]: Plot[]
[path: string]: PlotOutput[]
}

export type PlotError = {
Expand All @@ -140,12 +252,12 @@ export type PlotError = {
} & ErrorContents

export type RawPlotsOutput = {
data?: { [path: string]: Plot[] }
data?: { [path: string]: PlotOutput[] }
errors?: PlotError[]
}

export type PlotsOutput = RawPlotsOutput & {
data: { [path: string]: Plot[] }
data: { [path: string]: PlotOutput[] }
}

export type PlotsOutputOrError = PlotsOutput | DvcError
33 changes: 21 additions & 12 deletions extension/src/plots/data/collect.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
import { PlotsOutputOrError } from '../../cli/dvc/contract'
import {
PLOT_ANCHORS,
isImagePlotOutput,
PlotOutput,
PlotsOutputOrError,
TemplatePlotOutput
} from '../../cli/dvc/contract'
import { isDvcError } from '../../cli/dvc/reader'
import { uniqueValues } from '../../util/array'
import { isImagePlot, Plot, TemplatePlot } from '../webview/contract'

const collectImageFile = (acc: string[], file: string): void => {
if (acc.includes(file)) {
Expand All @@ -14,26 +19,30 @@ const collectFromDatapoint = (
acc: string[],
data: Record<string, unknown>
): void => {
const filename = (data as { dvc_data_version_info?: { filename?: string } })
?.dvc_data_version_info?.filename
const filename = data.filename

if (!filename || acc.includes(filename)) {
if (!filename || typeof filename !== 'string' || acc.includes(filename)) {
return
}
acc.push(filename)
}

const collectTemplateFiles = (acc: string[], plot: TemplatePlot): void => {
for (const datapoints of Object.values(plot.datapoints || {})) {
for (const datapoint of datapoints) {
collectFromDatapoint(acc, datapoint)
}
const collectTemplateFiles = (
acc: string[],
plot: TemplatePlotOutput
): void => {
for (const datapoint of plot.anchor_definitions[PLOT_ANCHORS.DATA] || []) {
collectFromDatapoint(acc, datapoint)
}
}

const collectKeyData = (acc: string[], key: string, plots: Plot[]): void => {
const collectKeyData = (
acc: string[],
key: string,
plots: PlotOutput[]
): void => {
for (const plot of plots) {
if (isImagePlot(plot)) {
if (isImagePlotOutput(plot)) {
collectImageFile(acc, key)
continue
}
Expand Down
49 changes: 20 additions & 29 deletions extension/src/plots/model/collect.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { join } from 'path'
import isEmpty from 'lodash.isempty'
import {
collectData,
collectTemplates,
collectTemplatesDetails,
collectCustomPlots,
collectOrderedRevisions,
collectImageUrl
Expand All @@ -12,15 +12,13 @@ import customPlotsFixture, {
customPlotsOrderFixture,
experimentsWithCommits
} from '../../test/fixtures/expShow/base/customPlots'
import { EXPERIMENT_WORKSPACE_ID } from '../../cli/dvc/contract'
import { sameContents } from '../../util/array'
import {
CustomPlotData,
DEFAULT_NB_ITEMS_PER_ROW,
DEFAULT_PLOT_HEIGHT,
ImagePlot,
TemplatePlot
} from '../webview/contract'
PLOT_ANCHORS,
EXPERIMENT_WORKSPACE_ID,
TemplatePlotOutput
} from '../../cli/dvc/contract'
import { sameContents } from '../../util/array'
import { CustomPlotData, ImagePlot } from '../webview/contract'
import { exists } from '../../fileSystem'
import { REVISIONS } from '../../test/fixtures/plotsDiff'

Expand All @@ -35,7 +33,7 @@ beforeEach(() => {
const logsLossPath = join('logs', 'loss.tsv')

const logsLossPlot = (plotsDiffFixture.data[logsLossPath][0] ||
{}) as TemplatePlot
{}) as TemplatePlotOutput

describe('collectCustomPlots', () => {
it('should return the expected data from the test fixture', () => {
Expand All @@ -46,8 +44,6 @@ describe('collectCustomPlots', () => {
range: ['#13adc7', '#f46837', '#48bb78', '#4299e1']
},
experiments: experimentsWithCommits,
height: DEFAULT_PLOT_HEIGHT,
nbItemsPerRow: DEFAULT_NB_ITEMS_PER_ROW,
plotsOrderValues: customPlotsOrderFixture
})
expect(data).toStrictEqual(expectedOutput)
Expand All @@ -60,11 +56,11 @@ describe('collectCustomPlots', () => {
range: ['#13adc7']
},
experiments: experimentsWithCommits,
height: DEFAULT_PLOT_HEIGHT,
nbItemsPerRow: DEFAULT_NB_ITEMS_PER_ROW,
plotsOrderValues: customPlotsOrderFixture
})
expect(data[0].values.slice(-1)[0].id).toStrictEqual('main')
expect(
(data[0].anchorDefinitions[PLOT_ANCHORS.DATA] || []).slice(-1)[0].id
).toStrictEqual('main')
})

it('should create custom plot scales that match the collected values', () => {
Expand All @@ -88,8 +84,6 @@ describe('collectCustomPlots', () => {
label: '123'
}
],
height: DEFAULT_PLOT_HEIGHT,
nbItemsPerRow: DEFAULT_NB_ITEMS_PER_ROW,
plotsOrderValues: customPlotsOrderFixture
})
expect(data).toStrictEqual(expectedOutput)
Expand All @@ -100,18 +94,12 @@ describe('collectData', () => {
it('should return the expected output from the test fixture', () => {
const { revisionData, comparisonData } = collectData(plotsDiffFixture)

const values =
(logsLossPlot?.datapoints as {
[revision: string]: Record<string, unknown>[]
}) || {}
const values = logsLossPlot?.anchor_definitions?.[PLOT_ANCHORS.DATA] || []

expect(isEmpty(values)).toBeFalsy()

for (const revision of REVISIONS) {
const expectedValues = values[revision]?.map(value => ({
...value,
rev: revision
}))
const expectedValues = values.filter(({ rev }) => rev === revision)
expect(revisionData[revision][logsLossPath]).toStrictEqual(expectedValues)
}

Expand Down Expand Up @@ -139,24 +127,27 @@ describe('collectData', () => {
expect(testBranchHeatmap).toBeDefined()
expect(testBranchHeatmap).toStrictEqual([
plotsDiffFixture.data[heatmapPlot].find(({ revisions }) =>
sameContents(revisions as string[], ['test-branch'])
sameContents(revisions, ['test-branch'])
)
])
})
})

describe('collectTemplates', () => {
it('should return the expected output from the test fixture', () => {
const { content } = logsLossPlot
const { content, anchor_definitions } = logsLossPlot

const templates = collectTemplates(plotsDiffFixture)
const templates = collectTemplatesDetails(plotsDiffFixture)
expect(Object.keys(templates)).toStrictEqual([
logsLossPath,
join('logs', 'acc.tsv'),
'predictions.json'
])

expect(JSON.parse(templates[logsLossPath])).toStrictEqual(content)
expect(templates[logsLossPath]).toStrictEqual({
anchorDefinitions: anchor_definitions,
content
})
})
})

Expand Down
Loading
Loading