feat: dimos spy - universal transport spy over LCM + Zenoh#2735
Conversation
Design ratified in ticket f6c74d39 (subscribe_all non-conflating decision confirmed by Ivan). Task package for a worker to implement: - TASK.md: goal, decisions, implementation map, acceptance - docs/usage/transports/spy.md: design doc (why, architecture, decisions) - spec.py: subscribe_all contract tightened (every message, no conflation); SubscribeLatestMixin.subscribe_latest() stub = the explicit conflation opt-in - protocol/pubsub/spy.py: TopicStats / SpySource / LCMSpySource / ZenohSpySource / TransportSpy stubs (typed, NotImplementedError bodies) - utils/cli/spy/run_spy.py + `dimos spy` CLI registration (TUI stub) - test_spy.py: 17 contract tests, 15 failing until implemented; the zenoh subscribe_all conflation bug fails sharply (2 of 50 messages delivered) mypy clean (796 files); existing pubsub suites green.
Implement the spy core (TopicStats, LCM/Zenoh SpySources, TransportSpy) that taps the raw-bytes pubsub layer — per-message hot path is (topic, len, ts), never decoding a payload — plus the Textual TUI (dimos spy) and CLI wiring. Spec fix: subscribe_all is now non-conflating on every transport; conflation moves to the opt-in SubscribeLatestMixin.subscribe_latest() (lifted from zenoh, transport-agnostic). Zenoh subscribe_all widens dimos/** -> ** and keeps its drain-stop bookkeeping via _register/_unregister_drain_stop hooks. Rerun bridge migrates to subscribe_latest. dimos lcmspy becomes a deprecated alias for dimos spy --transport lcm. Also strip 14 decorative section-banner comments from the delivered test_spy.py to satisfy codebase_checks/test_no_sections (approved by brain; no logic change).
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
- TransportSpy.start() rolls back already-started sources on failure - subscribe_latest stops+joins the drain thread if subscribe_all raises - SOURCE_FACTORIES registry: construct only the requested transports (a filtered-out transport is never imported or instantiated) - run_spy/SpyApp validate unknown --transport names up front - add contract tests for the failure-rollback paths and run_spy selection
Greptile SummaryThis PR adds a universal transport spy to replace the old LCM-only spy. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (18): Last reviewed commit: "Merge branch 'main' into feat/ivan/spy" | Re-trigger Greptile |
- dimos lcmspy subcommand: drop deprecation/guard, just run spy --transport lcm - lcmspy console-script repointed to run_spy:lcm_main shim (LCM-only spy) - delete the superseded dimos/utils/cli/lcmspy/ module (TUI + stats + tests)
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
View the full list of 2 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
- default_sources() skips backends that fail to import with a one-line stderr warning and errors only if no transport is available; an explicitly requested --transport keeps the hard error - spy web mode serves the child with sys.executable and forwards the --transport filter args (validated before serving) - restore the lcmspy --transport override guard dropped by the thin-alias refactor - tests: default degradation, explicit hard error, web command construction
dimos lcmspy web previously became spy --transport lcm web, which missed run_spy.main()'s web branch (web must be argv[0]) and silently launched the local TUI. Keep web first and forward the LCM filter behind it; the --transport override guard stays. Alias routing is now covered by CliRunner tests (tui, web, override rejection).
Reverses the ratified subscribe_all non-conflation spec change (Ivan's call): - zenoh subscribe_all: restore original latest-per-topic drain over dimos/** - spec: drop SubscribeLatestMixin/subscribe_latest entirely - rerun bridge: back to subscribe_all (its original conflating consumer) - spy is transport-agnostic — it just calls bus.subscribe_all(), accepting whatever delivery semantics each transport provides (LCM: every message; zenoh: latest-per-topic). No raw/session-level special-casing. - drop the now-invalid contract tests: subscribe_all-delivers-every[zenoh] and subscribe_latest[*]; docstrings updated to match. Keeps the three Greptile fixes already on feat/ivan/spy (default_sources degradation, web-mode filter forwarding, lcmspy-web routing).
Per maintainer decision, drop the browser 'web' mode entirely: - run_spy: remove the web branch + _web_command; main() just runs the TUI - docs: drop the 'dimos spy web' line Fix the standalone lcmspy console entry (Greptile P1): it discarded sys.argv[1:]. Extract _lcm_only_argv() — shared by lcm_main() and the dimos lcmspy alias — which forwards args to spy and rejects an explicit --transport override (LCM-only). Tests for forwarding + rejection on both entry points; drop the now-obsolete web tests.
import sys in run_spy and the test-only imports had no reason to be inline. The lazy backend imports in core.py stay inline but now carry a comment saying why (an unavailable backend must not break the others).
The 60.0s default was duplicated in TopicStats.__init__ and TransportSpy.__init__.
- App -> App[None], **kwargs: Any, DataTable -> DataTable[Text] - cursor_type=None -> the documented "none" CursorType literal; drop zebra_stripes=False (the default) - narrow self.table once at the top of refresh_table instead of three union-attr ignores
_parse_transports and SpyApp.__init__ built near-identical error messages; both now use validate_transport_names in core, with the CLI converting the ValueError to SystemExit.
Source construction/validation, the autoconf warning, and spy.start() move to a new start_spy() called from main(), which now owns the spy's lifecycle with try/finally around app.run(). SpyApp just receives the started TransportSpy, so a failed start can no longer leave a half-built app. Tests exercise start_spy() through a fixture that always stops the spy on teardown.
Replace the two print(..., file=sys.stderr) warnings in core with structured logger.warning calls. Tests assert on caplog (via a shared spy_warnings fixture that hooks the non-propagating dimos logger) instead of capsys.
If one untap() or source.stop() raised, the remaining sources were never stopped. Each failure is now logged and the shutdown loop continues.
Wrap the subscribe_all delivery test's unsub() and the two FakeSource TransportSpy tests' stop() in try/finally so a failing assertion no longer leaks the subscription / leaves the spy running. The source-counting tests and the SpyApp tests already clean up in finally.
Only tests called it; the TUI shows freq, bandwidth, total and age.
total_bytes/total_msgs/last_seen are written under the stats lock by transport threads but were read unlocked from the UI thread, and each refresh traversed+copied the history twice per row (freq + bytes_per_sec). window_stats() now returns a consistent WindowStats reading computed in a single locked pass without building intermediate lists; refresh_table sorts and renders from it. freq/bytes_per_sec stay as thin wrappers for the contract tests.
The feature is implemented on this branch; TASK.md and the agent/spy-architect branch no longer exist.
Buses, sources, publishers and the zenoh session pool were cleaned up with hand-rolled try/finally (or a contextmanager) in test bodies; move them to yield-fixtures / request.addfinalizer so teardown also runs when an assertion fails before the cleanup line.
Two review nits on PR #2735: - import sys was a local import in both main() and lcm_main(); hoist it to the module imports. - SpyApp subclassed the generic App with a type: ignore[type-arg]; parameterize it as App[None] (the app's run() return type) instead of ignoring.
Replace the fixed establishment sleeps with a probe loop: publish on a probe topic until the tap/wildcard subscription delivers one, with a deadline. The 0.01s pacing sleeps in publish loops stay (throttling, not waiting). Cuts the spy test suite from ~3.2s to ~1.4s.
Greptile P1 on PR #2735: 'dimos --transport zenoh spy' let the root callback consume --transport into GlobalConfig, so the spy saw no args and silently ran the default all-transport view (including LCM) despite the zenoh request. Root --transport sets the stack's pubsub backend (which single transport dimos processes participate on); the spy is an observer that watches every transport and takes its own repeatable --transport filter after the subcommand. They mean different things, so reject the root placement with a clear error pointing at 'dimos spy --transport <name>' rather than silently ignoring the filter. Env/ default DIMOS_TRANSPORT is unaffected (only an explicit root CLI flag triggers it).
Takes all 15 automated-fix commits from feat/ivan/spy-autofixes (authorship preserved). They supersede my two overlapping commits (try/finally cleanup, sys-import + App[None]) with more complete versions (fixture teardown, full type resolution). My reject-root-transport and cli.md docs commits are kept.
|
Merged Paul's autofix PR #2748 into this branch at bf819c1 — all 15 commits taken (authorship preserved via merge). They resolve several of the earlier review threads (type ignores, inline imports, thread-safety of the stats read, degrade-warning logging, test teardown). Gate green: 54 spy/CLI tests + codebase checks pass, mypy + ruff clean. |
replaces lcmspy