Page MenuHomeElementl

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

Authored by sidkmenon on Apr 14 2021, 3:51 PM.
Tags
None
Referenced Files
F2982990: D7424.id35676.diff
Sun, May 28, 11:29 AM
Unknown Object (File)
Sat, May 20, 6:22 PM
Unknown Object (File)
Sun, May 7, 10:10 PM
Unknown Object (File)
Fri, May 5, 1:07 PM
Unknown Object (File)
Wed, May 3, 5:26 PM
Unknown Object (File)
Apr 22 2023, 12:23 PM
Unknown Object (File)
Apr 20 2023, 10:05 AM
Unknown Object (File)
Apr 19 2023, 8:08 PM
Subscribers

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
Branch
python-client (branched from master)
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

sidkmenon held this revision as a draft.

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

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

sidkmenon marked 4 inline comments as done.

Addressing comments

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 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
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
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 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
  • ???
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?

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

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?

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.