Skip to content

fix: delete docker_module parallel hierarchy (#2038)#2224

Open
KrishnaH96 wants to merge 3 commits into
dimensionalOS:mainfrom
KrishnaH96:fix/issue-2038-delete-docker-module
Open

fix: delete docker_module parallel hierarchy (#2038)#2224
KrishnaH96 wants to merge 3 commits into
dimensionalOS:mainfrom
KrishnaH96:fix/issue-2038-delete-docker-module

Conversation

@KrishnaH96
Copy link
Copy Markdown

Problem

docker_module.py is a 693-line parallel ModuleProxyProtocol implementation that duplicates what WorkerManagerPython + RPCClient already provide. WorkerManagerDocker and DockerModuleProxy re-implement the same host-side proxy pattern just to swap docker run for a plain subprocess. The right design (proven by NativeModule) is a thin subclass that overrides process launch — not a separate hierarchy.

Closes #2038. Related: #1266 (re-add as NativeModule subclass if/when needed).

Solution

Delete the parallel Docker hierarchy and hold the two real consumers:

  • Delete docker_module.py, worker_manager_docker.py, their tests, and examples/docker_hello_world/
  • Drop WorkerManagerDocker from ModuleCoordinator
  • graspgen_module.py: swap DockerModuleConfigModuleConfig, deployment = "python", remove docker-specific config fields
  • pick_and_place_module.py: remove DockerModuleProxy usage, stub generate_grasps with NotImplementedError pointing to Unify Docker Modules and Native Modules #1266

How to Test

python3 -c "from dimos.manipulation.grasping.graspgen_module import GraspGenModule; from dimos.manipulation.pick_and_place_module import PickAndPlaceModule; print('imports ok')"

Notes

demo_grasping.py still passes docker-specific kwargs to graspgen() — these are silently ignored at blueprint creation and only fail at deploy time. Left as-is since the demo cannot run regardless (graspgen libs were Docker-only). Will be cleaned up when Docker support is re-added in #1266.

Contributor License Agreement

  • I have read and approved the CLA.

@KrishnaH96 KrishnaH96 requested a review from mustafab0 May 22, 2026 19:23
@spomichter
Copy link
Copy Markdown
Contributor

legend

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 22, 2026

Greptile Summary

This PR removes the parallel Docker module hierarchy (docker_module.py, worker_manager_docker.py, graspgen_module.py) that duplicated the existing WorkerManagerPython + RPCClient pattern, and stubs out the consumers that depended on it.

  • Deleted: docker_module.py (693 lines), worker_manager_docker.py, graspgen_module.py, their tests, and the examples/docker_hello_world/ example — ~1,400 lines of duplicate infrastructure gone.
  • module_coordinator.py: WorkerManagerDocker removed from manager_types; only WorkerManagerPython remains.
  • pick_and_place_module.py: All GraspGen/Docker config fields, imports, and the Docker-backed generate_grasps implementation removed; method now raises NotImplementedError pointing to issue Unify Docker Modules and Native Modules #1266 for future re-implementation as a NativeModule subclass.

Confidence Score: 5/5

Safe to merge — this is a straightforward deletion of dead infrastructure with no remaining live callers.

Every changed file is either deleted outright or has its Docker/GraspGen references cleanly excised. The two real consumers (module_coordinator.py and pick_and_place_module.py) are updated consistently. No surviving code imports from the removed modules, and the pick skill's internal heuristic path (_generate_grasps_for_pick) is unaffected.

No files require special attention. The stale docstring in pick_and_place_module.py was flagged in the previous review and remains unaddressed, but it has no runtime impact.

Important Files Changed

Filename Overview
dimos/core/coordination/module_coordinator.py Removes WorkerManagerDocker import and drops it from manager_types, leaving only WorkerManagerPython — minimal, correct change.
dimos/core/docker_module.py Deleted entirely — 693-line parallel DockerModuleProxy/DockerModuleInner/DockerModuleConfig hierarchy.
dimos/manipulation/pick_and_place_module.py All Docker/GraspGen imports and config fields removed; generate_grasps stubbed with NotImplementedError pointing to #1266.
dimos/robot/all_blueprints.py demo-grasping and grasp-gen-module entries correctly removed from the auto-generated registry.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    subgraph BEFORE
        MC_OLD[ModuleCoordinator manager_types: WorkerManagerDocker + WorkerManagerPython]
        WMD[WorkerManagerDocker]
        DMP[DockerModuleProxy]
        GGM[GraspGenModule]
        PAP_OLD[PickAndPlaceModule _graspgen: DockerRunner]
        MC_OLD --> WMD --> DMP
        GGM --> DMP
        PAP_OLD --> GGM
    end
    subgraph AFTER
        MC_NEW[ModuleCoordinator manager_types: WorkerManagerPython only]
        WMP[WorkerManagerPython]
        PAP_NEW[PickAndPlaceModule generate_grasps: NotImplementedError see 1266]
        MC_NEW --> WMP
    end
    BEFORE -.->|PR 2224 deletes Docker hierarchy| AFTER
Loading

Reviews (4): Last reviewed commit: "fix: remove deleted graspgen entries fro..." | Re-trigger Greptile

@codecov
Copy link
Copy Markdown

codecov Bot commented May 22, 2026

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
dimos/manipulation/pick_and_place_module.py 0.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@@ -63,7 +57,7 @@ class GraspGenConfig(DockerModuleConfig):

class GraspGenModule(Module):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This needs docker module.

As per @paul-nechifor's suggestion, we can delete this module for now.

@KrishnaH96 KrishnaH96 force-pushed the fix/issue-2038-delete-docker-module branch from fcdd847 to b3377eb Compare May 23, 2026 20:56
@KrishnaH96 KrishnaH96 requested a review from mustafab0 May 23, 2026 21:46
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.

Delete docker_module — re-add as NativeModule subclass if/when needed

3 participants