Skip to content

Auto-fixes for jeff/feat/dimos-graph#2358

Open
paul-nechifor wants to merge 14 commits into
jeff/feat/dimos-graphfrom
jeff/feat/dimos-graph-autofixes
Open

Auto-fixes for jeff/feat/dimos-graph#2358
paul-nechifor wants to merge 14 commits into
jeff/feat/dimos-graphfrom
jeff/feat/dimos-graph-autofixes

Conversation

@paul-nechifor

Copy link
Copy Markdown
Contributor

These are automated fixes. Each fix is a separate commit. Use git rebase -i to drop any you disagree with.

DEFAULT_IGNORED_CONNECTIONS/DEFAULT_IGNORED_MODULES were empty
module-level mutable sets used only as None-fallback defaults. Assign
set() directly in the None branches instead.
render_mermaid reached into _ColorAssigner._assigned from outside the
object. Expose an 'assigned' property that returns a copy and use it.
The theme dict has fixed keys with mixed value types (background and
mermaid_theme are str; nodes and edges are list[str]). A TypedDict types
it precisely and drops the Any, letting graph.py read keys directly.
The valid color themes are fixed (THEMES keys) and were restated in a
CLI help string. Define ThemeName once as a Literal and use it for THEMES,
DEFAULT_THEME, render_mermaid, the graph.py helpers, and the typer option
so typer generates the choice list and validates input. With the Literal,
THEMES lookups become direct instead of silently defaulting on a bad name.

ThemeName lives in the lightweight introspection.utils so the dimos CLI
can use it as an option type without importing the heavy mermaid module
at startup (mirrors RerunOpenOption in rerun/constants).
render_mermaid returned a 4-tuple unpacked positionally by callers, which
is easy to misorder. Return a MermaidRender NamedTuple and access fields by
name in graph.py.
The conflict/typo dicts hold mixed value types (str plus lists), so use
dict[str, Any] (consistent with per_bp_conflicts). Likewise the
log_message override takes *args: Any.
Producer-conflict detection and the levenshtein-based stream-typo
detection are blueprint-graph analysis, not HTML/serving concerns. Move
them (and _levenshtein) next to render_mermaid as find_producer_conflicts
and find_stream_typos, returning structured findings. graph.py keeps only
the color decoration for the template. Output is unchanged.
Importing graph.py ran get_data('mermaid.min.js') (an LFS pull that
blocks) and read/compiled the jinja template at module import. Move both
behind functools.lru_cache loaders called from _build_html, and resolve
the JS via LfsPath so the download happens only when the HTML is built.
The HTML contains non-ASCII characters and the corresponding reads already
pass encoding='utf-8'.
serve_graph created an HTTPServer and called handle_request() but never
server_close(). Close it in a finally so the socket is released.
Use 'str | None' defaulting to None and check 'is not None' rather than
the empty-string sentinel with a truthiness check.
…prints

Neither was used in this blueprint-fixture module.
The data/module classes and _build_blueprint duplicated the fixtures in
test_graph_blueprints.py (which the test loads from disk) and were never
used; their In/Out/Module/Blueprint/autoconnect imports go with them.
@greptile-apps

greptile-apps Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This automated cleanup PR refactors the Blueprint graph visualization tooling on the jeff/feat/dimos-graph branch: it extracts find_producer_conflicts and find_stream_typos from the ad-hoc inline code in graph.py into well-typed NamedTuple-backed functions in mermaid.py, replaces the raw 4-tuple return of render_mermaid with a MermaidRender NamedTuple, and fixes a socket leak in serve_graph with a finally: server.server_close() guard.

  • MermaidRender NamedTuple replaces the previously untyped tuple[str, dict, set, dict] return value, making call-sites self-documenting via named fields.
  • ThemeName Literal alias in utils.py gives typer proper enum-style validation on the --theme CLI option.
  • --html sentinel fix changes the default from \"\" to None, eliminating the subtle case where --html \"\" would silently fall through to serve_graph.
  • Blueprint fixtures moved to test_graph_blueprints.py so they can be loaded as a real Python file by _load_blueprints.

Confidence Score: 5/5

All changes are mechanical refactors with no altered business logic; the one behavioural fix (socket cleanup) reduces a resource leak. Safe to merge.

Every changed code path is either a type-annotation tightening, a straight extraction of inline logic into a named function with identical semantics, or a genuine small fix (server_close, UTF-8 encoding, None sentinel). No logic is changed in the conflict/typo detection, the Mermaid renderer, or the HTTP serving path.

No files require special attention. test_graph.py retains snapshot testing and HTTP round-trip coverage despite the blueprint definitions moving out of the file.

Important Files Changed

Filename Overview
dimos/core/introspection/mermaid.py Introduces MermaidRender NamedTuple, ProducerConflict/StreamTypo NamedTuples, and moves find_producer_conflicts/find_stream_typos here from graph.py; also adds _ColorAssigner.assigned property.
dimos/core/introspection/utils.py Adds ThemeName TypeAlias as a Literal type for the 5 available themes, kept in a lightweight module to avoid heavy imports in CLI entry-points.
dimos/robot/cli/dimos.py Changes --html option from empty-string sentinel to None, fixes if-html check to use is not None, and tightens --theme type to ThemeName for typer validation.
dimos/utils/cli/graph.py Lazy-loads template and mermaid.js via lru_cache, removes inline Levenshtein and conflict-detection code, adds server_close() in finally block, and adds UTF-8 encoding to file write.
dimos/utils/cli/test_graph.py Strips the inline blueprint/module definitions and delegates to test_graph_blueprints.py; retains snapshot and HTTP round-trip test.
dimos/utils/cli/test_graph_blueprints.py Receives module/blueprint definitions moved from test_graph.py, adds intentional typo and duplicate producers across three named blueprints.

Sequence Diagram

sequenceDiagram
    participant CLI as dimos CLI (dimos.py)
    participant Graph as graph.py
    participant Mermaid as mermaid.py
    participant Template as graph.html.jinja

    CLI->>Graph: serve_graph / save_html / print_markdown (ThemeName)
    Graph->>Graph: _load_blueprints(python_file)
    Graph->>Mermaid: render_mermaid(bp, theme) → MermaidRender
    Mermaid-->>Graph: MermaidRender(code, label_colors, disconnected, node_colors)
    Graph->>Mermaid: find_producer_conflicts(bp) → list[ProducerConflict]
    Mermaid-->>Graph: list[ProducerConflict]
    Graph->>Mermaid: find_stream_typos(bp) → list[StreamTypo]
    Mermaid-->>Graph: list[StreamTypo]
    Graph->>Graph: _load_template() [lru_cache]
    Graph->>Graph: _load_mermaid_js() [lru_cache]
    Graph->>Template: render(mermaid_code, conflicts, typos, ...)
    Template-->>Graph: HTML string
    Graph-->>CLI: HTML / markdown / file
Loading

Reviews (2): Last reviewed commit: "chore: remove dead code in test_graph.py" | Re-trigger Greptile

Comment thread dimos/utils/cli/test_graph.py Outdated
Comment thread dimos/utils/cli/graph.py
@codecov

codecov Bot commented Jun 4, 2026

Copy link
Copy Markdown

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
1824 1 1823 151
View the top 1 failed test(s) by shortest run time
dimos.robot.test_all_blueprints_generation::test_all_blueprints_is_current
Stack Traces | 2.67s run time
def test_all_blueprints_is_current() -> None:
        root = DIMOS_PROJECT_ROOT / "dimos"
        all_blueprints, all_modules = _scan_for_blueprints(root)
    
        common = set(all_blueprints.keys()) & set(all_modules.keys())
        assert not common, (
            f"Names must be unique across blueprints and modules, "
            f"but these appear in both: {sorted(common)}"
        )
    
        generated_content = _generate_all_blueprints_content(all_blueprints, all_modules)
    
        file_path = root / "robot" / "all_blueprints.py"
    
        if "CI" in os.environ:
            if not file_path.exists():
                pytest.fail(f"all_blueprints.py does not exist at {file_path}")
    
            current_content = file_path.read_text()
            if current_content != generated_content:
                diff = difflib.unified_diff(
                    current_content.splitlines(keepends=True),
                    generated_content.splitlines(keepends=True),
                    fromfile="all_blueprints.py (current)",
                    tofile="all_blueprints.py (generated)",
                )
                diff_str = "".join(diff)
>               pytest.fail(
                    f"all_blueprints.py is out of date. Run "
                    f"`pytest dimos/robot/test_all_blueprints_generation.py` locally to update.\n\n"
                    f"Diff:\n{diff_str}"
                )
E               Failed: all_blueprints.py is out of date. Run `pytest dimos/robot/test_all_blueprints_generation.py` locally to update.
E               
E               Diff:
E               --- all_blueprints.py (current)
E               +++ all_blueprints.py (generated)
E               @@ -131,10 +131,12 @@
E                    "arm-teleop-module": "dimos.teleop.quest.quest_extensions.ArmTeleopModule",
E                    "b-box-navigation-module": "dimos.navigation.bbox_navigation.BBoxNavigationModule",
E                    "b1-connection-module": "dimos.robot.unitree.b1.connection.B1ConnectionModule",
E               -    "camera-module": "dimos.hardware.sensors.camera.module.CameraModule",
E               +    "camera-module": "dimos.utils.cli.graph_blueprints.CameraModule",
E                    "cartesian-motion-controller": "dimos.manipulation.control.servo_control.cartesian_motion_controller.CartesianMotionController",
E                    "click-start-goal-router": "dimos.navigation.nav_stack.modules.click_start_goal_router.click_start_goal_router.ClickStartGoalRouter",
E                    "control-coordinator": "dimos.control.coordinator.ControlCoordinator",
E               +    "controller-module": "dimos.utils.cli.graph_blueprints.ControllerModule",
E               +    "controller-module2": "dimos.utils.cli.graph_blueprints.ControllerModule2",
E                    "cost-mapper": "dimos.mapping.costmapper.CostMapper",
E                    "demo-calculator-skill": "dimos.agents.skills.demo_calculator_skill.DemoCalculatorSkill",
E                    "demo-monitoring": "dimos.agents.demos.demo_capabilities.DemoMonitoring",
E               @@ -190,15 +192,19 @@
E                    "object-tracker2-d": "dimos.perception.object_tracker_2d.ObjectTracker2D",
E                    "object-tracker3-d": "dimos.perception.object_tracker_3d.ObjectTracker3D",
E                    "object-tracking": "dimos.perception.object_tracker.ObjectTracking",
E               +    "odometry-module": "dimos.utils.cli.graph_blueprints.OdometryModule",
E                    "osm-skill": "dimos.agents.skills.osm.OsmSkill",
E                    "path-follower": "dimos.navigation.nav_stack.modules.path_follower.path_follower.PathFollower",
E                    "patrolling-module": "dimos.navigation.patrolling.module.PatrollingModule",
E                    "perceive-loop-skill": "dimos.perception.perceive_loop_skill.PerceiveLoopSkill",
E               +    "perception-module": "dimos.utils.cli.graph_blueprints.PerceptionModule",
E                    "person-follow-skill-container": "dimos.agents.skills.person_follow.PersonFollowSkillContainer",
E                    "person-tracker": "dimos.perception.detection.person_tracker.PersonTracker",
E                    "pgo": "dimos.navigation.nav_stack.modules.pgo.pgo.PGO",
E                    "phone-teleop-module": "dimos.teleop.phone.phone_teleop_module.PhoneTeleopModule",
E                    "pick-and-place-module": "dimos.manipulation.pick_and_place_module.PickAndPlaceModule",
E               +    "planner-module": "dimos.utils.cli.graph_blueprints.PlannerModule",
E               +    "planner-module2": "dimos.utils.cli.graph_blueprints.PlannerModule2",
E                    "quest-teleop-module": "dimos.teleop.quest.quest_teleop_module.QuestTeleopModule",
E                    "ray-tracing-voxel-map": "dimos.mapping.ray_tracing.module.RayTracingVoxelMap",
E                    "real-sense-camera": "dimos.hardware.sensors.camera.realsense.camera.RealSenseCamera",
E               @@ -224,6 +230,7 @@
E                    "unitree-skill-container": "dimos.robot.unitree.unitree_skill_container.UnitreeSkillContainer",
E                    "unity-bridge-module": "dimos.simulation.unity.module.UnityBridgeModule",
E                    "video-arm-teleop-module": "dimos.teleop.quest.quest_extensions.VideoArmTeleopModule",
E               +    "visualizer-module": "dimos.utils.cli.graph_blueprints.VisualizerModule",
E                    "vlm-agent": "dimos.agents.vlm_agent.VLMAgent",
E                    "vlm-stream-tester": "dimos.agents.vlm_stream_tester.VlmStreamTester",
E                    "voxel-grid-mapper": "dimos.mapping.voxels.VoxelGridMapper",

all_blueprints = {'alfred-nav': 'dimos.robot.diy.alfred.blueprints.alfred_nav:alfred_nav', 'coordinator-basic': 'dimos.control.blueprin...sian_ik_mock', 'coordinator-cartesian-ik-piper': 'dimos.control.blueprints.teleop:coordinator_cartesian_ik_piper', ...}
all_modules = {'alfred-high-level': 'dimos.robot.diy.alfred.effector_high_level.AlfredHighLevel', 'arm-teleop-module': 'dimos.teleop..._navigation.BBoxNavigationModule', 'b1-connection-module': 'dimos.robot.unitree.b1.connection.B1ConnectionModule', ...}
common     = set()
current_content = '# Copyright 2025-2026 Dimensional Inc.\n#\n# Licensed under the Apache License, Version 2.0 (the "License");\n# you m...ebsocket_vis_module.WebsocketVisModule",\n    "zed-camera": "dimos.hardware.sensors.camera.zed.camera.ZEDCamera",\n}\n'
diff       = <generator object unified_diff at 0xff132381df20>
diff_str   = '--- all_blueprints.py (current)\n+++ all_blueprints.py (generated)\n@@ -131,10 +131,12 @@\n     "arm-teleop-module": ..."dimos.agents.vlm_stream_tester.VlmStreamTester",\n     "voxel-grid-mapper": "dimos.mapping.voxels.VoxelGridMapper",\n'
file_path  = PosixPath('.../dimos/robot/all_blueprints.py')
generated_content = '# Copyright 2025-2026 Dimensional Inc.\n#\n# Licensed under the Apache License, Version 2.0 (the "License");\n# you m...ebsocket_vis_module.WebsocketVisModule",\n    "zed-camera": "dimos.hardware.sensors.camera.zed.camera.ZEDCamera",\n}\n'
root       = PosixPath('.../dimos/dimos/dimos')

dimos/robot/test_all_blueprints_generation.py:66: Failed

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

@paul-nechifor paul-nechifor force-pushed the jeff/feat/dimos-graph-autofixes branch 2 times, most recently from cc1c060 to b5f87f6 Compare June 4, 2026 18:11
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