Page MenuHomePhabricator

[dagit] Create copy of DagsterGraphQLContext for each request
AbandonedPublic

Authored by sashank on Mon, Jan 4, 5:15 PM.

Details

Reviewers
None
Summary

This diff updates the Dagit webserver such that the DagsterGraphQLContext is request scoped and clones the current workspace is at the top of the request. This is needed due to the introduction of the repository location watch threads that can modify the context. Now the watch threads only run on the long-lived global context.

Test Plan

unit

Diff Detail

Repository
R1 dagster
Branch
request-context
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

sashank added a reviewer: alangenfeld.
sashank edited the summary of this revision. (Show Details)
  • Only add location subscribers on root context
  • Update location event subscription to use root context
python_modules/dagster-graphql/dagster_graphql/implementation/context.py
18–22

I think we should try to be a bit more clear and explicit with this set up - since I expect us to compound on it over time. Instead of this root_context stuff, lets have two different classes (no inheritance preferably), one for process scope and one for request scope. A quick search shows a limited number of DagsterGraphQLContext so renaming in same diff should be fine, not sure the DagsterGraphQL prefix is necessary. Ideally the request scoped context is an immutable namedtuple. You shouldn't need all the getters on the process scoped one

18–44

I think we should try to be a bit more clear and explicit with this set up - since I expect us to compound on it over time. Instead of this root_context stuff, lets have two different classes (no inheritance preferably), one for process scope and one for request scope.

155–158

i expect the class split should make this possible to model more cleanly

python_modules/dagster-graphql/dagster_graphql/schema/roots.py
532–535

^ this invocation should just return the new object directly

python_modules/dagster-graphql/dagster_graphql/implementation/context.py
18–22

That makes sense, great call. Just to be sure I'm understanding this correctly, does that mean most of the graphql tests should be updated to be against the request scoped context?

python_modules/dagster-graphql/dagster_graphql/implementation/context.py
18–22

Also, is it okay for the request scoped context to still have a reference to the process scoped context?

python_modules/dagster-graphql/dagster_graphql/implementation/context.py
18–22

Actually I'm realizing it doesn't make much sense for the request context to have a reference to the process scoped context since we don't want the process-scoped threads to be running for each request

python_modules/dagster-graphql/dagster_graphql/implementation/context.py
18–22

That makes sense, great call. Just to be sure I'm understanding this correctly, does that mean most of the graphql tests should be updated to be against the request scoped context?

Yea I expect most existing call sites should be againts the request scoped one. You might end up with like a for_test static method to construct an instance or something.

Split context into ProcessContext and RequestContext

  • Add with_process_context helper

Lots of failing tests to fix

could we also bring back the deployed_grpc test suites as part of this now that they should no longer be crashing? (or in a followup also fine)

could we also bring back the deployed_grpc test suites as part of this now that they should no longer be crashing? (or in a followup also fine)

yes good call, will do

  • Use correct context for GraphQL View