From ca80f9e5fccd93784aff4cea74a5dba241b6c113 Mon Sep 17 00:00:00 2001 From: Matt Seddon Date: Thu, 20 Apr 2023 13:27:16 +1000 Subject: [PATCH] Standardize how experiment information is displayed in plots ribbon --- extension/src/experiments/model/collect.ts | 4 +- extension/src/experiments/webview/contract.ts | 1 - extension/src/plots/model/index.test.ts | 10 ++-- extension/src/plots/model/index.ts | 7 ++- extension/src/plots/webview/contract.ts | 4 +- .../src/test/fixtures/expShow/base/rows.ts | 4 -- .../src/test/fixtures/plotsDiff/index.ts | 22 ++++----- .../src/test/suite/experiments/index.test.ts | 6 --- .../test/suite/experiments/model/tree.test.ts | 13 +++--- webview/src/plots/components/App.test.tsx | 14 +++--- .../comparisonTable/ComparisonTable.test.tsx | 18 ++++---- .../comparisonTable/ComparisonTableHead.tsx | 46 ++++++++++--------- .../src/plots/components/ribbon/Ribbon.tsx | 2 +- .../plots/components/ribbon/RibbonBlock.tsx | 33 ++++++++----- webview/src/stories/Ribbon.stories.tsx | 4 +- 15 files changed, 94 insertions(+), 94 deletions(-) diff --git a/extension/src/experiments/model/collect.ts b/extension/src/experiments/model/collect.ts index 4201bfe0ec..35101876e8 100644 --- a/extension/src/experiments/model/collect.ts +++ b/extension/src/experiments/model/collect.ts @@ -253,9 +253,7 @@ const collectExpRange = ( ) if (name) { - const description = `[${name}]` - experiment.description = description - experiment.logicalGroupName = description + experiment.description = `[${name}]` } collectExecutorInfo(acc, experiment, executor) diff --git a/extension/src/experiments/webview/contract.ts b/extension/src/experiments/webview/contract.ts index 02446dcae5..3dac98a247 100644 --- a/extension/src/experiments/webview/contract.ts +++ b/extension/src/experiments/webview/contract.ts @@ -35,7 +35,6 @@ export type Experiment = { executor?: Executor id: string label: string - logicalGroupName?: string metrics?: MetricOrParamColumns outs?: MetricOrParamColumns params?: MetricOrParamColumns diff --git a/extension/src/plots/model/index.test.ts b/extension/src/plots/model/index.test.ts index 8558d39c9d..815533b63d 100644 --- a/extension/src/plots/model/index.test.ts +++ b/extension/src/plots/model/index.test.ts @@ -158,7 +158,7 @@ describe('plotsModel', () => { ) expect( - model.getComparisonRevisions().map(({ revision }) => revision) + model.getComparisonRevisions().map(({ label }) => label) ).toStrictEqual(newOrder) }) @@ -170,7 +170,7 @@ describe('plotsModel', () => { model.setComparisonOrder(newOrder) expect( - model.getComparisonRevisions().map(({ revision }) => revision) + model.getComparisonRevisions().map(({ label }) => label) ).toStrictEqual([ ...newOrder, ...mockedSelectedRevisions @@ -196,19 +196,19 @@ describe('plotsModel', () => { model.setComparisonOrder(initialOrder) expect( - model.getComparisonRevisions().map(({ revision }) => revision) + model.getComparisonRevisions().map(({ label }) => label) ).toStrictEqual(initialOrder) model.setComparisonOrder() expect( - model.getComparisonRevisions().map(({ revision }) => revision) + model.getComparisonRevisions().map(({ label }) => label) ).toStrictEqual(initialOrder.filter(revision => revision !== 'main')) model.setComparisonOrder() expect( - model.getComparisonRevisions().map(({ revision }) => revision) + model.getComparisonRevisions().map(({ label }) => label) ).toStrictEqual([EXPERIMENT_WORKSPACE_ID, '71f31cf', 'main']) }) }) diff --git a/extension/src/plots/model/index.ts b/extension/src/plots/model/index.ts index 939664c4cb..a739eefaec 100644 --- a/extension/src/plots/model/index.ts +++ b/extension/src/plots/model/index.ts @@ -202,9 +202,9 @@ export class PlotsModel extends ModelWithPersistence { public getSelectedRevisionDetails() { const selectedRevisions: Revision[] = [] for (const experiment of this.experiments.getSelectedRevisions()) { - const { commit, description, label, displayColor, logicalGroupName, id } = - experiment + const { commit, description, label, displayColor, id } = experiment const revision: Revision = { + description: commit ? undefined : description, displayColor, errors: this.errors.getRevisionErrors(id), fetched: this.fetchedRevs.has(id), @@ -212,9 +212,8 @@ export class PlotsModel extends ModelWithPersistence { this.experiments.getFirstThreeColumnOrder(), experiment ), - group: logicalGroupName, id, - revision: label + label } if (commit) { diff --git a/extension/src/plots/webview/contract.ts b/extension/src/plots/webview/contract.ts index 40a786b9f8..854af2691a 100644 --- a/extension/src/plots/webview/contract.ts +++ b/extension/src/plots/webview/contract.ts @@ -61,9 +61,9 @@ export type Revision = { errors?: string[] fetched: boolean firstThreeColumns: RevisionFirstThreeColumns - group?: string + description: string | undefined id: string - revision: string + label: string } export interface PlotsComparisonData { diff --git a/extension/src/test/fixtures/expShow/base/rows.ts b/extension/src/test/fixtures/expShow/base/rows.ts index d46011420a..5c3432539f 100644 --- a/extension/src/test/fixtures/expShow/base/rows.ts +++ b/extension/src/test/fixtures/expShow/base/rows.ts @@ -155,7 +155,6 @@ const data: Commit[] = [ executor: Executor.DVC_TASK, id: 'exp-e7a67', label: '4fb124a', - logicalGroupName: '[exp-e7a67]', metrics: { 'summary.json': { loss: 2.0205044746398926, @@ -213,7 +212,6 @@ const data: Commit[] = [ description: '[test-branch]', id: 'test-branch', label: '42b8736', - logicalGroupName: '[test-branch]', metrics: { 'summary.json': { loss: 1.9293040037155151, @@ -271,7 +269,6 @@ const data: Commit[] = [ executor: Executor.WORKSPACE, id: 'exp-83425', label: EXPERIMENT_WORKSPACE_ID, - logicalGroupName: '[exp-83425]', metrics: { 'summary.json': { loss: 1.775016188621521, @@ -341,7 +338,6 @@ const data: Commit[] = [ error: "unable to read: 'summary.json', JSON file structure is corrupted", label: 'f0f9186', - logicalGroupName: '[exp-f13bca]', metrics: {}, params: { 'params.yaml': { diff --git a/extension/src/test/fixtures/plotsDiff/index.ts b/extension/src/test/fixtures/plotsDiff/index.ts index 6fa5a89d2e..bf6cbdeeb1 100644 --- a/extension/src/test/fixtures/plotsDiff/index.ts +++ b/extension/src/test/fixtures/plotsDiff/index.ts @@ -536,7 +536,8 @@ export const getRevisions = (): Revision[] => { return [ { id: EXPERIMENT_WORKSPACE_ID, - revision: EXPERIMENT_WORKSPACE_ID, + label: EXPERIMENT_WORKSPACE_ID, + description: undefined, displayColor: workspace, errors: undefined, fetched: true, @@ -556,8 +557,7 @@ export const getRevisions = (): Revision[] => { path: 'summary.json:accuracy', value: 0.5926499962806702 } - ], - group: undefined + ] }, { errors: undefined, @@ -580,9 +580,9 @@ export const getRevisions = (): Revision[] => { } ], id: 'main', - revision: 'main', + label: 'main', displayColor: main, - group: undefined + description: undefined }, { errors: undefined, @@ -605,9 +605,9 @@ export const getRevisions = (): Revision[] => { } ], id: 'exp-e7a67', - revision: '4fb124a', + label: '4fb124a', displayColor: _4fb124a, - group: '[exp-e7a67]' + description: '[exp-e7a67]' }, { errors: undefined, @@ -630,9 +630,9 @@ export const getRevisions = (): Revision[] => { } ], id: 'test-branch', - revision: '42b8736', + label: '42b8736', displayColor: _42b8736, - group: '[test-branch]' + description: '[test-branch]' }, { errors: undefined, @@ -655,9 +655,9 @@ export const getRevisions = (): Revision[] => { } ], id: 'exp-83425', - revision: EXPERIMENT_WORKSPACE_ID, + label: EXPERIMENT_WORKSPACE_ID, displayColor: _1ba7bcd, - group: '[exp-83425]' + description: '[exp-83425]' } ] } diff --git a/extension/src/test/suite/experiments/index.test.ts b/extension/src/test/suite/experiments/index.test.ts index 8b09d548da..7d6b973bd1 100644 --- a/extension/src/test/suite/experiments/index.test.ts +++ b/extension/src/test/suite/experiments/index.test.ts @@ -1587,7 +1587,6 @@ suite('Experiments Test Suite', () => { description: '[exp-1]', id: 'exp-1', label: '111111', - logicalGroupName: '[exp-1]', params: { 'params.yaml': { test: 2 } }, selected: false, starred: false @@ -1597,7 +1596,6 @@ suite('Experiments Test Suite', () => { description: '[exp-2]', id: 'exp-2', label: '222222', - logicalGroupName: '[exp-2]', params: { 'params.yaml': { test: 1 } }, selected: false, starred: false @@ -1607,7 +1605,6 @@ suite('Experiments Test Suite', () => { description: '[exp-3]', id: 'exp-3', label: '333333', - logicalGroupName: '[exp-3]', params: { 'params.yaml': { test: 3 } }, selected: false, starred: false @@ -1666,7 +1663,6 @@ suite('Experiments Test Suite', () => { description: '[exp-2]', id: 'exp-2', label: '222222', - logicalGroupName: '[exp-2]', params: { 'params.yaml': { test: 1 } }, selected: false, starred: false @@ -1676,7 +1672,6 @@ suite('Experiments Test Suite', () => { description: '[exp-1]', id: 'exp-1', label: '111111', - logicalGroupName: '[exp-1]', params: { 'params.yaml': { test: 2 } }, selected: false, starred: false @@ -1686,7 +1681,6 @@ suite('Experiments Test Suite', () => { description: '[exp-3]', id: 'exp-3', label: '333333', - logicalGroupName: '[exp-3]', params: { 'params.yaml': { test: 3 } }, selected: false, starred: false diff --git a/extension/src/test/suite/experiments/model/tree.test.ts b/extension/src/test/suite/experiments/model/tree.test.ts index b88b3be0e3..2c9ebcf550 100644 --- a/extension/src/test/suite/experiments/model/tree.test.ts +++ b/extension/src/test/suite/experiments/model/tree.test.ts @@ -193,13 +193,12 @@ suite('Experiments Tree Test Suite', () => { it('should be able to select / de-select experiments using dvc.views.experimentsTree.selectExperiments', async () => { const { plots, plotsModel, messageSpy } = await buildPlots(disposable) - const [{ revision: selectedDisplayName, displayColor }] = - plotsModel.getSelectedRevisionDetails() + const [{ label, displayColor }] = plotsModel.getSelectedRevisionDetails() const selectedItem = { - description: selectedDisplayName, + description: label, label: '', - value: { id: selectedDisplayName } + value: { id: label } } await plots.showWebview() @@ -239,12 +238,12 @@ suite('Experiments Tree Test Suite', () => { const { selectedRevisions } = getFirstArgOfLastCall(messageSpy) expect( - (selectedRevisions as Revision[]).map(({ displayColor, revision }) => ({ + (selectedRevisions as Revision[]).map(({ displayColor, label }) => ({ displayColor, - revision + label })), 'a message is sent with colors for the currently selected experiments' - ).to.deep.equal([{ displayColor, revision: selectedDisplayName }]) + ).to.deep.equal([{ displayColor, label }]) }).timeout(WEBVIEW_TEST_TIMEOUT) it('should be able to remove an experiment with dvc.views.experimentsTree.removeExperiment', async () => { diff --git a/webview/src/plots/components/App.test.tsx b/webview/src/plots/components/App.test.tsx index 73c4bae242..d0a11732aa 100644 --- a/webview/src/plots/components/App.test.tsx +++ b/webview/src/plots/components/App.test.tsx @@ -240,12 +240,12 @@ describe('App', () => { ], revisions: [ { + description: '[exp-a270a]', displayColor: '#945dd6', fetched: false, firstThreeColumns: [], - group: '[exp-a270a]', id: 'ad2b5ec', - revision: 'ad2b5ec' + label: 'ad2b5ec' } ], width: DEFAULT_NB_ITEMS_PER_ROW @@ -256,12 +256,12 @@ describe('App', () => { sectionCollapsed: DEFAULT_SECTION_COLLAPSED, selectedRevisions: [ { + description: '[exp-a270a]', displayColor: '#945dd6', fetched: false, firstThreeColumns: [], - group: '[exp-a270a]', id: 'ad2b5ec', - revision: 'ad2b5ec' + label: 'ad2b5ec' } ], template: null @@ -1903,7 +1903,7 @@ describe('App', () => { expect(getDisplayedRevisionOrder()).toStrictEqual( plotsRevisionsFixture.map(rev => - rev.group ? rev.group.slice(1, -1) + rev.revision : rev.revision + rev.description ? rev.label + rev.description : rev.label ) ) }) @@ -1980,7 +1980,7 @@ describe('App', () => { renderAppWithOptionalData({ comparison: comparisonTableFixture, selectedRevisions: plotsRevisionsFixture.map(rev => { - if (rev.revision === 'main') { + if (rev.label === 'main') { return { ...rev, errors: ['error'] @@ -1997,7 +1997,7 @@ describe('App', () => { renderAppWithOptionalData({ comparison: comparisonTableFixture, selectedRevisions: plotsRevisionsFixture.map(rev => { - if (rev.revision === 'main') { + if (rev.label === 'main') { return { ...rev, errors: ['error'], diff --git a/webview/src/plots/components/comparisonTable/ComparisonTable.test.tsx b/webview/src/plots/components/comparisonTable/ComparisonTable.test.tsx index 77042d5663..eb404b4f74 100644 --- a/webview/src/plots/components/comparisonTable/ComparisonTable.test.tsx +++ b/webview/src/plots/components/comparisonTable/ComparisonTable.test.tsx @@ -56,10 +56,10 @@ describe('ComparisonTable', () => { }) const selectedRevisions: Revision[] = plotsRevisionsFixture - const revisions = selectedRevisions.map(({ revision }) => revision) + const revisions = selectedRevisions.map(({ label }) => label) const ids = selectedRevisions.map(({ id }) => id) const namedRevisions = selectedRevisions.map( - ({ revision, group }) => `${revision}${group || ''}` + ({ label, description }) => `${label}${description || ''}` ) const renderTable = ( @@ -223,7 +223,7 @@ describe('ComparisonTable', () => { expect(headers).toStrictEqual(namedRevisions) const filteredRevisions = selectedRevisions.filter( - ({ revision }) => revision !== revisions[3] + ({ label }) => label !== revisions[3] ) renderTable( @@ -237,7 +237,7 @@ describe('ComparisonTable', () => { headers = getHeaders().map(header => header.textContent) const expectedRevisions = filteredRevisions.map( - ({ revision, group }) => `${revision}${group || ''}` + ({ label, description }) => `${label}${description || ''}` ) expect(headers).toStrictEqual(expectedRevisions) @@ -253,7 +253,7 @@ describe('ComparisonTable', () => { displayColor: '#000000', fetched: true, id: newRevName, - revision: newRevName + label: newRevName } ] as Revision[] @@ -286,12 +286,12 @@ describe('ComparisonTable', () => { revisions: [ ...comparisonTableFixture.revisions, { + description: undefined, displayColor: '#f56565', fetched: true, firstThreeColumns: [], - group: undefined, id: 'noData', - revision: revisionWithNoData + label: revisionWithNoData } ] }) @@ -319,12 +319,12 @@ describe('ComparisonTable', () => { revisions: [ ...comparisonTableFixture.revisions, { + description: undefined, displayColor: '#f56565', fetched: true, firstThreeColumns: [], - group: undefined, id: revisionWithNoData, - revision: 'noData' + label: 'noData' } ] }) diff --git a/webview/src/plots/components/comparisonTable/ComparisonTableHead.tsx b/webview/src/plots/components/comparisonTable/ComparisonTableHead.tsx index a66f5d7a7a..dacd451ff2 100644 --- a/webview/src/plots/components/comparisonTable/ComparisonTableHead.tsx +++ b/webview/src/plots/components/comparisonTable/ComparisonTableHead.tsx @@ -57,28 +57,32 @@ export const ComparisonTableHead: React.FC = ({ } }, [ribbonHeight, headRef]) - const items = columns.map(({ revision, id, displayColor, group }) => { - const isPinned = revision === pinnedColumn - return ( - - setPinnedColumn(id)} - displayColor={displayColor} + const items = columns.map( + ({ label, id, displayColor, description, commit }) => { + const isPinned = label === pinnedColumn + return ( + - {revision} - {group && {group}} - - - ) - }) + setPinnedColumn(id)} + displayColor={displayColor} + > + {label} + {!commit && description && ( + {description} + )} + + + ) + } + ) return ( diff --git a/webview/src/plots/components/ribbon/Ribbon.tsx b/webview/src/plots/components/ribbon/Ribbon.tsx index 2c78beba85..f589fd58f7 100644 --- a/webview/src/plots/components/ribbon/Ribbon.tsx +++ b/webview/src/plots/components/ribbon/Ribbon.tsx @@ -83,7 +83,7 @@ export const Ribbon: React.FC = () => { removeRevision(revision.id || '')} + onClear={() => removeRevision(revision.id)} /> ))} diff --git a/webview/src/plots/components/ribbon/RibbonBlock.tsx b/webview/src/plots/components/ribbon/RibbonBlock.tsx index a8ec8e4626..ee3f713154 100644 --- a/webview/src/plots/components/ribbon/RibbonBlock.tsx +++ b/webview/src/plots/components/ribbon/RibbonBlock.tsx @@ -33,29 +33,40 @@ export const RibbonBlock: React.FC = ({ onClear }) => { const { - firstThreeColumns, commit, + description, + displayColor, errors, fetched, - group, + firstThreeColumns, id, - revision: rev, - displayColor + label } = revision - const exp = group?.replace(/[[\]]/g, '') || rev const mainContent = (
  • -
    - {exp} - -
    - {group &&
    {rev}
    } + {description ? ( + <> +
    {label}
    +
    + {description} + +
    + + ) : ( +
    + {label} + +
    + )}
    diff --git a/webview/src/stories/Ribbon.stories.tsx b/webview/src/stories/Ribbon.stories.tsx index 61897631cd..ed4d88cc40 100644 --- a/webview/src/stories/Ribbon.stories.tsx +++ b/webview/src/stories/Ribbon.stories.tsx @@ -56,7 +56,7 @@ export const WithLoading = Template.bind({}) WithLoading.args = { data: { selectedRevisions: plotsRevisionsFixtureWithCommit.map(item => { - if (['main', '42b8736'].includes(item.revision)) { + if (['main', '42b8736'].includes(item.label)) { return { ...item, fetched: false } } return item @@ -68,7 +68,7 @@ export const WithErrors = Template.bind({}) WithErrors.args = { data: { selectedRevisions: plotsRevisionsFixtureWithCommit.map(item => { - if (['main', '42b8736'].includes(item.revision)) { + if (['main', '42b8736'].includes(item.label)) { return { ...item, errors: [