Temporal Memory #973
Temporal Memory #973
Conversation
ClaireBookworm
changed the title
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Summary
This PR introduces enhanced memory support with temporal memory, VLM integration, and improved multi-image vision model support. The changes add ~1844 lines across 15 files, focusing on temporal entity tracking and rolling summaries for video streams.
Key Changes:
-
Temporal Memory Module (
temporal_memory.py): New sophisticated system for maintaining entity rosters and temporal understanding of video streams using VLM queries and CLIP-based frame filtering. Implements VideoRAG concepts adapted for the dimos architecture. -
Vision-Language Models:
- Added
OpenAIVlModelwith multi-image query support (query_multi_images) - Extended
QwenVlModelwith similar multi-image capabilities - Enhanced
VLModelbase class with pickling support
- Added
-
CLIP Frame Filtering (
clip_filter.py): Implements greedy farthest-point sampling using CLIP embeddings to select diverse frames from video windows, with fallback to simple uniform sampling. -
VideoRAG Utilities (
videorag_utils.py): Port of sophisticated prompting and state management logic from VideoRAG for building entity rosters, rolling summaries, and temporal context. -
VLMAgent Enhancements: Extended with
response_formatparameter support for structured outputs from vision models.
Issues Found:
The PR mentions "still todo: fix pickling issue" and our review confirms this is a critical blocker. The temporal memory pickling implementation is incomplete and will fail when used with distributed computing frameworks (Dask, Ray, etc.).
Additional issues include:
- Incomplete
__setstate__restoration in TemporalMemory - Missing pickling support in QwenVlModel (inconsistent with OpenAI)
- Fragile assumptions about Image class attributes in CLIP filter
- Potential race conditions in temporal memory start() and frame buffer access
Assessment:
The overall architecture and design are solid, with good abstractions and error handling patterns. However, the incomplete pickling implementation and several edge case issues prevent this from being production-ready for distributed scenarios. The code quality is generally good with clear documentation and proper logging.
Confidence Score: 2/5
- This PR has critical unresolved issues that will cause failures in production use cases, particularly distributed computing and pickling scenarios.
- The PR introduces significant new functionality but has several blocking issues: (1) Incomplete pickling implementation that will fail when used with Dask/distributed frameworks - this is acknowledged as "still todo" in the PR description, (2) Missing state restoration in setstate for critical attributes like _clip_filter and _vlm, (3) Inconsistent pickling support between OpenAI and Qwen VLM models, (4) Race condition in temporal memory start() with _video_start_wall_time, (5) Fragile assumptions about Image class structure. The code quality is otherwise good with solid architecture, but these issues need resolution before merging.
- dimos/perception/temporal_memory.py (incomplete pickling is blocking issue), dimos/models/vl/qwen.py (missing pickling support), dimos/perception/clip_filter.py (fragile Image attribute assumptions)
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| dimos/perception/temporal_memory.py | 2/5 | Critical pickling issue: getstate calls super().getstate() but doesn't properly restore all attributes in setstate. Missing restoration of _clip_filter, _video_start_wall_time, and reactive streams. Race condition on _video_start_wall_time access in start() method. |
| dimos/perception/clip_filter.py | 3/5 | Module properly exports CLIP_AVAILABLE and select_diverse_frames_simple. Good error handling for missing CLIP dependency. No pickling support added for CLIPFrameFilter model/preprocessor attributes. |
| dimos/models/vl/openai.py | 4/5 | Adds OpenAIVlModel with multi-image support and query_multi_images() method. Proper pickling support with API key reloading. Implementation is solid. |
| dimos/models/vl/qwen.py | 3/5 | QwenVlModel implementation missing getstate/setstate pickling support unlike OpenAI. Won't handle API key reloading on unpickling in distributed scenarios. |
| dimos/agents/vlm_agent.py | 4/5 | VLMAgent properly extended with response_format parameter support for structured outputs. Changes appear well-designed and backward compatible. |
| dimos/perception/videorag_utils.py | 3/5 | Utility functions properly implemented with good edge case handling. Gracefully handles list responses from VLM. Functions well-structured with clear documentation. |
Sequence Diagram
sequenceDiagram
participant Frame as Video Frame
participant TM as TemporalMemory
participant CF as CLIPFrameFilter
participant VLM as VLModel
participant State as State Dict
Frame->>TM: on_frame(image)
TM->>TM: acquire state_lock
TM->>TM: append to frame_buffer
TM->>TM: release state_lock
Note over TM: interval(stride_s) triggers
TM->>TM: _analyze_window()
TM->>TM: acquire state_lock
TM->>TM: get_window_frames from buffer
TM->>TM: snapshot state
TM->>TM: release state_lock
alt CLIP Available
TM->>CF: select_diverse_frames()
CF->>CF: encode_images with CLIP
CF->>CF: greedy farthest-point sampling
CF->>TM: return filtered frames
else CLIP Unavailable
TM->>TM: select_diverse_frames_simple()
end
TM->>VLM: query(frame, prompt)
VLM->>VLM: call API (OpenAI/Qwen)
VLM->>TM: return structured response
TM->>TM: parse_window_response()
TM->>TM: acquire state_lock
TM->>State: update_state_from_window()
State->>State: update entity_roster
State->>State: update rolling_summary
TM->>TM: release state_lock
Note over TM: Check if summary update needed
alt Summary Interval Reached
TM->>TM: _update_rolling_summary()
TM->>VLM: query(latest_frame, summary_prompt)
VLM->>TM: return summary text
TM->>State: apply_summary_update()
end
dimos/perception/temporal_memory.py
Outdated
| def __getstate__(self) -> dict[str, Any]: | ||
| state = super().__getstate__() | ||
| if state is None: | ||
| # Parent doesn't implement __getstate__, so we need to manually exclude unpicklable attrs | ||
| state = self.__dict__.copy() | ||
| # Remove unpicklable attributes | ||
| state.pop("_disposables", None) | ||
| state.pop("_loop", None) | ||
| state.pop("_state_lock", None) | ||
| return state | ||
|
|
||
| def __setstate__(self, state: dict[str, Any]) -> None: | ||
| # Let parent restore what it needs | ||
| if hasattr(super(), "__setstate__"): | ||
| super().__setstate__(state) | ||
| else: | ||
| self.__dict__.update(state) | ||
| # Recreate unpicklable attributes | ||
| self._state_lock = threading.Lock() |
There was a problem hiding this comment.
Critical Pickling Issue: The __getstate__ and __setstate__ methods are incomplete and will cause failures when unpickling.
Current problems:
__getstate__doesn't handle removal of unpicklable_vlmattribute (the VlModel instance with OpenAI client)__setstate__doesn't restore_clip_filteror_video_start_wall_timewhich are removed during pickling_disposablesand reactive streams are not properly recreated, which will cause errors when the module tries to use them after unpickling- Missing restoration of
_llmand other reactive dependencies from parent SkillModule
Since the PR description mentions "still todo: fix pickling issue", this is a known issue. The pickling mechanism is incomplete and will fail when used with Dask or other distributed computing frameworks.
Suggested approach:
- Extend
__getstate__to also remove_vlm,_clip_filter, and all reactive attributes - Extend
__setstate__to properly recreate CLIP filter if enabled, and reinitialize all disposables after parent restoration
| def start(self) -> None: | ||
| super().start() | ||
|
|
||
| with self._state_lock: | ||
| if self._video_start_wall_time is None: | ||
| self._video_start_wall_time = time.time() | ||
|
|
There was a problem hiding this comment.
Race Condition: There's a potential race condition in the start() method. The _video_start_wall_time is initialized here, but in _analyze_window() at line 198, it's accessed without checking if it's None. If _analyze_window() is called by the reactive interval before this initialization completes, it will fail.
More critically, in _update_rolling_summary() at line 343, self._frame_buffer[-1] is accessed, which could be empty if frames haven't arrived yet, causing an IndexError.
Recommended fix:
def on_frame(image: Image) -> None:
with self._state_lock:
if self._video_start_wall_time is None:
# This shouldn't happen here, guard against it
return
video_start = self._video_start_wall_time
# ... rest of codeAlso ensure _analyze_window() guards against empty frame_buffer before accessing self._frame_buffer[-1].
| def _update_rolling_summary(self, w_end: float) -> None: | ||
| try: | ||
| # get state | ||
| with self._state_lock: | ||
| rolling_summary = str(self._state.get("rolling_summary", "")) | ||
| chunk_buffer = list(self._state.get("chunk_buffer", [])) | ||
| if self._frame_buffer: | ||
| latest_frame = self._frame_buffer[-1].image | ||
| else: | ||
| latest_frame = None | ||
|
|
||
| if not chunk_buffer or not latest_frame: | ||
| return |
There was a problem hiding this comment.
Potential Empty Frame Buffer Error: In _update_rolling_summary(), line 343 accesses self._frame_buffer[-1].image without checking if the buffer is empty. While there's a check at line 347 (if not chunk_buffer or not latest_frame), it only prevents further processing, not the IndexError that would occur at line 343.
This could happen if:
- Frames arrive and are added to buffer
- Summary update is triggered
- Meanwhile, the buffer is cleared/emptied by another thread before this line executes
The lock protects the state but the frame_buffer is accessed after releasing the lock, creating a window of vulnerability.
Recommended fix:
def _update_rolling_summary(self, w_end: float) -> None:
try:
# get state
with self._state_lock:
rolling_summary = str(self._state.get("rolling_summary", ""))
chunk_buffer = list(self._state.get("chunk_buffer", []))
if self._frame_buffer:
latest_frame = self._frame_buffer[-1].image
else:
latest_frame = None
if not chunk_buffer or not latest_frame: # <-- Already correct, but document intent
returnThe current code is actually safe, but this pattern should be consistent throughout to avoid future bugs.
dimos/perception/clip_filter.py
Outdated
| def _image_to_pil(self, image: Image) -> PILImage.Image: | ||
| """Convert dimos Image to PIL Image.""" | ||
| # Get numpy array from dimos Image | ||
| img_array = image.data # Assumes Image has .data attribute with numpy array | ||
|
|
||
| # Convert to PIL | ||
| return PILImage.fromarray(img_array) |
There was a problem hiding this comment.
Assumption about Image.data attribute: The _image_to_pil method assumes dimos Image has a .data attribute containing a numpy array. This is a fragile assumption that should either:
- Be documented clearly with a reference to Image class definition
- Have a try-except to gracefully handle if the attribute doesn't exist
- Use a more robust method like
image.to_numpy()if such a method exists on the Image class
If the Image class doesn't have a .data attribute or the attribute contains different data than expected, this will silently fail or produce incorrect results.
Suggested fix:
def _image_to_pil(self, image: Image) -> PILImage.Image:
"""Convert dimos Image to PIL Image."""
try:
# Try to get numpy array - check multiple possible attributes
if hasattr(image, 'to_numpy'):
img_array = image.to_numpy()
elif hasattr(image, 'data'):
img_array = image.data
else:
raise AttributeError(f"Image object has no to_numpy or data attribute")
return PILImage.fromarray(img_array)
except Exception as e:
logger.error(f"Failed to convert Image to PIL: {e}")
raise
Additional Comments (2)
When unpickling in a distributed environment (e.g., Dask worker), the environment variables might not be set. The OpenAI implementation handles this by reloading credentials from environment on unpickling, but Qwen doesn't. Recommended fix: Add pickling support to QwenVlModel matching the pattern in OpenAIVlModel (lines 36-55). This ensures the model can be safely serialized and deserialized across worker processes in distributed computing scenarios.
More importantly, on line 42, Potential fix: Ensure super().init is called before accessing or publishing to streams, or defer system message publishing to the |
dimos/models/vl/base.py
Outdated
| coords = [float(vlm_detection[i]) for i in range(1, 5)] | ||
| except (ValueError, TypeError) as e: | ||
| logger.debug(f"Invalid VLM detection coordinates: {vlm_detection[1:]}. Error: {e}") | ||
| logger.debug( |
There was a problem hiding this comment.
Revert these formatting changes we already have ruff setup that standardizes repo formatting and this change would mess up the git blame
dimos/models/vl/base.py
Outdated
|
|
||
| if len(vlm_point) != 3: | ||
| logger.debug(f"Invalid VLM point length: {len(vlm_point)}, expected 3. Got: {vlm_point}") | ||
| logger.debug( |
dimos/models/vl/base.py
Outdated
| return image.resize_to_fit(max_w, max_h) | ||
| return image, 1.0 | ||
|
|
||
| def __getstate__(self) -> dict[str, Any]: |
There was a problem hiding this comment.
Why would state be unpickleble? We've never done this overriding of state before so assume a better way to handle this.
dimos/models/vl/base.py
Outdated
| @abstractmethod | ||
| def query(self, image: Image, query: str, **kwargs) -> str: ... # type: ignore[no-untyped-def] | ||
|
|
||
| def query_multi_images(self, images: list[Image], query: str, **kwargs) -> str: # type: ignore[no-untyped-def] |
There was a problem hiding this comment.
This is a good method to have here but probably should include logic per my other comment below
There was a problem hiding this comment.
Oh wait no we already have query_batch in base.py. Looks like the same thing. LLM hallucination.
dimos/models/vl/openai.py
Outdated
|
|
||
| return response.choices[0].message.content # type: ignore[return-value] | ||
|
|
||
| def query_multi_images( |
There was a problem hiding this comment.
This looks too similar to its sister method in qwen. But anyway per my comment before we already have query_batch so not sure how these are different or why they need a new method.
There was a problem hiding this comment.
If openAI versus qwen has different context schema just pass schema to the parent method as a parameter. Otherwise once we have 10 more models in /vl we will have all this copied code of iterating through lists of images.
|
@ClaireBookworm I reviewed the first 2 files will wait to spend time on the rest until this is working |
|
haha, i'm unsure why that commit was labeled as ur git @spomichter my bad |
| from dimos.core.transport import LCMTransport | ||
| from dimos.protocol import pubsub | ||
|
|
||
| # Disable Rerun for tests (prevents viewer spawn and gRPC flush errors) |
There was a problem hiding this comment.
Revert changes to stuff outside your memory
dimos/models/vl/base.py
Outdated
| config: VlModelConfig | ||
|
|
||
| @abstractmethod | ||
| def is_set_up(self) -> None: |
There was a problem hiding this comment.
Delete this as an abstract method not worth enforcing in all children as we can just add a try catch method and not make everyone implement this method that can just be checked with a single line in query().
dimos/models/vl/openai.py
Outdated
| default_config = OpenAIVlModelConfig | ||
| config: OpenAIVlModelConfig | ||
|
|
||
| def is_set_up(self) -> None: |
There was a problem hiding this comment.
Jk yeah delete this its not used.
dimos/models/vl/qwen.py
Outdated
| default_config = QwenVlModelConfig | ||
| config: QwenVlModelConfig | ||
|
|
||
| def is_set_up(self) -> None: |
| @@ -0,0 +1,1018 @@ | |||
| # Copyright 2026 Dimensional Inc. | |||
There was a problem hiding this comment.
For production all of these entities need types and would need more modularity bc this is super long and hard to review. So lets just move this all into /perception/experimental and then we can merge this in today. Too much overhead to get this production ready and will be better to move quickly
|
@ClaireBookworm Everything in /models/vl is good where it is. Everything in /perception move to /perception/experimental/temporal_memory/ and then we're good to test on the g1 and tune! |
|
@greptile final review |
|
@codex final review check for breaking changes |
Greptile SummaryThis PR introduces a comprehensive temporal memory system for video streams, inspired by VideoRAG. The implementation adds entity tracking, relationship graphs, and VLM-based scene understanding across ~3,500 lines of new code. Key Changes:
Critical Issues:
Architecture Strengths:
Areas Requiring Attention:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant TemporalMemory
participant FrameBuffer
participant VLM as VLM (OpenAI)
participant GraphDB as EntityGraphDB
participant CLIPFilter
User->>TemporalMemory: start()
activate TemporalMemory
TemporalMemory->>TemporalMemory: Initialize state & buffers
TemporalMemory->>GraphDB: Initialize SQLite DB
TemporalMemory->>CLIPFilter: Initialize CLIP model (optional)
loop Every incoming frame
TemporalMemory->>FrameBuffer: Add frame (via sharpness filter)
Note over FrameBuffer: Stores last 50 frames
end
loop Every stride_s seconds
TemporalMemory->>TemporalMemory: _analyze_window()
TemporalMemory->>FrameBuffer: Get window frames
TemporalMemory->>TemporalMemory: Check scene staleness
alt Scene changed
TemporalMemory->>CLIPFilter: Select diverse frames (adaptive keyframes)
CLIPFilter-->>TemporalMemory: Selected frames
TemporalMemory->>VLM: query_batch(images, window_prompt)
VLM-->>TemporalMemory: JSON response (entities, relations, caption)
TemporalMemory->>TemporalMemory: Parse response
TemporalMemory->>GraphDB: save_window_data(entities, relations)
par Background distance estimation
TemporalMemory->>VLM: estimate_distances(entity_pairs)
VLM-->>TemporalMemory: Distance estimates
TemporalMemory->>GraphDB: add_distance(entity_a, entity_b)
end
TemporalMemory->>TemporalMemory: update_state_from_window()
alt Summary interval reached
TemporalMemory->>VLM: query(frame, summary_prompt)
VLM-->>TemporalMemory: Rolling summary text
TemporalMemory->>TemporalMemory: apply_summary_update()
end
end
end
User->>TemporalMemory: query("What entities are visible?")
TemporalMemory->>GraphDB: get_relations_for_entity()
GraphDB-->>TemporalMemory: Relations graph
TemporalMemory->>GraphDB: get_nearby_entities()
GraphDB-->>TemporalMemory: Distance graph
TemporalMemory->>GraphDB: get_semantic_relations()
GraphDB-->>TemporalMemory: Semantic graph
TemporalMemory->>TemporalMemory: build_query_prompt(context)
TemporalMemory->>VLM: query(latest_frame, prompt)
VLM-->>TemporalMemory: Answer
TemporalMemory-->>User: Answer text
User->>TemporalMemory: stop()
TemporalMemory->>GraphDB: commit() & close()
TemporalMemory->>CLIPFilter: close()
TemporalMemory->>TemporalMemory: Clear buffers & state
deactivate TemporalMemory
|
dimos/perception/clip_filter.py
Outdated
| while len(selected_indices) < max_frames and remaining_indices: | ||
| selected_embs = embeddings[selected_indices] | ||
| remaining_embs = embeddings[remaining_indices] | ||
|
|
||
| # Compute similarities between remaining and selected | ||
| # Shape: (num_remaining, num_selected) | ||
| similarities = remaining_embs @ selected_embs.T | ||
|
|
||
| # For each remaining frame, find its max similarity to any selected frame | ||
| # Shape: (num_remaining,) | ||
| max_similarities = similarities.max(dim=1)[0] | ||
|
|
||
| # Select frame with minimum max similarity (most different from all selected) | ||
| best_idx = int(max_similarities.argmin().item()) | ||
|
|
||
| selected_indices.append(remaining_indices[best_idx]) | ||
| remaining_indices.pop(best_idx) |
There was a problem hiding this comment.
syntax: Critical Syntax Error: Indentation issue will cause runtime failure. Lines 148-152 are incorrectly indented - they're outside the while loop when they should be inside it.
The logic should be:
- While we need more frames...
- Compute similarities
- Find max similarities
- Select best frame (line 149) - currently outside loop
- Append to selected (line 151) - currently outside loop
- Remove from remaining (line 152) - currently outside loop
| while len(selected_indices) < max_frames and remaining_indices: | |
| selected_embs = embeddings[selected_indices] | |
| remaining_embs = embeddings[remaining_indices] | |
| # Compute similarities between remaining and selected | |
| # Shape: (num_remaining, num_selected) | |
| similarities = remaining_embs @ selected_embs.T | |
| # For each remaining frame, find its max similarity to any selected frame | |
| # Shape: (num_remaining,) | |
| max_similarities = similarities.max(dim=1)[0] | |
| # Select frame with minimum max similarity (most different from all selected) | |
| best_idx = int(max_similarities.argmin().item()) | |
| selected_indices.append(remaining_indices[best_idx]) | |
| remaining_indices.pop(best_idx) | |
| while len(selected_indices) < max_frames and remaining_indices: | |
| selected_embs = embeddings[selected_indices] | |
| remaining_embs = embeddings[remaining_indices] | |
| # Compute similarities between remaining and selected | |
| # Shape: (num_remaining, num_selected) | |
| similarities = remaining_embs @ selected_embs.T | |
| # For each remaining frame, find its max similarity to any selected frame | |
| # Shape: (num_remaining,) | |
| max_similarities = similarities.max(dim=1)[0] | |
| # Select frame with minimum max similarity (most different from all selected) | |
| best_idx = int(max_similarities.argmin().item()) | |
| selected_indices.append(remaining_indices[best_idx]) | |
| remaining_indices.pop(best_idx) |
dimos/perception/temporal_memory.py
Outdated
| # Stop all stream transports to clean up LCM/shared memory threads | ||
| for stream in list(self.inputs.values()) + list(self.outputs.values()): | ||
| if stream.transport is not None and hasattr(stream.transport, "stop"): | ||
| stream.transport.stop() | ||
| stream._transport = None # type: ignore[attr-defined,assignment] |
There was a problem hiding this comment.
style: Accessing private _transport attribute directly could break if the stream implementation changes. stream._transport = None on line 274 is fragile.
| # Stop all stream transports to clean up LCM/shared memory threads | |
| for stream in list(self.inputs.values()) + list(self.outputs.values()): | |
| if stream.transport is not None and hasattr(stream.transport, "stop"): | |
| stream.transport.stop() | |
| stream._transport = None # type: ignore[attr-defined,assignment] | |
| # Stop all stream transports to clean up LCM/shared memory threads | |
| for stream in list(self.inputs.values()) + list(self.outputs.values()): | |
| if stream.transport is not None and hasattr(stream.transport, "stop"): | |
| stream.transport.stop() |
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!
dimos/perception/entity_graph_db.py
Outdated
| self._local.conn = sqlite3.connect(str(self.db_path), check_same_thread=False) | ||
| self._local.conn.row_factory = sqlite3.Row | ||
| return self._local.conn # type: ignore |
There was a problem hiding this comment.
style: SQLite connection created with check_same_thread=False but stored in thread-local storage. The check_same_thread=False parameter is unnecessary since connections are already thread-local via self._local.
| self._local.conn = sqlite3.connect(str(self.db_path), check_same_thread=False) | |
| self._local.conn.row_factory = sqlite3.Row | |
| return self._local.conn # type: ignore | |
| if not hasattr(self._local, "conn"): | |
| self._local.conn = sqlite3.connect(str(self.db_path)) | |
| self._local.conn.row_factory = sqlite3.Row | |
| return self._local.conn # type: ignore |
| threading.Thread( | ||
| target=self._graph_db.estimate_and_save_distances, | ||
| args=(parsed, mid_frame.image, self.vlm, w_end, self.config.max_distance_pairs), | ||
| daemon=True, | ||
| ).start() |
There was a problem hiding this comment.
style: Background thread for distance estimation runs without proper exception handling or cancellation mechanism. If stop() is called while distance estimation is running, the daemon thread will be abruptly terminated, potentially causing inconsistent database state. Ensure _stopped flag is checked inside estimate_and_save_distances() and graph DB commit happens safely.
dimos/perception/clip_filter.py
Outdated
| def _image_to_pil(self, image: Image) -> "PILImage.Image": | ||
| """Convert dimos Image to PIL Image.""" | ||
| # Get numpy array from dimos Image | ||
| img_array = image.data # Assumes Image has .data attribute with numpy array |
There was a problem hiding this comment.
style: Assumes Image has .data attribute containing numpy array. This assumption is fragile and will silently fail or produce incorrect results if the attribute doesn't exist or contains different data than expected.
dimos/perception/clip_filter.py
Outdated
| prev = frames[i - 1].image.data.astype(float) | ||
| curr = frames[i].image.data.astype(float) | ||
| diffs.append(np.abs(curr - prev).mean()) |
There was a problem hiding this comment.
style: Same .data attribute assumption here. If image.data doesn't exist, this will raise AttributeError. Consider adding error handling or using a more robust accessor method.
| diff = np.abs(first_img.data.astype(float) - last_img.data.astype(float)) | ||
| return bool(diff.mean() < stale_threshold) |
There was a problem hiding this comment.
style: Same .data attribute assumption. This pattern appears throughout the codebase - ensure the Image class consistently provides this attribute.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 581471f2f7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
dimos/perception/clip_filter.py
Outdated
| # Select frame with minimum max similarity (most different from all selected) | ||
| best_idx = int(max_similarities.argmin().item()) | ||
|
|
||
| selected_indices.append(remaining_indices[best_idx]) | ||
| remaining_indices.pop(best_idx) |
There was a problem hiding this comment.
In CLIPFrameFilter.select_diverse_frames, the selection update lines are outside the while loop, so selected_indices and remaining_indices never change inside the loop. When CLIP is available and len(frames) > max_frames, the loop condition stays true forever and the call hangs (e.g., temporal memory analysis with CLIP filtering enabled). These lines need to be indented into the loop body so the loop makes progress each iteration.
Useful? React with 👍 / 👎.
dimos/models/vl/openai.py
Outdated
| response = self._client.chat.completions.create(**api_kwargs) | ||
| return [response.choices[0].message.content] # type: ignore[list-item] |
There was a problem hiding this comment.
OpenAIVlModel.query_batch returns a list with a single response even when multiple images are provided. VlModel.query_batch documents one response per image, and callers that index results by frame or assert len(results) == len(images) will mis-handle outputs. If the API call is batched, the method should parse a per-image response (e.g., structured output) or fall back to sequential queries to keep the contract.
Useful? React with 👍 / 👎.
dimos/models/vl/qwen.py
Outdated
| response = self._client.chat.completions.create(**api_kwargs) | ||
| return [response.choices[0].message.content] # type: ignore[list-item] |
There was a problem hiding this comment.
QwenVlModel.query_batch returns a list with a single response even when multiple images are provided. This violates the base VlModel.query_batch contract (one response per image) and can break callers that depend on result count matching input frames. Consider returning a per-image list (via structured output) or using the default sequential implementation.
Useful? React with 👍 / 👎.
|
|
||
| # Disable Rerun for tests (prevents viewer spawn and gRPC flush errors) | ||
| _BUILD_WITHOUT_RERUN = { | ||
| "global_config": GlobalConfig(rerun_enabled=False, viewer_backend="foxglove"), |
There was a problem hiding this comment.
oh jk this isn't in experimental/ its in core. probably will need to remove this since will mess with all other tests
There was a problem hiding this comment.
wait i just copied this from main branch? it isn't an edit
There was a problem hiding this comment.
It showed a diff above for some reason
dimos/stream/video_operators.py
Outdated
| from datetime import datetime, timedelta | ||
| from enum import Enum | ||
| from typing import TYPE_CHECKING | ||
| from typing import TYPE_CHECKING, Any |
dimos/models/vl/base.py
Outdated
| return image.resize_to_fit(max_w, max_h) | ||
| return image, 1.0 | ||
|
|
||
| # Note: No custom pickle methods needed. In practice, VlModel instances |
dimos/models/vl/base.py
Outdated
| Default implementation calls query() for each image sequentially. | ||
| Subclasses may override for more efficient batched inference. | ||
| Subclasses may override for efficient batched inference. |
There was a problem hiding this comment.
Unnecessary comment changes mess up the git blame
dimos/models/vl/base.py
Outdated
| """ | ||
| warnings.warn( | ||
| f"{self.__class__.__name__}.query_batch() is using default sequential implementation. " | ||
| f"{self.__class__.__name__}.query_batch() using sequential implementation. " |
|
|
||
| self.device = device or ("cuda" if torch.cuda.is_available() else "cpu") | ||
| logger.info(f"Loading CLIP {model_name} on {self.device}") | ||
| self.model, self.preprocess = clip.load(model_name, device=self.device) |
There was a problem hiding this comment.
Need to use CLIP in dimensional not a new clip. If you wanted to use this version of clip also didnt add the new library to pip so all temporal memory tests fail if one builds a clean env
dimos/models/vl/moondream.py
Outdated
| return str(result) | ||
|
|
||
| def query_batch(self, images: list[Image], query: str, **kwargs) -> list[str]: # type: ignore[no-untyped-def] | ||
| def query_batch(self, images: list[Image], query: str, response_format: dict[str, Any] | None = None, **kwargs) -> list[str]: # type: ignore[no-untyped-def,override] |
There was a problem hiding this comment.
ALso here this is unrelated to this PR needs to be reverted
…sed RAG (#973) * temporal memory + vlm agent + blueprints * fixing module issue and style * fix skill registration * removing state functions unpickable * inheritancefixes and memory management * docstring for query * microcommit: fixing memory buffer * sharpness filter and simplified frame filtering * CI code cleanup * initial graph database implementation * db implementation, working and stylized, best reply is unitree_go2_office_walk2 * type checking issues * final edits, move into experimental, revert non-memory code edits, typechecking * persistent db flag enabled in config * Fix test to not run in CI due to LFS pull * Fix CLIP filter to use dimensional clip * Add path to temporal memory * revert video operators * Revert moondream * added temporal memory docs * Refactor move to /experimental/temporal_memory --------- Co-authored-by: Paul Nechifor <paul@nechifor.net> Co-authored-by: Stash Pomichter <pomichterstash@gmail.com> Co-authored-by: shreyasrajesh0308 <shreyasrajesh0308@users.noreply.github.com> Co-authored-by: spomichter <12108168+spomichter@users.noreply.github.com>
…sed RAG (#973) * temporal memory + vlm agent + blueprints * fixing module issue and style * fix skill registration * removing state functions unpickable * inheritancefixes and memory management * docstring for query * microcommit: fixing memory buffer * sharpness filter and simplified frame filtering * CI code cleanup * initial graph database implementation * db implementation, working and stylized, best reply is unitree_go2_office_walk2 * type checking issues * final edits, move into experimental, revert non-memory code edits, typechecking * persistent db flag enabled in config * Fix test to not run in CI due to LFS pull * Fix CLIP filter to use dimensional clip * Add path to temporal memory * revert video operators * Revert moondream * added temporal memory docs * Refactor move to /experimental/temporal_memory --------- Co-authored-by: Paul Nechifor <paul@nechifor.net> Co-authored-by: Stash Pomichter <pomichterstash@gmail.com> Co-authored-by: shreyasrajesh0308 <shreyasrajesh0308@users.noreply.github.com> Co-authored-by: spomichter <12108168+spomichter@users.noreply.github.com>
* feat(sim): add MJLab G1 velocity policy profile
Introduce a 'mujoco_profile' concept allowing self-contained MuJoCo
simulation bundles (MJCF + ONNX policy + assets) to be loaded by name.
Key changes:
- GlobalConfig: new 'mujoco_profile' field (--mujoco-profile CLI flag)
- model.py: profile-scoped asset loading and bundle.json support
- mujoco_process.py: read camera names from bundle.json per profile
- policy.py: MjlabVelocityOnnxController reads joint_names,
default_joint_pos, action_scale from ONNX metadata for exact
MJLab action contract (per-joint scaling & named ordering)
- mujoco_connection.py: skip menagerie download when profile is set
- blueprints.py / rerun_init.py: gate Rerun init on rerun_enabled
Bundle added to data/.lfs/mujoco_sim.tar.gz (LFS-tracked):
data/mujoco_sim/unitree_g1_mjlab/
├── model.xml (MJLab-compiled G1 MJCF with correct actuators)
├── policy.onnx (trained velocity policy with metadata)
├── bundle.json (camera name mappings)
└── assets/ (STL meshes from MJLab asset_zoo)
Usage:
dimos --simulation \
--mujoco-profile unitree_g1_mjlab \
run unitree-g1-basic-sim
* CI code cleanup
* fix(sim): resolve meshdir/profile asset conflicts for GO2 and G1
- mujoco_process.py: only use mujoco_profile when explicitly set
(fixes GO2 accidentally being treated as a profile bundle)
- model.py: rewrite scene XML to remove meshdir/texturedir attrs
and prefix mesh/texture filenames explicitly, preventing scene
compiler settings from hijacking robot mesh resolution
* configure unitree go2 mapper to use 10 cm voxels (#1032)
* feat(sim): add MuJoCo subprocess profiler for performance debugging
Adds a built-in timing breakdown for the MuJoCo simulation subprocess.
When enabled, logs rolling averages of time spent in each component:
- physics_ms: mj_step loop
- viewer_sync_ms: MuJoCo viewer synchronization
- rgb_render_ms, depth_render_ms: camera rendering
- pcd_ms: depth-to-pointcloud + voxel downsample
- *_shm_ms: shared memory writes
- ctrl_calls, ctrl_obs_ms, ctrl_onnx_ms: policy cost breakdown
This helps diagnose performance issues (e.g. 'molasses' effect) by
showing exactly where frame time is being spent.
Usage:
# Standard G1 sim with profiler:
dimos --simulation --mujoco-profiler --mujoco-profiler-interval-s 2 run unitree-g1-basic-sim
# MJLab bundle with profiler:
dimos --simulation --mujoco-profile unitree_g1_mjlab --mujoco-profiler --mujoco-profiler-interval-s 2 run unitree-g1-basic-sim
New GlobalConfig flags:
- mujoco_profiler: bool (default False)
- mujoco_profiler_interval_s: float (default 2.0)
* pre commit
* small docs clarification (#1043)
* Fix split view on wide monitors (#1048)
* fix print to be correct URL based on rerun web or not
* make width of rerun/command center adjustable
* swap sides
* Docs: Install & Develop (#1022)
* minimal edit
* rice the readme
* grammar
* formatting
* fix examples
* change links to reduce change count
* improve wording
* wording
* remove acknowledgements
* improve the humancli example
* formatting
* Update README.md
* switch to dev branch for development
* changes for paul
* Update README.md
* fix broken link
* update broken link
* Add uv to nix and fix resulting problems (#1021)
* add uv to nix and fix resulting problems
* fix for linux
* v0.0.8 version update (#1050)
* Style changes in docs (#1051)
* capitalization
* punctuation
* more small fixes
* Revert "Add uv to nix and fix resulting problems (#1021)" (#1053)
This reverts commit 8af8f8f.
* Transport benchmarks & Raw ROS transport (#1038)
* raw rospubsub and benchmarks
* typefixes, shm added to the benchmark
* SHM is not so important to tell us every time when it starts
* greptile comments
* Add co-authorship line to commit message filter patterns
* Remove unused contextmanager import
* lcmservice correct kernel settings reintroduced
* mixin mixin resolved
* lcmservice tests fix
* macos lcm rmem fix
* feat: default to rerun-web and auto-open browser on startup (#1019)
- Changed GlobalConfig.viewer_backend default from rerun-native to rerun-web
- WebsocketVisModule now opens dashboard in browser automatically on start
- Requested by Jeff
Co-authored-by: s <pomichterstash@gmail.com>
* chore: fix indentation in blueprints ambiguity check
* CI code cleanup
* use p controller to stop oscillations on unitree go2 (#1014)
* Dynamic session providers for onnxruntime (#983)
* refactor(policy): update inference session initialization
* refactor(policy): simplify inference session provider initialization
* Log the policy directory and provider
* Perception Full Refactor and Cleanup, deprecated Manipulation AIO Pipeline and replaced with Object Scene Registration (#936)
* added rate limiting and backpressure to pointcloud publishing
CI code cleanup
updated ZED module to the same standard as realsense
CI code cleanup
fixed stash's comments
CI code cleanup
mypy fixes + comments
removed property of camera_info
should pass CI now
added detection3d pointcloud types from depth image
added yoloe support and 3D object segmentation
CI code cleanup
use yoloe-s instead for nuc
CI code cleanup
removed deprecated perception code
some pointcloud color changes
major refactor and added object class for object scene registration
CI code cleanup
refactored, added objectDB for persistent object memory
CI code cleanup
made objectDB a normal class instead of a module
CI code cleanup
revert to dev
reverted more files
CI code cleanup
completely refactored object scene registration to work natively in dimos instead of using ROS as transport. Made everything super clean and working
CI code cleanup
bug fixed + use yoloe-l by default
added yolo object exlusion list
CI code cleanup
added zed camera to the object registration demo
CI code cleanup
added image and pointclou2 fixes and as_numpy function
working promptable object scene registration
CI code cleanup
bug fixes
bug fix + remove ros imports
should not fail CI now
CI code cleanup
more CI fixes, somehow local CI did not catch
changed prompt fixed bug
CI code cleanup
reverted some changes
Cleanup very dead code and fixed mypy errors
CI code cleanup
fixed more mypy
CI code cleanup
* one last mypy fix
* added default to imagedetection2d to not set off mypy
* fixed bug and default to open vocab for detection
* mypy fixes
* fixed one last mypy error
* fixed all of Stash's comments
* should pass mypy now
* added uv lock
* sync uv.lock with dev
* fixed the last mypy error
* fixed mypy errors from source
* reverted mypy import error fixes
* fixed Ivan's comment
* fixed last of ivan's comment
* remove all to_ros_msgs stuff in this commit
* passed Ivan's detector tests
* added README for depth camera integration
* fixed last of Stash's comments
* feat(cli): type-free topic echo via /topic#pkg.Msg inference, this mi… (#988)
* feat(cli): type-free topic echo via /topic#pkg.Msg inference, this mirrors ros topic echo functionality.
- Make type_name optional in 'dimos topic echo'
- Infer message type from LCM channel suffix (e.g. /odom#nav_msgs.Odometry)
- Dynamically import dimos.msgs.<pkg> and call cls.lcm_decode(data)
- Keep existing explicit-type mode working
- Update transports.md docs
* fix(cli): use LCMPubSubBase instead of raw lcm.LCM for topic echo, my bad
* verify blueprints (#1018)
* verify blueprints
* Fix geometry msgs check failure in CI
---------
Co-authored-by: stash <pomichterstash@gmail.com>
* Experimental Streamed Temporal Memory with SpatioTemporal & Entity based RAG (#973)
* temporal memory + vlm agent + blueprints
* fixing module issue and style
* fix skill registration
* removing state functions unpickable
* inheritancefixes and memory management
* docstring for query
* microcommit: fixing memory buffer
* sharpness filter and simplified frame filtering
* CI code cleanup
* initial graph database implementation
* db implementation, working and stylized, best reply is unitree_go2_office_walk2
* type checking issues
* final edits, move into experimental, revert non-memory code edits, typechecking
* persistent db flag enabled in config
* Fix test to not run in CI due to LFS pull
* Fix CLIP filter to use dimensional clip
* Add path to temporal memory
* revert video operators
* Revert moondream
* added temporal memory docs
* Refactor move to /experimental/temporal_memory
---------
Co-authored-by: Paul Nechifor <paul@nechifor.net>
Co-authored-by: Stash Pomichter <pomichterstash@gmail.com>
Co-authored-by: shreyasrajesh0308 <shreyasrajesh0308@users.noreply.github.com>
Co-authored-by: spomichter <12108168+spomichter@users.noreply.github.com>
* Control Orchestrator - Unified Controller for multi-arm and full body controller (#970)
* archive old driver to manipulators_old for redesign
* spec.py defining minimal protocol for an arm driver
* xarm driver driver added - driver owns control thread and robot state threads also invokes rpc calls to arm specific SDK backends
* xarm SDK specific wrapper to interface with dimos RPC calls from the driver
* removed type checking for old armdriver spec from the cartesian controller
* replicated piper driver to meet the new architecture
* added mock backend
* updated all blueprints to add new arm module
* Added readme explaining new driver architecture overview
* config now parsed in backend init instead of connect method
* addded dual arm control blueprint using trajectory controller
* adding a control orchestrator for single control loop for multiple arms and joint control - added dataclasses for orchestrator and protocol for ControlTask
* hardware interface protocol that wraps specific arm SDK to work with orchestrator. Also solves namespace for multiple arm and hardware
* main orchestrator module and control loop that claims resources computes next commands, and arbitrates priority of different tasks and controllers
* added a trajectory task implementation that performs trajecotry control
* added blueprints to launch orchestratory module with differnt arms for testing
* updated blueprints to add piper + xarm blueprint
* orchestrator client that can send tasks to the control orchestrator module
* added a readme
* added pytest and e2e test
* Update dimos/control/hardware_interface.py
explicit false added to the Torque Mode command sent, to avoid silent failing scenario
* CI code cleanup
* Fixed issues flagged by greptile
Mode conflict detection in routing: Added check in _route_to_hardware
Preemption tracking: Changed structure to {preempted_task: {joint: winning_task}}
Mode conflict preemption: Tasks dropped due to mode conflict at same priority
Trajectory completion edge case: Returns final position instead of None on completion
Dead code removal: and Piper backend cleanup
* Renamed deprecated old manipuialtion test file and Mypy type fixes
* fix mypy test
* mypy test fix added explicit type
* Remove deprecated manipulators_old folder
* fixed redef error in dual trajectory setter
* Fixed bugs identified by greptile overview:
1. tick_loop.py - Race condition in _route_to_hardware
2. orchestrator.py - Added hardware_added tracking list and rollback in outer except block
3. hardware_interface.py - Added disconnect() to both HardwareInterface protocol and BackendHardwareInterface
4. Added disconnect() to both HardwareInterface protocol and BackendHardwareInterface
5. orchestrator.py - Start order fix Moved super().start() to end, after tick loop starts successfully
6. trajectory_task.py - Added Empty joint_names validation
* addressed greptile suggestion:
hardware_interface.py - Torque mode logging fix
orchestrator.py - Fail hardware removal if joints in use
tick_loop.py - Rate control drift fix
* undo change to pyproject.toml
* Replaced _running bool with threading.Event (_stop_event) for thread safety
Removed duplicate _auto_start() call from __init__ - connection now only happens in start()
orchestrator_client.py IPython conversion
* added type ignore for ipythin
* removed check for has attribute in hardware interface
Moved super.start() at the beginning
replaced running bool with stop_event in tick_loop to improve thread safety
removed default ip from init
removed simple dataclasses test
* orchestrator.py: Use match statement for backend factory, restructure backend cleanup
task.py: Use match statement in get_values()
tick_loop.py: Add JointWinner NamedTuple for cleaner arbitration logic
xarm/backend.py: Extract unit conversions into static helper methods
* tick_loop.py: Notify preemption when lower-priority task loses to existing winner
hardware_interface.py: Call set_control_mode() before mode-specific writes, Convert if/elif to match statement for control mode dispatch
* tick_loop.py: Notify preemption when lower-priority task loses to existing winner
hardware_interface.py: Call set_control_mode() before mode-specific writes, convert if/elif to match statement
trajectory_task.py: Defer start time to first compute() for consistent timing
orchestrator.py: Extract _setup_hardware() helper for cleaner config setup
piper and xarm/backend.py: Fail fast on read_joint_positions(), map SERVO_POSITION to mode 1
hardware_interface.py: Retry initialization with proper error propagation,
spec.py: Add SERVO_POSITION control mode for confusion between position planning and position servo
task.py: added SERVO_POSITION to JointCommandOutput helper
* cleaned up legacy blueprints for manipulator drivers
* enforce ManipulatorBackend Protocol on the backend.py
* feat: add runtime protocol checks for manipulator backends
* added runtime checking for controlTask protocol
* Add TaskStatus dataclass, refactor get_trajectory_status and Explicitly inherit from ControlTask protocol
---------
Co-authored-by: stash <pomichterstash@gmail.com>
* configure unitree go2 mapper to use 10 cm voxels (#1032)
* Create DDSPubSubBase, DDSTopic
* Create PickleDDS
* Fix hash/equality inconsistency in DDSTopic
* Add DDSMsg
* Create DDSTransport
* Add broadcast and subscribe methods to DDSTransport
* Create DDSService
* Add CycloneDDS package
* Remove unnecessary attributes
* Add threading and serialization methods to DDSService
* Ensure broadcast and subscribe methods initialize DDS if not started
* Add Transport benchmarking capabilities to CycloneDDS (#1055)
* raw rospubsub and benchmarks
* typefixes, shm added to the benchmark
* SHM is not so important to tell us every time when it starts
* greptile comments
* Add co-authorship line to commit message filter patterns
* Remove unused contextmanager import
---------
Co-authored-by: Ivan Nikolic <lesh@sysphere.org>
* Fix DDS segmentation fault using bytearray for binary data storage
Replace base64 string encoding with native IDL bytearray type to eliminate
buffer overflow issues. The original base64 encoding exceeded CycloneDDS's
default string size limit (~256 bytes) and caused crashes on messages >= 1KB.
Key changes:
- Use make_idl_struct with bytearray field instead of string
- Convert bytes to bytearray when publishing to DDS
- Convert bytearray back to bytes when receiving from DDS
- Add _DDSMessageListener for async message dispatch
- Implement thread-safe DataWriter/DataReader management
- Add pickle support via __getstate__/__setstate__
Result: All 12 DDS benchmark tests pass (64B to 10MB messages).
* Refactor DDS PubSub implementation to use CycloneDDS Topic
* Remove DDS pickling
* CI code cleanup
* bugfix
* CI code cleanup
---------
Co-authored-by: leshy <lesh@sysphere.org>
Co-authored-by: Jeff Hykin <jeff.hykin@gmail.com>
Co-authored-by: Paul Nechifor <paul@nechifor.net>
Co-authored-by: s <pomichterstash@gmail.com>
Co-authored-by: Miguel Villa Floran <miguel.villafloran@gmail.com>
Co-authored-by: alexlin2 <44330195+alexlin2@users.noreply.github.com>
Co-authored-by: claire wang <clara32356@gmail.com>
Co-authored-by: shreyasrajesh0308 <shreyasrajesh0308@users.noreply.github.com>
Co-authored-by: spomichter <12108168+spomichter@users.noreply.github.com>
Co-authored-by: Mustafa Bhadsorawala <39084056+mustafab0@users.noreply.github.com>
new temporal PR based on codebase refactor. stilll todo: fix pickling issue, add entity graphs.
Temporal memory runs "Temporal/Spatial RAG" on streamed videos building an continuous entity-based
memory over time. It uses a VLM to extract evidence in sliding windows, tracks
entities across windows, maintains a rolling summary, and stores relations in a graph network.
Methodology
Setup
.env:OPENAI_API_KEY=...Quickstart
To run:
dimos --replay run unitree-go2-temporal-memoryIn another terminal:
humanclito chat with the agent and run memory queries.Artifacts
By default, artifacts are written under
assets/temporal_memory:evidence.jsonl(window evidence: captions, entities, relations)state.json(rolling summary + roster state)entities.json(current entity roster)frames_index.jsonl(timestamps for saved frames; written on stop)entity_graph.db(SQLite graph of relations/distances)Notes
TemporalMemoryConfig(output_dir=...).