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.

Sign up for GitHub

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

Make timestamp column hideable and draggable #2239

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
3f775a2
Make timestamp column hideable
julieg18 Aug 23, 2022
64f0b6a
make some suggestions (#2247)
mattseddon Aug 24, 2022
eda9ff3
Merge branch 'main' into make-timestamp-column-hideable
julieg18 Aug 24, 2022
2f69114
Add missing line in prettierignore
julieg18 Aug 24, 2022
03c2cea
Fix timestamp column dropdown bug
julieg18 Aug 24, 2022
caba7c7
Clean up
julieg18 Aug 24, 2022
49ffe3c
Merge branch 'main' into make-timestamp-column-hideable
julieg18 Aug 24, 2022
2294818
Update timestamp column.id to "Timestamp"
julieg18 Aug 25, 2022
b10574a
Update defaultColumn variable
julieg18 Aug 25, 2022
aa98a23
Clean up isDraggable
julieg18 Aug 25, 2022
0b1470e
Update timestamp column logic
julieg18 Aug 25, 2022
4c8c433
Update turbo.json with timestamp constants file
julieg18 Aug 25, 2022
d77d768
Merge branch 'main' into make-timestamp-column-hideable
julieg18 Aug 25, 2022
e90d2d5
Merge branch 'main' into make-timestamp-column-hideable
julieg18 Aug 26, 2022
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
2 changes: 1 addition & 1 deletion extension/.prettierignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@ dist
.wdio-vscode-service
coverage
src/test/fixtures/plotsDiff/vega.ts
CHANGELOG.md
CHANGELOG.md
3 changes: 3 additions & 0 deletions extension/src/experiments/columns/collect/index.test.ts
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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: ColumnType.TIMESTAMP,
Copy link
Member

Choose a reason for hiding this comment

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

[Q] Did you get a chance to look at making this optional? Having this field for the timestamp column does not really make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I attempted it, the timestamp disappeared entirely. Would need to debug further to figure out the issue 🤔

Copy link
Member

Choose a reason for hiding this comment

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

You'd need to update this method:

  public filterChildren(path?: string) {
    return this.data.filter(element =>
      path
        ? element.parentPath === path
        : Object.values<string>(ColumnType).includes(element.parentPath)
    )
  }

in extension/src/experiments/columns/model.ts.

The entry point to find that out would be getChildren in or around extension/src/experiments/columns/tree.ts.

path: 'Timestamp',
type: ColumnType.TIMESTAMP
}

export const collectTimestamp = (acc: ColumnAccumulator) => {
acc.timestamp = timestampColumn
}
2 changes: 2 additions & 0 deletions extension/src/experiments/columns/extract.ts
View file
Open in desktop
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 @@ -88,6 +89,7 @@ export const extractColumns = (
].join('\n')

const columns: Columns = {
Timestamp: experiment?.timestamp,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for using the capitalized Timestamp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using a capitalized Timestamp helps us to write less code since that's what the label is referred to as :)

Copy link
Member

Choose a reason for hiding this comment

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

I think now would be a good time to rename this to Created to match the CLI table:

image

Can do it in follow up if necessary.

deps: extractDeps(experiment.deps, branch),
metrics: metricsData?.columns,
params: paramsData?.columns
Expand Down
3 changes: 3 additions & 0 deletions extension/src/experiments/columns/model.test.ts
View file
Open in desktop
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
4 changes: 2 additions & 2 deletions extension/src/experiments/columns/model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,8 @@ export class ColumnsModel extends PathSelectionModel<Column> {
)
}

public hasColumns() {
return this.data.length > 0
public hasNonDefaultColumns() {
return this.data.length > 1
}

private async transformAndSetColumns(data: ExperimentsOutput) {
Expand Down
13 changes: 8 additions & 5 deletions extension/src/experiments/columns/quickPick.ts
View file
Open in desktop
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
)
}
19 changes: 18 additions & 1 deletion extension/src/experiments/columns/tree.test.ts
View file
Open in desktop
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 @@ -101,7 +102,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 +116,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 +203,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
13 changes: 8 additions & 5 deletions extension/src/experiments/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Event, EventEmitter, Memento } from 'vscode'
import omit from 'lodash.omit'
import { addStarredToColumns } from './columns/like'
import { setContextForEditorTitleIcons } from './context'
import { ExperimentsModel } from './model'
Expand Down Expand Up @@ -33,7 +34,7 @@ import { pickPaths } from '../path/selection/quickPick'
import { Toast } from '../vscode/toast'

export const ExperimentsScale = {
...ColumnType,
...omit(ColumnType, 'TIMESTAMP'),
HAS_CHECKPOINTS: 'hasCheckpoints',
NO_CHECKPOINTS: 'noCheckpoints'
} as const
Expand Down Expand Up @@ -182,7 +183,9 @@ export class Experiments extends BaseRepository<TableData> {
const acc = createTypedAccumulator(ExperimentsScale)

for (const { type } of this.columns.getTerminalNodes()) {
acc[type] = acc[type] + 1
if (type in acc) {
acc[<keyof typeof acc>type] = acc[<keyof typeof acc>type] + 1
}
}
const checkpointType = this.hasCheckpoints()
? ExperimentsScale.HAS_CHECKPOINTS
Expand Down Expand Up @@ -277,7 +280,7 @@ export class Experiments extends BaseRepository<TableData> {
}

public getExperimentCount() {
if (!this.columns.hasColumns()) {
if (!this.columns.hasNonDefaultColumns()) {
return 0
}

Expand Down Expand Up @@ -349,7 +352,7 @@ export class Experiments extends BaseRepository<TableData> {
}

public getExperiments() {
if (!this.columns.hasColumns()) {
if (!this.columns.hasNonDefaultColumns()) {
return []
}

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

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

Expand Down
5 changes: 4 additions & 1 deletion extension/src/experiments/model/collect.ts
View file
Open in desktop
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
18 changes: 9 additions & 9 deletions extension/src/experiments/model/status/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,17 @@ describe('canSelect', () => {
describe('limitToMaxSelected', () => {
const mockedExperiments = [
{ id: '1', label: 'A' },
{ id: '2', label: 'B', timestamp: null },
{ id: '3', label: 'C', timestamp: '2022-02-20T09:10:52' },
{ id: '4', label: 'D', timestamp: '2022-02-20T09:10:53' },
{ id: '5', label: 'E', timestamp: '2022-02-20T09:10:54' },
{ id: '6', label: 'F', timestamp: '2022-02-20T09:10:55' },
{ id: '7', label: 'G', timestamp: '2022-02-20T09:10:56' },
{ id: '8', label: 'H', timestamp: '2022-02-20T09:10:57' },
{ id: '9', label: 'I', timestamp: '2022-02-20T09:10:58' }
{ Timestamp: null, id: '2', label: 'B' },
{ Timestamp: '2022-02-20T09:10:52', id: '3', label: 'C' },
{ Timestamp: '2022-02-20T09:10:53', id: '4', label: 'D' },
{ Timestamp: '2022-02-20T09:10:54', id: '5', label: 'E' },
{ Timestamp: '2022-02-20T09:10:55', id: '6', label: 'F' },
{ Timestamp: '2022-02-20T09:10:56', id: '7', label: 'G' },
{ Timestamp: '2022-02-20T09:10:57', id: '8', label: 'H' },
{ Timestamp: '2022-02-20T09:10:58', id: '9', label: 'I' }
] as Experiment[]

it('should return the first 7 selected by timestamp', () => {
it('should return the first 7 selected by Timestamp', () => {
expect(
limitToMaxSelected(mockedExperiments)
.map(({ label }) => label)
Expand Down
2 changes: 1 addition & 1 deletion extension/src/experiments/model/status/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ const getEpoch = (timestamp: string | null | undefined) =>
new Date(timestamp || 0).getTime()

const compareTimestamps = (a: Experiment, b: Experiment) =>
getEpoch(b.timestamp) - getEpoch(a.timestamp)
getEpoch(b.Timestamp) - getEpoch(a.Timestamp)

export const limitToMaxSelected = (experiments: Experiment[]) =>
experiments
Expand Down
4 changes: 3 additions & 1 deletion extension/src/experiments/webview/contract.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ export interface Experiment extends BaseExperimentFields {
selected?: boolean
sha?: string
starred?: boolean
Timestamp?: string
}

export interface Row extends Experiment {
Expand All @@ -45,7 +46,8 @@ export interface ColumnAggregateData {
export enum ColumnType {
METRICS = 'metrics',
PARAMS = 'params',
DEPS = 'deps'
DEPS = 'deps',
TIMESTAMP = 'timestamp'
}

export interface Column extends ColumnAggregateData {
Expand Down
2 changes: 1 addition & 1 deletion extension/src/experiments/webview/messages.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ export class WebviewMessages {
),
filters: this.experiments.getFilterPaths(),
hasCheckpoints: this.checkpoints.hasCheckpoints(),
hasColumns: this.columns.hasColumns(),
hasColumns: this.columns.hasNonDefaultColumns(),
hasRunningExperiment: this.experiments.hasRunningExperiment(),
rows: this.experiments.getRowData(),
sorts: this.experiments.getSorts()
Expand Down
4 changes: 2 additions & 2 deletions extension/src/path/selection/model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,15 +76,15 @@ export abstract class PathSelectionModel<

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

if (!!selectedElement !== !!selected) {
this.toggleStatus(path)
}
}
})
}

protected setNewStatuses(data: { path: string }[]) {
Expand Down
1 change: 0 additions & 1 deletion extension/src/path/selection/quickPick.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
View file
Open in desktop
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
7 changes: 4 additions & 3 deletions extension/src/test/fixtures/expShow/dataTypes.ts
View file
Open in desktop
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 All @@ -174,7 +175,7 @@ export const rows: Row[] = [
selected: true,
sha: '53c3851f46955fa3e2b8f6e1c52999acc8c9ea77',
starred: false,
timestamp: '2020-11-21T19:58:22'
Timestamp: '2020-11-21T19:58:22'
}
]

Expand Down
Loading