Details
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
Adding in automatic inference of repository location and/or repository if not specified
Fixing repo location & repo name inference to work based on the number of unique (repo loc, repo, pipeline) tuples
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 |
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. |
python_modules/dagster-graphql/dagster_graphql/client/client.py | ||
---|---|---|
50 | nit: type - weird how this isn't caught by mypy? |
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? |
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. |
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 its not obvious whats the best path here, we could
|
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 |
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"
IMO it's worth prioritizing launch -> submit, but definitely not a blocker for this diff.