Skip to content

Commit

Permalink
Merge branch 'main' into add-metric-param-plots
Browse files Browse the repository at this point in the history
  • Loading branch information
julieg18 authored Mar 1, 2023
2 parents fbf6e2c + a09c1e2 commit 74e70a0
Show file tree
Hide file tree
Showing 19 changed files with 122 additions and 17 deletions.
1 change: 1 addition & 0 deletions extension/src/experiments/index.ts
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
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
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
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
34 changes: 34 additions & 0 deletions extension/src/experiments/workspace.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { OutputChannel } from '../vscode/outputChannel'
import { Title } from '../vscode/title'
import { Args } from '../cli/dvc/constants'
import { findOrCreateDvcYamlFile, getFileExtension } from '../fileSystem'
import { Toast } from '../vscode/toast'

const mockedShowWebview = jest.fn()
const mockedDisposable = jest.mocked(Disposable)
Expand Down Expand Up @@ -595,5 +596,38 @@ describe('Experiments', () => {
mockedDvcRoot
)
})

it('should show a toast if the dvc.yaml file is invalid', async () => {
const showErrorSpy = jest.spyOn(Toast, 'showError')

mockedQuickPickOne.mockResolvedValueOnce(mockedDvcRoot)
mockedListStages.mockResolvedValueOnce(undefined)

await workspaceExperiments.getCwdThenRun(mockedCommandId)

expect(showErrorSpy).toHaveBeenCalledWith(
'Cannot perform task. Your dvc.yaml file is invalid.'
)
})

it('should not ask to create a stage if the dvc.yaml file is invalid', async () => {
mockedQuickPickOne.mockResolvedValueOnce(mockedDvcRoot)
mockedListStages.mockResolvedValueOnce(undefined)

await workspaceExperiments.getCwdThenRun(mockedCommandId)

expect(mockedGetValidInput).not.toHaveBeenCalled()
})

it('should not show a toast if the dvc.yaml file is valid', async () => {
const showErrorSpy = jest.spyOn(Toast, 'showError')

mockedQuickPickOne.mockResolvedValueOnce(mockedDvcRoot)
mockedListStages.mockResolvedValueOnce('train')

await workspaceExperiments.getCwdThenRun(mockedCommandId)

expect(showErrorSpy).not.toHaveBeenCalled()
})
})
})
30 changes: 21 additions & 9 deletions extension/src/experiments/workspace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -444,18 +444,30 @@ export class WorkspaceExperiments extends BaseWorkspaceWebviews<
cwd
)

if (stages === undefined) {
await Toast.showError(
'Cannot perform task. Your dvc.yaml file is invalid.'
)
return false
}

if (!stages) {
const stageName = await this.askForStageName()
if (!stageName) {
return false
}
return this.addPipeline(cwd)
}
return true
}

const { trainingScript, command } = await this.askForTrainingScript()
if (!trainingScript) {
return false
}
void findOrCreateDvcYamlFile(cwd, trainingScript, stageName, command)
private async addPipeline(cwd: string) {
const stageName = await this.askForStageName()
if (!stageName) {
return false
}

const { trainingScript, command } = await this.askForTrainingScript()
if (!trainingScript) {
return false
}
void findOrCreateDvcYamlFile(cwd, trainingScript, stageName, command)
return true
}

Expand Down
1 change: 1 addition & 0 deletions extension/src/test/fixtures/expShow/base/tableData.ts
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
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
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
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
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
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
14 changes: 13 additions & 1 deletion webview/src/experiments/components/AddStage.tsx
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 = () => (
interface AddStageProps {
hasValidDvcYaml: boolean
}

export const AddStage: React.FC<AddStageProps> = ({ hasValidDvcYaml }) => (
<div className={styles.addConfigButton}>
<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
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
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
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
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
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
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
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

0 comments on commit 74e70a0

Please sign in to comment.