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

Make timestamp column hideable and draggable #2239

Merged
merged 14 commits into from
Aug 26, 2022
Merged
3 changes: 3 additions & 0 deletions extension/src/experiments/columns/collect/index.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { join } from 'path'
import { collectChanges, collectColumns } from '.'
import { timestampColumn } from './timestamp'
import { buildDepPath, buildMetricOrParamPath } from '../paths'
import { Column, ColumnType } from '../../webview/contract'
import outputFixture from '../../../test/fixtures/expShow/output'
Expand Down Expand Up @@ -335,6 +336,7 @@ describe('collectColumns', () => {
})

expect(columns.map(({ path }) => path)).toStrictEqual([
timestampColumn.path,
buildMetricOrParamPath(ColumnType.PARAMS, 'params.yaml'),
buildMetricOrParamPath(
ColumnType.PARAMS,
Expand Down Expand Up @@ -419,6 +421,7 @@ describe('collectColumns', () => {
it('should collect all params and metrics from the test fixture', () => {
expect(collectColumns(outputFixture).map(({ path }) => path)).toStrictEqual(
[
timestampColumn.path,
buildMetricOrParamPath(ColumnType.METRICS, 'summary.json'),
buildMetricOrParamPath(ColumnType.METRICS, 'summary.json', 'loss'),
buildMetricOrParamPath(ColumnType.METRICS, 'summary.json', 'accuracy'),
Expand Down
2 changes: 2 additions & 0 deletions extension/src/experiments/columns/collect/index.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { join } from 'path'
import { ColumnAccumulator } from './util'
import { collectDepChanges, collectDeps } from './deps'
import { collectTimestamp } from './timestamp'
import {
collectMetricAndParamChanges,
collectMetricsAndParams
Expand All @@ -19,6 +20,7 @@ const collectFromExperiment = (
) => {
const { data } = experiment
if (data) {
collectTimestamp(acc)
julieg18 marked this conversation as resolved.
Show resolved Hide resolved
collectMetricsAndParams(acc, data)
collectDeps(acc, data)
}
Expand Down
14 changes: 14 additions & 0 deletions extension/src/experiments/columns/collect/timestamp.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import { ColumnAccumulator } from './util'
import { ColumnType } from '../../webview/contract'

export const timestampColumn = {
julieg18 marked this conversation as resolved.
Show resolved Hide resolved
hasChildren: false,
label: 'Timestamp',
parentPath: 'timestamp',
path: 'timestamp:timestamp',
julieg18 marked this conversation as resolved.
Show resolved Hide resolved
type: ColumnType.TIMESTAMP
}

export const collectTimestamp = (acc: ColumnAccumulator) => {
acc.timestamp = timestampColumn
}
4 changes: 3 additions & 1 deletion extension/src/experiments/columns/extract.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ type Columns = {
deps: DepColumns | undefined
metrics: MetricOrParamColumns | undefined
params: MetricOrParamColumns | undefined
timestamp: string | null | undefined
}

export const extractColumns = (
Expand All @@ -90,7 +91,8 @@ export const extractColumns = (
const columns: Columns = {
deps: extractDeps(experiment.deps, branch),
metrics: metricsData?.columns,
params: paramsData?.columns
params: paramsData?.columns,
timestamp: experiment?.timestamp
}

if (error) {
Expand Down
3 changes: 3 additions & 0 deletions extension/src/experiments/columns/model.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { ColumnsModel } from './model'
import { appendColumnToPath, buildMetricOrParamPath } from './paths'
import { timestampColumn } from './collect/timestamp'
import { buildMockMemento } from '../../test/util'
import { Status } from '../../path/selection/model'
import { PersistenceKey } from '../../persistence/constants'
Expand Down Expand Up @@ -61,6 +62,7 @@ describe('ColumnsModel', () => {
const model = new ColumnsModel(exampleDvcRoot, buildMockMemento())
await model.transformAndSet(exampleData)
expect(model.getSelected()).toStrictEqual([
timestampColumn,
{
hasChildren: true,
label: 'params.yaml',
Expand Down Expand Up @@ -93,6 +95,7 @@ describe('ColumnsModel', () => {
)
await model.transformAndSet(exampleData)
expect(model.getSelected()).toStrictEqual([
timestampColumn,
{
hasChildren: true,
label: 'params.yaml',
Expand Down
13 changes: 8 additions & 5 deletions extension/src/experiments/columns/quickPick.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { ColumnLike } from './like'
import { timestampColumn } from './collect/timestamp'
import { definedAndNonEmpty } from '../../util/array'
import {
QuickPickOptionsWithTitle,
Expand All @@ -14,11 +15,13 @@ export const pickFromColumnLikes = (
return Toast.showError('There are no columns to select from.')
}
return quickPickValue<ColumnLike>(
columnLikes.map(columnLike => ({
description: columnLike.path,
label: columnLike.label,
value: columnLike
})),
columnLikes
.filter(columnLike => columnLike.path !== timestampColumn.path)
Copy link
Member

Choose a reason for hiding this comment

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

[C] I do not think we want to filter this here. We will actually want to make it possible to sort/filter by timestamp in the future.

Copy link
Member

Choose a reason for hiding this comment

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

This is definitely causing at least one test failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

[C] I do not think we want to filter this here. We will actually want to make it possible to sort/filter by timestamp in the future.

I did test out sorting with timestamp but it wasn't working so I disabled both for now :)

.map(columnLike => ({
description: columnLike.path,
label: columnLike.label,
value: columnLike
})),
quickPickOptions
)
}
24 changes: 22 additions & 2 deletions extension/src/experiments/columns/tree.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
buildMetricOrParamPath,
splitColumnPath
} from './paths'
import { timestampColumn } from './collect/timestamp'
import columnsFixture from '../../test/fixtures/expShow/columns'
import { Resource, ResourceLocator } from '../../resourceLocator'
import { RegisteredCommands } from '../../commands/external'
Expand Down Expand Up @@ -78,7 +79,10 @@ describe('ExperimentsColumnsTree', () => {
.map(column => ({
...column,
descendantStatuses: [],
label: getLabel(column.path),
label:
column.type === ColumnType.TIMESTAMP
? timestampColumn.label
: getLabel(column.path),
status: Status.SELECTED
}))

Expand All @@ -101,7 +105,7 @@ describe('ExperimentsColumnsTree', () => {
)
})

it('should return the params and metrics if there is only a single repository and no path is provided', async () => {
it('should return the params, metrics, and timestamp if there is only a single repository and no path is provided', async () => {
const experimentsColumnsTree = new ExperimentsColumnsTree(
mockedExperiments,
mockedInternalCommands,
Expand All @@ -115,6 +119,14 @@ describe('ExperimentsColumnsTree', () => {
const children = await experimentsColumnsTree.getChildren()

expect(children).toStrictEqual([
{
collapsibleState: 0,
description: undefined,
dvcRoot: mockedDvcRoot,
iconPath: mockedSelectedCheckbox,
label: timestampColumn.label,
path: timestampColumn.path
},
{
collapsibleState: 1,
description: undefined,
Expand Down Expand Up @@ -194,6 +206,14 @@ describe('ExperimentsColumnsTree', () => {
const processPath = appendColumnToPath(paramsPath, 'process')

expect(children).toStrictEqual([
{
collapsibleState: 0,
description: undefined,
dvcRoot: mockedDvcRoot,
iconPath: mockedSelectedCheckbox,
label: timestampColumn.label,
path: timestampColumn.path
},
{
collapsibleState: 1,
description: undefined,
Expand Down
7 changes: 6 additions & 1 deletion extension/src/experiments/columns/tree.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { timestampColumn } from './collect/timestamp'
import {
BasePathSelectionTree,
PathSelectionItem
Expand Down Expand Up @@ -26,7 +27,11 @@ export class ExperimentsColumnsTree extends BasePathSelectionTree<WorkspaceExper
internalCommands.registerExternalCommand<PathSelectionItem>(
RegisteredCommands.EXPERIMENT_METRICS_AND_PARAMS_TOGGLE,
({ dvcRoot, path }) =>
this.workspace.getRepository(dvcRoot).toggleColumnStatus(path)
this.workspace
.getRepository(dvcRoot)
.toggleColumnStatus(
path === timestampColumn.path ? timestampColumn.parentPath : path
)
)
}

Expand Down
5 changes: 4 additions & 1 deletion extension/src/experiments/model/collect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -125,11 +125,14 @@ const transformColumns = (
experimentFields: ExperimentFields,
branch?: Experiment
) => {
const { error, metrics, params, deps } = extractColumns(
const { error, metrics, params, deps, timestamp } = extractColumns(
experimentFields,
branch
)

if (timestamp) {
experiment.timestamp = timestamp
}
if (metrics) {
experiment.metrics = metrics
}
Expand Down
3 changes: 2 additions & 1 deletion extension/src/experiments/webview/contract.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@ export interface ColumnAggregateData {
export enum ColumnType {
METRICS = 'metrics',
PARAMS = 'params',
DEPS = 'deps'
DEPS = 'deps',
TIMESTAMP = 'timestamp'
}

export interface Column extends ColumnAggregateData {
Expand Down
11 changes: 7 additions & 4 deletions extension/src/path/selection/model.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import { Memento } from 'vscode'
import { PersistenceKey } from '../../persistence/constants'
import { ModelWithPersistence } from '../../persistence/model'
import { Column } from '../../experiments/webview/contract'
import { Column, ColumnType } from '../../experiments/webview/contract'
import { PlotPath } from '../../plots/paths/collect'
import { timestampColumn } from '../../experiments/columns/collect/timestamp'

export enum Status {
SELECTED = 2,
Expand Down Expand Up @@ -76,15 +77,17 @@ export abstract class PathSelectionModel<

public setSelected(elements: (T & { selected: boolean })[]) {
const terminalNodes = this.getTerminalNodes()
for (const { path, selected } of terminalNodes) {
terminalNodes.map(({ path, selected, type }) => {
const selectedElement = elements.find(
({ path: selectedPath }) => path === selectedPath
)

if (!!selectedElement !== !!selected) {
this.toggleStatus(path)
const statusPath =
type === ColumnType.TIMESTAMP ? timestampColumn.parentPath : path
this.toggleStatus(statusPath)
}
}
})
}

protected setNewStatuses(data: { path: string }[]) {
Expand Down
5 changes: 2 additions & 3 deletions extension/src/path/selection/quickPick.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import {
quickPickManyValues
} from '../../vscode/quickPick'
import { Title } from '../../vscode/title'
import { Column } from '../../experiments/webview/contract'
import { Column, ColumnType } from '../../experiments/webview/contract'
import { PlotPath } from '../../plots/paths/collect'

type PathType = PlotPath | Column
Expand All @@ -15,7 +15,7 @@ type PathWithSelected<T extends PathType> = T & {
}

const getItem = <T extends PathType>(element: PathWithSelected<T>) => ({
label: element.path,
label: element.type === ColumnType.TIMESTAMP ? element.label : element.path,
picked: element.selected,
value: element
})
Expand All @@ -41,7 +41,6 @@ export const pickPaths = <T extends PathType>(
if (!definedAndNonEmpty(paths)) {
return Toast.showError(`There are no ${type} to select.`)
}

const items = collectItems(paths)

return quickPickManyValues<PathWithSelected<T>>(items, {
Expand Down
2 changes: 2 additions & 0 deletions extension/src/test/fixtures/expShow/columns.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,12 @@ import {
buildDepPath,
buildMetricOrParamPath
} from '../../../experiments/columns/paths'
import { timestampColumn } from '../../../experiments/columns/collect/timestamp'

const nestedParamsFile = join('nested', 'params.yaml')

const data: Column[] = [
timestampColumn,
{
type: ColumnType.METRICS,
hasChildren: true,
Expand Down
5 changes: 3 additions & 2 deletions extension/src/test/fixtures/expShow/dataTypes.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { ExperimentsOutput } from '../../../cli/dvc/reader'
import { timestampColumn } from '../../../experiments/columns/collect/timestamp'
import {
Column,
ColumnType,
Expand Down Expand Up @@ -45,6 +46,7 @@ export const dataTypesOutput: ExperimentsOutput = {
}

export const columns: Column[] = [
timestampColumn,
{
hasChildren: true,
label: 'params.yaml',
Expand Down Expand Up @@ -160,8 +162,7 @@ export const rows: Row[] = [
queued: false,
running: false,
selected: true,
starred: false,
timestamp: null
starred: false
},
{
displayColor: '#13adc7',
Expand Down
3 changes: 2 additions & 1 deletion extension/src/test/fixtures/expShow/deeplyNested.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { ExperimentsOutput } from '../../../cli/dvc/reader'
import { timestampColumn } from '../../../experiments/columns/collect/timestamp'
import {
Column,
ColumnType,
Expand Down Expand Up @@ -72,6 +73,7 @@ export const deeplyNestedOutput: ExperimentsOutput = {
}

export const columns: Column[] = [
timestampColumn,
{
hasChildren: true,
label: 'params.yaml',
Expand Down Expand Up @@ -218,7 +220,6 @@ export const rows = [
{
id: 'workspace',
label: 'workspace',
timestamp: null,
queued: false,
running: false,
executor: null,
Expand Down
3 changes: 1 addition & 2 deletions extension/src/test/fixtures/expShow/rows.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,7 @@ const data: Row[] = [
queued: false,
running: true,
selected: true,
starred: false,
timestamp: null
starred: false
},
{
deps: {
Expand Down
Loading