Details

Reviewers

• rexledesma
alangenfeld
• catherinewu
• sandyryza

Commits

R1:12f5de0addf9: [dagster-graphql] Dagster GraphQL Python Client [3/4]

Summary

Adding submit_pipeline_execution to the Dagster GraphQL client

Depends on D7409, D7421

Test Plan

added tests to check that 1) success conditions are handled correctly and 2) failure conditions (both within client and from server) are handled gracefully

Diff Detail

Repository

R1 dagster

Branch

python-client (branched from master)

Lint

Lint Passed

Unit

No Test Coverage

Event Timeline

• sidkmenon created this revision.Apr 14 2021, 3:51 PM

• sidkmenon held this revision as a draft.

Harbormaster failed remote builds in B28811: Diff 35366!Apr 14 2021, 4:08 PM

• sidkmenon edited the summary of this revision. (Show Details)Apr 14 2021, 5:37 PM

• sidkmenon added parent revisions: D7421: [dagster-graphql] Dagster GraphQL Python Client [2/4], D7409: [RFC] [dagster-graphql] Python Client Implementation [1/4].

Adding in automatic inference of repository location and/or repository if not specified

Adding error message if multiple repository locations & repositories are present

Harbormaster failed remote builds in B28824: Diff 35380!Apr 14 2021, 6:07 PM

Harbormaster failed remote builds in B28825: Diff 35381!Apr 14 2021, 6:09 PM

Fixing repo location & repo name inference to work based on the number of unique (repo loc, repo, pipeline) tuples

query backcompat checks

Harbormaster failed remote builds in B28834: Diff 35391!Apr 14 2021, 6:44 PM

Harbormaster failed remote builds in B28833: Diff 35390!

Adding about half of the required tests

Harbormaster completed remote builds in B28889: Diff 35462.Apr 15 2021, 5:21 AM

• sidkmenon edited the test plan for this revision. (Show Details)Apr 15 2021, 2:27 PM

Adding all tests

Fixing typo

• sidkmenon published this revision for review.Apr 15 2021, 2:29 PM

• sidkmenon added a reviewer: • rexledesma.

Harbormaster completed remote builds in B28904: Diff 35478.Apr 15 2021, 2:49 PM

Harbormaster completed remote builds in B28905: Diff 35479.Apr 15 2021, 2:51 PM

Rebasing

Harbormaster failed remote builds in B29002: Diff 35596!Apr 16 2021, 6:23 PM

Moving submit_pipeline_execution tests to the GraphQL python client's test suite

Rebasing

Harbormaster failed remote builds in B29010: Diff 35606!Apr 16 2021, 8:15 PM

Harbormaster failed remote builds in B29015: Diff 35611!Apr 16 2021, 8:28 PM

• sidkmenon edited the test plan for this revision. (Show Details)Apr 16 2021, 8:30 PM

• sidkmenon added reviewers: alangenfeld, • catherinewu.

rebasing

Harbormaster completed remote builds in B29026: Diff 35623.Apr 16 2021, 9:37 PM

Modifying commit message

• sidkmenon retitled this revision from [dagster-graphql] Dagster GraphQL Python Client [3/3] to [dagster-graphql] Dagster GraphQL Python Client [3/4].Apr 19 2021, 5:18 PM

Harbormaster completed remote builds in B29068: Diff 35676.Apr 19 2021, 5:38 PM

• sidkmenon added a child revision: D7504: [docs] Dagster GraphQL Python Client [4/4].Apr 19 2021, 6:36 PM

• rexledesma added inline comments.Apr 19 2021, 9:53 PM

python_modules/dagster-graphql/dagster_graphql/client/client.py
50–56
68–70	this should be flattened for readability
103–105	unnecessary?
132–133	spacing, and remove `+` for concat multiline strings
python_modules/dagster-graphql/dagster_graphql/client/utils.py
21	this is more about the information for a specific a pipeline, rather than the repository or repository location

Addressing comments

Harbormaster completed remote builds in B29214: Diff 35849.Apr 20 2021, 5:44 PM

• sidkmenon marked an inline comment as done.Apr 20 2021, 5:57 PM

• sidkmenon added inline comments.

python_modules/dagster-graphql/dagster_graphql/client/client.py
103–105	the extra `optional` seems to be part of the autogenerated doc strings. but it looks like I accidentally deleted the `optional`s or something on `repository_location_name` and `repository_name`, etc, so I added them back for consistency.

• rexledesma accepted this revision.Apr 20 2021, 6:21 PM

• rexledesma added inline comments.

python_modules/dagster-graphql/dagster_graphql/client/client.py
50	nit: type - weird how this isn't caught by mypy?

This revision is now accepted and ready to land.Apr 20 2021, 6:21 PM

• sidkmenon added a reviewer: • sandyryza.Apr 20 2021, 7:18 PM

Addressing nits

• sandyryza requested changes to this revision.Apr 21 2021, 4:50 PM

• sandyryza added inline comments.

python_modules/dagster-graphql/dagster_graphql/client/client.py
79	Am I understanding correctly that the underlying GraphQL mutation is called launchPipelineExecution? If so, any reason not to call this method `launch_pipeline_execution`?

This revision now requires changes to proceed.Apr 21 2021, 4:50 PM

Harbormaster completed remote builds in B29283: Diff 35941.Apr 21 2021, 4:54 PM

• sidkmenon added inline comments.Apr 21 2021, 6:06 PM

python_modules/dagster-graphql/dagster_graphql/client/client.py
50	yeah that is pretty weird actually.... ?
79	I think after a conversation with Alex, he recommended calling this `submit_pipeline_execution`? I think we have been doing a rename under the hood from `launch_pipeline` to `submit_pipeline`, I guess ostensibly because the submit change now sends the run to the daemon which launches it instead. From conversations with Johann, it seems a bit messy, so I'm happy to rename this to `launch_pipeline_execution` if you think that's better.

alangenfeld added a subscriber: • johann.Apr 21 2021, 6:30 PM

alangenfeld added inline comments.

python_modules/dagster-graphql/dagster_graphql/client/client.py
79	cc @johann https://github.com/dagster-io/dagster/issues/3182 for context - when we introduced the `RunCoordinator` for run queueing we made `launch` from dagit / graphql actually do `submit` but didn't follow up on any renames in the product or GraphQL schema https://dagster.phacility.com/source/dagster/browse/master/python_modules/dagster-graphql/dagster_graphql/implementation/execution/launch_execution.py$36-38 its not obvious whats the best path here, we could add this forward looking as `submit` - will be confusing til we add `submitPipelineExecution` to the GraphQL schema add as `launch` - adds surface area likely to go through breaking change cycle ???

rebasing

Harbormaster completed remote builds in B29301: Diff 35965.Apr 21 2021, 9:01 PM

• sandyryza added inline comments.Apr 21 2021, 9:45 PM

python_modules/dagster-graphql/dagster_graphql/client/client.py
79	Ah - got it. How difficult would it be to just add submitPipelineExecution to the GraphQL schema?

alangenfeld added inline comments.Apr 22 2021, 3:50 PM

python_modules/dagster-graphql/dagster_graphql/client/client.py
79	adding it - pretty easy putting it under sufficient test - not complicated but will take time

• sidkmenon added inline comments.Apr 22 2021, 5:06 PM

python_modules/dagster-graphql/dagster_graphql/client/client.py
79	Idea - could we land this & then change the backing query on the client when submitPipelineExecution is ready? I have the backwards compat setup already handled so I think we should be good?