-
Notifications
You must be signed in to change notification settings - Fork 28
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
Handle non-standard experiment pipeline configurations #4264
Conversation
4399389
to
9928cb9
Compare
@@ -218,19 +256,17 @@ export const buildExperimentsData = ( | |||
|
|||
export const stubWorkspaceExperimentsGetters = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[F] This function needs a rewrite now (should use buildExperiments
) but doing that in this PR will just add even more noise. I'll branch off this and make the change.
default: | ||
return '' | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[F] moved to Pipeline
.
} | ||
|
||
private async addPipeline(cwd: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[F] Moved to Pipeline
as well.
this.pipelines.create(this.getRoots()) | ||
]) | ||
this.experiments.create( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[F] Now that an Experiments
needs a Pipeline
we have to move it out of the initial promise list.
@@ -20,7 +20,7 @@ import { | |||
} from '.' | |||
import { dvcDemoPath } from '../test/util' | |||
import { DOT_DVC } from '../cli/dvc/constants' | |||
import { scriptCommand } from '../experiments/workspace' | |||
import { ScriptCommand } from '../pipeline' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[F] Gave the enum the proper casing.
@@ -173,8 +173,7 @@ export const findOrCreateDvcYamlFile = ( | |||
? relative(cwd, trainingScript) | |||
: format(parse(trainingScript)) | |||
|
|||
const pipeline = ` | |||
# Type dvc-help in this file and hit enter to get more information on how the extension can help to setup pipelines | |||
const pipeline = `# Type dvc-help in this file and hit enter to get more information on how the extension can help to setup pipelines |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[F] Dropped the initial newline
return this.notifyChanged(dag) | ||
const [dag, fileList] = await Promise.all([ | ||
this.internalCommands.executeCommand(AvailableCommands.DAG, this.dvcRoot), | ||
this.findDvcYamls() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[F] Unfortunately dvc stage list -R .
does not fail for broken dvc.yaml
s (that fail validation). We need that information because we've updated the logic to count invalid dvc.yaml
(s) towards there being stages/pipelines. This is because we don't want to show the Add Stage
button under the circumstances that there is a pipeline that has failed validation. There should already be enough information in the experiments table to work that out and the user has already passed the stage of needing a basic introduction to pipelines/stages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so the logic is now
- find all
dvc.yaml
s for the project. - run
dvc stage list
against each directory - return an object of the format
{ [dir:string]: stdout }
to the model for processing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately dvc stage list -R . does not fail for broken dvc.yamls (that fail validation).
We can use dvc stage list -R . --fail
to detect if at least one dvc.yaml
failing validation, but we would probably need more information per dvc.yaml
so we'd have to call on each directory anyway.
super() | ||
this.dvcRoot = dvcRoot | ||
this.data = this.dispose.track(new PipelineData(dvcRoot, internalCommands)) | ||
this.data = this.dispose.track( | ||
data || new PipelineData(dvcRoot, internalCommands) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[F] Have to pass data
in for testing purposes.
return this.model.hasPipeline() | ||
} | ||
|
||
public async getCwd() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[F] Logic for deciding which pipeline to run a command against.
return | ||
} | ||
|
||
const dataUpdated = new Promise(resolve => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[F] This promise is added so that if a user tries to run an experiment and has to add a stage the experiment will start running after the new stage has been added.
@@ -15,7 +15,6 @@ const tableDataFixture: TableData = { | |||
hasConfig: true, | |||
hasMoreCommits: { main: true }, | |||
hasRunningWorkspaceExperiment: true, | |||
hasValidDvcYaml: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[F] We no long show Add Stage
(disabled or not) if there is an invalid dvc.yaml
.
@@ -574,8 +573,6 @@ suite('Experiments Tree Test Suite', () => { | |||
}) | |||
|
|||
it('should be able to queue an experiment from an existing one with dvc.views.experiments.queueExperiment', async () => { | |||
stub(DvcReader.prototype, 'listStages').resolves('train') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[F] This stub is now wrapped in buildDependencies
👍🏻
this.deferred.resolve() | ||
this.dispose.untrack(waitForInitialData) | ||
waitForInitialData.dispose() | ||
this.dispose.track( | ||
this.pipeline.onDidUpdate(() => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[F] This replaces changeHasConfig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
return | ||
} | ||
const repository = this.getRepository(cwd) | ||
const repository = this.getRepository(project) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, should we move this logic (get project then repository) into its own function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take a look once I've merged down
return this.notifyChanged(dag) | ||
const [dag, fileList] = await Promise.all([ | ||
this.internalCommands.executeCommand(AvailableCommands.DAG, this.dvcRoot), | ||
this.findDvcYamls() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately dvc stage list -R . does not fail for broken dvc.yamls (that fail validation).
We can use dvc stage list -R . --fail
to detect if at least one dvc.yaml
failing validation, but we would probably need more information per dvc.yaml
so we'd have to call on each directory anyway.
Code Climate has analyzed commit e321dc5 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 89.5% (85% is the threshold). This pull request will bring the total coverage in the repository to 95.2% (0.1% change). View more on Code Climate. |
Closes #4222
This PR separates out checking of stages from experiments. Separating out the logic makes it easier for us to target different pipelines when running experiments.
The pipeline selection logic is as follows:
dvc.yaml
is counted as a potential pipeline within a project.Add Stage
button in the experiments table and block running/queuing experiments.I have added comments inline to help with the size of the change.
Demo
Add pipeline from Run Experiment
Screen.Recording.2023-07-14.at.11.14.55.am.mov
Invalid vs empty pipeline
Screen.Recording.2023-07-14.at.11.15.56.am.mov
Non-root dvc.yaml
Screen.Recording.2023-07-14.at.11.17.51.am.mov
Choice of pipelines
Screen.Recording.2023-07-14.at.11.19.05.am.mov
Demo project
Screen.Recording.2023-07-14.at.11.21.12.am.mov