Skip to content

fix(blueprints): get module by name#1689

Open
paul-nechifor wants to merge 2 commits intodevfrom
paul/fix/module-names
Open

fix(blueprints): get module by name#1689
paul-nechifor wants to merge 2 commits intodevfrom
paul/fix/module-names

Conversation

@paul-nechifor
Copy link
Copy Markdown
Contributor

@paul-nechifor paul-nechifor commented Mar 26, 2026

Problem

In #1606 I broke the ability to get modules by name.

Closes DIM-XXX

Solution

  • Also specify the class name in all_modules.

Breaking Changes

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 26, 2026

Greptile Summary

This PR fixes a longstanding bug in get_module_by_name where the old implementation derived the attribute name from the CLI name via name.replace("-", "_"), yielding a snake_case string (e.g. b_box_navigation_module) that never matched the actual CamelCase class (e.g. BBoxNavigationModule). The result was that every get_module_by_name call would raise an AttributeError at runtime.\n\nThe fix is applied consistently across all three files:\n- all_blueprints.py — every all_modules value is updated from \"module.path\" to \"module.path.ClassName\".\n- get_all_blueprints.pyget_module_by_name now splits on the last . to extract the class name and calls .blueprint() on the class rather than instantiating it.\n- test_all_blueprints_generation.py — the scanner's _find_blueprints_in_file returns (snake_case_name, OriginalClassName) tuples so the auto-generator produces the new format in all_blueprints.py.\n\nOne thing worth noting: the old code called getattr(module, attr)() (instantiation), while the new code calls getattr(module, class_name).blueprint() (a class method). This is a behavioural change in addition to the naming fix — it should be correct assuming all Module subclasses expose a blueprint() class method, but there is no fallback or error message if a module class is missing that method.

Confidence Score: 4/5

Safe to merge — the fix is internally consistent and corrects a real runtime bug; the only open question is whether all Module subclasses expose .blueprint().

The three-file change is coherent: the registry format, the loader, and the generator all move in lock-step. The original code was provably broken (snake_case lookup against CamelCase class names). The new approach is correct. One point deducted because the PR description leaves "How to Test" empty and there is no explicit test covering the new .blueprint() call path.

dimos/robot/get_all_blueprints.py — the .blueprint() call assumes all registered Module subclasses expose that method; worth verifying with a small test.

Important Files Changed

Filename Overview
dimos/robot/all_blueprints.py Updated all_modules values from plain module paths to "module.path.ClassName" format so the loader can derive the correct class name at runtime.
dimos/robot/get_all_blueprints.py get_module_by_name fixed to split the stored path on the last "." to extract class_name, then call .blueprint() on the class instead of using the broken snake_case name derivation.
dimos/robot/test_all_blueprints_generation.py Scanner updated to return (snake_case_name, OriginalClassName) tuples from _find_blueprints_in_file so the generator writes the new "module.path.ClassName" format into all_blueprints.py.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant get_module_by_name
    participant all_modules
    participant Python Import

    Caller->>get_module_by_name: name = "b-box-navigation-module"
    get_module_by_name->>all_modules: lookup name
    all_modules-->>get_module_by_name: "dimos.navigation.bbox_navigation.BBoxNavigationModule"
    get_module_by_name->>get_module_by_name: rsplit(".", 1) → module_path, class_name
    get_module_by_name->>Python Import: __import__(module_path, fromlist=[class_name])
    Python Import-->>get_module_by_name: python_module
    get_module_by_name->>python_module: getattr(python_module, class_name).blueprint()
    python_module-->>Caller: Blueprint
Loading

Reviews (1): Last reviewed commit: "fix(blueprints): get module by name" | Re-trigger Greptile

@jeff-hykin
Copy link
Copy Markdown
Member

jeff-hykin commented Mar 27, 2026

does this change mean we can do something like

dimos run CameraModule

instead of dimos run camera-module (which needed camera_module to exist)

@paul-nechifor
Copy link
Copy Markdown
Contributor Author

does this change mean we can do something like

dimos run CameraModule

instead of dimos run camera-module (which needed camera_module to exist)

No. It just means camera-module is a blueprint for CameraModule.blueprint() like before.

def _find_blueprints_in_file(
file_path: Path, module_classes: set[str] | None = None
) -> tuple[list[str], list[str]]:
) -> tuple[list[str], list[tuple[str, str]]]:
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.

We could use NamedTuple for code readability.

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.

Looks alright to me. Can't find any issues

@paul-nechifor paul-nechifor enabled auto-merge (squash) March 28, 2026 00:31
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.

3 participants