Skip to content

Ivan/fix/go2recording#1996

Open
leshy wants to merge 16 commits intodevfrom
ivan/fix/go2recording
Open

Ivan/fix/go2recording#1996
leshy wants to merge 16 commits intodevfrom
ivan/fix/go2recording

Conversation

@leshy
Copy link
Copy Markdown
Contributor

@leshy leshy commented May 6, 2026

memory2 recorder wasn't appending poses to Observations stored.
now Recorder module uses the transform system to find global coordinates and timestamp of a message (according to its frame ID)

@leshy leshy marked this pull request as ready for review May 7, 2026 05:21
@leshy leshy added the PlzReview label May 7, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 7, 2026

Greptile Summary

This PR fixes the memory2 Recorder module, which was storing observations without their robot pose. The recorder now looks up each message's world-frame transform via self.tf at record time, attaching pose and timestamp to every Observation written to the SQLite store.

  • Recorder._port_to_stream replaces the old direct stream.append(msg) with a new on_msg callback that resolves the tf transform (world → message's frame_id) and passes ts and pose to stream.append.
  • RecorderConfig gains default_frame_id and tf_tolerance fields; when tf is unavailable the recording proceeds without a pose and a warning is emitted.
  • Test fixtures switch from the go2_bigoffice legacy dataset to go2_short, and test_codecs.py replaces TimedSensorReplay with a direct SqliteStore read.

Confidence Score: 5/5

Safe to merge; the core recording path degrades gracefully when tf is unavailable and all observations are still written to the store.

The change is well-scoped: tf lookup failures are handled with a None-pose fallback so recording is never blocked. The only findings are a redundant field declaration in RecorderConfig and a per-message warning that could flood logs when tf is absent — neither affects correctness or data integrity.

dimos/memory2/module.py — the RecorderConfig field duplication and warning-log volume are worth a quick look before shipping to production deployments with high-frequency sensors.

Important Files Changed

Filename Overview
dimos/memory2/module.py Core fix: Recorder._port_to_stream now uses self.tf to attach world-frame pose to every observation; RecorderConfig gains default_frame_id, tf_tolerance, and a redundant db_path field already defined by the parent MemoryModuleConfig
dimos/memory2/codecs/test_codecs.py Replaces TimedSensorReplay with SqliteStore to source JPEG codec test frames; get_data and SqliteStore calls are outside the skip-guard try/except block
dimos/memory2/test_e2e.py Swaps bigoffice DB references for go2_short to match the updated LFS test fixture

Sequence Diagram

sequenceDiagram
    participant Sensor as Sensor (In port)
    participant Recorder
    participant LCMTF as self.tf (LCMTF)
    participant Stream
    participant SqliteStore

    Recorder->>Recorder: start() - register _port_to_stream per In port
    Sensor->>Recorder: on_msg(msg)
    Recorder->>Recorder: "ts = msg.ts or time.time()"
    Recorder->>Recorder: "frame_id = msg.frame_id or default_frame_id"
    Recorder->>LCMTF: "get(world, frame_id, time_point=ts, tf_tolerance)"
    alt transform found
        LCMTF-->>Recorder: Transform
        Recorder->>Recorder: "pose = transform.to_pose()"
    else transform not found
        LCMTF-->>Recorder: None
        Recorder->>Recorder: "pose = None, log warning"
    end
    Recorder->>Stream: "append(msg, ts=ts, pose=pose)"
    Stream->>SqliteStore: persist Observation(data, ts, pose)
Loading

Reviews (5): Last reviewed commit: "Merge branch 'dev' into ivan/fix/go2reco..." | Re-trigger Greptile

Comment thread dimos/memory2/module.py
Comment thread dimos/memory2/module.py
Comment thread dimos/memory2/codecs/test_codecs.py
Comment thread dimos/memory2/codecs/test_codecs.py
Comment thread dimos/memory2/codecs/test_codecs.py Outdated
Comment thread dimos/memory2/codecs/test_codecs.py Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented May 7, 2026

Codecov Report

❌ Patch coverage is 25.00000% with 24 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
dimos/memory2/module.py 35.00% 13 Missing ⚠️
dimos/memory2/codecs/test_codecs.py 0.00% 8 Missing ⚠️
dimos/memory2/test_e2e.py 25.00% 3 Missing ⚠️

📢 Thoughts on this report? Let us know!

@leshy leshy enabled auto-merge (squash) May 7, 2026 07:09
@leshy leshy mentioned this pull request May 8, 2026
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants