From ba4482bd5c828310294276704f6e954e71eec29f Mon Sep 17 00:00:00 2001 From: Stephanie Roy Date: Wed, 1 Mar 2023 09:38:41 -0500 Subject: [PATCH 1/2] Watch for dvc.yaml changes for manually added stages (#3365) * Trigger a new webview message on dvc yaml change * Correctly update the webview if there is a stage or not and if the yaml is valid * Add test * Remove useless test id --------- Co-authored-by: Matt Seddon <37993418+mattseddon@users.noreply.github.com> --- extension/src/experiments/index.ts | 1 + extension/src/experiments/webview/contract.ts | 1 + extension/src/experiments/webview/messages.ts | 10 ++++++--- .../test/fixtures/expShow/base/tableData.ts | 1 + .../fixtures/expShow/dataTypes/tableData.ts | 1 + .../expShow/deeplyNested/tableData.ts | 1 + .../fixtures/expShow/survival/tableData.ts | 1 + .../src/test/suite/experiments/index.test.ts | 4 ++++ .../experiments/model/filterBy/tree.test.ts | 3 +++ .../src/experiments/components/AddStage.tsx | 14 +++++++++++- .../src/experiments/components/App.test.tsx | 22 +++++++++++++++++++ .../experiments/components/Experiments.tsx | 4 ++-- .../components/table/tableDataSlice.ts | 1 + .../src/shared/components/button/Button.tsx | 5 ++++- .../shared/components/button/IconButton.tsx | 4 +++- webview/src/stories/Table.stories.tsx | 1 + webview/src/test/sort.ts | 1 + 17 files changed, 67 insertions(+), 8 deletions(-) diff --git a/extension/src/experiments/index.ts b/extension/src/experiments/index.ts index 5cd7aae9e1..b04a2af4bb 100644 --- a/extension/src/experiments/index.ts +++ b/extension/src/experiments/index.ts @@ -156,6 +156,7 @@ export class Experiments extends BaseRepository { if (hadCheckpoints !== this.hasCheckpoints()) { this.checkpointsChanged.fire() } + void this.webviewMessages.changeHasConfig(true) }) ) diff --git a/extension/src/experiments/webview/contract.ts b/extension/src/experiments/webview/contract.ts index 3e29e47fe5..59e346580d 100644 --- a/extension/src/experiments/webview/contract.ts +++ b/extension/src/experiments/webview/contract.ts @@ -100,6 +100,7 @@ export type TableData = { hasColumns: boolean hasConfig: boolean hasRunningExperiment: boolean + hasValidDvcYaml: boolean rows: Row[] sorts: SortDefinition[] filteredCounts: FilteredCounts diff --git a/extension/src/experiments/webview/messages.ts b/extension/src/experiments/webview/messages.ts index e5de97c1af..11fae18b13 100644 --- a/extension/src/experiments/webview/messages.ts +++ b/extension/src/experiments/webview/messages.ts @@ -43,6 +43,7 @@ export class WebviewMessages { private readonly hasStages: () => Promise private hasConfig = false + private hasValidDvcYaml = true private readonly addStage: () => Promise @@ -74,8 +75,11 @@ export class WebviewMessages { this.addStage = addStage } - public async changeHasConfig() { - this.hasConfig = !!(await this.hasStages()) + public async changeHasConfig(update?: boolean) { + const stages = await this.hasStages() + this.hasValidDvcYaml = stages !== undefined + this.hasConfig = !!stages + update && this.sendWebviewMessage() } public sendWebviewMessage() { @@ -218,6 +222,7 @@ export class WebviewMessages { hasColumns: this.columns.hasNonDefaultColumns(), hasConfig: this.hasConfig, hasRunningExperiment: this.experiments.hasRunningExperiment(), + hasValidDvcYaml: this.hasValidDvcYaml, rows: this.experiments.getRowData(), sorts: this.experiments.getSorts() } @@ -225,7 +230,6 @@ export class WebviewMessages { private async addConfiguration() { await this.addStage() - await this.changeHasConfig() } private async setMaxTableHeadDepth() { diff --git a/extension/src/test/fixtures/expShow/base/tableData.ts b/extension/src/test/fixtures/expShow/base/tableData.ts index 5dcc761e17..74e165fcac 100644 --- a/extension/src/test/fixtures/expShow/base/tableData.ts +++ b/extension/src/test/fixtures/expShow/base/tableData.ts @@ -10,6 +10,7 @@ const tableDataFixture: TableData = { hasCheckpoints: true, hasConfig: true, hasRunningExperiment: true, + hasValidDvcYaml: true, hasColumns: true, sorts: [], changes: [], diff --git a/extension/src/test/fixtures/expShow/dataTypes/tableData.ts b/extension/src/test/fixtures/expShow/dataTypes/tableData.ts index e2b626bc9c..1970a5ff04 100644 --- a/extension/src/test/fixtures/expShow/dataTypes/tableData.ts +++ b/extension/src/test/fixtures/expShow/dataTypes/tableData.ts @@ -11,6 +11,7 @@ export const data: TableData = { hasCheckpoints: false, hasConfig: true, hasRunningExperiment: false, + hasValidDvcYaml: true, sorts: [], columns, hasColumns: true, diff --git a/extension/src/test/fixtures/expShow/deeplyNested/tableData.ts b/extension/src/test/fixtures/expShow/deeplyNested/tableData.ts index 1350c28a0f..0a9882e9c4 100644 --- a/extension/src/test/fixtures/expShow/deeplyNested/tableData.ts +++ b/extension/src/test/fixtures/expShow/deeplyNested/tableData.ts @@ -14,6 +14,7 @@ const data: TableData = { hasCheckpoints: false, hasConfig: true, hasRunningExperiment: false, + hasValidDvcYaml: true, sorts: [ { path: 'params:params.yaml:nested1.doubled', diff --git a/extension/src/test/fixtures/expShow/survival/tableData.ts b/extension/src/test/fixtures/expShow/survival/tableData.ts index dd426296a0..f7252bfd09 100644 --- a/extension/src/test/fixtures/expShow/survival/tableData.ts +++ b/extension/src/test/fixtures/expShow/survival/tableData.ts @@ -11,6 +11,7 @@ const data: TableData = { hasConfig: true, hasRunningExperiment: true, hasColumns: true, + hasValidDvcYaml: 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 ece954f630..de155e74ed 100644 --- a/extension/src/test/suite/experiments/index.test.ts +++ b/extension/src/test/suite/experiments/index.test.ts @@ -153,6 +153,7 @@ suite('Experiments Test Suite', () => { hasColumns: true, hasConfig: true, hasRunningExperiment: true, + hasValidDvcYaml: true, rows: rowsFixture, sorts: [] } @@ -208,6 +209,7 @@ suite('Experiments Test Suite', () => { hasColumns: true, hasConfig: false, hasRunningExperiment: true, + hasValidDvcYaml: true, rows: rowsFixture, sorts: [] } @@ -236,6 +238,7 @@ suite('Experiments Test Suite', () => { hasColumns: true, hasConfig: true, hasRunningExperiment: true, + hasValidDvcYaml: true, rows: rowsFixture, sorts: [] } @@ -968,6 +971,7 @@ suite('Experiments Test Suite', () => { hasColumns: true, hasConfig: true, hasRunningExperiment: true, + hasValidDvcYaml: 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 b7c77fd740..5d0329a799 100644 --- a/extension/src/test/suite/experiments/model/filterBy/tree.test.ts +++ b/extension/src/test/suite/experiments/model/filterBy/tree.test.ts @@ -127,6 +127,7 @@ suite('Experiments Filter By Tree Test Suite', () => { hasColumns: true, hasConfig: true, hasRunningExperiment: true, + hasValidDvcYaml: true, rows: filteredRows, sorts: [] } @@ -159,6 +160,7 @@ suite('Experiments Filter By Tree Test Suite', () => { hasColumns: true, hasConfig: true, hasRunningExperiment: true, + hasValidDvcYaml: true, rows: [workspace, main], sorts: [] } @@ -426,6 +428,7 @@ suite('Experiments Filter By Tree Test Suite', () => { hasColumns: true, hasConfig: true, hasRunningExperiment: true, + hasValidDvcYaml: true, rows: filteredRows, sorts: [] } diff --git a/webview/src/experiments/components/AddStage.tsx b/webview/src/experiments/components/AddStage.tsx index 498bc7702d..bdc5759304 100644 --- a/webview/src/experiments/components/AddStage.tsx +++ b/webview/src/experiments/components/AddStage.tsx @@ -5,16 +5,28 @@ import { IconButton } from '../../shared/components/button/IconButton' import { Add } from '../../shared/components/icons' import { sendMessage } from '../../shared/vscode' -export const AddStage: React.FC = () => ( +interface AddStageProps { + hasValidDvcYaml: boolean +} + +export const AddStage: React.FC = ({ hasValidDvcYaml }) => (

Easily and efficiently reproduce your experiments

+ hasValidDvcYaml && sendMessage({ type: MessageFromWebviewType.ADD_CONFIGURATION }) } text="Add a Pipeline Stage" + disabled={!hasValidDvcYaml} /> + {!hasValidDvcYaml && ( +

+ Your dvc.yaml file should contain valid yaml before adding any pipeline + stages. +

+ )}

Learn more diff --git a/webview/src/experiments/components/App.test.tsx b/webview/src/experiments/components/App.test.tsx index edd4d4a208..b1d80eef30 100644 --- a/webview/src/experiments/components/App.test.tsx +++ b/webview/src/experiments/components/App.test.tsx @@ -1495,5 +1495,27 @@ describe('App', () => { type: MessageFromWebviewType.ADD_CONFIGURATION }) }) + + it('should disable the button and add an error message if the dvc.yaml file contains invalid yaml', async () => { + renderTable() + setTableData({ + ...tableDataFixture, + hasConfig: false, + hasValidDvcYaml: false + }) + const addPipelineButton = await screen.findByText('Add a Pipeline Stage') + + fireEvent.click(addPipelineButton) + + expect(mockPostMessage).not.toHaveBeenCalledWith({ + type: MessageFromWebviewType.ADD_CONFIGURATION + }) + + expect( + screen.getByText( + 'Your dvc.yaml file should contain valid yaml before adding any pipeline stages.' + ) + ).toBeInTheDocument() + }) }) }) diff --git a/webview/src/experiments/components/Experiments.tsx b/webview/src/experiments/components/Experiments.tsx index 8a8a285b2c..37a6cc1619 100644 --- a/webview/src/experiments/components/Experiments.tsx +++ b/webview/src/experiments/components/Experiments.tsx @@ -133,6 +133,7 @@ export const ExperimentsTable: React.FC = () => { columnWidths, hasColumns, hasConfig, + hasValidDvcYaml, rows: data } = useSelector((state: ExperimentsState) => state.tableData) @@ -195,11 +196,10 @@ export const ExperimentsTable: React.FC = () => { /> ) } - return ( - {!hasConfig && } + {!hasConfig && } ) } diff --git a/webview/src/experiments/components/table/tableDataSlice.ts b/webview/src/experiments/components/table/tableDataSlice.ts index 6be7a26a4d..caa4b50df4 100644 --- a/webview/src/experiments/components/table/tableDataSlice.ts +++ b/webview/src/experiments/components/table/tableDataSlice.ts @@ -24,6 +24,7 @@ export const tableDataInitialState: TableDataState = { hasConfig: false, hasData: false, hasRunningExperiment: false, + hasValidDvcYaml: true, rows: [], sorts: [] } diff --git a/webview/src/shared/components/button/Button.tsx b/webview/src/shared/components/button/Button.tsx index ff1837994c..3d78bfb766 100644 --- a/webview/src/shared/components/button/Button.tsx +++ b/webview/src/shared/components/button/Button.tsx @@ -8,6 +8,7 @@ export type ButtonProps = { text: string isNested?: boolean children?: React.ReactNode + disabled?: boolean } export const Button: React.FC = ({ @@ -15,13 +16,15 @@ export const Button: React.FC = ({ children, onClick, text, - isNested + isNested, + disabled }: ButtonProps) => { return ( {text} {children} diff --git a/webview/src/shared/components/button/IconButton.tsx b/webview/src/shared/components/button/IconButton.tsx index 55fdc76127..d2773224ce 100644 --- a/webview/src/shared/components/button/IconButton.tsx +++ b/webview/src/shared/components/button/IconButton.tsx @@ -9,7 +9,8 @@ export const IconButton: React.FC = ({ onClick, icon, isNested, - text + text, + disabled }: IconButtonProps) => { return (