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

Jump to bottom

Remove check for stages in plot wizard #4904

Merged
merged 5 commits into from
Oct 27, 2023
Merged

Remove check for stages in plot wizard #4904

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
215bf04
Remove check for stages in plot wizard
julieg18 Oct 25, 2023
ae70d2d
Add vscode test
julieg18 Oct 25, 2023
32b2de9
Improve test name
julieg18 Oct 26, 2023
710519b
Minor test improvements
julieg18 Oct 26, 2023
ecbfd98
Merge branch 'main' into remove-stage-check-in-plot-wizard
julieg18 Oct 27, 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
29 changes: 29 additions & 0 deletions extension/src/fileSystem/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -640,6 +640,29 @@ describe('addPlotToDvcYamlFile', () => {
' y:',
' data.json: accuracy'
]

it('should add a plots list with the new plot if the dvc.yaml file does not exist', () => {
const mockPlotYamlContent = ['', 'plots:', ...mockNewPlotLines, ''].join(
'\n'
)
mockedReadFileSync.mockReturnValueOnce('')
mockedReadFileSync.mockReturnValueOnce('')

addPlotToDvcYamlFile('/', {
template: 'simple',
title: 'Simple Plot',
x: { 'data.json': ['epochs'] },
y: { 'data.json': ['accuracy'] }
})

expect(mockedEnsureFileSync).toHaveBeenCalledWith('//dvc.yaml')
expect(mockedOpenTextDocument).toHaveBeenCalledTimes(1)
expect(mockedWriteFileSync).toHaveBeenCalledWith(
'//dvc.yaml',
mockPlotYamlContent
)
})

it('should add a plots list with the new plot if the dvc.yaml file has no plots', () => {
const mockDvcYamlContent = mockStagesLines.join('\n')
const mockPlotYamlContent = ['', 'plots:', ...mockNewPlotLines, ''].join(
Expand All @@ -655,6 +678,7 @@ describe('addPlotToDvcYamlFile', () => {
y: { 'data.json': ['accuracy'] }
})

expect(mockedOpenTextDocument).toHaveBeenCalledTimes(1)
expect(mockedWriteFileSync).toHaveBeenCalledWith(
'//dvc.yaml',
mockDvcYamlContent + mockPlotYamlContent
Expand Down Expand Up @@ -684,6 +708,7 @@ describe('addPlotToDvcYamlFile', () => {
y: { 'acc.json': ['accuracy'] }
})

expect(mockedOpenTextDocument).toHaveBeenCalledTimes(1)
expect(mockedWriteFileSync).toHaveBeenCalledWith(
'//dvc.yaml',
mockDvcYamlContent + mockPlotYamlContent
Expand Down Expand Up @@ -714,6 +739,7 @@ describe('addPlotToDvcYamlFile', () => {
y: { 'data.json': ['accuracy', 'epochs'] }
})

expect(mockedOpenTextDocument).toHaveBeenCalledTimes(1)
expect(mockedWriteFileSync).toHaveBeenCalledWith(
'//dvc.yaml',
mockDvcYamlContent + mockPlotYamlContent
Expand All @@ -735,6 +761,7 @@ describe('addPlotToDvcYamlFile', () => {

mockDvcYamlContent.splice(7, 0, ...mockPlotYamlContent)

expect(mockedOpenTextDocument).toHaveBeenCalledTimes(1)
expect(mockedWriteFileSync).toHaveBeenCalledWith(
'//dvc.yaml',
mockDvcYamlContent.join('\n')
Expand All @@ -756,6 +783,7 @@ describe('addPlotToDvcYamlFile', () => {
y: { 'data.json': ['accuracy'] }
})

expect(mockedOpenTextDocument).toHaveBeenCalledTimes(1)
expect(mockedWriteFileSync).toHaveBeenCalledWith(
'//dvc.yaml',
mockDvcYamlContent + mockPlotYamlContent
Expand Down Expand Up @@ -794,6 +822,7 @@ describe('addPlotToDvcYamlFile', () => {
y: { 'data.json': ['accuracy'] }
})

expect(mockedOpenTextDocument).toHaveBeenCalledTimes(1)
expect(mockedWriteFileSync).toHaveBeenCalledWith(
'//dvc.yaml',
mockDvcYamlContent + mockPlotYamlContent
Expand Down
6 changes: 4 additions & 2 deletions extension/src/fileSystem/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,7 @@ const getPlotsYaml = (plotObj: PlotConfigData, indentSearchLines: string[]) => {

export const addPlotToDvcYamlFile = (cwd: string, plotObj: PlotConfigData) => {
const dvcYamlFile = `${cwd}/dvc.yaml`
ensureFileSync(dvcYamlFile)
const dvcYamlDoc = loadYamlAsDoc(dvcYamlFile)

if (!dvcYamlDoc) {
Expand All @@ -276,8 +277,9 @@ export const addPlotToDvcYamlFile = (cwd: string, plotObj: PlotConfigData) => {
if (!plots?.range) {
const plotYaml = getPlotsYaml(plotObj, dvcYamlLines)
dvcYamlLines.push(...plotYaml)
writeFileSync(dvcYamlFile, dvcYamlLines.join('\n'))
return
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This "no plots found" route is actually missing the openFileInEditor line. I've added it and updated the tests to check for this function being called.


void openFileInEditor(dvcYamlFile)
return writeFileSync(dvcYamlFile, dvcYamlLines.join('\n'))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is an empty line on top of the dvc.yaml in the case of there being an empty file:


plots:
  - step vs acc:
      template: confusion
      x: step
      y:
        training/plots/metrics/train/acc.json: acc

If we wanted to get rid of the top line, we could need to add an extra if check. I decided to just keep it to avoid extra complexity.

}

const plotsEndPos = lineCounter.linePos(plots.range[2]).line
Expand Down
51 changes: 25 additions & 26 deletions extension/src/pipeline/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,28 +83,8 @@ export class Pipeline extends DeferredDisposable {
}

public async getCwd() {
const focusedPipeline = this.getFocusedPipeline()
if (focusedPipeline) {
return focusedPipeline
}

await this.checkOrAddPipeline()

const pipelines = this.model.getPipelines()
if (!pipelines?.size) {
return
}
if (pipelines.has(this.dvcRoot)) {
return this.dvcRoot
}
if (pipelines.size === 1) {
return [...pipelines][0]
}

return quickPickOne(
[...pipelines],
'Select a Pipeline to Run Command Against'
)
return this.findPipelineCwd()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Going to mention it here since it was discussed asynchronously with Matt, but it would simplify things to just have the Plot Wizard automatically create a dvc.yaml in the root vs looking for one elsewhere.

While this means that it wouldn't work for people with workspaces or nested dvc.yamls, users who do those are most likely advanced DVC users who don't have a need for the plot wizard.

I chose to keep it like it is for now but I'm happy to update if preferred.

}

public async checkOrAddPipeline() {
Expand Down Expand Up @@ -158,11 +138,7 @@ export class Pipeline extends DeferredDisposable {
}

public async addDataSeriesPlot() {
const cwd = await this.getCwd()

if (!cwd) {
return
}
const cwd = (await this.findPipelineCwd()) || this.dvcRoot
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We now default to this.dvcRoot if a cwd isn't found.


const plotConfiguration = await pickPlotConfiguration(cwd)

Expand All @@ -177,6 +153,29 @@ export class Pipeline extends DeferredDisposable {
return appendFileSync(join(this.dvcRoot, TEMP_DAG_FILE), '\n')
}

private findPipelineCwd() {
const focusedPipeline = this.getFocusedPipeline()
if (focusedPipeline) {
return focusedPipeline
}

const pipelines = this.model.getPipelines()
if (!pipelines?.size) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of scope, but since we have a lint rule to test zero length explicitly (something.length === 0 instead of !something.length). Should we have a similar rule for size as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense to me. I can look into a eslint rule for it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like the lint rule we have (explicit-link-check) works for both arrays and size. This is allowed though since we have an undefined check there as well.

return
}
if (pipelines.has(this.dvcRoot)) {
return this.dvcRoot
}
if (pipelines.size === 1) {
return [...pipelines][0]
}

return quickPickOne(
[...pipelines],
'Select a Pipeline to Run Command Against'
)
}

private async initialize() {
this.dispose.track(
this.data.onDidUpdate(({ dag, stages }) => {
Expand Down
46 changes: 40 additions & 6 deletions extension/src/test/suite/pipeline/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,10 +323,9 @@ suite('Pipeline Test Suite', () => {
})
})

it('should add a data series plot', async () => {
it('should add a data series plot when a dvc.yaml file exists', async () => {
const { pipeline } = buildPipeline({
disposer: disposable,
dvcRoot: dvcDemoPath
disposer: disposable
})
const mockPickPlotConfiguration = stub(
PipelineQuickPick,
Expand All @@ -340,7 +339,7 @@ suite('Pipeline Test Suite', () => {

await pipeline.addDataSeriesPlot()

expect(mockPickPlotConfiguration).to.be.calledOnce
expect(mockPickPlotConfiguration).to.be.calledOnceWithExactly(dvcDemoPath)
expect(mockAddPlotToDvcFile).not.to.be.called

const mockPlotConfig: PipelineQuickPick.PlotConfigData = {
Expand All @@ -354,8 +353,43 @@ suite('Pipeline Test Suite', () => {

await pipeline.addDataSeriesPlot()

expect(mockPickPlotConfiguration).to.be.calledTwice
expect(mockAddPlotToDvcFile).to.be.called
expect(mockPickPlotConfiguration).to.be.calledWithExactly(dvcDemoPath)
expect(mockAddPlotToDvcFile).to.be.calledOnceWithExactly(
dvcDemoPath,
mockPlotConfig
)
})

it('should add a data series plot without trying to add a missing dvc.yaml file or stage', async () => {
const { pipeline } = buildPipeline({
disposer: disposable,
dvcYamls: []
})
const mockPickPlotConfiguration = stub(
PipelineQuickPick,
'pickPlotConfiguration'
)
const mockAddPlotToDvcFile = stub(FileSystem, 'addPlotToDvcYamlFile')
const mockCheckOrAddPipeline = stub(pipeline, 'checkOrAddPipeline')
const mockPlotConfig: PipelineQuickPick.PlotConfigData = {
template: 'simple',
title: 'Great Plot Name',
x: { 'results.json': ['step'] },
y: { 'results.json': ['acc'] }
}

mockPickPlotConfiguration.onFirstCall().resolves(mockPlotConfig)

await pipeline.isReady()
await pipeline.addDataSeriesPlot()

expect(mockCheckOrAddPipeline, 'should not check for a pipeline stage').not
.to.be.called
expect(mockPickPlotConfiguration).to.be.calledOnceWithExactly(dvcDemoPath)
expect(mockAddPlotToDvcFile).to.be.calledOnceWithExactly(
dvcDemoPath,
mockPlotConfig
)
})

it('should set the appropriate context value when a dvc.yaml is open in the active editor', async () => {
Expand Down
Loading