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

Watch for dvc.yaml changes for manually added stages #3365

Merged
merged 9 commits into from
Mar 1, 2023
Merged

Watch for dvc.yaml changes for manually added stages #3365

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
238eaef
Trigger a new webview message on dvc yaml change
Feb 27, 2023
15f8c9b
Correctly update the webview if there is a stage or not and if the ya…
Feb 27, 2023
c780695
Add test
Feb 28, 2023
08fb633
Merge branch 'main' into watch-dvc-stages
sroy3 Feb 28, 2023
604634e
Merge branch 'main' into watch-dvc-stages
sroy3 Feb 28, 2023
0f4d8a9
Merge branch 'main' into watch-dvc-stages
mattseddon Feb 28, 2023
cb49803
Remove useless test id
Mar 1, 2023
fff3a20
Merge branch 'watch-dvc-stages' of https://github.com/iterative/vscod…
Mar 1, 2023
78866e1
Merge branch 'main' into watch-dvc-stages
sroy3 Mar 1, 2023
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@ export class Experiments extends BaseRepository<TableData> {
if (hadCheckpoints !== this.hasCheckpoints()) {
this.checkpointsChanged.fire()
}
void this.webviewMessages.changeHasConfig(true)
})
)

Expand Down
1 change: 1 addition & 0 deletions extension/src/experiments/webview/contract.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ export type TableData = {
hasColumns: boolean
hasConfig: boolean
hasRunningExperiment: boolean
hasValidDvcYaml: boolean
rows: Row[]
sorts: SortDefinition[]
filteredCounts: FilteredCounts
Expand Down
10 changes: 7 additions & 3 deletions extension/src/experiments/webview/messages.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ export class WebviewMessages {
private readonly hasStages: () => Promise<string>

private hasConfig = false
private hasValidDvcYaml = true

private readonly addStage: () => Promise<boolean>

Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

[Q] Is there any way that this.hasValidDvcYaml isn't !this.hasStages? Is this.hasValidDvcYaml === !this.hasConfig always true? What values of stages mean that we need both variables? Can we refactor to make it more obvious? (my brain is tiny)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dvc stage list will return '' if no pipelines were found. It will return an error ('./dvc.yaml' validation failed.) if the dvc.yaml file is broken. the listStages command will return the result, but it has a catch clause that will return undefined.

In other words, hasInvalidDvcYaml = hasStages() === undefined and doesNotHaveConfig = hasStages() === ''.

We cannot test accurately for hasConfig if hasValidDvcYaml is true, but I chose to set it to false because it shows the message at the bottom of the table.

update && this.sendWebviewMessage()
}

public sendWebviewMessage() {
Expand Down Expand Up @@ -218,14 +222,14 @@ 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()
}
}

private async addConfiguration() {
await this.addStage()
await this.changeHasConfig()
}

private async setMaxTableHeadDepth() {
Expand Down
1 change: 1 addition & 0 deletions extension/src/test/fixtures/expShow/base/tableData.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ const tableDataFixture: TableData = {
hasCheckpoints: true,
hasConfig: true,
hasRunningExperiment: true,
hasValidDvcYaml: true,
hasColumns: true,
sorts: [],
changes: [],
Expand Down
1 change: 1 addition & 0 deletions extension/src/test/fixtures/expShow/dataTypes/tableData.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ export const data: TableData = {
hasCheckpoints: false,
hasConfig: true,
hasRunningExperiment: false,
hasValidDvcYaml: true,
sorts: [],
columns,
hasColumns: true,
Expand Down
1 change: 1 addition & 0 deletions extension/src/test/fixtures/expShow/deeplyNested/tableData.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ const data: TableData = {
hasCheckpoints: false,
hasConfig: true,
hasRunningExperiment: false,
hasValidDvcYaml: true,
sorts: [
{
path: 'params:params.yaml:nested1.doubled',
Expand Down
1 change: 1 addition & 0 deletions extension/src/test/fixtures/expShow/survival/tableData.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ const data: TableData = {
hasConfig: true,
hasRunningExperiment: true,
hasColumns: true,
hasValidDvcYaml: true,
sorts: [],
changes: [],
columnOrder: [],
Expand Down
4 changes: 4 additions & 0 deletions extension/src/test/suite/experiments/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ suite('Experiments Test Suite', () => {
hasColumns: true,
hasConfig: true,
hasRunningExperiment: true,
hasValidDvcYaml: true,
rows: rowsFixture,
sorts: []
}
Expand Down Expand Up @@ -208,6 +209,7 @@ suite('Experiments Test Suite', () => {
hasColumns: true,
hasConfig: false,
hasRunningExperiment: true,
hasValidDvcYaml: true,
rows: rowsFixture,
sorts: []
}
Expand Down Expand Up @@ -236,6 +238,7 @@ suite('Experiments Test Suite', () => {
hasColumns: true,
hasConfig: true,
hasRunningExperiment: true,
hasValidDvcYaml: true,
rows: rowsFixture,
sorts: []
}
Expand Down Expand Up @@ -968,6 +971,7 @@ suite('Experiments Test Suite', () => {
hasColumns: true,
hasConfig: true,
hasRunningExperiment: true,
hasValidDvcYaml: true,
rows: rowsFixture,
sorts: []
}
Expand Down
3 changes: 3 additions & 0 deletions extension/src/test/suite/experiments/model/filterBy/tree.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ suite('Experiments Filter By Tree Test Suite', () => {
hasColumns: true,
hasConfig: true,
hasRunningExperiment: true,
hasValidDvcYaml: true,
rows: filteredRows,
sorts: []
}
Expand Down Expand Up @@ -159,6 +160,7 @@ suite('Experiments Filter By Tree Test Suite', () => {
hasColumns: true,
hasConfig: true,
hasRunningExperiment: true,
hasValidDvcYaml: true,
rows: [workspace, main],
sorts: []
}
Expand Down Expand Up @@ -426,6 +428,7 @@ suite('Experiments Filter By Tree Test Suite', () => {
hasColumns: true,
hasConfig: true,
hasRunningExperiment: true,
hasValidDvcYaml: true,
rows: filteredRows,
sorts: []
}
Expand Down
16 changes: 14 additions & 2 deletions webview/src/experiments/components/AddStage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = () => (
<div className={styles.addConfigButton}>
interface AddStageProps {
hasValidDvcYaml: boolean
}

export const AddStage: React.FC<AddStageProps> = ({ hasValidDvcYaml }) => (
<div className={styles.addConfigButton} data-testid="aaa">
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we name this id to be a more clear name? Though it looks like it isn't being used anywhere 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch It was for quick testing (had a lot of trouble testing the shadow DOM button), but I forgot to remove it after.

<p>Easily and efficiently reproduce your experiments </p>
<IconButton
icon={Add}
onClick={() =>
hasValidDvcYaml &&
sendMessage({ type: MessageFromWebviewType.ADD_CONFIGURATION })
}
text="Add a Pipeline Stage"
disabled={!hasValidDvcYaml}
/>
{!hasValidDvcYaml && (
<p className={styles.error}>
Your dvc.yaml file should contain valid yaml before adding any pipeline
stages.
</p>
)}
<p>
<a href="https://dvc.org/doc/user-guide/project-structure/dvcyaml-files#stages">
Learn more
Expand Down
22 changes: 22 additions & 0 deletions webview/src/experiments/components/App.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
})
})
})
4 changes: 2 additions & 2 deletions webview/src/experiments/components/Experiments.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ export const ExperimentsTable: React.FC = () => {
columnWidths,
hasColumns,
hasConfig,
hasValidDvcYaml,
rows: data
} = useSelector((state: ExperimentsState) => state.tableData)

Expand Down Expand Up @@ -195,11 +196,10 @@ export const ExperimentsTable: React.FC = () => {
/>
)
}

return (
<RowSelectionProvider>
<Table instance={instance} onColumnOrderChange={setColumnOrder} />
{!hasConfig && <AddStage />}
{!hasConfig && <AddStage hasValidDvcYaml={hasValidDvcYaml} />}
</RowSelectionProvider>
)
}
Expand Down
1 change: 1 addition & 0 deletions webview/src/experiments/components/table/tableDataSlice.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export const tableDataInitialState: TableDataState = {
hasConfig: false,
hasData: false,
hasRunningExperiment: false,
hasValidDvcYaml: true,
rows: [],
sorts: []
}
Expand Down
5 changes: 4 additions & 1 deletion webview/src/shared/components/button/Button.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,23 @@ export type ButtonProps = {
text: string
isNested?: boolean
children?: React.ReactNode
disabled?: boolean
}

export const Button: React.FC<ButtonProps> = ({
appearance,
children,
onClick,
text,
isNested
isNested,
disabled
}: ButtonProps) => {
return (
<VSCodeButton
appearance={appearance}
onClick={onClick}
className={isNested && styles.secondaryButton}
disabled={disabled}
>
{text}
{children}
Expand Down
4 changes: 3 additions & 1 deletion webview/src/shared/components/button/IconButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,16 @@ export const IconButton: React.FC<IconButtonProps> = ({
onClick,
icon,
isNested,
text
text,
disabled
}: IconButtonProps) => {
return (
<Button
appearance={appearance}
isNested={isNested}
onClick={onClick}
text={text}
disabled={disabled}
>
<span slot="start">
<Icon icon={icon} width={18} height={18} />
Expand Down
1 change: 1 addition & 0 deletions webview/src/stories/Table.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ const tableData: TableDataState = {
hasConfig: true,
hasData: true,
hasRunningExperiment: true,
hasValidDvcYaml: true,
rows: addCommitDataToMainBranch(rowsFixture).map(row => ({
...row,
subRows: row.subRows?.map(experiment => ({
Expand Down
1 change: 1 addition & 0 deletions webview/src/test/sort.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ export const tableData: TableData = {
hasColumns: true,
hasConfig: true,
hasRunningExperiment: false,
hasValidDvcYaml: true,
rows: [
{
id: EXPERIMENT_WORKSPACE_ID,
Expand Down