Page MenuHomeElementl

[dagster-graphql] Dagster GraphQL Python Client [3/4]
ClosedPublic

Authored by sidkmenon on Apr 14 2021, 3:51 PM.

Details

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
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

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

Adding error message if multiple repository locations & repositories are present

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

Adding about half of the required tests

sidkmenon edited the test plan for this revision. (Show Details)

Adding all tests

sidkmenon added a reviewer: rexledesma.

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

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
python_modules/dagster-graphql/dagster_graphql/client/client.py
50–55
68–70

this should be flattened for readability

104–106

unnecessary?

133–134

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

sidkmenon marked 4 inline comments as done.

Addressing comments

sidkmenon added inline comments.
python_modules/dagster-graphql/dagster_graphql/client/client.py
104–106

the extra optional seems to be part of the autogenerated doc strings. but it looks like I accidentally deleted the optionals or something on repository_location_name and repository_name, etc, so I added them back for consistency.

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 marked an inline comment as done.

Addressing nits

sandyryza added inline comments.
python_modules/dagster-graphql/dagster_graphql/client/client.py
80

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
python_modules/dagster-graphql/dagster_graphql/client/client.py
50

yeah that is pretty weird actually.... ?

80

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 inline comments.
python_modules/dagster-graphql/dagster_graphql/client/client.py
80

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
  • ???
python_modules/dagster-graphql/dagster_graphql/client/client.py
80

Ah - got it. How difficult would it be to just add submitPipelineExecution to the GraphQL schema?

python_modules/dagster-graphql/dagster_graphql/client/client.py
80

adding it - pretty easy
putting it under sufficient test - not complicated but will take time

python_modules/dagster-graphql/dagster_graphql/client/client.py
80

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?

Addressing upstream docs comments - also (hopefully) improved error message structure for "InvalidOutputError"

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

IMO it's worth prioritizing launch -> submit, but definitely not a blocker for this diff.