Handle non-standard experiment pipeline configurations #4264

mattseddon · 2023-07-12T00:59:06Z

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:

Each dvc.yaml is counted as a potential pipeline within a project.
In order to be counted as a pipeline the stage list must either fail (return undefined) or be non-empty
When there are no pipelines show the Add Stage button in the experiments table and block running/queuing experiments.
If there is only one pipeline then use it as the cwd for experiment commands
If there are multiple pipelines then automatically select the root pipeline first
If there are multiple pipelines and none at the root then offer the user the choice each time they try to run a command.

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

extension/src/pipeline/model/collect.ts

mattseddon · 2023-07-13T23:21:42Z

extension/src/test/suite/experiments/util.ts

@@ -218,19 +256,17 @@ export const buildExperimentsData = (

 export const stubWorkspaceExperimentsGetters = (


[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.

extension/src/experiments/webview/messages.ts

mattseddon · 2023-07-13T23:42:06Z

extension/src/experiments/workspace.ts

-    default:
-      return ''
-  }
-}


[F] moved to Pipeline.

extension/src/experiments/workspace.ts

mattseddon · 2023-07-13T23:44:11Z

extension/src/experiments/workspace.ts

  }

-  private async addPipeline(cwd: string) {


[F] Moved to Pipeline as well.

mattseddon · 2023-07-13T23:45:52Z

extension/src/extension.ts

      this.pipelines.create(this.getRoots())
    ])
+    this.experiments.create(


[F] Now that an Experiments needs a Pipeline we have to move it out of the initial promise list.

mattseddon · 2023-07-13T23:46:13Z

extension/src/fileSystem/index.test.ts

@@ -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'


[F] Gave the enum the proper casing.

mattseddon · 2023-07-13T23:46:35Z

extension/src/fileSystem/index.ts

@@ -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


[F] Dropped the initial newline

mattseddon · 2023-07-13T23:50:01Z

extension/src/pipeline/data.ts

-    return this.notifyChanged(dag)
+    const [dag, fileList] = await Promise.all([
+      this.internalCommands.executeCommand(AvailableCommands.DAG, this.dvcRoot),
+      this.findDvcYamls()


[F] Unfortunately dvc stage list -R . does not fail for broken dvc.yamls (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.

so the logic is now

find all dvc.yamls for the project.

run dvc stage list against each directory

return an object of the format { [dir:string]: stdout } to the model for processing

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.

mattseddon · 2023-07-13T23:53:05Z

extension/src/pipeline/index.ts

    super()
    this.dvcRoot = dvcRoot
-    this.data = this.dispose.track(new PipelineData(dvcRoot, internalCommands))
+    this.data = this.dispose.track(
+      data || new PipelineData(dvcRoot, internalCommands)


[F] Have to pass data in for testing purposes.

mattseddon · 2023-07-13T23:53:27Z

extension/src/pipeline/index.ts

+    return this.model.hasPipeline()
+  }
+
+  public async getCwd() {


[F] Logic for deciding which pipeline to run a command against.

mattseddon · 2023-07-14T00:20:04Z

extension/src/pipeline/index.ts

+      return
+    }
+
+    const dataUpdated = new Promise(resolve => {


[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.

mattseddon · 2023-07-14T00:21:18Z

extension/src/test/fixtures/expShow/base/tableData.ts

@@ -15,7 +15,6 @@ const tableDataFixture: TableData = {
  hasConfig: true,
  hasMoreCommits: { main: true },
  hasRunningWorkspaceExperiment: true,
-  hasValidDvcYaml: true,


[F] We no long show Add Stage (disabled or not) if there is an invalid dvc.yaml.

mattseddon · 2023-07-14T00:22:27Z

extension/src/test/suite/experiments/model/tree.test.ts

@@ -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')


[F] This stub is now wrapped in buildDependencies 👍🏻

mattseddon · 2023-07-14T00:37:39Z

extension/src/experiments/index.ts

        this.deferred.resolve()
        this.dispose.untrack(waitForInitialData)
        waitForInitialData.dispose()
+        this.dispose.track(
+          this.pipeline.onDidUpdate(() =>


[F] This replaces changeHasConfig

julieg18

Great work!

julieg18 · 2023-07-16T17:51:48Z

extension/src/experiments/workspace.ts

-      return
-    }
-    const repository = this.getRepository(cwd)
+    const repository = this.getRepository(project)


Minor, should we move this logic (get project then repository) into its own function?

I'll take a look once I've merged down

julieg18 · 2023-07-16T18:47:36Z

extension/src/pipeline/data.ts

-    return this.notifyChanged(dag)
+    const [dag, fileList] = await Promise.all([
+      this.internalCommands.executeCommand(AvailableCommands.DAG, this.dvcRoot),
+      this.findDvcYamls()


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.

codeclimate · 2023-07-16T21:48:44Z

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.

add pipeline model for stages data

bf393d3

mattseddon added the 🏠 housekeeping label Jul 12, 2023

mattseddon self-assigned this Jul 12, 2023

mattseddon added 5 commits July 12, 2023 18:52

start to move functionality across to pipeline

b622744

fix integration tests

e80d0f6

move check or add pipeline into pipeline class (no-verify)

be234e0

move check or add pipeline tests into mocha

ad619be

fix test pipeline builders

9928cb9

mattseddon force-pushed the add-pipeline-model branch from 4399389 to 9928cb9 Compare July 13, 2023 08:21

mattseddon added 2 commits July 14, 2023 09:04

stub experiments for all getters

d1727ce

Merge branch 'main' into add-pipeline-model

f27450b

codeclimate bot reviewed Jul 13, 2023

View reviewed changes

extension/src/pipeline/model/collect.ts Outdated Show resolved Hide resolved

mattseddon commented Jul 13, 2023

View reviewed changes

mattseddon added 2 commits July 14, 2023 09:32

move reader stubs into build dependencies

4529c6c

add stubs for on did update

b9736cf

mattseddon commented Jul 13, 2023

View reviewed changes

extension/src/experiments/webview/messages.ts Outdated Show resolved Hide resolved

mattseddon commented Jul 13, 2023

View reviewed changes

extension/src/experiments/workspace.ts Outdated Show resolved Hide resolved

mattseddon commented Jul 13, 2023

View reviewed changes

refactor pipeline collection

151dad7

mattseddon commented Jul 14, 2023

View reviewed changes

refactor workspace experiments

58034f6

mattseddon commented Jul 14, 2023

View reviewed changes

add more unit tests for pipeline collection

2c77241

mattseddon changed the title ~~Split pipelines data (stages) out from experiments~~ Handle non-standard experiment pipelines configuration Jul 14, 2023

mattseddon added product PR that affects product and removed 🏠 housekeeping labels Jul 14, 2023

Merge branch 'main' into add-pipeline-model

207a11f

mattseddon marked this pull request as ready for review July 14, 2023 01:47

mattseddon requested review from sroy3 and julieg18 as code owners July 14, 2023 01:47

mattseddon requested a review from shcheklein July 14, 2023 01:47

This was referenced Jul 14, 2023

Refactor stubWorkspaceExperimentsGetters #4272

Merged

Focus pipeline when dvc.yaml file is open in the active editor #4273

Merged

mattseddon changed the title ~~Handle non-standard experiment pipelines configuration~~ Handle non-standard experiment pipeline configurations Jul 14, 2023

julieg18 approved these changes Jul 16, 2023

View reviewed changes

Merge branch 'main' into add-pipeline-model

e321dc5

mattseddon enabled auto-merge (squash) July 16, 2023 21:41

mattseddon merged commit ad00496 into main Jul 16, 2023
3 checks passed

mattseddon deleted the add-pipeline-model branch July 16, 2023 21:57

mattseddon mentioned this pull request Jul 26, 2023

Fix Setup Experiments being stuck on loading #4349

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle non-standard experiment pipeline configurations #4264

Handle non-standard experiment pipeline configurations #4264

mattseddon commented Jul 12, 2023 •

edited

Loading

mattseddon Jul 13, 2023

mattseddon Jul 13, 2023

mattseddon Jul 13, 2023

mattseddon Jul 13, 2023

mattseddon Jul 13, 2023

mattseddon Jul 13, 2023

mattseddon Jul 13, 2023 •

edited

Loading

mattseddon Jul 13, 2023

julieg18 Jul 16, 2023

mattseddon Jul 13, 2023

mattseddon Jul 13, 2023

mattseddon Jul 14, 2023

mattseddon Jul 14, 2023

mattseddon Jul 14, 2023

mattseddon Jul 14, 2023

julieg18 left a comment

julieg18 Jul 16, 2023

mattseddon Jul 16, 2023

julieg18 Jul 16, 2023

codeclimate bot commented Jul 16, 2023

		@@ -218,19 +256,17 @@ export const buildExperimentsData = (

		export const stubWorkspaceExperimentsGetters = (

Handle non-standard experiment pipeline configurations #4264

Handle non-standard experiment pipeline configurations #4264

Conversation

mattseddon commented Jul 12, 2023 • edited Loading

Demo

Add pipeline from Run Experiment

Invalid vs empty pipeline

Non-root dvc.yaml

Choice of pipelines

Demo project

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattseddon Jul 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julieg18 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeclimate bot commented Jul 16, 2023

mattseddon commented Jul 12, 2023 •

edited

Loading

mattseddon Jul 13, 2023 •

edited

Loading