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 "Parents" Columns #4691

Merged
merged 13 commits into from
Sep 27, 2023
14 changes: 13 additions & 1 deletion extension/src/experiments/columns/collect/order.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
import { Column, ColumnType } from '../../webview/contract'
import { EXPERIMENT_COLUMN_ID } from '../constants'
import {
EXPERIMENT_COLUMN_ID,
BRANCH_COLUMN_ID,
COMMIT_COLUMN_ID
} from '../constants'

export const collectColumnOrder = async (
existingColumnOrder: string[],
Expand All @@ -25,6 +29,14 @@ export const collectColumnOrder = async (
existingColumnOrder.unshift(EXPERIMENT_COLUMN_ID)
}

if (!existingColumnOrder.includes(BRANCH_COLUMN_ID)) {
existingColumnOrder.splice(1, 0, BRANCH_COLUMN_ID)
}

if (!existingColumnOrder.includes(COMMIT_COLUMN_ID)) {
existingColumnOrder.splice(2, 0, COMMIT_COLUMN_ID)
}

return [
...existingColumnOrder,
...acc.timestamp,
Expand Down
9 changes: 9 additions & 0 deletions extension/src/experiments/columns/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,12 @@ export const timestampColumn: Column = {
}

export const EXPERIMENT_COLUMN_ID = 'id'

export const COMMIT_COLUMN_ID = 'commit'
export const BRANCH_COLUMN_ID = 'branch'

export const DEFAULT_COLUMN_IDS = [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added two more default columns (ids are commt and branch). These always stay in the order on the backend and appear on the frontend if the table is sorted. Similar to the id column, they are not draggable and can't be unselected.

EXPERIMENT_COLUMN_ID,
BRANCH_COLUMN_ID,
COMMIT_COLUMN_ID
]
16 changes: 14 additions & 2 deletions extension/src/experiments/columns/model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@ import {
limitSummaryOrder
} from './util'
import { collectColumnOrder } from './collect/order'
import {
BRANCH_COLUMN_ID,
COMMIT_COLUMN_ID,
DEFAULT_COLUMN_IDS
} from './constants'
import { Column, ColumnType } from '../webview/contract'
import { ExpShowOutput } from '../../cli/dvc/contract'
import { PersistenceKey } from '../../persistence/constants'
Expand Down Expand Up @@ -104,10 +109,10 @@ export class ColumnsModel extends PathSelectionModel<Column> {

public selectFirst(firstColumns: string[]) {
const columnOrder = [
'id',
...DEFAULT_COLUMN_IDS,
...firstColumns,
...this.getColumnOrder().filter(
column => !['id', ...firstColumns].includes(column)
column => ![...DEFAULT_COLUMN_IDS, ...firstColumns].includes(column)
)
]
this.setColumnOrder(columnOrder)
Expand Down Expand Up @@ -201,6 +206,13 @@ export class ColumnsModel extends PathSelectionModel<Column> {
return this.setColumnOrderFromData(selectedColumns)
}
}

const maybeMissingDefaultColumns = [COMMIT_COLUMN_ID, BRANCH_COLUMN_ID]
for (const id of maybeMissingDefaultColumns) {
if (!this.columnOrderState.includes(id)) {
return this.setColumnOrderFromData(selectedColumns)
}
}
}

private transformAndSetChanges(data: ExpShowOutput) {
Expand Down
2 changes: 2 additions & 0 deletions extension/src/test/fixtures/expShow/base/columns.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ const nestedParamsFile = join('nested', 'params.yaml')

export const dataColumnOrder: string[] = [
'id',
'branch',
'commit',
'Created',
'metrics:summary.json:accuracy',
'metrics:summary.json:loss',
Expand Down
5 changes: 3 additions & 2 deletions extension/src/test/suite/experiments/columns/tree.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -363,8 +363,9 @@ suite('Experiments Columns Tree Test Suite', () => {

const firstColumns = []
const otherColumns = []
const defaultColumns = ['id', 'branch', 'commit']
for (const column of columnsOrder) {
if (column === 'id') {
if (defaultColumns.includes(column)) {
continue
}
if (
Expand Down Expand Up @@ -410,7 +411,7 @@ suite('Experiments Columns Tree Test Suite', () => {
])

expect(columnsModel.getColumnOrder()).to.deep.equal([
'id',
...defaultColumns,
...firstColumns,
...otherColumns
])
Expand Down
10 changes: 8 additions & 2 deletions extension/src/test/suite/experiments/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1096,9 +1096,12 @@ suite('Experiments Test Suite', () => {
})
await messageSent

const [id, firstColumn] = messageSpy.lastCall.args[0].columnOrder
const [id, branch, commit, firstColumn] =
messageSpy.lastCall.args[0].columnOrder

expect(id).to.equal('id')
expect(commit).to.equal('commit')
expect(branch).to.equal('branch')
expect(firstColumn).to.equal(movedColumn)
}).timeout(WEBVIEW_TEST_TIMEOUT)

Expand Down Expand Up @@ -1126,8 +1129,11 @@ suite('Experiments Test Suite', () => {

expect(paramsYamlColumns).to.be.greaterThan(6)

const [id, ...columns] = messageSpy.lastCall.args[0].columnOrder
const [id, branch, commit, ...columns] =
messageSpy.lastCall.args[0].columnOrder
expect(id).to.equal('id')
expect(branch).to.equal('branch')
expect(commit).to.equal('commit')

let params = 0
let other = 0
Expand Down
67 changes: 64 additions & 3 deletions webview/src/experiments/components/App.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
} from '@testing-library/react'
import '@testing-library/jest-dom'
import tableDataFixture from 'dvc/src/test/fixtures/expShow/base/tableData'
import sortedTableData from 'dvc/src/test/fixtures/expShow/sorted/tableData'
import { MessageFromWebviewType } from 'dvc/src/webview/contract'
import {
Column,
Expand All @@ -25,7 +26,7 @@ import { vsCodeApi } from '../../shared/api'
import {
commonColumnFields,
expectHeaders,
tableData as sortingTableDataFixture
tableData as simplifiedSortedTableDataFixture
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed this to differentiate from the sortedTableDataFixture which has data based on the expShow data. I did consider replacing simplifiedSortedTableData with the backend one, but that would complicate multiple tests so I decided to keep it.

} from '../../test/sort'
import {
NORMAL_TOOLTIP_DELAY,
Expand Down Expand Up @@ -61,6 +62,11 @@ const tableStateFixture = {
columnData: collectColumnData(tableDataFixture.columns)
}

const sortedTableDataFixture = {
julieg18 marked this conversation as resolved.
Show resolved Hide resolved
...sortedTableData,
columnData: collectColumnData(sortedTableData.columns)
}

jest.mock('../../shared/api')
jest.mock('../../util/styles')
jest.mock('./overflowHoverTooltip/useIsFullyContained', () => ({
Expand Down Expand Up @@ -99,6 +105,19 @@ describe('App', () => {
expect(noColumnsState).toBeInTheDocument()
})

it('should show the no columns selected empty state when there are no columns provided and the table is sorted', () => {
const { columns } = tableStateFixture
const sortPath = columns[columns.length - 1].path
renderTable({
...tableDataFixture,
columns: [],
sorts: [{ descending: true, path: sortPath }]
})

const noColumnsState = screen.queryByText('No Columns Selected.')
expect(noColumnsState).toBeInTheDocument()
})

it('should not show the no columns selected empty state when only the timestamp column is provided', () => {
renderTable({
...tableStateFixture,
Expand Down Expand Up @@ -144,9 +163,9 @@ describe('App', () => {
const { getDraggableHeaderFromText } = renderTableWithSortingData()

setTableData({
...sortingTableDataFixture,
...simplifiedSortedTableDataFixture,
columns: [
...sortingTableDataFixture.columns,
...simplifiedSortedTableDataFixture.columns,
{
...commonColumnFields,
id: 'D',
Expand All @@ -164,6 +183,48 @@ describe('App', () => {
await expectHeaders(['A', 'C', 'D', 'B'])
})

it('should add a "branch/tags" column if the table is sorted', () => {
renderTable(sortedTableDataFixture)

const branchHeader = screen.getByTestId('header-branch')
expect(branchHeader).toBeInTheDocument()

const branchHeaderTextContent =
within(branchHeader).getByText('Branch/Tags')
expect(branchHeader).toBeInTheDocument()

fireEvent.mouseEnter(branchHeaderTextContent, { bubbles: true })
expect(screen.getByRole('tooltip')).toBeInTheDocument()
expect(screen.getByRole('tooltip')).toHaveTextContent(
'The table has limited functionality while sorted. Clear all sorts to have nested rows and increase/decrease commits.'
)

expect(screen.getByTestId('branch___main').textContent).toStrictEqual(
'main'
)
})

it('should add a "parents" column if the table is sorted', () => {
julieg18 marked this conversation as resolved.
Show resolved Hide resolved
renderTable(sortedTableDataFixture)

const commitHeader = screen.getByTestId('header-commit')
expect(commitHeader).toBeInTheDocument()

const commitHeaderTextContent = within(commitHeader).getByText('Parent')
expect(commitHeader).toBeInTheDocument()

fireEvent.mouseEnter(commitHeaderTextContent, { bubbles: true })
expect(screen.getByRole('tooltip')).toBeInTheDocument()
expect(screen.getByRole('tooltip')).toHaveTextContent(
'The table has limited functionality while sorted. Clear all sorts to have nested rows and increase/decrease commits.'
)

expect(screen.getByTestId('commit___main').textContent).toStrictEqual('')
expect(screen.getByTestId('commit___exp-83425').textContent).toStrictEqual(
'53c3851'
)
})

describe('Row expansion', () => {
const experimentLabel = '4fb124a'

Expand Down
16 changes: 11 additions & 5 deletions webview/src/experiments/components/Experiments.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import { WebviewWrapper } from '../../shared/components/webviewWrapper/WebviewWr
import { EmptyState } from '../../shared/components/emptyState/EmptyState'
import { ExperimentsState } from '../store'
import { resizeColumn } from '../util/messages'
import { isDefaultColumn } from '../util/columns'

const DEFAULT_COLUMN_WIDTH = 90
const MINIMUM_COLUMN_WIDTH = 90
Expand Down Expand Up @@ -56,12 +57,15 @@ export const ExperimentsTable: React.FC = () => {
columnOrder: columnOrderData,
columnWidths,
hasConfig,
rows: data
rows: data,
sorts
} = useSelector((state: ExperimentsState) => state.tableData)

const [expanded, setExpanded] = useState({})

const [columns, setColumns] = useState(buildColumns(columnData))
const [columns, setColumns] = useState(
buildColumns(columnData, sorts.length > 0)
)
const [columnSizing, setColumnSizing] =
useState<ColumnSizingState>(columnWidths)
const [columnOrder, setColumnOrder] = useState(columnOrderData)
Expand All @@ -72,8 +76,8 @@ export const ExperimentsTable: React.FC = () => {
}, [columnSizing, columnWidths])

useEffect(() => {
setColumns(buildColumns(columnData))
}, [columnData])
setColumns(buildColumns(columnData, sorts.length > 0))
}, [columnData, sorts])

const getRowId = useCallback(
(experiment: Commit, relativeIndex: number, parent?: TableRow<Commit>) =>
Expand Down Expand Up @@ -108,7 +112,9 @@ export const ExperimentsTable: React.FC = () => {
toggleAllRowsExpanded()
}, [toggleAllRowsExpanded])

const hasOnlyDefaultColumns = columns.length <= 1
const hasOnlyDefaultColumns = columns.every(
({ id }) => id && isDefaultColumn(id)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated this to check for the column ids since there are now 1 or 3 default columns depending on if the table is sorted.

Copy link
Member

Choose a reason for hiding this comment

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

[Q] Does every return true if the array is empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it does :)

)
if (hasOnlyDefaultColumns) {
return <AddColumns />
}
Expand Down
Loading