-
Notifications
You must be signed in to change notification settings - Fork 5
RPC Rework: Part 5: Add ModuleProxy type #1142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
…lueprint to be consistent with docs
…o jeff/blueprint/3
Greptile OverviewGreptile SummaryThis PR continues the RPC rework series by adding the Key Changes
Critical Issues FoundTwo logic bugs were identified that will cause runtime failures:
Type Safety ImprovementsThe addition of Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client as Client Code
participant MC as ModuleCoordinator
participant Dask as DimosCluster (Dask)
participant RPC as RPCClient
participant Module as Module Instance
Client->>MC: deploy(ModuleClass, args, kwargs)
MC->>Dask: deploy(ModuleClass, args, kwargs)
Note over Dask: Submits module construction<br/>to dask worker
Dask->>Module: ModuleClass(*args, **kwargs)
activate Module
Module-->>Dask: module instance
deactivate Module
Dask->>RPC: RPCClient(actor, ModuleClass)
activate RPC
Note over RPC: Creates RPC client with<br/>LCMRPC transport
RPC-->>Dask: rpc_client instance
deactivate RPC
Dask-->>MC: cast to ModuleProxy
Note over MC: ModuleProxy is TYPE_CHECKING only<br/>Tells type checker RPCClient has<br/>Module methods
MC-->>Client: ModuleProxy (actually RPCClient)
Note over Client,Module: Module method calls via proxy
Client->>RPC: proxy.some_rpc_method()
Note over RPC: __getattr__ intercepts call<br/>Creates RpcCall wrapper
RPC->>Module: LCMRPC.call_sync(method, args)
Module-->>RPC: result
RPC-->>Client: result
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 files reviewed, 2 comments
| # linking to specific/known module directly | ||
| elif isinstance(getattr(module, name, None), Module): | ||
| other_module = getattr(module, name) | ||
| module_refs.append(ModuleRef(name=name, rpc_method_names=other_module.rpc_calls)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rpc_calls is a list attribute on Module instances, not a class attribute. This line will fail at runtime when isinstance(getattr(module, name, None), Module) evaluates to True because module here is a class, not an instance.
| module_refs.append(ModuleRef(name=name, rpc_method_names=other_module.rpc_calls)) | |
| module_refs.append(ModuleRef(name=name, rpc_method_names=tuple(module.rpcs.keys()))) |
| from dimos.dashboard.rerun_init import init_rerun_server | ||
|
|
||
| server_addr = init_rerun_server(viewer_mode=global_config.viewer_backend) | ||
| global_config.model_copy(update={"rerun_server_addr": server_addr}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
model_copy returns a new instance but the result is not assigned to anything. This means rerun_server_addr is never actually updated in the global config.
| global_config.model_copy(update={"rerun_server_addr": server_addr}) | |
| global_config = global_config.model_copy(update={"rerun_server_addr": server_addr}) |
No description provided.