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 created this revision.Mon, Jan 4, 5:15 PM

Harbormaster failed remote builds in B23551: Diff 28636!Mon, Jan 4, 5:29 PM

sashank edited the summary of this revision. (Show Details)Mon, Jan 4, 5:34 PM

sashank added a reviewer: alangenfeld.

Harbormaster failed remote builds in B23553: Diff 28638!Mon, Jan 4, 5:36 PM

Harbormaster completed remote builds in B23554: Diff 28639.Mon, Jan 4, 5:45 PM

sashank requested review of this revision.Mon, Jan 4, 5:45 PM

Only add location subscribers on root context

Update location event subscription to use root context

Harbormaster completed remote builds in B23563: Diff 28648.Mon, Jan 4, 6:42 PM

Harbormaster completed remote builds in B23562: Diff 28647.Mon, Jan 4, 6:47 PM

alangenfeld added inline comments.Mon, Jan 4, 6:54 PM

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

Harbormaster completed remote builds in B23567: Diff 28655.Mon, Jan 4, 7:05 PM

sashank added inline comments.Mon, Jan 4, 7:11 PM

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?

sashank added inline comments.Mon, Jan 4, 7:29 PM

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?

sashank added inline comments.Mon, Jan 4, 7:39 PM

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

alangenfeld added inline comments.Mon, Jan 4, 7:42 PM

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.