async recorder callbacks#2577
Open
jeff-hykin wants to merge 7 commits into
Open
Conversation
❌ 3 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
jeff-hykin
force-pushed
the
jeff/fix/pose_setter_for
branch
from
27a5a72 to
6276f5b
Compare
Convert the memory2 Recorder from thread/disposable rx subscriptions to manual async callbacks via process_observable, and let pose_setter_for methods be async (awaited in _resolve_pose). Update the fastlio and go2 recorders accordingly.
jeff-hykin
force-pushed
the
jeff/fix/pose_setter_for
branch
from
6276f5b to
dba1e5a
Compare
Raise TypeError at decoration time if a non-async function is decorated, and always await the setter in _resolve_pose.
…imos into jeff/fix/pose_setter_for
jeff-hykin
changed the title
jeff-hykin
requested review from
leshy,
mustafab0,
paul-nechifor and
spomichter
as code owners
Contributor
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| stream.append(msg, ts=ts, pose=pose) | ||
|
|
||
| self.register_disposable(Disposable(input_topic.subscribe(on_msg))) | ||
| self.process_observable(input_topic.pure_observable(), on_msg) |
Contributor
There was a problem hiding this comment.
process_observable uses a single-slot LATEST mailbox: if on_msg is still awaiting _resolve_pose / stream.append when the next message arrives, the queued-but-unprocessed item is silently overwritten. The old Disposable(input_topic.subscribe(on_msg)) approach would block the publisher instead of dropping.
For typical sensor rates and a fast SQLite write this should be fine in practice, but there's no dropped-frame counter or warning anywhere — if the DB slows down or a pose setter blocks, missing frames would be invisible. Consider logging a warning (or incrementing a counter) inside the LATEST-coalescing slot in _make_async_dispatch, or in on_msg itself if a pre-existing slot is detected, so operators know when data is being lost.
Member
Author
There was a problem hiding this comment.
good idea, I'll add a warning for dropped stuff
process_observable gains an optional on_drop callback fired once per message dropped by the dispatcher's single-slot LATEST mailbox. The Recorder uses it to count dropped frames per stream and log a throttled warning, so a slow sink no longer loses data silently.
github-actions
Bot
added
the
ready-to-merge
github-actions
Bot
removed
the
ready-to-merge
| """Mark a method ``(self, msg) -> Pose | None`` as the pose setter for the | ||
| given recorded stream(s). Streams without a setter fall back to the tf-based | ||
| ``world <- frame_id`` lookup.""" | ||
| """Mark an ``async def`` method ``(self, msg) -> Pose | None`` as the pose |
Contributor
There was a problem hiding this comment.
You've changed pose_setter_for to only work on async functions, but it is still decorating some non-async function, for example PointlioPoseRecorder._odom_pose.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Async
pose_setter_forfor memory2. Updates the fastlio and go2 recorders to match.