go2 mem2 recorder fixes, misc fixes, new recording#2109
Conversation
Skip the "Select device" prompt when discovery returns exactly one robot. Multi-device path still prompts.
Greptile SummaryThis PR fixes recorder timestamp handling in
Confidence Score: 3/5Two defects in the recorder need fixing before merging: the logger format mismatch will silently swallow missing-tf diagnostics, and the timestamp decoupling between tf lookup and stream storage may corrupt recording fidelity for sensor-timestamped data. The recorder changes in module.py introduce a broken log format (4 placeholders, 3 args) that suppresses diagnostic output every time a tf lookup fails, and separately stores stream entries under wall-clock time while still resolving pose transforms against the message's original timestamp — creating a mismatch between stored timestamps and their associated poses. The lidar control and tf identity-shortcut additions look correct, and the CLI topic import refactor works if the message package layout uses per-class sub-modules as expected. dimos/memory2/module.py has two issues that warrant a close look; dimos/robot/cli/topic.py should be verified against the actual message package layout to confirm every message type is reachable via the new sub-module import path. Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| getattr(msg, "ts", None), | ||
| ) | ||
| stream.append(msg, ts=ts, pose=pose) | ||
| stream.append(msg, ts=time.time(), pose=pose) |
There was a problem hiding this comment.
ts was previously computed as getattr(msg, "ts", None) or time.time() and passed through to both the tf lookup and stream.append. It is still used for the tf lookup on line 309, but stream.append now always receives time.time(). Any message that carries its own hardware or protocol timestamp will be stored in the stream under the wall-clock arrival time instead of the message's own ts. For replayed or sensor-timestamped data this will cause the recording index to disagree with the pose timestamps that were resolved against the message's original ts.
| def _resolve_type(type_name: str) -> type: | ||
| for module_name in _modules_to_try: | ||
| try: | ||
| module = importlib.import_module(module_name) | ||
| if hasattr(module, type_name): | ||
| return getattr(module, type_name) # type: ignore[no-any-return] | ||
| module = importlib.import_module(f"{module_name}.{type_name}") | ||
| except ImportError: | ||
| continue | ||
| if hasattr(module, type_name): | ||
| return getattr(module, type_name) # type: ignore[no-any-return] |
There was a problem hiding this comment.
_resolve_type no longer handles types that are exported from a package __init__
The old code imported the package (e.g. dimos.msgs.geometry_msgs) and checked whether the class was an attribute of that module — which works for packages that re-export their classes in __init__.py. The new code tries to import a sub-module named after the type (e.g. dimos.msgs.geometry_msgs.Twist). If that sub-module doesn't exist but the class is still accessible via the package init, ImportError is caught, the loop continues, and all entries are exhausted, ending in a ValueError. Any call-site that previously worked through the package-level export will silently break. Were the dimos.msgs.* packages recently reorganised so every message class now lives in its own sub-module? If the package inits still re-export classes, the old approach would still work and the new one may miss some types.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
1 participant
No description provided.