feat: remove rpyc#2094
Conversation
Greptile SummaryThis PR replaces the
Confidence Score: 4/5The transport swap is well-structured and the tests cover the new paths; the main things to double-check are the silent timeout cap in peek_stream and the assert in _get_descriptor. The removal of rpyc is clean and all three removed files have direct replacements. The backward-compatible RunEntry.load() guards against stale registry files, and the mappingproxy pickling fix is correctly moved to blueprints.py. The only issues are minor: a leftover 25-second cap in peek_stream that now has no documented justification, a bare assert that optimized builds would skip, and uncleaned _unsub_fns on _RemoteProxy invalidation. dimos/porcelain/dimos.py (silent 25s timeout cap) and dimos/porcelain/remote_module_source.py (assert + _RemoteProxy cleanup gap) Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as dimos CLI / script
participant Coord as ModuleCoordinator
participant LCM as LCM bus
participant Client as Dimos.connect()
participant RMS as RemoteModuleSource
participant Mod as Module (worker)
CLI->>Coord: start_rpc_service()
Coord->>LCM: serve_rpc Coordinator/ping, list_modules, load_blueprint
Coord-->>CLI: (running)
Client->>RMS: "RemoteModuleSource(timeout=5)"
RMS->>LCM: call_sync Coordinator/ping
LCM->>Coord: ping request
Coord-->>LCM: pong
LCM-->>RMS: pong
RMS-->>Client: source ready
Client->>RMS: get_module(StressTestModule)
RMS->>LCM: call_sync Coordinator/list_modules
LCM->>Coord: list_modules request
Coord-->>LCM: [ModuleDescriptor(class_name, qualified_path, rpc_names)]
LCM-->>RMS: descriptors
RMS->>RMS: importlib.import_module(qualified_path)
alt class importable
RMS-->>Client: "RPCClient(cls, rpc=shared_rpc)"
else ImportError
RMS-->>Client: _RemoteProxy(rpc_names)
end
Client->>Mod: module.ping() via RpcCall
RpcCall->>LCM: call_sync StressTestModule/ping
LCM->>Mod: ping request
Mod-->>LCM: pong
LCM-->>RpcCall: pong
RpcCall-->>Client: pong
Reviews (1): Last reviewed commit: "feat: remove rpyc" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| assert self._descriptors is not None | ||
| if name not in self._descriptors: |
There was a problem hiding this comment.
assert statements are stripped when Python is run with -O (optimized mode). _refresh_descriptors() always assigns self._descriptors, so the assertion is logically sound, but using a hard check here makes the invariant visible and survives optimization flags.
| assert self._descriptors is not None | |
| if name not in self._descriptors: | |
| if self._descriptors is None: | |
| raise RuntimeError("_refresh_descriptors() did not populate _descriptors") | |
| if name not in self._descriptors: |
| def invalidate(self, name: str) -> None: | ||
| with self._lock: | ||
| entry = self._cache.pop(name, None) | ||
| if entry is not None: | ||
| self._descriptors = None | ||
| if isinstance(entry, RPCClient): | ||
| try: | ||
| entry[0].close() | ||
| entry.stop_rpc_client() | ||
| except Exception: | ||
| logger.warning("Failed to close RPyC connection for module %s", name, exc_info=True) | ||
| logger.warning("Failed to release proxy for %s", name, exc_info=True) |
There was a problem hiding this comment.
_RemoteProxy unsub functions not cleaned up on invalidation
invalidate() calls stop_rpc_client() only for RPCClient entries; _RemoteProxy entries are dropped silently without clearing their _unsub_fns. Each call through a _RemoteProxy appends one closure to that list. For long-running clients that invalidate and recreate proxies repeatedly, the discarded proxy's accumulated closures are not freed. The same gap exists in close(). Consider adding a close() path for _RemoteProxy, or calling entry._unsub_fns.clear() before discarding.
❌ 1 Tests Failed:
View the top 2 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
|
|
||
| # `Blueprint` carries `mappingproxy` fields (e.g. `global_config_overrides`, | ||
| # `remapping_map`) that are not picklable by default — `MappingProxyType` lives | ||
| # at `builtins.mappingproxy` which isn't a real builtins attribute, so pickle |
There was a problem hiding this comment.
Shouldn't it just be able to get it from types.MappingProxyType the same as we do?
There was a problem hiding this comment.
types.MappingProxyType is the real location. But it incorrectly reports its location as builtins.mappingproxy (if you use __module__ and __qualname__). Looks like this is incorrect by choice as they don't want you to serialize/deserialize mapping proxies.
But it's true that this is ugly. I'll change Blueprints to be serializable instead.
| client = LCMRPC() | ||
| client.start() | ||
| try: | ||
| result, _unsub = client.call_sync(f"{COORDINATOR_RPC_NAME}/ping", ([], {}), rpc_timeout=2.0) | ||
| assert result == "pong" | ||
| finally: | ||
| client.stop() |
There was a problem hiding this comment.
This is probably a good oppurtunity to improve the class to work as a context manager?
| client = LCMRPC() | |
| client.start() | |
| try: | |
| result, _unsub = client.call_sync(f"{COORDINATOR_RPC_NAME}/ping", ([], {}), rpc_timeout=2.0) | |
| assert result == "pong" | |
| finally: | |
| client.stop() | |
| with LCMRPC() as client: | |
| result, _unsub = client.call_sync(f"{COORDINATOR_RPC_NAME}/ping", ([], {}), rpc_timeout=2.0) | |
| assert result == "pong" |
There was a problem hiding this comment.
I've tried to use context managers before, but wasn't really liked.
The class itself isn't really used as a context manager, and I think it's not a good idea to add something if it's only meant for tests.
| return PeekNotFound() | ||
| try: | ||
| return stream.get_next(timeout) | ||
| except Exception: |
There was a problem hiding this comment.
There should be a lint rule for this, CC keeps doing this far too frequently.
We should be clear what exception we expect here. This masks mistakes in spelling the attribute etc.
2 participants
Problem
Closes DIM-XXX
Solution
How to Test
Contributor License Agreement