ModuleCoordinator & Blueprint.py changes

[jeff-hykin]

Draft b/c: contains misc, and isn't up to date with dev

Problem

Closes DIM-XXX

Solution

Breaking Changes

How to Test

Contributor License Agreement

• I have read and approved the CLA.

[greptile-apps]

Greptile Summary

This PR is a significant architectural refactor of the DimOS module deployment pipeline. It consolidates Docker tooling (docker_build.py + docker_runner.py → docker_module.py), splits the monolithic WorkerManager into WorkerManagerDocker and WorkerManagerPython, and introduces a clean compile-then-execute pattern in Blueprint.build() via the new DeploySpec intermediate representation. New utilities (ThreadSafeVal, ModuleThread, AsyncModuleThread, ModuleProcess, safe_thread_map) provide well-tested thread-safety primitives used throughout the new managers.

Key changes and concerns:

• P1 — FieldInfo instead of dict in RPCClient.__init__: getattr(default_config, "rpc_timeouts", ...) where default_config is a Pydantic model class returns a FieldInfo descriptor, not the factory default. This will cause dict(FieldInfo) to fail inside PubSubRPCMixin.__init__ for every Python-worker module. Acknowledged in changes.md as "not addressed".
• P1 — g=global_config forwarded into Docker module config: Both WorkerManagerDocker.deploy() and deploy_parallel() pass g=GlobalConfig() as a kwarg to DockerModuleOuter, which then passes all kwargs to config_class(**kwargs). Unless BaseConfig silently ignores unknown fields, this is a Pydantic ValidationError for every Docker module deployment.
• P2 — Build hash misses DimOS footer: _compute_build_hash hashes the raw Dockerfile but build_image builds the DimOS-converted file. Footer / module-install.sh changes won't trigger a rebuild.
• P2 — Undocumented breaking defaults: docker_gpus changed from "all" to None and docker_restart_policy from "on-failure:3" to "no" with no migration guidance in the PR description.
• P2 — changes.md committed: Developer scratch notes ("Reviewer was wrong on", "Not addressed") should not be part of the repository history.

Confidence Score: 4/5

• Not safe to merge as-is — two P1 defects will break RPCClient construction for every module and Docker module deployment.
• The architectural direction is sound and the thread utilities are excellent, but the FieldInfo/Pydantic class-vs-instance issue in rpc_client.py and the spurious g=global_config forwarding in worker_manager_docker.py are active runtime defects that would surface immediately in any end-to-end test exercising Python workers or Docker modules respectively.
• dimos/core/rpc_client.py (FieldInfo bug) and dimos/core/worker_manager_docker.py (g=global_config forwarding) require fixes before merge.

Important Files Changed

Filename	Overview
dimos/core/rpc_client.py	Adds ModuleProxyProtocol, per-module RPC timeout support, and a build() stub on ModuleProxy; but RPCClient.__init__ reads rpc_timeouts from a Pydantic model class (returns FieldInfo, not a dict), which will break at runtime.
dimos/core/worker_manager_docker.py	New manager for Docker-backed modules; deploy() and deploy_parallel() both pass g=global_config into DockerModuleOuter.__init__, which forwards it to config_class(**kwargs) — a Pydantic validation error for any module with strict extra-field handling.
dimos/core/docker_module.py	Merges docker_build.py into docker_runner.py and renames to docker_module.py; adds content-hash rebuild detection, reconnect mode, DockerModuleOuter/DockerModuleInner split, and __main__ module-path resolution; breaking defaults for docker_gpus and docker_restart_policy undocumented; build hash omits DimOS footer.
dimos/core/blueprints.py	Refactors Blueprint to a compile-then-execute pattern with DeploySpec; logic correctness preserved via equivalent if-elif chains; _compile_rpc_wiring now stores method names instead of callables (deferred to execution phase).
dimos/core/module_coordinator.py	Split WorkerManager into WorkerManagerDocker + WorkerManagerPython; adds _build_all_modules() lifecycle step; raises ValueError for empty deployments which may be overly strict; stop() now suppresses exceptions per-module, logging them instead.
dimos/core/worker_manager_python.py	New file: replaces WorkerManager with dynamic worker scaling (worker_to_module_ratio), proper cleanup on partial deploy failure, and health_check delegated to worker PIDs; looks correct.
dimos/utils/thread_utils.py	New file: introduces ThreadSafeVal (RLock-backed), ModuleThread, AsyncModuleThread, ModuleProcess, and safe_thread_map; well-designed with RLock re-entrancy fix, idempotent stops, and comprehensive test coverage.
dimos/protocol/rpc/spec.py	Extracts DEFAULT_RPC_TIMEOUT/DEFAULT_RPC_TIMEOUTS constants; refactors call_sync timeout resolution to consult per-method rpc_timeouts map (full topic name, then bare method name fallback); protocol updated accordingly.
dimos/core/module.py	Adds build() RPC method (24h timeout default), per-module RPC timeout fields on ModuleConfig, and configurable loop-thread join timeout; removes configure_stream in favour of the unified set_transport path.
dimos/utils/typing_utils.py	New file: polyfills ExceptionGroup for Python < 3.11 and TypeVar for Python < 3.13; clean and correct.
changes.md	Developer working notes file; should not be committed to the repository.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["Blueprint.build()"] --> B["Phase 1: Config\nglobal_config.update()"]
    B --> C["Phase 2: Validate\n_check_requirements()"]
    C --> D["Phase 3: Compile\nDeploySpec (pure)"]
    D --> D1["_compile_module_specs()"]
    D --> D2["_compile_stream_wiring()"]
    D --> D3["_compile_rpc_wiring()"]
    D --> D4["_compile_module_ref_wiring()"]
    D1 & D2 & D3 & D4 --> E["Phase 4: Execute\nModuleCoordinator(deploy_spec)"]
    E --> F["deploy_parallel()"]
    F --> G{Docker module?}
    G -- Yes --> H["WorkerManagerDocker\n.deploy_parallel()"]
    G -- No --> I["WorkerManagerPython\n.deploy_parallel()"]
    H --> J["DockerModuleOuter.build()\n(pull/build image, start container,\nwait for RPC)"]
    I --> K["Worker.deploy_module()\n+ RPCClient"]
    J & K --> L["_wire_streams()"]
    L --> M["_wire_rpc_methods()"]
    M --> N["_wire_module_refs()"]
    N --> O["_build_all_modules()\n(calls build() RPC on each)"]
    O --> P["start_all_modules()\n(calls start() RPC on each)"]

Loading

Comments Outside Diff (2)

1. dimos/core/rpc_client.py, line 1808-1811 (link)

   FieldInfo returned instead of dict for class-level Pydantic access

   default_config here is a Pydantic model class (e.g. ModuleConfig), not an instance. In Pydantic v2, accessing a field with default_factory at the class level returns the FieldInfo descriptor object, not the factory's result. The getattr(default_config, "rpc_timeouts", dict(DEFAULT_RPC_TIMEOUTS)) fallback only fires when the attribute is absent, but rpc_timeouts is defined on ModuleConfig, so it will always be present as a FieldInfo. Passing that FieldInfo to LCMRPC(rpc_timeouts=FieldInfo(...)) and then calling dict(FieldInfo) inside PubSubRPCMixin.__init__ will raise a TypeError.

   This is acknowledged in changes.md under "Not addressed", but it will break RPCClient construction for any module that uses the default ModuleConfig.

   A safe fix is to instantiate a temporary config object to read the defaults:

   default_config_cls = getattr(actor_class, "default_config", None)
   try:
       _tmp = default_config_cls() if default_config_cls is not None else None
   except Exception:
       _tmp = None
   self.rpc = LCMRPC(
       rpc_timeouts=getattr(_tmp, "rpc_timeouts", dict(DEFAULT_RPC_TIMEOUTS)),
       default_rpc_timeout=getattr(_tmp, "default_rpc_timeout", DEFAULT_RPC_TIMEOUT),
   )

2. dimos/core/docker_module.py, line 1214-1224 (link)

   Build hash covers original Dockerfile, not the DimOS-converted one

   _compute_build_hash hashes cfg.docker_file.read_bytes() — the raw, unmodified Dockerfile — but build_image actually builds from the converted file that includes DIMOS_FOOTER. This means the stored hash will never reflect changes to DIMOS_FOOTER, docker/python/module-install.sh, or the DimOS source tree itself. An updated DimOS version would not trigger a rebuild even though the resulting image would be different.

   changes.md acknowledges this under "Not addressed". At minimum the hash should include the DIMOS_FOOTER string so footer changes force a rebuild. Ideally it would also hash module-install.sh and the DimOS source, but that's a bigger change.

_{Reviews (1): Last reviewed commit: "split ModuleCoordinator from Blueprint" | Re-trigger Greptile}

[greptile-apps]

g=global_config leaks into Docker module config constructor

DockerModuleOuter.__init__ accepts **kwargs and passes them all directly to config_class(**kwargs). So this call adds g=GlobalConfig(...) to the kwargs dict that becomes arguments to DockerModuleConfig(...) (or a subclass). Unless BaseConfig / ModuleConfig is configured with model_config = ConfigDict(extra='ignore'), Pydantic will raise a ValidationError for the unknown g field — making every Docker module deployment fail.

The # type: ignore[arg-type] annotation hints the author is aware of a typing mismatch, but the runtime failure risk is real. The global_config argument does not belong in the Docker module's config; it should be used only to configure the manager itself (as WorkerManagerPython does).

# In deploy(), use config only to route; don't forward g= into the module config
mod = DockerModuleOuter(module_class, **kwargs)  # type: ignore[arg-type]

[greptile-apps]

Same g=global_config issue in deploy_parallel

Same problem as in deploy() — g=spec[1] (which is a GlobalConfig) is forwarded into DockerModuleOuter.__init__ and ultimately into config_class(g=GlobalConfig(), ...).

mod = DockerModuleOuter(spec[0], **spec[2])  # type: ignore[arg-type]

[greptile-apps]

Silent breaking defaults for GPU and restart policy

Two defaults have changed relative to the previous release:

• docker_gpus: "all" → None — disables GPU passthrough
• docker_restart_policy: "on-failure:3" → "no" — containers no longer restart on crash

Both are acknowledged in changes.md as "intentional breaking change?" with no migration guidance. Users upgrading who relied on GPU access or automatic restarts will silently lose those behaviours. The PR description's Breaking Changes section says only "None".

These should either be documented as breaking changes or preserved as opt-in/opt-out flags with a deprecation path.

[greptile-apps]

Developer scratch notes committed to the repository

changes.md appears to be a personal working notes file ("Commits (local, not pushed)", "Reviewer was wrong on", "Not addressed (need Jeff's input)"). Committing this kind of internal review dialogue to the repo history adds noise and may inadvertently expose in-flight design discussions. Consider removing it from the PR and keeping it in a local branch note, PR description body, or Jira comment instead.