Fix previous mm data copy#67
Open
cdreetz wants to merge 1 commit into
Open
Conversation
ApprovabilityVerdict: Approved Straightforward bug fix converting shallow copies to deep copies to prevent unintended mutation of multimodal data during turn bridging. The change is small, isolated, and includes a comprehensive regression test. You can customize Macroscope's approvability policy. Learn more. |
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
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.
Summary:
dict(previous_multi_modal_data.mm_items) copies only the outer dictionary, but the per-modality values are still the exact same list objects, so calling .extend(...) on merged_items["image"] also mutates previous_multi_modal_data.mm_items["image"].
Note
Low Risk
Localized fix to multimodal merge logic in two renderers plus a regression test; no auth, API, or inference-path changes.
Overview
Fixes a shallow-copy bug in
bridge_to_next_turnfor Qwen3.5 and Qwen3-VL when merging prior-turnMultiModalDatawith the new turn.Merging used
dict(...)onmm_hashes,mm_placeholders, andmm_items, so per-modality lists stayed aliased toprevious_multi_modal_data. Appending with.extend()mutated the caller’s prior snapshot—problematic when trainers keep per-stepRenderedTokensfor loss reconstruction.The bridge now builds fresh outer dicts and copies each modality’s list (
list(vals)per key) before extending with new-turn entries.Adds
test_multimodal_bridge_does_not_mutate_previous_mm_dataacross the multimodal matrix: prior lists unchanged after bridge, and bridged inner lists are not the same objects as the prior turn’s.Reviewed by Cursor Bugbot for commit 2a8fba0. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Fix mutation of previous turn's multimodal data in
bridge_to_next_turndict(...)copies ofmm_hashes,mm_placeholders, andmm_itemswith dict comprehensions that copy each per-modality list inqwen35.pyandqwen3_vl.py.MultiModalData's lists due to shared list references.tests/test_multimodal.pythat asserts previous turn mm data is neither mutated nor aliased after bridging.Macroscope summarized 2a8fba0.