Page MenuHomeElementl

Remove ManagedGrpcPythonEnvRepositoryLocationHandle, share more grpc server logic between dagit and the daemon
ClosedPublic

Authored by dgibson on Mar 8 2021, 4:10 AM.

Details

Summary

Instead of having one path for loading gRPC servers in dagit and a totally different one in the daemon, consolidate them. The Workspace now also uses a ProcessGrpcServerRegistry to manage location lifecycles, just like the daemon.

This also opens up a future where we can pass in a workspace instead of an ExternalPipeline to RunLauncher.launch_run, which will be a big step to being able to resolve https://github.com/dagster-io/dagster/issues/3778.

Test Plan

Integration

End goal here is to consolidate code for loading managed gRPC processes between the daemon and dagit / all dagster processes, with the goal of making so that you can pass in a handlemanager to RunLauncher rather than an ExternalPipeline. This will allow most run launchers to execute without ever having to load the ExternalPipeline.

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Mar 8 2021, 4:44 AM
Harbormaster failed remote builds in B26951: Diff 32951!
Harbormaster failed remote builds in B26952: Diff 32952!
python_modules/dagster/dagster/cli/workspace/cli_target.py
596

nit: interval=0 is confusing to me - does this continuously cleanup or never (just at exit)? it has to be the latter but still made me stall when i hit this

python_modules/dagster/dagster/cli/workspace/workspace.py
52–71

maybe just need some comments but why these properties are the way they are doesn't immediately line up for me

its not clear to me why the server registry is known to the workspace

from a mental model perspective it seems like a workspace is-a location manager - i could get how impl wise they may need to be separate but I dont immediately understand why

78–89

what is this handle managment doing out side of the handle manager

python_modules/dagster/dagster/core/host_representation/handle.py
74–76

*

will at minimum add some comments in the workspace. I do think they are different but I agree it is unclear

dgibson edited the summary of this revision. (Show Details)

new take on this based on alex's observations. workspace now still has the server registry but not the handlemanager (as alex correctly observed, there's a whole lot of overlap there, and a workspace is basically a 'handle manager' already).

The way I see this possibly progressing from here is:

  • rename RepositoryLocationHandleManager (currently used by the daemon) to something like DynamicWorkspace (distinct from the current Workspace class in that it isn't a fixed list of origins but rather lazily loaded/shared based on what origins hppen
  • remove distinction between handles and locations, go straight from origin => location instead
  • both workspace and dynamicworkspace implement a shared IWorkspace interface that lets you go from an origin to a location, that's what the run launcher takes in as an argument

If there are subtle behavior changes you want to discuss, maybe do a request review but if youre confident in this I think its a pretty good step towards the end goal

python_modules/dagster/dagster/core/host_representation/grpc_server_registry.py
42–44

i cant quite discern - does the new callsites that leverage this introduce subtle behavior differences?

58–65

Should be higher than reload_interval

can we assert this in code?

71–79

verbose name worth while here i think, comment works too

113

comment explaining what the del accomplishes

python_modules/dagster/dagster/core/host_representation/origin.py
186–190

unfortunate, but I suppose the way out of this interface on the base class is the

go straight from origin => location instead

mentioned on where to go from here

This revision is now accepted and ready to land.Mon, Mar 29, 4:30 PM
python_modules/dagster/dagster/core/host_representation/grpc_server_registry.py
42–44

no intentional behavior differences - just mimicing the logic that used to live in the managed handle