Conversation
Greptile SummaryThis PR kickstarts a memory visualization layer: it adds a Key issues found:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant T as Test / Visualizer
participant S as SqliteStore
participant VS as SqliteVectorStore
participant CLIP as CLIPModel
participant VLM as MoondreamVlModel
T->>S: stream("color_image", Image)
T->>CLIP: embed_text("a door")
CLIP-->>T: Embedding (query vector)
T->>S: stream("color_image_embedded", Image).search(query, k=10)
S->>VS: search("color_image_embedded", query, k=10)
Note over VS: try execute SELECT ... MATCH<br/>except OperationalError → []
VS-->>S: [(rowid, similarity), ...]
S-->>T: Observation[]
opt Object detection path
T->>VLM: query_detections(obs.data, "bottle")
VLM-->>T: DetectionResult[]
T->>S: lidar.at(obs.ts).first().data
S-->>T: PointCloud2
end
opt Embed & save path
T->>S: video.transform(QualityWindow).transform(EmbedImages(clip)).save(embedded)
loop for each downsampled frame
S->>CLIP: embed_image(frame)
CLIP-->>S: Embedding
S->>VS: put("color_image_embedded", key, embedding)
end
end
Last reviewed commit: "vis kickstart" |
dimos/memory2/conftest.py
Outdated
| @pytest.fixture(params=["file_blob_store", "sqlite_blob_store"]) | ||
| def blob_store(request: pytest.FixtureRequest) -> BlobStore: | ||
| return request.getfixturevalue(request.param) | ||
| return request.getfixturevalue(request.param) |
There was a problem hiding this comment.
return statement is dead code
Line 96 is an exact copy of line 95. The second return can never be reached and should be removed.
| return request.getfixturevalue(request.param) |
(Delete the duplicate line entirely.)
dimos/memory2/vectorstore/sqlite.py
Outdated
| except sqlite3.OperationalError: | ||
| return [] |
There was a problem hiding this comment.
OperationalError catch silently hides real failures
The original guard (if stream_name not in self._tables: return []) only suppressed the "stream hasn't been written yet" case. The replacement catches every sqlite3.OperationalError, which includes database-is-locked errors, corrupt pages, out-of-disk-space errors, and malformed SQL — all of which would now silently return [] or no-op instead of surfacing to the caller.
Consider narrowing the catch to the specific "no such table" message:
try:
rows = self._conn.execute(
f'SELECT rowid, distance FROM "{stream_name}_vec" WHERE embedding MATCH ? AND k = ?',
(json.dumps(vec), k),
).fetchall()
except sqlite3.OperationalError as e:
if "no such table" in str(e).lower():
return []
raiseThe same pattern applies to the delete method (lines 102–105).
dimos/memory2/test_visualizer.py
Outdated
| lidar.at(det.ts).first().data | ||
| ) # get a related lidar frame (can try and project) |
There was a problem hiding this comment.
det.ts should likely be obs.ts
det is a detection result returned by vlm.query_detections(obs.data, "bottle"). Detection objects represent spatial regions within a single image frame, so they don't carry a per-frame timestamp — ts lives on the parent observation (obs). Calling det.ts will raise an AttributeError at runtime.
| lidar.at(det.ts).first().data | |
| ) # get a related lidar frame (can try and project) | |
| lidar.at(obs.ts).first().data |
dimos/memory2/test_visualizer.py
Outdated
| assert obs.pose is not None | ||
| assert obs.pose is not None |
There was a problem hiding this comment.
Line 115 is an identical copy of line 114. The second assert is dead code.
| assert obs.pose is not None | |
| assert obs.pose is not None | |
| def test_search_reconstruct_full_path(self, store: SqliteStore) -> None: | |
| for obs in store.streams.color_image_embedded: | |
| assert obs.pose is not None |
dimos/memory2/test_visualizer.py
Outdated
| @pytest.fixture(scope="module") | ||
| def store() -> Iterator[SqliteStore]: | ||
| db = SqliteStore(path=str(DB_PATH)) | ||
| with db: | ||
| yield db | ||
| db.stop() |
There was a problem hiding this comment.
db.stop() called after context manager already closes the store
The with db: block invokes __exit__, which should close the connection and call stop() internally. Calling db.stop() again on line 39 after the with block exits risks a double-close on the underlying SQLite connection, which can raise exceptions or silently corrupt state.
| @pytest.fixture(scope="module") | |
| def store() -> Iterator[SqliteStore]: | |
| db = SqliteStore(path=str(DB_PATH)) | |
| with db: | |
| yield db | |
| db.stop() | |
| @pytest.fixture(scope="module") | |
| def store() -> Iterator[SqliteStore]: | |
| with SqliteStore(path=str(DB_PATH)) as db: | |
| yield db |
| # Actual prerequisite for this is a good python API | ||
|
|
||
|
|
||
| class TestVisualizer: |
There was a problem hiding this comment.
TestVisualizer is missing the @pytest.mark.tool marker
All integration tests in test_e2e.py that depend on the large replay database (go2_bigoffice.db) are guarded with @pytest.mark.tool. TestVisualizer hits the same database but lacks this marker, so these tests will run in a standard pytest invocation and fail if the LFS-backed database is not present.
| class TestVisualizer: | |
| @pytest.mark.tool | |
| class TestVisualizer: |
- SqliteVectorStore: only catch "no such table" OperationalError, re-raise others - test_visualizer: fix det.ts → obs.ts, add @pytest.mark.tool, remove double teardown and duplicate assertion - conftest: remove unreachable duplicate return
…ypes - EmbeddingModel/CLIPModel: @overload so single-arg returns Embedding, multi-arg returns list - conftest: cast getfixturevalue returns to correct Store/BlobStore types
- MobileCLIPModel, TorchReIDModel: add matching overloads for embed/embed_text - Remove now-redundant cast in memory/embedding.py
memory vis kickstart branch