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

Handle project having experiments with no data #5045

Merged
merged 2 commits into from
Dec 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
Loading