Added implementations for get_run_status and reload_repository_location
In forthcoming diffs:
- Backcompatability scaffolding
- Submitting Pipeline Execution
Details
Details
Added testing for success conditions as well as for 1) query validation failures and 2) runtime errors on the server side (i.e. a requested run does not exist, etc.)
Diff Detail
Diff Detail
- Repository
- R1 dagster
- Lint
Lint Passed - Unit
No Test Coverage
Event Timeline
Comment Actions
Add to graphql documentation as well? there are directive you can use to make the classes and associated methods show up
| python_modules/dagster-graphql/dagster_graphql/client/client.py | ||
|---|---|---|
| 48 | Is there a more specific Exception given by the gql client? | |
| 78–88 | nit: add space after all your check expressions (since it's not part of the core logic, fix for other methods as well) | |
| python_modules/dagster-graphql/dagster_graphql_tests/client_tests/test_python_client.py | ||
| 1 ↗ | (On Diff #35322) | should probably test each method in its own file, and it should run in its own tox suite |
| 29 ↗ | (On Diff #35322) | Use enum instead of raw "SUCCESS" |
| python_modules/dagster-graphql/dagster_graphql/client/client.py | ||
|---|---|---|
| 48 | Unfortunately not - the gql client just throws a generic Exception. Added a comment to clarify that | |
| python_modules/dagster-graphql/dagster_graphql/client/client.py | ||
|---|---|---|
| 29 | Should the client need to be closed when we're done with it? | |
| 45 | What's this needed for? | |
| 60 | Are the values accepted here different from the values that are accepted in other APIs that accept the argument "run_config"? If not, would it make sense to call this argument run_config? | |
| 62 | For symmetry with "mode", should we just call this "preset"? | |
| 96 | Is the idea that this reloads all repositories inside the repository location? Might be helpful to say that explicitly | |
| python_modules/dagster-graphql/dagster_graphql/client/utils.py | ||
| 30 | This is a little bit confusing to me. We can give a failure message if the status is success? Also, is the "optional" to the right of "Optional[str]" something that sphinx knows how to deal with? | |
| python_modules/dagster-graphql/dagster_graphql/client/client.py | ||
|---|---|---|
| 29 | No I don't think so - from the examples linked on GH and on the docs, it doesn't look like closing the gql.Client is necessary / idiomatic so I extended that behavior to the client. Does that make sense or would closing be a more natural behavior? | |
| 45 | Figured that the super call is standard operating procedure for initializing objects in python - are you saying that the init call is unnecessary since this just inherits from type/object? | |
| 60 | I named it run_config_data since that's the argument name in the launch pipeline/submit pipeline mutation - happy to rename it though if that's more consistent! | |
| 96 | makes sense! | |
| python_modules/dagster-graphql/dagster_graphql/client/utils.py | ||
| 30 | you're absolutely right that was a typo - meant for it to be FAILURE. As far as sphinx I think the optional part is part of the sphinx auto generated stuff. | |
| python_modules/dagster-graphql/dagster_graphql/client/client.py | ||
|---|---|---|
| 29 | If gql.Clients don't get closed, then I think this is fine. | |
| 45 | Exactly - unnecessary since it just inherits from object. | |
| 60 | Ah ok - any opinions @alangenfeld? | |
| python_modules/dagster-graphql/dagster_graphql/client/client.py | ||
|---|---|---|
| 60 | ya going with the names as they line up in the python api feels right | |