Skip to content

fix: dimos topic send#2760

Open
paul-nechifor wants to merge 2 commits into
mainfrom
paul/fix/init-bug
Open

fix: dimos topic send#2760
paul-nechifor wants to merge 2 commits into
mainfrom
paul/fix/init-bug

Conversation

@paul-nechifor

Copy link
Copy Markdown
Contributor

dimos topic send has been broken for certain messages since I removed __init__.py files.

Closes DIM-1124

@greptile-apps

greptile-apps Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes dimos topic send for namespace-package message modules. The main changes are:

  • Message classes are discovered by walking message submodules.
  • topic_send builds its eval context from discovered message classes.
  • Tests cover eval-context discovery and LCM delivery for a Twist message.

Confidence Score: 4/5

This is close, but the import failure path should be fixed before merging.

  • Happy-path message discovery now works for namespace packages.
  • Discovered message modules with missing import-time dependencies are still skipped silently.
  • Affected send commands can still fail with a misleading missing-name error.

dimos/robot/cli/topic.py

Important Files Changed

Filename Overview
dimos/robot/cli/topic.py Adds namespace-package message discovery for topic_send, but still suppresses import-time failures from discovered message modules.
dimos/robot/cli/test_topic.py Adds tests for eval-context discovery and LCM delivery on the happy path.

Reviews (2): Last reviewed commit: "Merge branch 'main' into paul/fix/init-b..." | Re-trigger Greptile

Comment thread dimos/robot/cli/topic.py
Comment on lines +146 to +147
except ImportError:
continue

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.

P1 Message Imports Disappear Silently

When a discovered message module has a missing import-time dependency, this branch skips it and removes that message class from the eval context. A valid command such as sending a class from that module then fails later with NameError instead of reporting the module import failure, so dimos topic send remains broken for those message types.

@codecov

codecov Bot commented Jul 7, 2026

Copy link
Copy Markdown

❌ 2 Tests Failed:

Tests completed Failed Passed Skipped
2690 2 2688 71
View the top 1 failed test(s) by shortest run time
dimos.navigation.cmu_nav.modules.far_planner.test_far_planner_rosbag.TestFarPlannerRosbag::test_waypoint_accuracy
Stack Traces | 73.9s run time
self = <dimos.navigation.cmu_nav.modules.far_planner.test_far_planner_rosbag.TestFarPlannerRosbag object at 0x7755d4cda4e0>

    def test_waypoint_accuracy(self) -> None:
        """Feed identical inputs at original timing and compare waypoint output."""
        if not FAR_PLANNER_BIN.exists():
            pytest.skip(f"FAR planner binary not found: {FAR_PLANNER_BIN}")
    
        window = load_rosbag_window()
        ref_wp = window.way_point
        assert len(ref_wp) > 0, "No reference waypoints in fixture"
    
        lcm = lcmlib.LCM()
        wp_collector = LcmCollector(topic=WAYPOINT_OUT_LCM, msg_type=PointStamped)
        wp_collector.start(lcm)
    
        stop_event = threading.Event()
        handle_thread = threading.Thread(
            target=lcm_handle_loop, args=(lcm, stop_event), daemon=True
        )
        handle_thread.start()
    
        runner = NativeProcessRunner(binary_path=str(FAR_PLANNER_BIN), args=_far_planner_args())
    
        try:
            runner.start()
            assert runner.is_running, "FAR planner binary failed to start"
            time.sleep(_PROCESS_STARTUP_SEC)
    
            # Feed at original timing (1:1 with rosbag)
            feed_at_original_timing(
                lcm,
                window,
                topic_map={
                    "odom": ODOM_LCM,
                    "scan": SCAN_LCM,
                    "terrain": TERRAIN_LCM,
                    "terrain_ext": TERRAIN_EXT_LCM,
                    "goal": GOAL_LCM,
                },
            )
    
            time.sleep(_POST_FEED_DRAIN_SEC)
    
        finally:
            runner.stop()
            stop_event.set()
            handle_thread.join(timeout=DEFAULT_THREAD_JOIN_TIMEOUT)
            wp_collector.stop(lcm)
    
        our_wps = [(msg.x, msg.y) for msg in wp_collector.messages]
    
        # Compute deviation score
        score = _compute_waypoint_deviation(our_wps, ref_wp)
    
        # Log score for visibility
        logger.info(f"\n{'=' * 60}")
        logger.info("FAR PLANNER DEVIATION SCORE")
        logger.info(f"  Our waypoints:     {len(our_wps)}")
        logger.info(f"  Reference:         {len(ref_wp)}")
        logger.info(f"  Count ratio:       {score['count_ratio']:.3f}")
        logger.info(f"  Mean error:        {score['mean_error_m']:.3f} m")
        logger.info(f"  Max error:         {score['max_error_m']:.3f} m")
        logger.info(f"  Mean X diff:       {score['mean_x_diff']:.3f} m")
        logger.info(f"  Mean Y diff:       {score['mean_y_diff']:.3f} m")
        logger.info(f"{'=' * 60}\n")
    
        # Assertions — generous thresholds, the point is to measure
        assert len(our_wps) > 0, "FAR planner produced no waypoints"
>       assert score["mean_error_m"] < 5.0, (
            f"Mean waypoint error {score['mean_error_m']:.2f}m exceeds 5m threshold"
        )
E       AssertionError: Mean waypoint error 5.17m exceeds 5m threshold
E       assert 5.1664396514849225 < 5.0

handle_thread = <Thread(Thread-157 (lcm_handle_loop), stopped daemon 131209660180160)>
lcm        = <LCM object at 0x7755e1099920>
our_wps    = [(6.297216415405273, -3.322401762008667), (6.306988716125488, -3.3080596923828125), (6.3008599281311035, -3.2252717018...81311035, -3.225271701812744), (6.3008599281311035, -3.225271701812744), (6.3008599281311035, -3.225271701812744), ...]
ref_wp     = array([[     5.5327,        -0.3,         2.5,        0.75],
       [     5.7342,        -0.3,         2.5,        0.7...,      7.8589,      1.4869,    0.095978],
       [     65.337,      7.8589,      1.4869,    0.095978]], shape=(300, 4))
runner     = NativeProcessRunner(binary_path='.../result/bin/far_planner_...hred', '4', '--new_point_counter', '5', '--obs_inflate_size', '1', '--visualize_ratio', '0.4', '--wp_churn_dist', '0'])
score      = {'count_ratio': 0.55, 'max_error_m': 8.713269220479598, 'mean_error_m': 5.1664396514849225, 'mean_x_diff': 4.151850025653839, ...}
self       = <dimos.navigation.cmu_nav.modules.far_planner.test_far_planner_rosbag.TestFarPlannerRosbag object at 0x7755d4cda4e0>
stop_event = <threading.Event at 0x7755d797dcd0: set>
window     = RosbagWindow(odom=array([[     5.3674,           0,           0, ...,           0,           0,           1],
       [...,           1],
       [     0.2975,           0,           0,           0,           0,           0,           1]]))])
wp_collector = LcmCollector(topic='/rbfp_wp#geometry_msgs.PointStamped', msg_type=<class 'dimos.msgs.geometry_msgs.PointStamped.Point...1417, 5877690.999504224, 5877691.196800777, 5877691.398209056, 5877691.59548678, 5877691.794883912, 5877691.994291641])

.../modules/far_planner/test_far_planner_rosbag.py:276: AssertionError
View the full list of 1 ❄️ flaky test(s)
dimos.perception.test_image_embedding.TestImageEmbedding::test_clip_embedding_initialization

Flake rate in main: 30.00% (Passed 7 times, Failed 3 times)

Stack Traces | 26.3s run time
request = <SubRequest 'monitor_threads' for <Function test_clip_embedding_initialization>>

    @pytest.fixture(autouse=True)
    def monitor_threads(request):
        # Capture threads before test runs
        test_name = request.node.nodeid
        with _seen_threads_lock:
            _before_test_threads[test_name] = {
                t.ident for t in threading.enumerate() if t.ident is not None
            }
    
        yield
    
        with _seen_threads_lock:
            before = _before_test_threads.get(test_name, set())
    
        # Threads intentionally left running for the whole process and cleaned up on
        # exit, so they don't count as per-test leaks.
        expected_persistent_thread_prefixes = [
            "Dask-Offload",
            # HuggingFace safetensors conversion thread - no user cleanup API
            # https://github..../transformers/issues/29513
            "Thread-auto_conversion",
        ]
    
        def live_new_threads():
            # Threads created during this test that are still running. A thread that
            # has already stopped is not a leak -- it's done, just not yet reaped
            # from threading's registry -- so we key on is_alive(), not presence.
            result = []
            for t in threading.enumerate():
                if t.ident is None or t.ident in before or t.name == "MainThread":
                    continue
                if any(t.name.startswith(prefix) for prefix in expected_persistent_thread_prefixes):
                    continue
                if t.is_alive():
                    result.append(t)
            return result
    
        # Some C extensions tear their callback threads down asynchronously, so a
        # thread can stay alive briefly after the test cleaned up its owner (notably
        # zenoh's pyo3-closure threads, freed shortly after the session is closed).
        # A single snapshot races that teardown and flags a thread that is about to
        # exit. Give new threads a grace period to drain; only ones that stay alive
        # are real leaks (a genuinely leaked thread never exits).
        deadline = time.monotonic() + 5.0
        leaked = live_new_threads()
        while leaked and time.monotonic() < deadline:
            time.sleep(0.02)
            leaked = live_new_threads()
    
        if not leaked:
            return
    
        with _seen_threads_lock:
            # Report each leaked thread only once across the session.
            truly_new = [t for t in leaked if t.ident not in _seen_threads]
            for t in leaked:
                _seen_threads.add(t.ident)
    
        if not truly_new:
            return
    
        thread_names = [t.name for t in truly_new]
    
>       pytest.fail(
            f"Non-closed threads created during this test. Thread names: {thread_names}. "
            "Please look at the first test that fails and fix that."
        )
E       Failed: Non-closed threads created during this test. Thread names: ['Thread-249']. Please look at the first test that fails and fix that.

before     = {131208072812224, 131208081204928, 131208097990336, 131208106383040, 131208123168448, 131208131561152, ...}
deadline   = 5878350.437798342
expected_persistent_thread_prefixes = ['Dask-Offload', 'Thread-auto_conversion']
leaked     = [<TMonitor(Thread-249, started daemon 131208056026816)>]
live_new_threads = <function monitor_threads.<locals>.live_new_threads at 0x7755a57a2980>
request    = <SubRequest 'monitor_threads' for <Function test_clip_embedding_initialization>>
t          = <TMonitor(Thread-249, started daemon 131208056026816)>
test_name  = 'dimos/perception/test_image_embedding.py::TestImageEmbedding::test_clip_embedding_initialization'
thread_names = ['Thread-249']
truly_new  = [<TMonitor(Thread-249, started daemon 131208056026816)>]

dimos/conftest.py:273: 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 changed the title fix: be able to send messages fix: dimos topic send Jul 7, 2026
@paul-nechifor paul-nechifor enabled auto-merge (squash) July 7, 2026 07:30

@leshy leshy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

needs to become proto agnostic now with zenoh

Comment thread dimos/robot/cli/topic.py
Comment on lines +144 to +147
try:
submodule = importlib.import_module(f"{package_name}.{name}")
except ImportError:
continue

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.

P1 Import errors disappear

This still hides import-time failures from discovered message modules. When a message file is found but one of its dependencies is missing, importlib.import_module() raises ImportError, this branch skips the module, and the message class never enters the eval context. A command like topic_send("/camera", "Image(...)") then reports name 'Image' is not defined instead of the real missing import, so valid message types can still fail with a misleading parse error.

Suggested change
try:
submodule = importlib.import_module(f"{package_name}.{name}")
except ImportError:
continue
submodule = importlib.import_module(f"{package_name}.{name}")

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