Skip to content

Revert "Module config adjustments (#1413)"#1417

Merged
paul-nechifor merged 1 commit intodevfrom
revert-6e948f71d
Mar 5, 2026
Merged

Revert "Module config adjustments (#1413)"#1417
paul-nechifor merged 1 commit intodevfrom
revert-6e948f71d

Conversation

@mustafab0
Copy link
Contributor

This reverts commit 6e948f7.

Problem

unitree-go2 blueprint is broken

Solution

revret module config adjustments

Breaking Changes

None

How to Test

dimos --simulation run unitree-go2

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 5, 2026

Greptile Summary

This PR reverts commit 6e948f71 (PR #1413 "Module config adjustments") which broke the unitree-go2 blueprint by changing the module deployment API to pass robot ip and other constructor arguments as keyword args, while the GO2Connection.__init__ expected them as positional args under the new calling convention.

Key changes restored by this revert:

  • ModuleConfig reverted from Pydantic BaseModel back to a plain @dataclass; removes runtime Pydantic field validation
  • Module.__init__ / NativeModule.__init__ signature reverted from (global_config: GlobalConfig, **kwargs) to (*args, **kwargs) — constructor arguments flow through positionally
  • Deploy pipeline (ModuleCoordinator, WorkerManager, Worker) updated to pass *args tuples instead of an explicit GlobalConfig parameter
  • _deploy_all_modules now uses inspect.signature to detect if a module declares a cfg parameter, injecting global_config only when explicitly declared — this replaces the previous universal global_config injection
  • pyproject.toml re-enables ruff TC001/TC002/TC003 rules with per-site # noqa suppressions where runtime imports are intentional
  • Notable bug fixed: PR Module config adjustments #1413 introduced a no-op request["global_config"] expression statement in _worker_loop that read the dict value but never used it; this revert corrects it with proper args handling

Confidence Score: 4/5

  • This PR is safe to merge as an emergency revert; it restores working unitree-go2 and related robot blueprints at the cost of some type-safety reductions that are pre-existing or acknowledged with # type: ignore.
  • The revert is mechanically correct and directly addresses the broken blueprint. The deploy pipeline changes are internally consistent across ModuleCoordinator, WorkerManager, and Worker. One point deducted because the inspect.signature-based cfg injection in _deploy_all_modules is a new fragile pattern (not present in the original pre-Module config adjustments #1413 code) that could silently fail for future modules needing global_config, and NativeModule loses its generic type parameter reducing compile-time safety for hardware module subclasses.
  • dimos/core/blueprints.py (fragile cfg introspection logic in _deploy_all_modules) and dimos/hardware/sensors/lidar/livox/module.py (NativeModule non-generic typing for Mid360).

Important Files Changed

Filename Overview
dimos/core/module.py Reverts ModuleConfig from Pydantic BaseModel back to plain @dataclass; reverts Module.__init__ from (global_config, **kwargs) to (*args, **kwargs); removes _BlueprintPartial Protocol; changes ModuleSpec to ModuleT TypeVar — core module infrastructure revert is sound.
dimos/core/blueprints.py Switches _deploy_all_modules from explicit global_config param to inspect.signature-based cfg injection; adds args support to _BlueprintAtom; the introspection approach for passing global_config is brittle (see comment at line 298).
dimos/core/worker.py Fixes a no-op request["global_config"] expression bug from #1413; replaces explicit global_config parameter with args tuple in the worker deploy path — changes are correct and an improvement.
dimos/core/worker_manager.py Switches deploy and deploy_parallel from explicit GlobalConfig parameter to variadic *args/**kwargs; type annotations updated accordingly — clean and consistent with the broader revert.
dimos/core/module_coordinator.py Converts deploy from keyword-explicit global_config param to variadic *args/**kwargs; deploy_parallel updated to use (module_class, args_tuple, kwargs_dict) tuples — straightforward revert, no issues.
dimos/core/native_module.py Switches NativeModuleConfig from Pydantic to @dataclass(kw_only=True); replaces Pydantic Field(default_factory=...) with standard field(...); removes generic TypeVar — NativeModule is no longer generic, reducing type-checked config access for subclasses.
dimos/protocol/service/spec.py Removes Pydantic BaseConfig/BaseModel; ConfigT TypeVar is now unbounded; Configurable.__init__ is untyped (**kwargs) — reduces type safety but simplifies the config protocol back to dataclasses.
dimos/robot/unitree/go2/connection.py Changes dimos.deploy(GO2Connection, ip=ip) to dimos.deploy(GO2Connection, ip) — this is the root fix for the broken unitree-go2 blueprint; GO2Connection.__init__ accepts ip as the first positional arg, so the call is correct.
dimos/robot/unitree/g1/connection.py Mirrors the Go2 fix — dimos.deploy(G1Connection, ip=ip) becomes dimos.deploy(G1Connection, ip); consistent with how positional args now flow through the deploy pipeline.
dimos/robot/unitree/b1/connection.py Removes B1ConnectionConfig (Pydantic config class) and replaces it with explicit constructor params; MockB1ConnectionModule uses *args unpacking after keyword argument (type: ignore[misc] suppressed); functionally correct but mixing positional/keyword ordering is a minor footgun.
dimos/hardware/sensors/lidar/livox/module.py Mid360 no longer typed as NativeModule[Mid360Config]self.config field loses specific type information at the type-checker level (see comment); Mid360Config is still defined and used as default_config, so runtime behavior is unaffected.
dimos/hardware/sensors/camera/module.py CameraModule now explicitly declares cfg: GlobalConfig = global_config in its __init__, making it compatible with the inspect.signature-based cfg injection in _deploy_all_modules — correct approach.
pyproject.toml Re-enables ruff TC001/TC002/TC003 rules (TYPE_CHECKING import enforcement) by removing them from the ignore list; affected files add # noqa: TC001 where runtime use of the import is intentional — clean change.

Sequence Diagram

sequenceDiagram
    participant B as Blueprint / deploy()
    participant MC as ModuleCoordinator
    participant WM as WorkerManager
    participant W as Worker (subprocess)
    participant M as Module.__init__

    B->>MC: deploy(GO2Connection, ip) or<br/>deploy_parallel([(Module, args, kwargs)])
    MC->>WM: deploy(module_class, *args, **kwargs)
    WM->>W: deploy_module(module_class, args=(ip,), kwargs={})
    Note over W: _worker_loop receives msg<br/>{"type":"deploy_module", "args":(ip,), "kwargs":{}}
    W->>M: module_class(*args, **kwargs)<br/>→ GO2Connection(ip)
    M-->>W: instance
    W-->>WM: module_id
    WM-->>MC: RPCClient(actor, module_class)
    MC-->>B: ModuleProxy

    Note over B,MC: For Blueprint._deploy_all_modules:<br/>inspect.signature checks for "cfg" param<br/>→ injects global_config only if declared
Loading

Last reviewed commit: 9d8ee8f

Comment on lines 309 to 316

if req_type == "deploy_module":
module_class = request["module_class"]
request["global_config"]
args = request.get("args", ())
kwargs = request.get("kwargs", {})
module_id = request["module_id"]
instance = module_class(global_config, **kwargs)
instance = module_class(*args, **kwargs)
instances[module_id] = instance
Copy link
Contributor

Choose a reason for hiding this comment

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

Silent no-op expression fixed by this revert

The original PR #1413 introduced a subtle bug where request["global_config"] was used as a bare expression statement (its value was never assigned or used):

# Original (buggy) code from #1413:
module_class = request["module_class"]
request["global_config"]   # ← value read but never stored/used
kwargs = request.get("kwargs", {})
instance = module_class(global_config, **kwargs)

This revert correctly replaces it with the args tuple approach. Just noting this for posterity — the fix is correct in the reverted code.

Comment on lines 383 to +386
class MockB1ConnectionModule(B1ConnectionModule):
"""Test connection module that prints commands instead of sending UDP."""

def __init__(self, global_config: GlobalConfig = global_config, **kwargs: Any) -> None: # type: ignore[no-untyped-def]
def __init__(self, ip: str = "127.0.0.1", port: int = 9090, *args, **kwargs) -> None: # type: ignore[no-untyped-def]
Copy link
Contributor

Choose a reason for hiding this comment

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

*args after keyword params may cause unexpected ordering

The call site:

super().__init__(ip, port, test_mode=True, *args, **kwargs)  # type: ignore[misc]

*args is unpacked after a keyword argument (test_mode=True), which is valid in Python 3.5+ (PEP 448). However, the # type: ignore[misc] acknowledges mypy flags this. If *args ever contains values (e.g., if the caller passes extra positional args), they would be appended as extra positional arguments after test_mode, potentially landing in the parent's *args but with confusing ordering. Since B1ConnectionModule.__init__ only ever receives ip and port as positional args from deploy call sites, this is low-risk in practice, but the mixing of positional and keyword ordering is a footgun to document.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 5, 2026

Additional Comments (2)

dimos/core/blueprints.py
Fragile cfg detection via inspect.signature

_deploy_all_modules uses inspect.signature to decide whether to inject global_config as cfg:

sig = inspect.signature(blueprint.module.__init__)
if "cfg" in sig.parameters:
    kwargs["cfg"] = global_config

This will silently skip passing global_config to any module that:

  • Accepts the config through a parent class **kwargs without explicitly declaring cfg
  • Uses a different parameter name for the global config

For example, CameraModule.__init__ explicitly declares cfg: GlobalConfig = global_config so it works, but a future module that accepts global config through **kwargs will never receive it, with no error at deployment time. Consider adding a docstring or convention note to ModuleBase/Module clarifying that the cfg parameter name is required if a module needs the global config.


dimos/hardware/sensors/lidar/livox/module.py
NativeModule no longer generic — typed config access is lost for subclasses

NativeModule changed from Module[_NativeConfig] (generic over the config TypeVar) to Module[NativeModuleConfig] (concrete type). As a result, Mid360(NativeModule, ...) has self.config typed as NativeModuleConfig at the type-checker level, even though at runtime default_config = Mid360Config ensures the actual value is a Mid360Config.

Any access to Mid360Config-specific fields (e.g., self.config.host_ip, self.config.cmd_data_port) will not be type-checked and IDEs will not provide autocomplete for them. If any code path accesses these fields through self.config, it will generate a spurious type error at the call site or silently lose type safety.

Consider either re-introducing the generic TypeVar on NativeModule, or annotating self.config explicitly in each subclass (e.g., config: Mid360Config).

@paul-nechifor paul-nechifor merged commit 3df8857 into dev Mar 5, 2026
28 checks passed
aphexcx added a commit to aphexcx/dimos that referenced this pull request Mar 6, 2026
The Module config adjustments (dimensionalOS#1413) were reverted upstream (dimensionalOS#1417).
WavefrontFrontierExplorer has no Config dataclass, so self.config.*
would crash at runtime. Use direct instance attributes instead.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Dreamsorcerer pushed a commit that referenced this pull request Mar 6, 2026
paul-nechifor added a commit that referenced this pull request Mar 12, 2026
* Reapply "Module config adjustments (#1413)" (#1417)

This reverts commit 3df8857.

* Fixes

* Move global_config

* Why did this not commit?

* Default

* Fix

* Fix

* Fix

* Docs

* tentative fix for timeout error

* Fix

* Use create_autospec

* TemporalMemory fix

* Forbid extra

* Fix

---------

Co-authored-by: Sam Bull <Sam.B@snowfalltravel.com>
Co-authored-by: Paul Nechifor <paul@nechifor.net>
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