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

Add toggle to show only changed columns in experiments table #4402

Merged
merged 9 commits into from
Aug 2, 2023
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 = (
mattseddon marked this conversation as resolved.
Show resolved Hide resolved
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()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[I] Follow up by making all of this async

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