Repository navigation
Address the six Phase 7.3b WP3 review findings - #55
Merged
Merged
Conversation
The method's Javadoc promised that decoding before prefill finishes has no valid coordinate, but it only rejected a negative result, so a mid-prompt cache position returned a meaningless coordinate. Prefill is the only consumer of per-token prompt coordinates (no production call site passes a mid-prompt position), so tighten the contract: reject cachePosition < expandedLength() and drop the now-unreachable negative-result check (verifyDelta plus the non-negative axes make the result provably positive once cachePosition >= expandedLength). Update the three affected test assertions to express the chosen contract.
rederive_rope_all now honors the frame count of the reference get_rope_index image path: T = base + frame index while H/W restart per frame, and the cursor advances past the whole t * lh * lw run (modular_qwen3_vl.py:929-944). The t = 1 output is unchanged. Note in the script and in the phase7-3b findings (section 6.8) that the pinned WP3 golden is t = 1 only, so the t > 1 path is verified by logic against the reference, not by golden coverage. Verified: golden re-derives byte-identically under the pinned venv, and the extended re-derivation matches Qwen3VLModel.get_rope_index exactly on synthetic t = 2, 3 and 5 frame grids plus a t = 1 regression.
Add the opt-in qwen3vl_rope_multiframe_check.py, which compares qwen3vl_wp3_prompt.rederive_rope_all against the pinned 4.57.6 Qwen3VLModel.get_rope_index on synthetic multi-frame image grids (t = 1, 2, 3 and 5) plus a two-image prompt with a text run between the images, requiring exact equality of all three position axes and the mrope delta. It runs under the pinned venv, is never Gradle- invoked and writes no golden, so nothing changes in goldens/ or provenance.json; point the findings section 6.8 bullet at it.
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
1 participant
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.
Resolves all six findings from the WP3 code review (all low severity), one commit per finding:
1b8b142) —Qwen3VlPromptTokens.resolvenow resolves the four Qwen3-VL sentinels through a new neutralHfTokenizer.tokenId(String)(plain name-to-ID vocabulary lookup, no EOS/special-token role semantics) instead ofeosTokenId. New focused tokenizer test.a36361a) —resolvenow skips the video sentinel vocabulary lookup when the config declaresvideo_token_id = -1(the record's "no video token" convention), instead of always failing. Test covers both a fixture with the video added token removed and the full fixture.ropePositionAtcontract tightened (f817113) — the method now rejectscachePosition < expandedLength(), matching what its Javadoc already promised; the old negative-result check is provably unreachable and was dropped. Affected tests now assert the rejection.109abb5) —goldenExpansionsAndPositionsMatchThePinnedReferenceandsingleImageCasesReproduceTheWp1ProbeNumbersnow assert they exercised exactly 16 and 2 pinned case variants respectively, so an empty or key-renamed golden fails loudly.feab8a4) — aSTART VIDEO ENDwrapper now reports the "video is not supported" error (via a sharedvideoErrorhelper, all other messages byte-for-byte unchanged) instead of the less helpful malformed-wrapper message. New test.95f41e3) —rederive_rope_allin the WP3 Python tool now handles t > 1 faithfully to the pinned 4.57.6 reference (T = base + frame, H/W restart per frame); the pinned WP3 golden is t = 1 only, so the t > 1 path is verified by logic againstget_rope_index(exact match on synthetic t = 2/3/5 grids), now noted in the script and findings §6.8.Verification (local, macOS 26 / Apple Silicon):
:jmlx-tokenizer:checkgreen — 206 tests, 0 failures (includes the newtokenIdtest).:jmlx-models:checkgreen — 372 ordinary + 31 float32Golden + thread probe tests, 0 failures, 0 skipped (native suites ran; CI-execution assertion passed).verifyHfReferenceGoldensgreen; the WP3 golden re-derives byte-identically under the pinned venv after the t > 1 extension (no golden or provenance changes in this PR).git diff --checkclean; spotless/checkstyle green on both touched modules.No behavior change to the accepted WP3 token contract or the pinned artifact; no native pins, bindings, or fixture changes.