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

Rework experiment table context menus to vary based on whether or not experiments have checkpoints #1739

Merged
merged 3 commits into from
May 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions extension/src/experiments/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,7 @@ export class Experiments extends BaseRepository<TableData> {
columnOrder: this.columns.getColumnOrder(),
columnWidths: this.columns.getColumnWidths(),
columns: this.columns.getSelected(),
hasCheckpoints: this.hasCheckpoints(),
rows: this.experiments.getRowData(),
sorts: this.experiments.getSorts()
}
Expand Down
7 changes: 4 additions & 3 deletions extension/src/experiments/webview/contract.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, number>
hasCheckpoints: boolean
rows: Row[]
sorts: SortDefinition[]
}

export type InitiallyUndefinedTableData = TableData | undefined
1 change: 1 addition & 0 deletions extension/src/test/fixtures/expShow/deeplyNested.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ const deeplyNestedTableData: TableData = {
changes: [],
columnOrder: [],
columnWidths: {},
hasCheckpoints: false,
sorts: [
{
path: 'params:params.yaml:nested1.doubled',
Expand Down
1 change: 1 addition & 0 deletions extension/src/test/fixtures/expShow/tableData.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import columnsFixture from './columns'
const tableDataFixture: TableData = {
rows: rowsFixture,
columns: columnsFixture,
hasCheckpoints: true,
sorts: [],
changes: [],
columnOrder: [],
Expand Down
1 change: 1 addition & 0 deletions extension/src/test/suite/experiments/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ suite('Experiments Test Suite', () => {
columnOrder: [],
columnWidths: {},
columns: columnsFixture,
hasCheckpoints: true,
rows: rowsFixture,
sorts: []
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ suite('Experiments Filter By Tree Test Suite', () => {
columnOrder: [],
columnWidths: {},
columns: columnsFixture,
hasCheckpoints: true,
rows: filteredRows,
sorts: []
})
Expand All @@ -133,6 +134,7 @@ suite('Experiments Filter By Tree Test Suite', () => {
columnOrder: [],
columnWidths: {},
columns: columnsFixture,
hasCheckpoints: true,
rows: [workspace, main],
sorts: []
}
Expand Down
3 changes: 2 additions & 1 deletion webview/src/experiments/components/App.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {
DND_DIRECTION_RIGHT
} from 'react-beautiful-dnd-test-utils'
import {
Column,
ColumnType,
Row,
TableData
Expand Down Expand Up @@ -221,7 +222,7 @@ describe('App', () => {
id: 'D',
name: 'D',
path: 'params:D'
}
} as Column
]
}

Expand Down
1 change: 1 addition & 0 deletions webview/src/experiments/components/Experiments.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ export const ExperimentsTable: React.FC<{
columnOrder: [],
columnWidths: {},
columns: [],
hasCheckpoints: false,
rows: [],
sorts: [],
...initiallyUndefinedTableData
Expand Down
36 changes: 28 additions & 8 deletions webview/src/experiments/components/table/Row.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ const experimentMenuOption = (
}

export const RowContextMenu: React.FC<RowProp> = ({
projectHasCheckpoints = false,
row: {
original: { queued },
depth,
Expand Down Expand Up @@ -75,17 +76,25 @@ export const RowContextMenu: React.FC<RowProp> = ({
)
])

pushIf(depth <= 1 || isWorkspace, [
const isNotCheckpoint = depth <= 1 || isWorkspace

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 Reset and Run',
'Modify, Reset and Run',
MessageFromWebviewType.VARY_EXPERIMENT_PARAMS_RESET_AND_RUN
),
)
])

pushIf(isNotCheckpoint, [
experimentMenuOption(
id,
'Modify and Queue',
Expand All @@ -94,14 +103,20 @@ export const RowContextMenu: React.FC<RowProp> = ({
])

return menuOptions
}, [queued, isWorkspace, depth, id])
}, [queued, isWorkspace, depth, id, projectHasCheckpoints])

return <MessagesMenu options={contextMenuOptions} />
}

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],
Expand All @@ -124,7 +139,12 @@ export const RowContent: React.FC<
return (
<ContextMenu
disabled={contextMenuDisabled}
content={<RowContextMenu row={row} />}
content={
<RowContextMenu
row={row}
projectHasCheckpoints={projectHasCheckpoints}
/>
}
>
<div
{...getRowProps({
Expand Down
1 change: 1 addition & 0 deletions webview/src/experiments/components/table/Table.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ describe('Table', () => {
columnOrder: [],
columnWidths: {},
columns: [],
hasCheckpoints: false,
rows: [],
sorts: []
}
Expand Down
23 changes: 19 additions & 4 deletions webview/src/experiments/components/table/Table.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,22 +9,25 @@ import { InstanceProp, RowProp, TableProps, WithChanges } from './interfaces'
export const NestedRow: React.FC<RowProp & InstanceProp> = ({
row,
instance,
contextMenuDisabled
contextMenuDisabled,
projectHasCheckpoints
}) => {
instance.prepareRow(row)
return (
<RowContent
row={row}
className={styles.nestedRow}
contextMenuDisabled={contextMenuDisabled}
projectHasCheckpoints={projectHasCheckpoints}
/>
)
}

export const ExperimentGroup: React.FC<RowProp & InstanceProp> = ({
row,
instance,
contextMenuDisabled
contextMenuDisabled,
projectHasCheckpoints
}) => {
instance.prepareRow(row)
return (
Expand All @@ -38,6 +41,7 @@ export const ExperimentGroup: React.FC<RowProp & InstanceProp> = ({
row={row}
instance={instance}
contextMenuDisabled={contextMenuDisabled}
projectHasCheckpoints={projectHasCheckpoints}
/>
{row.isExpanded &&
row.subRows.map(row => (
Expand All @@ -46,6 +50,7 @@ export const ExperimentGroup: React.FC<RowProp & InstanceProp> = ({
instance={instance}
key={row.id}
contextMenuDisabled={contextMenuDisabled}
projectHasCheckpoints={projectHasCheckpoints}
/>
))}
</div>
Expand All @@ -56,7 +61,8 @@ export const TableBody: React.FC<RowProp & InstanceProp & WithChanges> = ({
row,
instance,
changes,
contextMenuDisabled
contextMenuDisabled,
projectHasCheckpoints
}) => {
instance.prepareRow(row)
return (
Expand All @@ -73,6 +79,7 @@ export const TableBody: React.FC<RowProp & InstanceProp & WithChanges> = ({
>
<RowContent
row={row}
projectHasCheckpoints={projectHasCheckpoints}
changes={changes}
contextMenuDisabled={contextMenuDisabled}
/>
Expand All @@ -83,6 +90,7 @@ export const TableBody: React.FC<RowProp & InstanceProp & WithChanges> = ({
instance={instance}
key={subRow.values.id}
contextMenuDisabled={contextMenuDisabled}
projectHasCheckpoints={projectHasCheckpoints}
/>
))}
</div>
Expand All @@ -94,7 +102,13 @@ export const Table: React.FC<TableProps & WithChanges> = ({
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 = (
Expand All @@ -120,6 +134,7 @@ export const Table: React.FC<TableProps & WithChanges> = ({
key={row.id}
changes={changes}
contextMenuDisabled={someExperimentsRunning}
projectHasCheckpoints={hasCheckpoints}
/>
))}
</div>
Expand Down
1 change: 1 addition & 0 deletions webview/src/experiments/components/table/interfaces.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export interface WithChanges {
export interface RowProp {
row: Row<Experiment>
contextMenuDisabled?: boolean
projectHasCheckpoints?: boolean
}

export interface CellProp {
Expand Down
1 change: 1 addition & 0 deletions webview/src/stories/Table.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 => ({
Expand Down
5 changes: 3 additions & 2 deletions webview/src/test/sort.ts
Original file line number Diff line number Diff line change
@@ -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']
Expand Down Expand Up @@ -31,11 +31,12 @@ export const columns = [
}
]

export const tableData = {
export const tableData: TableData = {
changes: [],
columnOrder: [],
columnWidths: {},
columns,
hasCheckpoints: false,
rows: [
{
id: 'workspace',
Expand Down