Fix/Organize Types for ModuleCoordiator/DimosCluster/WorkerManager/RPCClient/ModuleProxy #1164
+854
−340
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…lueprint to be consistent with docs
Greptile OverviewGreptile SummaryThis PR improves typing throughout the core module system by introducing a Major Changes:
The changes significantly improve type safety and enable better architectural patterns through interface-based module dependencies. Confidence Score: 4.5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Blueprint
participant ModuleCoordinator
participant DeployerProtocol
participant WorkerDeployer
participant DaskDeployer
participant ModuleProxy
participant Module
Note over Blueprint,ModuleCoordinator: Refactor: ModuleBlueprintSet → Blueprint
User->>Blueprint: create via Module.blueprint()
Note over Blueprint: Now includes Spec support via ModuleRef
User->>Blueprint: autoconnect(blueprints)
Blueprint->>Blueprint: Resolve stream connections
Blueprint->>Blueprint: Resolve module refs via Spec matching
User->>Blueprint: build()
Blueprint->>ModuleCoordinator: new ModuleCoordinator()
ModuleCoordinator->>ModuleCoordinator: start()
alt Using Dask
Note over ModuleCoordinator,DaskDeployer: Refactor: DimosCluster → DaskDeployer
ModuleCoordinator->>DaskDeployer: core.start()
Note over DaskDeployer: Implements DeployerProtocol
else Using Workers
Note over ModuleCoordinator,WorkerDeployer: Refactor: WorkerManager → WorkerDeployer
ModuleCoordinator->>WorkerDeployer: new WorkerDeployer()
Note over WorkerDeployer: Implements DeployerProtocol
end
ModuleCoordinator->>DeployerProtocol: deploy(module_class, args, kwargs)
DeployerProtocol-->>ModuleCoordinator: ModuleProxy
Note over ModuleProxy: New type: RPCClient + Module for better typing
Blueprint->>ModuleCoordinator: _connect_streams()
Blueprint->>ModuleCoordinator: _connect_rpc_methods()
Blueprint->>ModuleCoordinator: _connect_module_refs()
Note over Blueprint,Module: New: Connects modules via Spec protocol matching
ModuleCoordinator->>ModuleProxy: set_module_ref(name, target)
ModuleProxy->>Module: setattr(name, target)
ModuleCoordinator->>ModuleProxy: start()
ModuleProxy->>Module: start()
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def deploy(self, module_class: type[ModuleT], *args: Any, **kwargs: Any) -> ModuleProxy: | ||
| if self._closed: | ||
| raise RuntimeError("WorkerManager is closed") | ||
| raise RuntimeError("PureDeployer is closed") |
There was a problem hiding this comment.
Error message references PureDeployer but class is now named WorkerDeployer
Suggested change
| raise RuntimeError("PureDeployer is closed") | |
| raise RuntimeError("WorkerDeployer is closed") |
| ) -> list[ModuleProxy]: | ||
| if self._closed: | ||
| raise RuntimeError("WorkerManager is closed") | ||
| raise RuntimeError("PureDeployer is closed") |
There was a problem hiding this comment.
Error message references PureDeployer but class is now named WorkerDeployer
Suggested change
| raise RuntimeError("PureDeployer is closed") | |
| raise RuntimeError("WorkerDeployer is closed") |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
NOTE: only merge this after the RPC rework merge
DimosCluster is basically untyped cause its a dynamic hack of the dask Client
This PR basically adds proper typing to DimosCluster, enforcing that both DimosCluster and WorkerManager follow a
DeployerProtocol, which fixes a lot of the mypy ignores in ModuleCoordiator.After adding the protocol, I think it made sense to use consistent names:
WorkerManager => WorkerDeployerandDimosCluster => DaskDeployer(and we would eventually have aDockerDeployer) so those renames are in this PR.