fix(test): rerank corpus test needs q8 weights, not q4_k_m - #50
Open
lloyal-research wants to merge 2 commits into
Open
fix(test): rerank corpus test needs q8 weights, not q4_k_m#50lloyal-research wants to merge 2 commits into
lloyal-research wants to merge 2 commits into
Conversation
testRerankLargeCorpus scores 20 documents on q4_0 KV. At q4_k_m weights that stacks two lossy axes, and the pointwise judge returns NEGATIVE for every document, including the correct one — Paris scores -5.5427 and ranks 5th, behind "The Great Wall of China is over 13,000 miles long". The ranking is noise among rejects, not a near-miss between similar documents. Measured 2x2; only that corner fails: q4_k_m + q4_0 KV -> rank 5, Paris -5.5427 (trails rank 2 by 1.23) q4_k_m + f16 KV -> rank 0, Paris +3.3558 q8 + q4_0 KV -> rank 0, Paris +1.7955 q8 + f16 KV -> rank 0, Paris +4.7235 The 1.23 deficit is the size of q4_0 KV's own noise floor (~1.27 logits). Fixing either axis restores rank 0; q8 is the one that keeps the quantized-KV path under test, and it matches what rig's catalog actually ships (qwen3-reranker-0.6b-q8) — the test was the only place exercising a reranker quant that is never shipped. Also corrects __rerank-large-corpus-standalone.ts, which attributed the failure to nSeqMax: the 2-lease tax for trunk+queryBranch means the test's hardcoded 8 yields 6 effective leaves, and the comment claimed the resulting GEMM tiling was what q4_k_m could not distinguish. Measured: the relevant doc scores -5.5427 at BOTH nSeqMax 8 and 10, identical to four decimals. Only the distractors shuffle (doc 13: -1.347 -> -3.698), moving it 5th to 4th — still failing. Tiling is not the cause. Its docstring also referenced a decode_each KV-pressure crash that no longer reproduces (suite runs clean). Registers the reranker in test/matrix.json so download-test-models.sh provisions it; previously no reranker was in the matrix at all and all three rerank tests silently skipped on a fresh clone.
There was a problem hiding this comment.
Pull request overview
Stabilizes the testRerankLargeCorpus integration coverage by switching the reranker model used in tests from q4_k_m to q8, and ensures the reranker is provisioned in fresh checkouts by registering it in the test model matrix. Also updates the standalone rerank harness commentary to reflect measured behavior and its intended purpose.
Changes:
- Register
qwen3-reranker-0.6b-q8intest/matrix.jsonso rerank sidecar models are downloaded for tests. - Update integration rerank model auto-detection to prefer the shipped
q8reranker model (instead ofq4_k_m) with measured rationale in comments. - Update the standalone large-corpus rerank runner to use the
q8reranker model and correct the explanatory notes around the prior failure diagnosis.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/matrix.json | Adds the reranker model to the matrix so it is provisioned by the download script. |
| test/integration.ts | Switches rerank model selection to q8 and documents the quantization-related rationale. |
| test/__rerank-large-corpus-standalone.ts | Updates the standalone harness to default to the q8 reranker and refreshes diagnostic commentary. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+32
to
+34
| // returns NEGATIVE for every document, including the correct one (Paris | ||
| // -5.54, ranking 5th behind "the Great Wall is 13,000 miles long"). The | ||
| // ranking is then noise among rejects. Measured 2x2 — only that corner fails: |
Contributor
Author
There was a problem hiding this comment.
Fixed in ed9b139 — the comment now quotes the fixture verbatim ('The Great Wall of China is over 13,000 miles long.', test/integration.ts:2128) so the failure is greppable.
zuhair-naqvi
pushed a commit
that referenced
this pull request
Aug 30, 2026
Addresses Copilot review on #49, and picks up liblloyal d84f249. - CreateContext held ctx and mtmdCtx raw until initializeContext / initializeMultimodal took ownership. Anything throwing in between — a missing mmproj, a failed projector load, ctor.New({}), Unwrap — leaked them; the two error paths hand-rolled llama_free(ctx) and the rest did not. An RAII guard now makes cleanup unconditional and both manual frees are gone. The leak predates multimodal (8df3392); mtmd widened it. - _storePrefillMultimodal accepted ANY typed array while the error message said Buffer/Uint8Array, so an Int32Array passed the guard and was reinterpreted as raw bytes. Now checks napi_uint8_array explicitly. - _storePrefillMultimodal did not reject duplicate handles. decode_scatter guards against them, but this path dispatches one handle at a time so the pair never meets there — the same branch would be prefilled twice, each advancing its position. Now fails loud like _storePrefill/_storeCommit. - README: the multimodal snippet used `handle` without creating it, and the documented options are typed through @lloyal-labs/sdk's ContextOptions, so they only typecheck once an SDK carrying them is installed. Says so. Suite: multimodal 22/22 including the fan-out. The one failure is testRerankLargeCorpus, pre-existing and unrelated — fixed in #50.
The comment paraphrased the distractor. Using the exact fixture string makes the failure greppable against test/integration.ts:2128.
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.
testRerankLargeCorpushas been failing: the relevant document ranks 5th when the test requires top-3. Root cause is quantization, and it is not a near-miss.What's actually happening
The test scores 20 documents on
q4_0KV. Atq4_k_mweights that stacks two lossy axes, and the pointwise judge returns negative for every document — including the correct one. Paris scores −5.5427 and lands 5th, behind "The Great Wall of China is over 13,000 miles long" and "The speed of light is approximately 299,792 km/s". The ranking is noise among rejects, not a close call between semantically similar documents.Measured 2×2 — only that one corner fails:
The 1.234 deficit is the size of
q4_0KV's own noise floor (~1.27 logits) — it loses by exactly the amount of noise being injected.Fixing either axis restores rank 0. This takes q8 weights, because that keeps the quantized-KV path under test rather than quietly dropping coverage — and because rig's catalog already ships
qwen3-reranker-0.6b-q8. The test was the only place exercising a reranker quant that is never shipped in production.Also corrected: a wrong diagnosis in the standalone harness
__rerank-large-corpus-standalone.tsattributed this tonSeqMax. The observation behind it is real — the 2-lease tax for trunk + queryBranch means the test's hardcodednSeqMax: 8yields only 6 effective leaves — but the causal claim ("different GEMM tiling that q4_k_m's quantization floor can't distinguish from distractors") does not survive measurement:The relevant document's score is identical to four decimals at both settings. Only the distractors shuffle (doc 13: −1.347 → −3.698), nudging it 5th → 4th — still failing. Tiling moves rejects around; it does not touch the correct document. The comment now records that measurement instead.
Its docstring also referenced a
decode_eachKV-pressure crash at b9581 as the reason to bypass the suite; that no longer reproduces (the suite runs clean), so the docstring is updated to what the harness is actually for.Matrix
Registers the reranker in
test/matrix.jsonsodownload-test-models.shprovisions it. Previously no reranker was in the matrix at all, so on a fresh clone all three rerank tests silently skipped. Sidecars download unconditionally, which is intended here.Verification
Full suite green with the change in place.