Auto-fixes for feat/ivan/spy#2748
Conversation
It is imported by dimos/robot/cli/dimos.py, so it is public API of run_spy, not a module-private helper.
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.
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.
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.
paul-nechifor
requested review from
leshy,
mustafab0 and
spomichter
as code owners
Greptile SummaryThis PR updates the spy CLI and its tests. The main changes are:
Confidence Score: 4/5This is close, but these issues should be fixed before merging.
dimos/utils/cli/spy/core.py Important Files Changed
Reviews (2): Last reviewed commit: "add fix" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| for source in self._live: | ||
| source.stop() | ||
| try: | ||
| source.stop() | ||
| except Exception as exc: | ||
| logger.warning("Error stopping spy source", transport=source.name, error=str(exc)) | ||
| self._live.clear() |
There was a problem hiding this comment.
When source.stop() raises after leaving its transport session or subscription active, this path logs the error and then clears _live. Later stop() calls cannot retry that source, so a long-lived caller that creates and stops spies can leak an active LCM or Zenoh subscription after one transient stop failure.
| logger.warning( | ||
| "Skipping spy transport that failed to start", | ||
| transport=source.name, | ||
| error=str(exc), | ||
| ) |
There was a problem hiding this comment.
This warning used to go directly to stderr, but the spy logger is configured with a stdout console handler and propagate=False. A user running dimos spy 2>errors.log, or a wrapper that watches stderr for startup diagnostics, no longer sees that a transport was skipped.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| def stop(self) -> None: | ||
| """Untap and stop everything; an error in one source never skips the rest.""" | ||
| for untap in self._untaps: | ||
| untap() | ||
| try: | ||
| untap() | ||
| except Exception as exc: | ||
| logger.warning("Error untapping spy source", error=str(exc)) | ||
| self._untaps.clear() | ||
| for source in self._live: | ||
| source.stop() | ||
| try: | ||
| source.stop() | ||
| except Exception as exc: | ||
| logger.warning("Error stopping spy source", transport=source.name, error=str(exc)) | ||
| self._live.clear() |
There was a problem hiding this comment.
When source.stop() raises after only partially shutting down its transport, this method logs the error and then clears _live anyway. A later stop() call has no source left to retry, so an LCM or Zenoh subscription that failed during teardown can stay active for the rest of the process. Keep failed sources tracked until their stop succeeds, while still stopping the healthy sources.
| def stop(self) -> None: | |
| """Untap and stop everything; an error in one source never skips the rest.""" | |
| for untap in self._untaps: | |
| untap() | |
| try: | |
| untap() | |
| except Exception as exc: | |
| logger.warning("Error untapping spy source", error=str(exc)) | |
| self._untaps.clear() | |
| for source in self._live: | |
| source.stop() | |
| try: | |
| source.stop() | |
| except Exception as exc: | |
| logger.warning("Error stopping spy source", transport=source.name, error=str(exc)) | |
| self._live.clear() | |
| def stop(self) -> None: | |
| """Untap and stop everything; an error in one source never skips the rest.""" | |
| remaining_untaps = [] | |
| for untap in self._untaps: | |
| try: | |
| untap() | |
| except Exception as exc: | |
| logger.warning("Error untapping spy source", error=str(exc)) | |
| remaining_untaps.append(untap) | |
| self._untaps = remaining_untaps | |
| remaining_live = [] | |
| for source in self._live: | |
| try: | |
| source.stop() | |
| except Exception as exc: | |
| logger.warning("Error stopping spy source", transport=source.name, error=str(exc)) | |
| remaining_live.append(source) | |
| self._live = remaining_live |
| logger.warning( | ||
| "Skipping spy transport that failed to start", | ||
| transport=source.name, | ||
| error=str(exc), | ||
| ) |
There was a problem hiding this comment.
These skipped-transport warnings still go through the spy logger, which is configured with propagate=False and a stdout console handler. A user running dimos spy 2>errors.log, or a wrapper that watches stderr for startup diagnostics, will not see that a backend was skipped. Emit these startup degradation messages to stderr or add an explicit stderr sink for this path.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Merged into feat/ivan/spy at bf819c1 — took all 15 commits, none dropped; authorship preserved (merge, not squash). Reviewed each one: the type-resolution, import-hoisting, DRY validation extraction, Two of my later commits on the base (try/finally test cleanup; sys-hoist + Gate after merge: 54 spy/CLI/codebase-check tests pass, mypy + ruff clean. Thanks! This PR can be closed. |
2 participants
These are automated fixes. Each fix is a separate commit. Use
git rebase -ito drop any you disagree with.