Page MenuHomeElementl

[RFC] [dagster-graphql] Python Client Implementation [1/4]
ClosedPublic

Authored by sidkmenon on Apr 13 2021, 6:12 PM.

Details

Summary

Added implementations for get_run_status and reload_repository_location

In forthcoming diffs:

  1. Backcompatability scaffolding
  2. Submitting Pipeline Execution
Test Plan

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

Repository
R1 dagster
Lint
Lint Warnings
SeverityLocationCodeMessage
Warningpython_modules/dagster-graphql/dagster_graphql/client/client.py:1W0611Unused Import
Warningpython_modules/dagster-graphql/dagster_graphql/client/client.py:8W0611Unused Import
Warningpython_modules/dagster-graphql/dagster_graphql_tests/client_tests/test_python_client.py:3W0611Unused Import
Warningpython_modules/dagster-graphql/dagster_graphql_tests/client_tests/test_python_client.py:3W0611Unused Import
Warningpython_modules/dagster-graphql/dagster_graphql_tests/client_tests/test_python_client.py:17W0212Protected Access
Warningpython_modules/dagster-graphql/dagster_graphql_tests/client_tests/test_python_client.py:20W0621Redefined Outer Name
Warningpython_modules/dagster-graphql/dagster_graphql_tests/client_tests/test_python_client.py:29W0621Redefined Outer Name
Warningpython_modules/dagster-graphql/dagster_graphql_tests/client_tests/test_python_client.py:40W0621Redefined Outer Name
Warningpython_modules/dagster-graphql/dagster_graphql_tests/client_tests/test_python_client.py:51W0621Redefined Outer Name
Warningpython_modules/dagster-graphql/dagster_graphql_tests/client_tests/test_python_client.py:54W0612Unused Variable
Warningpython_modules/dagster-graphql/dagster_graphql_tests/client_tests/test_python_client.py:58W0621Redefined Outer Name
Warningpython_modules/dagster-graphql/dagster_graphql_tests/client_tests/test_python_client.py:68W0621Redefined Outer Name
Warningpython_modules/dagster-graphql/dagster_graphql_tests/client_tests/test_python_client.py:83W0621Redefined Outer Name
Warningpython_modules/dagster-graphql/dagster_graphql_tests/client_tests/test_python_client.py:86W0612Unused Variable
Unit
No Test Coverage

Event Timeline

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
2

should probably test each method in its own file, and it should run in its own tox suite

30

Use enum instead of raw "SUCCESS"

This revision now requires changes to proceed.Apr 13 2021, 7:50 PM
sidkmenon marked 4 inline comments as done.

Addressing comments, including seperating tests into their own tox suite

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

This revision is now accepted and ready to land.Apr 16 2021, 4:41 PM
sidkmenon retitled this revision from [RFC] [dagster-graphql] Python Client Implementation [1/3] to [RFC] [dagster-graphql] Python Client Implementation [1/4].Apr 19 2021, 5:16 PM

Moving DagsterGraphQLClient to the top level

Adding experimental warning to DagsterGraphQLClient

Adding in docstrings to public-facing namedtuples

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?

sidkmenon marked 6 inline comments as done.

Responding to some of @sandyryza's comments

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

sidkmenon marked 4 inline comments as done.

Responding to more @sandyryza's comments and some upstream docs changes