Add reusable LRU response store for replay state#22
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a standalone, thread-safe in-memory LRU store to persist “Responses” input/output snapshots for future replay, along with pure unit tests validating save/get/eviction and replay reconstruction behavior.
Changes:
- Introduces
ResponseStore/StoredResponsebacked by anOrderedDictLRU with bounded size and locking. - Implements
replay_input()to rebuild replayable input from stored input plus selected output items (output_textparts andfunction_calls). - Adds unit tests covering validation, snapshot immutability, LRU eviction, replay behavior, and clearing.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
mlx_vlm/responses_store.py |
Adds the reusable LRU response snapshot store and replay reconstruction logic. |
mlx_vlm/tests/test_responses_store.py |
Adds pure unit tests for store semantics and replay behavior without importing the package __init__. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _load_module(name: str, filename: str): | ||
| """Load a sibling module without importing mlx_vlm.__init__.""" | ||
| module_path = Path(__file__).parent.parent / filename | ||
| spec = importlib.util.spec_from_file_location(name, module_path) |
There was a problem hiding this comment.
spec_from_file_location() can return None (e.g., if the path is wrong). As written, module_from_spec(spec) would then raise a less-informative error before the existing spec.loader assertion. Add an explicit assert spec is not None (or raise a clear error) before calling module_from_spec() to make failures easier to diagnose.
| spec = importlib.util.spec_from_file_location(name, module_path) | |
| spec = importlib.util.spec_from_file_location(name, module_path) | |
| if spec is None: | |
| raise ImportError(f"Could not load module spec for {name!r} from {module_path}") |
| def _maybe_mapping(value: Any) -> Mapping[str, Any] | None: | ||
| """Normalize dict-like or model-like objects into mappings.""" | ||
| if isinstance(value, Mapping): | ||
| return value | ||
| if hasattr(value, "model_dump"): | ||
| return value.model_dump() | ||
| if hasattr(value, "dict"): | ||
| return value.dict() |
There was a problem hiding this comment.
_maybe_mapping() returns the result of model_dump()/dict() without verifying it is actually a Mapping. If a model returns a non-mapping (or None), callers will hit attribute errors later (e.g., output_dict.get(...)). Ensure the returned value is a Mapping (otherwise return None) to match the function’s contract and keep replay_input() robust.
| return deepcopy(entry) | ||
|
|
There was a problem hiding this comment.
get() holds _lock while performing deepcopy(entry). For larger stored payloads, this can substantially increase lock hold time and block concurrent save()/get() calls. Consider copying outside the critical section (grab the entry + update LRU order under the lock, then deepcopy after releasing it).
| return deepcopy(entry) | |
| return deepcopy(entry) |
| if isinstance(original_input, str): | ||
| items.append({"role": "user", "content": original_input}) | ||
| elif isinstance(original_input, list): | ||
| items.extend(deepcopy(original_input)) |
There was a problem hiding this comment.
In replay_input(), entry is already a deep-copied snapshot (via self.get()), but the list-input branch deep-copies original_input again before extending. This extra deepcopy is redundant and can be expensive for large histories; consider extending directly from original_input here.
| items.extend(deepcopy(original_input)) | |
| items.extend(original_input) |
Summary
Notes
/responsesyet