Skip to content

Commit

Permalink
Handle project having experiments with no data (#5045)
Browse files Browse the repository at this point in the history
  • Loading branch information
mattseddon authored Dec 1, 2023
1 parent c674f9d commit e7dcdc6
Show file tree
Hide file tree
Showing 9 changed files with 57 additions and 23 deletions.
4 changes: 2 additions & 2 deletions extension/src/experiments/columns/collect/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down
5 changes: 1 addition & 4 deletions extension/src/experiments/columns/collect/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 => {
Expand Down
4 changes: 2 additions & 2 deletions extension/src/experiments/columns/model.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,15 @@ describe('ColumnsModel', () => {
const exampleDvcRoot = 'test'
const mockedColumnsOrderOrStatusChanged = buildMockedEventEmitter<void>()

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(),
mockedColumnsOrderOrStatusChanged
)
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 () => {
Expand Down
9 changes: 3 additions & 6 deletions extension/src/experiments/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -481,10 +481,7 @@ export class Experiments extends BaseRepository<TableData> {
}

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

Expand All @@ -496,7 +493,7 @@ export class Experiments extends BaseRepository<TableData> {
}

public getSelectedRevisions() {
if (!this.columns.hasNonDefaultColumns()) {
if (!this.experiments.hasData(this.columns.hasNonDefaultColumns())) {
return []
}

Expand Down Expand Up @@ -603,7 +600,7 @@ export class Experiments extends BaseRepository<TableData> {
if (this.deferred.state === 'none') {
return
}
return this.columns.hasNonDefaultColumns()
return this.experiments.hasData(this.columns.hasNonDefaultColumns())
}

public getRelativeMetricsFiles() {
Expand Down
34 changes: 34 additions & 0 deletions extension/src/experiments/model/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})
})
10 changes: 10 additions & 0 deletions extension/src/experiments/model/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
7 changes: 4 additions & 3 deletions extension/src/experiments/webview/messages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
Expand Down
4 changes: 1 addition & 3 deletions extension/src/setup/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<WorkspaceScale>

private readonly setupRun: EventEmitter<void> = this.dispose.track(
Expand Down Expand Up @@ -153,7 +152,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()))

Expand Down Expand Up @@ -254,7 +252,7 @@ export class Setup
public shouldBeShown(): { dvc: boolean; experiments: boolean } {
return {
dvc: !!this.getCliCompatible() && this.hasRoots(),
experiments: !!(this.getExpShowError() || this.getHasData())
experiments: !!this.getHasData()
}
}

Expand Down
3 changes: 0 additions & 3 deletions extension/src/test/suite/setup/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -107,7 +105,6 @@ export const buildSetup = ({
internalCommands,
{
columnsChanged: mockEmitter,
getCliError: () => cliError,
getHasData: () => hasData,
isReady: () => Promise.resolve(),
showWebview: mockShowWebview
Expand Down

0 comments on commit e7dcdc6

Please sign in to comment.