From d60e0a7bc84020a5016d30971a1c4f37b67fa456 Mon Sep 17 00:00:00 2001 From: Matt Seddon Date: Thu, 19 May 2022 15:58:12 +1000 Subject: [PATCH 1/3] update experiment table context menus based on whether or not there are checkpoints in the project --- extension/src/experiments/index.ts | 1 + extension/src/experiments/webview/contract.ts | 7 ++-- .../src/test/fixtures/expShow/deeplyNested.ts | 1 + .../src/test/fixtures/expShow/tableData.ts | 1 + .../src/test/suite/experiments/index.test.ts | 1 + .../experiments/model/filterBy/tree.test.ts | 2 + .../src/experiments/components/App.test.tsx | 3 +- .../experiments/components/Experiments.tsx | 1 + .../src/experiments/components/table/Row.tsx | 38 ++++++++++++++++--- .../components/table/Table.test.tsx | 1 + .../experiments/components/table/Table.tsx | 23 +++++++++-- .../components/table/interfaces.ts | 1 + webview/src/stories/Table.stories.tsx | 1 + webview/src/test/sort.ts | 5 ++- 14 files changed, 70 insertions(+), 16 deletions(-) diff --git a/extension/src/experiments/index.ts b/extension/src/experiments/index.ts index ca3c7e5657..0ce673c39e 100644 --- a/extension/src/experiments/index.ts +++ b/extension/src/experiments/index.ts @@ -379,6 +379,7 @@ export class Experiments extends BaseRepository { columnOrder: this.columns.getColumnOrder(), columnWidths: this.columns.getColumnWidths(), columns: this.columns.getSelected(), + hasCheckpoints: this.hasCheckpoints(), rows: this.experiments.getRowData(), sorts: this.experiments.getSorts() } diff --git a/extension/src/experiments/webview/contract.ts b/extension/src/experiments/webview/contract.ts index 8aba385ae9..c0a48fe185 100644 --- a/extension/src/experiments/webview/contract.ts +++ b/extension/src/experiments/webview/contract.ts @@ -43,12 +43,13 @@ export interface Column extends ColumnAggregateData { } export type TableData = { - rows: Row[] - columns: Column[] - sorts: SortDefinition[] changes: string[] columnOrder: string[] + columns: Column[] columnWidths: Record + hasCheckpoints: boolean + rows: Row[] + sorts: SortDefinition[] } export type InitiallyUndefinedTableData = TableData | undefined diff --git a/extension/src/test/fixtures/expShow/deeplyNested.ts b/extension/src/test/fixtures/expShow/deeplyNested.ts index 6ccc3119d8..514af884d7 100644 --- a/extension/src/test/fixtures/expShow/deeplyNested.ts +++ b/extension/src/test/fixtures/expShow/deeplyNested.ts @@ -83,6 +83,7 @@ const deeplyNestedTableData: TableData = { changes: [], columnOrder: [], columnWidths: {}, + hasCheckpoints: false, sorts: [ { path: 'params:params.yaml:nested1.doubled', diff --git a/extension/src/test/fixtures/expShow/tableData.ts b/extension/src/test/fixtures/expShow/tableData.ts index 44d4c22a9b..be189879b6 100644 --- a/extension/src/test/fixtures/expShow/tableData.ts +++ b/extension/src/test/fixtures/expShow/tableData.ts @@ -5,6 +5,7 @@ import columnsFixture from './columns' const tableDataFixture: TableData = { rows: rowsFixture, columns: columnsFixture, + hasCheckpoints: true, sorts: [], changes: [], columnOrder: [], diff --git a/extension/src/test/suite/experiments/index.test.ts b/extension/src/test/suite/experiments/index.test.ts index 2e2acbfe84..d736756e45 100644 --- a/extension/src/test/suite/experiments/index.test.ts +++ b/extension/src/test/suite/experiments/index.test.ts @@ -112,6 +112,7 @@ suite('Experiments Test Suite', () => { columnOrder: [], columnWidths: {}, columns: columnsFixture, + hasCheckpoints: true, rows: rowsFixture, sorts: [] } diff --git a/extension/src/test/suite/experiments/model/filterBy/tree.test.ts b/extension/src/test/suite/experiments/model/filterBy/tree.test.ts index 9e67d7ce55..3483e817e0 100644 --- a/extension/src/test/suite/experiments/model/filterBy/tree.test.ts +++ b/extension/src/test/suite/experiments/model/filterBy/tree.test.ts @@ -109,6 +109,7 @@ suite('Experiments Filter By Tree Test Suite', () => { columnOrder: [], columnWidths: {}, columns: columnsFixture, + hasCheckpoints: true, rows: filteredRows, sorts: [] }) @@ -133,6 +134,7 @@ suite('Experiments Filter By Tree Test Suite', () => { columnOrder: [], columnWidths: {}, columns: columnsFixture, + hasCheckpoints: true, rows: [workspace, main], sorts: [] } diff --git a/webview/src/experiments/components/App.test.tsx b/webview/src/experiments/components/App.test.tsx index 1935789a04..3809a0f1b1 100644 --- a/webview/src/experiments/components/App.test.tsx +++ b/webview/src/experiments/components/App.test.tsx @@ -25,6 +25,7 @@ import { DND_DIRECTION_RIGHT } from 'react-beautiful-dnd-test-utils' import { + Column, ColumnType, Row, TableData @@ -221,7 +222,7 @@ describe('App', () => { id: 'D', name: 'D', path: 'params:D' - } + } as Column ] } diff --git a/webview/src/experiments/components/Experiments.tsx b/webview/src/experiments/components/Experiments.tsx index ec7a286c5f..e6d26f7d55 100644 --- a/webview/src/experiments/components/Experiments.tsx +++ b/webview/src/experiments/components/Experiments.tsx @@ -134,6 +134,7 @@ export const ExperimentsTable: React.FC<{ columnOrder: [], columnWidths: {}, columns: [], + hasCheckpoints: false, rows: [], sorts: [], ...initiallyUndefinedTableData diff --git a/webview/src/experiments/components/table/Row.tsx b/webview/src/experiments/components/table/Row.tsx index a8d69b3083..2288174bc9 100644 --- a/webview/src/experiments/components/table/Row.tsx +++ b/webview/src/experiments/components/table/Row.tsx @@ -39,7 +39,11 @@ const experimentMenuOption = ( } as MessagesMenuOptionProps } +const isNotCheckpoint = (depth: number, isWorkspace: boolean): boolean => + depth <= 1 || isWorkspace + export const RowContextMenu: React.FC = ({ + projectHasCheckpoints = false, row: { original: { queued }, depth, @@ -75,17 +79,28 @@ export const RowContextMenu: React.FC = ({ ) ]) - pushIf(depth <= 1 || isWorkspace, [ + pushIf(isNotCheckpoint(depth, isWorkspace) && !projectHasCheckpoints, [ experimentMenuOption( id, 'Modify and Run', MessageFromWebviewType.VARY_EXPERIMENT_PARAMS_AND_RUN + ) + ]) + + pushIf(isNotCheckpoint(depth, isWorkspace) && projectHasCheckpoints, [ + experimentMenuOption( + id, + 'Modify and Resume', + MessageFromWebviewType.VARY_EXPERIMENT_PARAMS_AND_RUN ), experimentMenuOption( id, - 'Modify Reset and Run', + 'Modify, Reset and Run', MessageFromWebviewType.VARY_EXPERIMENT_PARAMS_RESET_AND_RUN - ), + ) + ]) + + pushIf(isNotCheckpoint(depth, isWorkspace), [ experimentMenuOption( id, 'Modify and Queue', @@ -94,14 +109,20 @@ export const RowContextMenu: React.FC = ({ ]) return menuOptions - }, [queued, isWorkspace, depth, id]) + }, [queued, isWorkspace, depth, id, projectHasCheckpoints]) return } export const RowContent: React.FC< RowProp & { className?: string } & WithChanges -> = ({ row, className, changes, contextMenuDisabled }): JSX.Element => { +> = ({ + row, + className, + changes, + contextMenuDisabled, + projectHasCheckpoints +}): JSX.Element => { const { getRowProps, cells: [firstCell, ...cells], @@ -124,7 +145,12 @@ export const RowContent: React.FC< return ( } + content={ + + } >
{ columnOrder: [], columnWidths: {}, columns: [], + hasCheckpoints: false, rows: [], sorts: [] } diff --git a/webview/src/experiments/components/table/Table.tsx b/webview/src/experiments/components/table/Table.tsx index af4940d64c..6c7d144e5e 100644 --- a/webview/src/experiments/components/table/Table.tsx +++ b/webview/src/experiments/components/table/Table.tsx @@ -9,7 +9,8 @@ import { InstanceProp, RowProp, TableProps, WithChanges } from './interfaces' export const NestedRow: React.FC = ({ row, instance, - contextMenuDisabled + contextMenuDisabled, + projectHasCheckpoints }) => { instance.prepareRow(row) return ( @@ -17,6 +18,7 @@ export const NestedRow: React.FC = ({ row={row} className={styles.nestedRow} contextMenuDisabled={contextMenuDisabled} + projectHasCheckpoints={projectHasCheckpoints} /> ) } @@ -24,7 +26,8 @@ export const NestedRow: React.FC = ({ export const ExperimentGroup: React.FC = ({ row, instance, - contextMenuDisabled + contextMenuDisabled, + projectHasCheckpoints }) => { instance.prepareRow(row) return ( @@ -38,6 +41,7 @@ export const ExperimentGroup: React.FC = ({ row={row} instance={instance} contextMenuDisabled={contextMenuDisabled} + projectHasCheckpoints={projectHasCheckpoints} /> {row.isExpanded && row.subRows.map(row => ( @@ -46,6 +50,7 @@ export const ExperimentGroup: React.FC = ({ instance={instance} key={row.id} contextMenuDisabled={contextMenuDisabled} + projectHasCheckpoints={projectHasCheckpoints} /> ))}
@@ -56,7 +61,8 @@ export const TableBody: React.FC = ({ row, instance, changes, - contextMenuDisabled + contextMenuDisabled, + projectHasCheckpoints }) => { instance.prepareRow(row) return ( @@ -73,6 +79,7 @@ export const TableBody: React.FC = ({ > @@ -83,6 +90,7 @@ export const TableBody: React.FC = ({ instance={instance} key={subRow.values.id} contextMenuDisabled={contextMenuDisabled} + projectHasCheckpoints={projectHasCheckpoints} /> ))} @@ -94,7 +102,13 @@ export const Table: React.FC = ({ tableData }) => { const { getTableProps, rows } = instance - const { sorts, columns, changes, rows: experimentRows } = tableData + const { + sorts, + columns, + changes, + rows: experimentRows, + hasCheckpoints + } = tableData const someExperimentsRunning = React.useMemo(() => { const findRunningExperiment: (experiments?: Row[]) => boolean = ( @@ -120,6 +134,7 @@ export const Table: React.FC = ({ key={row.id} changes={changes} contextMenuDisabled={someExperimentsRunning} + projectHasCheckpoints={hasCheckpoints} /> ))} diff --git a/webview/src/experiments/components/table/interfaces.ts b/webview/src/experiments/components/table/interfaces.ts index f42a15656d..02320bae31 100644 --- a/webview/src/experiments/components/table/interfaces.ts +++ b/webview/src/experiments/components/table/interfaces.ts @@ -16,6 +16,7 @@ export interface WithChanges { export interface RowProp { row: Row contextMenuDisabled?: boolean + projectHasCheckpoints?: boolean } export interface CellProp { diff --git a/webview/src/stories/Table.stories.tsx b/webview/src/stories/Table.stories.tsx index 1baed1913d..6e85b76419 100644 --- a/webview/src/stories/Table.stories.tsx +++ b/webview/src/stories/Table.stories.tsx @@ -17,6 +17,7 @@ const tableData: TableData = { 'params:params.yaml:dvc_logs_dir': 300 }, columns: columnsFixture, + hasCheckpoints: true, rows: rowsFixture.map(row => ({ ...row, subRows: row.subRows?.map(experiment => ({ diff --git a/webview/src/test/sort.ts b/webview/src/test/sort.ts index 61e0ff8958..90aeb8570d 100644 --- a/webview/src/test/sort.ts +++ b/webview/src/test/sort.ts @@ -1,5 +1,5 @@ import { screen } from '@testing-library/react' -import { ColumnType } from 'dvc/src/experiments/webview/contract' +import { ColumnType, TableData } from 'dvc/src/experiments/webview/contract' import { DND_DRAGGABLE_DATA_ATTR } from 'react-beautiful-dnd-test-utils' export const defaultColumns = ['Experiment', 'Timestamp'] @@ -31,11 +31,12 @@ export const columns = [ } ] -export const tableData = { +export const tableData: TableData = { changes: [], columnOrder: [], columnWidths: {}, columns, + hasCheckpoints: false, rows: [ { id: 'workspace', From c04a40f176f5153ab224b82295da20d3a4a4aa04 Mon Sep 17 00:00:00 2001 From: Matt Seddon Date: Thu, 19 May 2022 16:05:51 +1000 Subject: [PATCH 2/3] evaluate is not checkpoint once --- webview/src/experiments/components/table/Row.tsx | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/webview/src/experiments/components/table/Row.tsx b/webview/src/experiments/components/table/Row.tsx index 2288174bc9..91132635a6 100644 --- a/webview/src/experiments/components/table/Row.tsx +++ b/webview/src/experiments/components/table/Row.tsx @@ -39,9 +39,6 @@ const experimentMenuOption = ( } as MessagesMenuOptionProps } -const isNotCheckpoint = (depth: number, isWorkspace: boolean): boolean => - depth <= 1 || isWorkspace - export const RowContextMenu: React.FC = ({ projectHasCheckpoints = false, row: { @@ -79,7 +76,9 @@ export const RowContextMenu: React.FC = ({ ) ]) - pushIf(isNotCheckpoint(depth, isWorkspace) && !projectHasCheckpoints, [ + const isNotCheckpoint = depth <= 1 || isWorkspace + + pushIf(isNotCheckpoint && !projectHasCheckpoints, [ experimentMenuOption( id, 'Modify and Run', @@ -87,7 +86,7 @@ export const RowContextMenu: React.FC = ({ ) ]) - pushIf(isNotCheckpoint(depth, isWorkspace) && projectHasCheckpoints, [ + pushIf(isNotCheckpoint && projectHasCheckpoints, [ experimentMenuOption( id, 'Modify and Resume', @@ -100,7 +99,7 @@ export const RowContextMenu: React.FC = ({ ) ]) - pushIf(isNotCheckpoint(depth, isWorkspace), [ + pushIf(isNotCheckpoint, [ experimentMenuOption( id, 'Modify and Queue', From d2ce25dbd48451b6c93462b05151b131fd259628 Mon Sep 17 00:00:00 2001 From: Matt Seddon Date: Thu, 19 May 2022 16:16:44 +1000 Subject: [PATCH 3/3] be smarter with how we push items into the menu --- webview/src/experiments/components/table/Row.tsx | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/webview/src/experiments/components/table/Row.tsx b/webview/src/experiments/components/table/Row.tsx index 91132635a6..d80ba36a89 100644 --- a/webview/src/experiments/components/table/Row.tsx +++ b/webview/src/experiments/components/table/Row.tsx @@ -78,20 +78,15 @@ export const RowContextMenu: React.FC = ({ const isNotCheckpoint = depth <= 1 || isWorkspace - pushIf(isNotCheckpoint && !projectHasCheckpoints, [ + pushIf(isNotCheckpoint, [ experimentMenuOption( id, - 'Modify and Run', + projectHasCheckpoints ? 'Modify and Resume' : 'Modify and Run', MessageFromWebviewType.VARY_EXPERIMENT_PARAMS_AND_RUN ) ]) pushIf(isNotCheckpoint && projectHasCheckpoints, [ - experimentMenuOption( - id, - 'Modify and Resume', - MessageFromWebviewType.VARY_EXPERIMENT_PARAMS_AND_RUN - ), experimentMenuOption( id, 'Modify, Reset and Run',