Skip to content

Commit

Permalink
Add toggle to show only changed columns in experiments table (#4402)
Browse files Browse the repository at this point in the history
* add show only changed columns to experiments table

* persist show only changed between sessions

* add experiment webview tests

* add experiment webview tests

* add unit tests for changed column collection

* refactor collect columns with changed values

* add disable only changed button to get started screen

* address review comments
  • Loading branch information
mattseddon authored Aug 2, 2023
1 parent d54107e commit d672c12
Show file tree
Hide file tree
Showing 22 changed files with 322 additions and 24 deletions.
60 changes: 59 additions & 1 deletion extension/src/experiments/columns/collect/index.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
/* eslint-disable sort-keys-fix/sort-keys-fix */
import { collectChanges, collectColumns, collectRelativeMetricsFiles } from '.'
import {
collectChanges,
collectColumns,
collectColumnsWithChangedValues,
collectRelativeMetricsFiles
} from '.'
import { timestampColumn } from '../constants'
import { buildMetricOrParamPath } from '../paths'
import { ColumnType } from '../../webview/contract'
Expand All @@ -15,6 +20,8 @@ import {
} from '../../../cli/dvc/contract'
import { getConfigValue } from '../../../vscode/config'
import { generateTestExpShowOutput } from '../../../test/util/experiments'
import rowsFixture from '../../../test/fixtures/expShow/base/rows'
import { Operator, filterExperiment } from '../../model/filterBy'

jest.mock('../../../vscode/config')

Expand Down Expand Up @@ -525,3 +532,54 @@ describe('collectRelativeMetricsFiles', () => {
expect(collectRelativeMetricsFiles([])).toStrictEqual(existingFiles)
})
})

describe('collectColumnsWithChangedValues', () => {
it('should return the expected columns from the test fixture', () => {
const changedColumns = collectColumnsWithChangedValues(
columnsFixture,
rowsFixture,
[]
)
expect(changedColumns).toStrictEqual(
columnsFixture.filter(({ path }) =>
[
'metrics:summary.json',
'metrics:summary.json:loss',
'metrics:summary.json:accuracy',
'metrics:summary.json:val_loss',
'metrics:summary.json:val_accuracy',
'params:params.yaml',
'params:params.yaml:code_names',
'params:params.yaml:epochs',
'params:params.yaml:learning_rate',
'params:params.yaml:dropout',
'params:params.yaml:process',
'params:params.yaml:process.threshold',
'params:params.yaml:process.test_arg'
].includes(path)
)
)
})

it('should return the expected columns when applying filters (calculate changed after filters)', () => {
const uniformColumn = 'params:params.yaml:dropout'
const filters = [
{
path: uniformColumn,
operator: Operator.EQUAL,
value: 0.124
}
]
const filteredRows = [...rowsFixture]
for (const row of filteredRows) {
row.subRows = row.subRows?.filter(exp => filterExperiment(filters, exp))
}

const changedColumns = collectColumnsWithChangedValues(
columnsFixture,
filteredRows,
filters
)
expect(changedColumns.map(({ path }) => path)).not.toContain(uniformColumn)
})
})
74 changes: 72 additions & 2 deletions extension/src/experiments/columns/collect/index.ts
Original file line number Diff line number Diff line change
@@ -1,22 +1,26 @@
import { join } from 'path'
import get from 'lodash.get'
import isEqual from 'lodash.isequal'
import { ColumnAccumulator } from './util'
import { collectDepChanges, collectDeps } from './deps'
import {
collectMetricAndParamChanges,
collectMetricsAndParams
} from './metricsAndParams'
import { Column } from '../../webview/contract'
import { Column, Commit, Experiment } from '../../webview/contract'
import {
ExpRange,
ExpShowOutput,
ExpState,
ExpData,
experimentHasError
experimentHasError,
Value,
EXPERIMENT_WORKSPACE_ID
} from '../../../cli/dvc/contract'
import { standardizePath } from '../../../fileSystem/path'
import { timestampColumn } from '../constants'
import { sortCollectedArray, uniqueValues } from '../../../util/array'
import { FilterDefinition, filterExperiment } from '../../model/filterBy'

const collectFromExperiment = (
acc: ColumnAccumulator,
Expand Down Expand Up @@ -136,3 +140,69 @@ export const collectRelativeMetricsFiles = (

return uniqueValues(files)
}

const getValue = (
experiment: Commit | Experiment,
pathArray: string[]
): Value => get(experiment, pathArray) as Value

const collectChangedPath = (
acc: string[],
path: string,
pathArray: string[],
records: (Commit | Experiment)[]
) => {
let initialValue
for (const experiment of records) {
if (initialValue === undefined) {
initialValue = getValue(experiment, pathArray)
continue
}
const value = getValue(experiment, pathArray)
if (value === undefined || isEqual(value, initialValue)) {
continue
}

acc.push(path)
return
}
}

const collectChangedPaths = (
columns: Column[],
records: (Commit | Experiment)[]
) => {
const acc: string[] = []
for (const { pathArray, path, hasChildren } of columns) {
if (!pathArray || hasChildren) {
continue
}
collectChangedPath(acc, path, pathArray, records)
}
return acc
}

export const collectColumnsWithChangedValues = (
selectedColumns: Column[],
rows: Commit[],
filters: FilterDefinition[]
): Column[] => {
const records = []
for (const commit of rows) {
if (
commit.id === EXPERIMENT_WORKSPACE_ID ||
filterExperiment(filters, commit as Experiment)
) {
records.push(commit)
}
if (commit.subRows) {
records.push(...commit.subRows)
}
}

const changedPaths = collectChangedPaths(selectedColumns, records)

return selectedColumns.filter(({ path }) =>
changedPaths.find(changedPath => changedPath.startsWith(path))
)
}
11 changes: 11 additions & 0 deletions extension/src/experiments/columns/model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export class ColumnsModel extends PathSelectionModel<Column> {
private columnsChanges: string[] = []
private paramsFiles = new Set<string>()
private relativeMetricsFiles: string[] = []
private showOnlyChanged: boolean

constructor(
dvcRoot: string,
Expand All @@ -44,6 +45,7 @@ export class ColumnsModel extends PathSelectionModel<Column> {
PersistenceKey.METRICS_AND_PARAMS_COLUMN_WIDTHS,
{}
)
this.showOnlyChanged = this.revive(PersistenceKey.SHOW_ONLY_CHANGED, false)
}

public getColumnOrder(): string[] {
Expand Down Expand Up @@ -119,6 +121,15 @@ export class ColumnsModel extends PathSelectionModel<Column> {
)
}

public getShowOnlyChanged() {
return this.showOnlyChanged
}

public toggleShowOnlyChanged() {
this.showOnlyChanged = !this.showOnlyChanged
this.persist(PersistenceKey.SHOW_ONLY_CHANGED, this.showOnlyChanged)
}

public getChildren(path: string | undefined) {
return this.filterChildren(path).map(element => {
return {
Expand Down
4 changes: 0 additions & 4 deletions extension/src/experiments/model/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -246,10 +246,6 @@ export class ExperimentsModel extends ModelWithPersistence {
return [...this.filters.values()]
}

public getFilterPaths() {
return this.getFilters().map(({ path }) => path)
}

public addFilter(filter: FilterDefinition) {
this.filters.set(getFilterId(filter), filter)
this.applyAndPersistFilters()
Expand Down
1 change: 1 addition & 0 deletions extension/src/experiments/webview/contract.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ export type TableData = {
hasRunningWorkspaceExperiment: boolean
isShowingMoreCommits: Record<string, boolean>
rows: Commit[]
showOnlyChanged: boolean
selectedBranches: string[]
selectedForPlotsCount: number
sorts: SortDefinition[]
Expand Down
31 changes: 28 additions & 3 deletions extension/src/experiments/webview/messages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import { Title } from '../../vscode/title'
import { ConfigKey, setConfigValue } from '../../vscode/config'
import { NUM_OF_COMMITS_TO_INCREASE } from '../../cli/dvc/constants'
import { Pipeline } from '../../pipeline'
import { collectColumnsWithChangedValues } from '../columns/collect'

export class WebviewMessages {
private readonly dvcRoot: string
Expand Down Expand Up @@ -201,6 +202,9 @@ export class WebviewMessages {
case MessageFromWebviewType.REFRESH_EXP_DATA:
return this.refreshData()

case MessageFromWebviewType.TOGGLE_SHOW_ONLY_CHANGED:
return this.toggleShowOnlyChanged()

default:
Logger.error(`Unexpected message: ${JSON.stringify(message)}`)
}
Expand Down Expand Up @@ -258,14 +262,34 @@ export class WebviewMessages {
return this.update()
}

private toggleShowOnlyChanged() {
this.columns.toggleShowOnlyChanged()
this.sendWebviewMessage()
sendTelemetryEvent(
EventName.VIEWS_EXPERIMENTS_TABLE_REFRESH,
undefined,
undefined
)
}

private getWebviewData(): TableData {
const filters = this.experiments.getFilters()
const rows = this.experiments.getRowData()
const selectedColumns = this.columns.getSelected()

const showOnlyChanged = this.columns.getShowOnlyChanged()

const columns = showOnlyChanged
? collectColumnsWithChangedValues(selectedColumns, rows, filters)
: selectedColumns

return {
changes: this.columns.getChanges(),
cliError: this.experiments.getCliError() || null,
columnOrder: this.columns.getColumnOrder(),
columnWidths: this.columns.getColumnWidths(),
columns: this.columns.getSelected(),
filters: this.experiments.getFilterPaths(),
columns,
filters: filters.map(({ path }) => path),
hasBranchesToSelect:
this.experiments.getAvailableBranchesToShow().length > 0,
hasCheckpoints: this.experiments.hasCheckpoints(),
Expand All @@ -275,9 +299,10 @@ export class WebviewMessages {
hasRunningWorkspaceExperiment:
this.experiments.hasRunningWorkspaceExperiment(),
isShowingMoreCommits: this.experiments.getIsShowingMoreCommits(),
rows: this.experiments.getRowData(),
rows,
selectedBranches: this.experiments.getSelectedBranches(),
selectedForPlotsCount: this.experiments.getSelectedRevisions().length,
showOnlyChanged,
sorts: this.experiments.getSorts()
}
}
Expand Down
3 changes: 2 additions & 1 deletion extension/src/persistence/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ export enum PersistenceKey {
PLOT_SECTION_COLLAPSED = 'plotSectionCollapsed:',
PLOT_SELECTED_METRICS = 'plotSelectedMetrics:',
PLOTS_SMOOTH_PLOT_VALUES = 'plotSmoothPlotValues:',
PLOT_TEMPLATE_ORDER = 'plotTemplateOrder:'
PLOT_TEMPLATE_ORDER = 'plotTemplateOrder:',
SHOW_ONLY_CHANGED = 'columnsShowOnlyChanged:'
}

export enum GlobalPersistenceKey {
Expand Down
3 changes: 3 additions & 0 deletions extension/src/telemetry/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,8 @@ export const EventName = Object.assign(
'views.experimentsTable.showMoreCommits',
VIEWS_EXPERIMENTS_TABLE_SORT_COLUMN:
'views.experimentsTable.columnSortAdded',
VIEWS_EXPERIMENTS_TABLE_TOGGLE_SHOW_ONLY_CHANGED:
'views.experimentsTable.toggleShowOnlyChanged',

VIEWS_PLOTS_CLOSED: 'views.plots.closed',
VIEWS_PLOTS_COMPARISON_ROWS_REORDERED:
Expand Down Expand Up @@ -266,6 +268,7 @@ export interface IEventNamePropertyMapping {
[EventName.VIEWS_EXPERIMENTS_TABLE_OPEN_PARAMS_FILE]: {
path: string
}
[EventName.VIEWS_EXPERIMENTS_TABLE_TOGGLE_SHOW_ONLY_CHANGED]: undefined

[EventName.VIEWS_PLOTS_CLOSED]: undefined
[EventName.VIEWS_PLOTS_CREATED]: undefined
Expand Down
5 changes: 3 additions & 2 deletions extension/src/test/fixtures/expShow/base/tableData.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,8 @@ import rowsFixture from './rows'
import columnsFixture from './columns'

const tableDataFixture: TableData = {
selectedBranches: [],
cliError: null,
changes: [],
cliError: null,
columnOrder: [],
columns: columnsFixture,
columnWidths: {},
Expand All @@ -18,7 +17,9 @@ const tableDataFixture: TableData = {
hasRunningWorkspaceExperiment: true,
isShowingMoreCommits: { main: true },
rows: rowsFixture,
selectedBranches: [],
selectedForPlotsCount: 2,
showOnlyChanged: false,
sorts: []
}

Expand Down
19 changes: 19 additions & 0 deletions extension/src/test/suite/experiments/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1297,6 +1297,25 @@ suite('Experiments Test Suite', () => {
expect(mockShowPlots).to.be.calledWith(dvcDemoPath)
}).timeout(WEBVIEW_TEST_TIMEOUT)

it('should be able to handle a message to toggle only changed columns', async () => {
const { columnsModel, messageSpy, mockMessageReceived } =
await buildExperimentsWebview({
disposer: disposable
})

expect(columnsModel.getShowOnlyChanged()).to.be.false
messageSpy.resetHistory()

mockMessageReceived.fire({
type: MessageFromWebviewType.TOGGLE_SHOW_ONLY_CHANGED
})

expect(columnsModel.getShowOnlyChanged()).to.be.true
expect(messageSpy).to.be.calledWithMatch({
showOnlyChanged: true
})
}).timeout(WEBVIEW_TEST_TIMEOUT)

it('should handle a message to stop experiments running', async () => {
const { experiments, dvcExecutor } =
stubWorkspaceExperimentsGetters(disposable)
Expand Down
2 changes: 2 additions & 0 deletions extension/src/webview/contract.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ export enum MessageFromWebviewType {
SET_EXPERIMENTS_AND_OPEN_PLOTS = 'set-experiments-and-open-plots',
SET_STUDIO_SHARE_EXPERIMENTS_LIVE = 'set-studio-share-experiments-live',
TOGGLE_PLOTS_SECTION = 'toggle-plots-section',
TOGGLE_SHOW_ONLY_CHANGED = 'toggle-show-only-changed',
REMOTE_ADD = 'remote-add',
REMOTE_MODIFY = 'remote-modify',
REMOTE_REMOVE = 'remote-remove',
Expand Down Expand Up @@ -228,6 +229,7 @@ export type MessageFromWebview =
| { type: MessageFromWebviewType.REFRESH_REVISIONS }
| { type: MessageFromWebviewType.SELECT_COLUMNS }
| { type: MessageFromWebviewType.SELECT_FIRST_COLUMNS }
| { type: MessageFromWebviewType.TOGGLE_SHOW_ONLY_CHANGED }
| { type: MessageFromWebviewType.FOCUS_FILTERS_TREE }
| { type: MessageFromWebviewType.FOCUS_SORTS_TREE }
| { type: MessageFromWebviewType.OPEN_PLOTS_WEBVIEW }
Expand Down
1 change: 1 addition & 0 deletions webview/icons/codicons.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ export const codicons = [
'sort-precedence',
'star-empty',
'star-full',
'table',
'trash',
'warning'
]
Loading

0 comments on commit d672c12

Please sign in to comment.