Page MenuHomePhabricator

RFC: Add mutation for executing pipeline with preset
ClosedPublic

Authored by sashank on Sat, Aug 31, 8:10 AM.

Details

Reviewers
schrockn
Group Reviewers
Restricted Project
Commits
R1:903a3ab8f8cd: RFC: Add mutation for executing pipeline with preset
Summary

Add mutation and client method for starting a pipeline execution with a preset from dagster-graphql. If this is an appropriate solution, can clean up implementation and add tests.

API:

variables = {
      'executionParams': {
          'selector': {'name': "myPipelineName},
          'preset': 'passing',
     }
}
result = execute_start_pipeline_execution_mutation(handle, variables)
Test Plan

unit

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

sashank created this revision.Sat, Aug 31, 8:10 AM
sashank edited the summary of this revision. (Show Details)Sat, Aug 31, 8:13 AM
sashank edited the summary of this revision. (Show Details)
sashank updated this revision to Diff 4163.Sat, Aug 31, 8:30 AM

Extract duplicated query body

alangenfeld added inline comments.
python_modules/dagster-graphql/dagster_graphql/client/query.py
204–205 ↗(On Diff #4163)

we should use graphql fragments here - there should be examples in similar files

python_modules/dagster-graphql/dagster_graphql/implementation/execution.py
48 ↗(On Diff #4163)

what are your thoughts around just supporting preset in ExecutionParams instead and staying with a single mutation?

sashank added inline comments.Tue, Sep 3, 4:53 PM
python_modules/dagster-graphql/dagster_graphql/implementation/execution.py
48 ↗(On Diff #4163)

I had originally decided against this because I thought it would be a confusing API with the ability to set both environmentConfigData, mode, and solidSubset alongside preset.

It wouldn't be obvious which one takes precedence, and whether you could override certain preset environment_dict keys with environmentConfigData and how the two would be merged.

We could just error if environmentConfigData, mode, or solidSubset mode is set when preset is set. Do you think that's better?

curious to get others thoughts here - not obvious which path is best

python_modules/dagster-graphql/dagster_graphql/implementation/execution.py
48 ↗(On Diff #4163)

you are right its a bit confusing - though i think definitely worth discussing since a separate mutation has its own complexity cost. For example you don't support re-execution with presets here.

We could just error if environmentConfigData, mode, or solidSubset mode is set when preset is set. Do you think that's better?

I think this is the right behavior if we do go down the single mutation path - we could toy with the input type structure if that helps.

Looking at it right now we already have a kind of goofy behavior with step_keys and selector.solid_subset.

python_modules/dagster/dagster/core/definitions/pipeline.py
330–342

rebase past D911

sashank updated this revision to Diff 4224.Wed, Sep 4, 4:56 PM

Use fragments

sashank marked 2 inline comments as done and an inline comment as not done.Wed, Sep 4, 5:14 PM
sashank updated this revision to Diff 4229.Wed, Sep 4, 5:31 PM

Fix tests

alangenfeld added a subscriber: max.Fri, Sep 6, 3:26 PM

in light of @max adding another execute mutation for blocking which would require us to double this - i think pursuing the argument instead of new mutation approach is worth it

alangenfeld requested changes to this revision.Fri, Sep 6, 3:34 PM

also CI still failing - to your queue

This revision now requires changes to proceed.Fri, Sep 6, 3:34 PM
sashank planned changes to this revision.Mon, Sep 9, 6:16 PM
sashank updated this revision to Diff 4503.Mon, Sep 9, 11:28 PM

Merge into one mutation

sashank updated this revision to Diff 4504.Mon, Sep 9, 11:29 PM

Clean up tests

Harbormaster failed remote builds in B3608: Diff 4504!
sashank updated this revision to Diff 4506.Mon, Sep 9, 11:44 PM

Update snapshot

alangenfeld requested changes to this revision.Tue, Sep 10, 3:09 PM

test plan & bk

python_modules/dagster-graphql/dagster_graphql/schema/roots.py
299–307

would it not make more sense to resolve ExecutionParams using the preset here instead of passing it through?

This revision now requires changes to proceed.Tue, Sep 10, 3:09 PM
sashank edited the test plan for this revision. (Show Details)Tue, Sep 10, 4:54 PM
sashank updated this revision to Diff 4523.Tue, Sep 10, 5:27 PM
  • Move preset logic to create_execution_params
  • create PresetNotFoundError
sashank updated this revision to Diff 4524.Tue, Sep 10, 5:29 PM

Remove comment and typo

sashank updated this revision to Diff 4526.Tue, Sep 10, 5:33 PM

Update tests

sashank updated this revision to Diff 4528.Tue, Sep 10, 5:45 PM

make graphql

sashank updated this revision to Diff 4532.Tue, Sep 10, 6:01 PM

Rerun tests

sashank marked an inline comment as done.Tue, Sep 10, 8:05 PM
sashank added inline comments.
python_modules/dagster-graphql/dagster_graphql/schema/roots.py
299–307

good call - updated

schrockn accepted this revision.Wed, Sep 11, 2:07 PM
schrockn added a subscriber: schrockn.

I'm into this.

This diff (and the discussion around it) is a good data point for whether or not to add input unions to GraphQL, actually. https://github.com/graphql/graphql-spec/issues/488

sashank updated this revision to Diff 4606.Wed, Sep 11, 4:33 PM
sashank marked an inline comment as done.

rebase

This revision is now accepted and ready to land.Wed, Sep 11, 4:33 PM
sashank edited the summary of this revision. (Show Details)Wed, Sep 11, 7:40 PM