From 033973ae061d7735dacec435ebfd5d5c384185ca Mon Sep 17 00:00:00 2001 From: Matt Seddon Date: Fri, 1 Dec 2023 09:52:10 +1100 Subject: [PATCH] Handle project having experiments with no data --- .../experiments/columns/collect/index.test.ts | 4 +-- .../src/experiments/columns/collect/index.ts | 5 +-- .../src/experiments/columns/model.test.ts | 4 +-- extension/src/experiments/index.ts | 9 ++--- extension/src/experiments/model/index.test.ts | 34 +++++++++++++++++++ extension/src/experiments/model/index.ts | 10 ++++++ extension/src/experiments/webview/messages.ts | 7 ++-- extension/src/setup/index.ts | 4 +-- extension/src/test/suite/setup/util.ts | 3 -- 9 files changed, 57 insertions(+), 23 deletions(-) diff --git a/extension/src/experiments/columns/collect/index.test.ts b/extension/src/experiments/columns/collect/index.test.ts index 77992423ca..c12a1b5298 100644 --- a/extension/src/experiments/columns/collect/index.test.ts +++ b/extension/src/experiments/columns/collect/index.test.ts @@ -73,9 +73,9 @@ describe('collectColumns', () => { expect(metrics).toBeDefined() }) - it('should return an empty array if no params and metrics are provided', async () => { + it('should return the timestamp column if no params, metrics or deps are provided', async () => { const columns = await collectColumns(generateTestExpShowOutput({})) - expect(columns).toStrictEqual([]) + expect(columns).toStrictEqual([timestampColumn]) }) it('should aggregate multiple different field names', async () => { diff --git a/extension/src/experiments/columns/collect/index.ts b/extension/src/experiments/columns/collect/index.ts index 7e083f3a3b..83ab3df1d8 100644 --- a/extension/src/experiments/columns/collect/index.ts +++ b/extension/src/experiments/columns/collect/index.ts @@ -71,10 +71,7 @@ export const collectColumns = async ( } await Promise.all(promises) - const columns = Object.values(acc.columns) - const hasNoData = isEqual(columns, [timestampColumn]) - - return hasNoData ? [] : columns + return Object.values(acc.columns) } export const getExpData = (expState: ExpState): ExpData | undefined => { diff --git a/extension/src/experiments/columns/model.test.ts b/extension/src/experiments/columns/model.test.ts index ef446ba6a5..12e7db4185 100644 --- a/extension/src/experiments/columns/model.test.ts +++ b/extension/src/experiments/columns/model.test.ts @@ -46,7 +46,7 @@ describe('ColumnsModel', () => { const exampleDvcRoot = 'test' const mockedColumnsOrderOrStatusChanged = buildMockedEventEmitter() - it('should return no columns when given an output with no data', async () => { + it('should return the timestamp column when given output with no params, metrics or deps', async () => { const model = new ColumnsModel( '', buildMockMemento(), @@ -54,7 +54,7 @@ describe('ColumnsModel', () => { ) await model.transformAndSet(generateTestExpShowOutput({})) - expect(model.getSelected()).toStrictEqual([]) + expect(model.getSelected()).toStrictEqual([timestampColumn]) }) it('should return the expected columns when given the default output fixture', async () => { diff --git a/extension/src/experiments/index.ts b/extension/src/experiments/index.ts index 2261582476..1f29d15182 100644 --- a/extension/src/experiments/index.ts +++ b/extension/src/experiments/index.ts @@ -481,10 +481,7 @@ export class Experiments extends BaseRepository { } public getWorkspaceAndCommits() { - if ( - !this.experiments.getCliError() && - !this.columns.hasNonDefaultColumns() - ) { + if (!this.experiments.hasData(this.columns.hasNonDefaultColumns())) { return [] } @@ -496,7 +493,7 @@ export class Experiments extends BaseRepository { } public getSelectedRevisions() { - if (!this.columns.hasNonDefaultColumns()) { + if (!this.experiments.hasData(this.columns.hasNonDefaultColumns())) { return [] } @@ -603,7 +600,7 @@ export class Experiments extends BaseRepository { if (this.deferred.state === 'none') { return } - return this.columns.hasNonDefaultColumns() + return this.experiments.hasData(this.columns.hasNonDefaultColumns()) } public getRelativeMetricsFiles() { diff --git a/extension/src/experiments/model/index.test.ts b/extension/src/experiments/model/index.test.ts index cc71c4dd15..644d58adbb 100644 --- a/extension/src/experiments/model/index.test.ts +++ b/extension/src/experiments/model/index.test.ts @@ -611,4 +611,38 @@ describe('ExperimentsModel', () => { expect(model.getCliError()).toBe(undefined) }) + + it('should return the correct values for hasData', () => { + const model = new ExperimentsModel('', buildMockMemento()) + + expect(model.hasData(false)).toBe(false) + expect(model.hasData(true)).toBe(true) + + model.transformAndSetLocal( + outputFixture, + gitLogFixture, + { running: false }, + rowOrderFixture, + { main: 6 } + ) + + expect(model.hasData(false)).toBe(true) + expect(model.hasData(true)).toBe(true) + + model.transformAndSetLocal( + [ + { + error: { msg: 'CLI is broken', type: 'CLI ERROR' }, + rev: EXPERIMENT_WORKSPACE_ID + } + ], + gitLogFixture, + { running: false }, + rowOrderFixture, + { main: 6 } + ) + + expect(model.hasData(false)).toBe(true) + expect(model.hasData(true)).toBe(true) + }) }) diff --git a/extension/src/experiments/model/index.ts b/extension/src/experiments/model/index.ts index e25b411539..22d2581edb 100644 --- a/extension/src/experiments/model/index.ts +++ b/extension/src/experiments/model/index.ts @@ -524,6 +524,16 @@ export class ExperimentsModel extends ModelWithPersistence { })) } + public hasData(hasNonDefaultColumns: boolean) { + if (this.getCliError()) { + return true + } + + const hasRows = this.getWorkspaceAndCommits().length > 0 + const hasExperiments = this.getExperimentsAndQueued().length > 0 + return (hasNonDefaultColumns || hasExperiments) && hasRows + } + public setNbfCommitsToShow(numberOfCommitsToShow: number, branch: string) { this.numberOfCommitsToShow[branch] = numberOfCommitsToShow this.persistNbOfCommitsToShow() diff --git a/extension/src/experiments/webview/messages.ts b/extension/src/experiments/webview/messages.ts index 86b2a4cad5..7bd4f2c6a7 100644 --- a/extension/src/experiments/webview/messages.ts +++ b/extension/src/experiments/webview/messages.ts @@ -84,10 +84,11 @@ export class WebviewMessages { } const data = await this.getWebviewData() - const hasNoRows = data.rows.length === 0 - const hasNoData = !this.columns.hasNonDefaultColumns() || hasNoRows + const hasNoData = !this.experiments.hasData( + this.columns.hasNonDefaultColumns() + ) - if (hasNoData && !data.cliError) { + if (hasNoData) { await commands.executeCommand(RegisteredCommands.SETUP_SHOW_EXPERIMENTS) return webview.dispose() } diff --git a/extension/src/setup/index.ts b/extension/src/setup/index.ts index 90def128cc..2f2695f18e 100644 --- a/extension/src/setup/index.ts +++ b/extension/src/setup/index.ts @@ -85,7 +85,6 @@ export class Setup private readonly webviewMessages: WebviewMessages private readonly getHasData: () => boolean | undefined - private readonly getExpShowError: () => string | undefined private readonly collectWorkspaceScale: () => Promise private readonly setupRun: EventEmitter = this.dispose.track( @@ -155,7 +154,6 @@ export class Setup void this.sendDataToWebview() this.getHasData = () => experiments.getHasData() - this.getExpShowError = () => experiments.getCliError() const onDidChangeHasData = experiments.columnsChanged.event this.dispose.track(onDidChangeHasData(() => this.updateProjectHasData())) @@ -256,7 +254,7 @@ export class Setup public shouldBeShown(): { dvc: boolean; experiments: boolean } { return { dvc: !!this.getCliCompatible() && this.hasRoots(), - experiments: !!(this.getExpShowError() || this.getHasData()) + experiments: !!this.getHasData() } } diff --git a/extension/src/test/suite/setup/util.ts b/extension/src/test/suite/setup/util.ts index b8ff85ee0e..627eed760f 100644 --- a/extension/src/test/suite/setup/util.ts +++ b/extension/src/test/suite/setup/util.ts @@ -26,14 +26,12 @@ export const TEMP_DIR = join(dvcDemoPath, 'temp-empty-watcher-dir') export const buildSetup = ({ disposer, - cliError = undefined, gitVersion, hasData = false, noDvcRoot = true, noGitCommits = true, noGitRoot = true }: { - cliError?: undefined disposer: Disposer gitVersion?: string | null hasData?: boolean @@ -107,7 +105,6 @@ export const buildSetup = ({ internalCommands, { columnsChanged: mockEmitter, - getCliError: () => cliError, getHasData: () => hasData, isReady: () => Promise.resolve(), showWebview: mockShowWebview