Page MenuHomeElementl

Re-rache repository locations on WorkspaceProcessContext everytime a request is made
ClosedPublic

Authored by sashank on Apr 30 2021, 5:24 PM.

Details

Summary

Previously, the repository locations from the workspace were cached on the WorkspaceProcessContext when initialized. This is because the process context assumes that the workspace only changes when reload_workspace is called on the ProcessContext. With the new async workspaces, this is not true anymore.

This diff changes the process context to not cache the locations anymore and directly access the underlying workspace instead

Test Plan

Existing tests

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

sashank published this revision for review.Apr 30 2021, 5:26 PM
dgibson requested changes to this revision.Apr 30 2021, 5:30 PM

as per slack convo can this diff remove WorkspaceProcessContext.repository_locations altogether and always delegate to Workspace.repository_locations

This revision now requires changes to proceed.Apr 30 2021, 5:30 PM
dgibson requested changes to this revision.Apr 30 2021, 5:44 PM

this seems better, the reload_workspace path has a problem now though

python_modules/dagster/dagster/cli/workspace/context.py
242–246

there's something that's still a little funky about the logic for when add_state_subscriber should be applied now that this class is no longer the thing that constructs the RepositoryLocation, but that was the case before this diff too.

251–252

i think this is no longer calling add_state_subscriber on the newly reloaded locations though? what you had before where it adds the state subscribers to any locations that didn't already have it may be the way to go

This revision now requires changes to proceed.Apr 30 2021, 5:44 PM
dgibson added inline comments.
python_modules/dagster/dagster/cli/workspace/context.py
204

i'm not sure you actually need this check / _previous_repository_locations? Reloading the workspace should cause the location dict to be totally cleared out and reset

This revision is now accepted and ready to land.Apr 30 2021, 5:57 PM