Skip to content

chore(rpc): remove old rpc linking#1696

Open
paul-nechifor wants to merge 1 commit intodevfrom
paul/chore/remove-old-rpcs
Open

chore(rpc): remove old rpc linking#1696
paul-nechifor wants to merge 1 commit intodevfrom
paul/chore/remove-old-rpcs

Conversation

@paul-nechifor
Copy link
Copy Markdown
Contributor

Problem

Still had the two old RPC linking systems.

Closes DIM-527

Solution

  • Remove them.

Breaking Changes

Old RPCs are gone.

How to Test

Contributor License Agreement

  • I have read and approved the CLA.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 28, 2026

Greptile Summary

This PR completes the migration away from two legacy RPC-linking mechanisms — the string-keyed rpc_calls: list[str] + get_rpc_calls("ClassName.method") lookup pattern, and the set_ModuleX_method() RPC callback convention — replacing both with typed, spec-based module references (e.g., _navigation: NavigationInterfaceSpec) that are wired at blueprint build time and fail fast if a dependency is missing.

Key changes:

  • ModuleBase loses rpc_calls, _bound_rpc_calls, get_rpc_calls, set_rpc_method, and get_rpc_method_names; Blueprint._connect_rpc_methods is removed entirely.
  • Ten new *Spec Protocol files added (NavigationInterfaceSpec, SpatialMemorySpec, ObjectTrackingSpec, ObjectSceneRegistrationSpec, GraspGenSpec, ArmDriverSpec, VLMAgentSpec, WebsocketVisSpec, G1ConnectionSpec, GO2ConnectionSpec) to give consumers a stable, typed interface.
  • Blueprint._BlueprintAtom now detects SomeSpec | None = None annotations and marks them as optional ModuleRefs, so the blueprint silently skips wiring rather than raising when no match is found.
  • Docker readiness polling now calls get_skills instead of get_rpc_method_names.
  • All tests updated with proper stub modules instead of manually injecting _bound_rpc_calls.
  • One behavioural change worth confirming: GpsNavSkillContainer._vis: WebsocketVisSpec is now a required dependency, whereas the old guard (if self._set_gps_travel_goal_points:) made the visualization step fully optional.

Confidence Score: 5/5

Safe to merge — the changes are a clean, well-tested removal of legacy RPC plumbing with no correctness defects found.

All remaining findings are P2. The single noteworthy point (vis module now required in GpsNavSkillContainer) is likely intentional given the PR's stated breaking-change scope and the tests explicitly requiring the stub, so it does not block merge.

dimos/agents/skills/gps_nav_skill.py — confirm the behavioural shift from optional to required WebsocketVisSpec dependency is intentional for all production blueprints.

Important Files Changed

Filename Overview
dimos/core/blueprints.py Core change: removes _connect_rpc_methods entirely and adds optional ModuleRef support via Union/UnionType annotation detection; logic looks correct.
dimos/core/module.py Removes rpc_calls, _bound_rpc_calls, get_rpc_calls, set_rpc_method, and get_rpc_method_names from ModuleBase; clean removal with no leftover references.
dimos/core/docker_module.py Readiness polling endpoint changed from get_rpc_method_names to get_skills; old RPC call bookkeeping removed from DockerModuleProxy.
dimos/agents/skills/gps_nav_skill.py Replaces optional RpcCall with required _vis: WebsocketVisSpec; old code guarded the call with if self._set_gps_travel_goal_points:, making vis optional — now it is mandatory.
dimos/agents/skills/navigation.py Replaces all rpc_calls/get_rpc_calls patterns with typed spec fields; _object_tracking is correctly typed optional with a guard at the top of _navigate_to_object.
dimos/robot/unitree/unitree_skill_container.py Cleanly replaces rpc_calls list with typed _navigation: NavigationInterfaceSpec and _connection: GO2ConnectionSpec fields; all usages updated consistently.
dimos/manipulation/control/servo_control/cartesian_motion_controller.py Removes legacy arm_driver config field and dual-fallback FK/IK path; now uses _arm_driver: ArmDriverSpec exclusively via the new blueprint wiring.
dimos/manipulation/grasping/grasping.py Replaces rpc_calls list with _scene_registration: ObjectSceneRegistrationSpec and _grasp_gen: GraspGenSpec; call sites simplified and error handling preserved.
dimos/agents/skills/test_navigation.py Tests now use proper StubSpatialMemory, StubNavigation, and StubObjectTracking modules instead of clearing rpc_calls; stub blueprints added to all test cases.
dimos/agents/skills/test_unitree_skill_container.py Test refactored to use StubNavigation and StubGO2Connection modules; old _bound_rpc_calls injection removed in favour of proper stub blueprints.

Sequence Diagram

sequenceDiagram
    participant User
    participant Blueprint
    participant BlueprintAtom
    participant ModuleCoordinator
    participant ModuleProxy

    User->>Blueprint: build()
    Blueprint->>BlueprintAtom: from_module(cls)
    BlueprintAtom->>BlueprintAtom: inspect type hints
    Note over BlueprintAtom: _field: SomeSpec → required ModuleRef<br/>_field: SomeSpec | None → optional ModuleRef
    BlueprintAtom-->>Blueprint: module_refs (required & optional)

    Blueprint->>ModuleCoordinator: deploy all modules
    Blueprint->>Blueprint: _connect_streams()
    Blueprint->>Blueprint: _connect_module_refs()

    loop for each ModuleRef
        Blueprint->>ModuleCoordinator: find candidate satisfying spec
        alt candidate found
            Blueprint->>ModuleProxy: set_module_ref(name, target_proxy)
            ModuleProxy-->>ModuleProxy: setattr(self, name, RPCClient)
        else no candidate & optional
            Blueprint->>Blueprint: skip (no error)
        else no candidate & required
            Blueprint-->>User: raise Exception (missing dependency)
        end
    end

    Blueprint->>ModuleCoordinator: build_all_modules()
Loading

Comments Outside Diff (1)

  1. dimos/agents/skills/gps_nav_skill.py, line 37 (link)

    P2 Visualization dependency is now required where it was previously optional

    The old code stored the RPC callable as _set_gps_travel_goal_points: RpcCall | None = None and guarded its use with:

    if self._set_gps_travel_goal_points:
        self._set_gps_travel_goal_points(new_points)

    The new _vis: WebsocketVisSpec (no = None) means any blueprint that includes GpsNavSkillContainer must also include a module satisfying WebsocketVisSpec, or blueprint build will raise an error. The tests enforce this by adding StubWebsocketVis.blueprint().

    If there are real deployment blueprints that used GpsNavSkillContainer without a websocket vis module, they will break at startup rather than silently skipping the visualization. Consider whether this is intentional or whether _vis: WebsocketVisSpec | None = None with a guard is the right shape:

    if self._vis is not None:
        self._vis.set_gps_travel_goal_points(new_points)

Reviews (1): Last reviewed commit: "chore(rpc): remove old rpc linking" | Re-trigger Greptile

Copy link
Copy Markdown
Contributor

@mustafab0 mustafab0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems all good

@paul-nechifor paul-nechifor enabled auto-merge (squash) March 28, 2026 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants