Skip to content

Add WinML reranking support for cross-encoder/ms-marco-MiniLM-L6-v2 - #1322

Open
ssss141414 wants to merge 15 commits into
mainfrom
producer/ms-marco-minilm-l6-v2-reranking
Open

ssss141414 wants to merge 15 commits into
mainfrom
producer/ms-marco-minilm-l6-v2-reranking

Conversation

@ssss141414

@ssss141414 ssss141414 commented Aug 20, 2026 •

Copy link
Copy Markdown
Contributor

Summary

This PR adds canonical WinML reranking support for cross-encoder/ms-marco-MiniLM-L6-v2, including task resolution, pairwise inference, grouped evaluation, ranking metrics, a bounded MS MARCO fixture builder, and CPU fp32/fp16 recipes. It ships Effort L2 / Outcome L2 and reaches the committed Goal L3: L0-L3 PASS with full required CPU fp32/fp16 coverage on final candidate 11422d97cc9a31ffdf64fb98462976b13ad25c19. The L3 result is a bounded functional smoke only, not representative accuracy or benchmark quality.

Model metadata

What the model does

This English cross-encoder jointly tokenizes a query and candidate passage, encodes the pair with a six-layer MiniLM/BERT encoder, and emits one raw relevance logit used to rank passages.

  • Evidence/confidence: pinned Hugging Face metadata/config and the candidate reranking evaluator contract (verified).

Primary user stories

  • A user supplies a search query and candidate passages to obtain a relevance ordering for second-stage retrieval reranking (verified; checkpoint pipeline_tag=text-ranking and MS MARCO passage-ranking model-card family).
  • A user supplies a query and a bounded evidence set to select the most relevant passages before downstream question answering or display (inferred from verified text-ranking behavior).

Supported tasks

Task Support surface Evidence Confidence
text-ranking checkpoint Pinned checkpoint pipeline_tag verified
text-classification Transformers / Optimum ONNX BertForSequenceClassification export boundary mapped
reranking WinML Resolution, inference, evaluator, metric, and recipes in this contribution mapped

Model architecture

BertForSequenceClassification
+-- BertModel
|   +-- BertEmbeddings (vocab 30522, hidden 384, positions 512)
|   +-- BertEncoder x 6
|   |   +-- Self-attention (12 heads)
|   |   +-- Feed-forward (384 -> 1536 -> 384)
|   |   \-- Residual + LayerNorm
|   \-- BertPooler
+-- Dropout
\-- Linear relevance head (384 -> 1 raw logit)
  • Source/confidence: pinned checkpoint revision 233902d25c440f23af6f7d6e94d2946bac0bee0a, BertForSequenceClassification source structure, and checked-in recipe I/O (verified).

Validation and support evidence

Baseline

The planner required a FULL-RERUN because the previous validated base 02179d1 to exact base f4e7eafd4e2365016371686dc2d86fa03bfc475a changed dependencies and shared config/build/export/quantization/runtime/perf/eval/analyze/CI surfaces and directly overlapped seven contribution paths. No historical executable stage was reused. Fresh base execution used the current-main WinML 0.3.1 declaration:

  • Auto-config succeeded and selected loader.task: text-classification, AutoModelForSequenceClassification, BERT, opset 17, and three [1, 512] INT32 inputs.
  • Recipe-free CPU build succeeded in 19.4 seconds.
  • CPU perf succeeded at 57.72 ms mean, 56.38 ms p50, and 17.32 samples/s.
  • Canonical reranking schema resolution was unavailable (exit 2); legacy text-classification Eval rejected the checkpoint/dataset label alignment (exit 1).
  • Optimum's established export boundary is vendor text-classification; WinML adds the reranking semantic surface while retaining that exporter. The planner's direct pre-rebase registry import was unavailable because of a stale Optimum/Transformers symbol mismatch, while candidate inspect resolved BertIOConfig through TasksManager.

The producer rebased all 15 contribution commits onto exact base f4e7eafd.... Range-diff paired all commits in order; 14 were exact and one retained both this PR's RerankingMetric registration and upstream's TensorSimilarityMetric registration. All 24 owned paths retained identical contribution line counts, and the final branch/PR head is 11422d97....

Goal

  • Effort: L2, because this adds a shared reranking task family across resolution, inference, evaluator, metrics, dataset handling, utilities, tests, fixture generation, and recipes.
  • Goal ceiling: L3, requiring L0 build/structure, L1 perf, L2 PyTorch parity with ranking agreement, and one bounded final-SHA fp32 CPU functional smoke.
  • Outcome: L2, shipping the shared reranking implementation, fixture builder, CPU recipes, regressions, and task-family evidence.
  • Success definition: both required CPU fp32/fp16 tuples pass L0 and L1 with concrete measurements; both precisions pass semantic parity; one final-SHA fp32 CPU smoke validates grouped reranking semantics and independently reproduced metrics.

Outcome

Tester verdict: PASS_WITH_ENVIRONMENT_EXCEPTIONS. L0, L1, L2, and L3 all PASS; the highest Goal verdict is L3 PASS. Coverage is full, with no deferred tuples.

Shipped recipes:

  • examples/recipes/cross-encoder_ms-marco-MiniLM-L6-v2/cpu/cpu/reranking_fp32_config.json
  • examples/recipes/cross-encoder_ms-marco-MiniLM-L6-v2/cpu/cpu/reranking_fp16_config.json

The learner appended checkpoint-scoped BERT knowledge for exact CPU recipe/HTP structure, fp16 size/performance behavior, fp32/fp16 ranking parity, and bounded pinned-source fixture semantics. No new methodology finding or feature-gap issue was required.

Per-EP/device/precision results and Functional smoke Eval

Goal ladder

Tier Verdict Evidence
L0 PASS Both CPU recipes build; correct opset, named I/O, true initializer precision, external data, and approximately half-size fp16 artifact
L1 PASS Concrete CPU latency, throughput, RAM, provider, and precision evidence for fp32 and fp16
L2 PASS Both precisions preserve complete descending reranking order and top-1 against PyTorch over two groups / eight pairs
L3 PASS Final-SHA fp32 CPU grouped reranking smoke; CLI metrics exactly match independent recomputation

Required CPU tuples

Tier EP / Device Precision Verdict Artifact / graph Mean p50 Throughput RAM delta VRAM delta
L0/L1 CPUExecutionProvider / cpu fp32 PASS 90,921,926 bytes; 194 nodes; 108 FLOAT + 31 INT64 initializers 61.00 ms 60.36 ms 16.39 samples/s +607.3 MiB not applicable
L0/L1 CPUExecutionProvider / cpu fp16 PASS 45,494,814 bytes; 195 nodes; 108 FLOAT16 + 31 INT64 initializers 85.02 ms 84.02 ms 11.76 samples/s +605.8 MiB not applicable

Both artifacts use opset 17; named input_ids, attention_mask, and token_type_ids INT32 inputs at [1, 512]; FLOAT logits at [1, 1]; vocabulary boundary 30522; valid external-data references; and CPUExecutionProvider. The fp16/fp32 size ratio is 0.5003723084352613.

PyTorch parity

Precision Cosine Max absolute delta Descending-order agreement Top-1 agreement
fp32 0.9999999999999681 4.291534423828125e-06 PASS PASS
fp16 0.9999999983262556 0.0008955001831054688 PASS PASS

Parity used identical real query/document pairs and named tokenizer inputs (text=query, text_pair=document) for PyTorch and ONNX.

Functional smoke Eval

Final candidate 11422d97... ran one CPU fp32 grouped reranking Eval against orgrctera/msmarco_passage_ranking revision a7388b9efd4dd4b87a0db91314e5b3f0e4b0d9e6 plus pinned official MS MARCO dev queries, qrels, and Top-1000 candidates. Source checksums matched. The bounded builder skipped absent/nonviable IDs 1048578 and 1048579, selected viable IDs 1048585 and 2, and retained one shared positive plus three official negatives per group in authoritative Top-1000 order.

  • Requested / processed / skipped groups: 2 / 2 / 0
  • Expanded / processed pairs: 8 / 8
  • Fan-out caps: 2 groups, 4 candidates per group, 8 total pairs, sequence length 512
  • Schema/semantics: query and document pairing, normalized IDs, relevance labels, source order, raw-logit descending ranking, positive membership, and accounting all verified
  • Raw task metrics: MRR@10 1.0, Recall@1 1.0, Recall@10 1.0, 2 scored groups, 0 groups without a positive
  • Independent recomputation: MRR@10 1.0, Recall@1 1.0, Recall@10 1.0

These numbers prove bounded end-to-end evaluator/model-path operability only. They are not representative accuracy and not a benchmark-quality claim.

Compatibility and quality gates

  • Focused reranking compatibility: 287 passed, 0 failed, 0 skipped.
  • Local CI partitions: analyze 1,529 passed / 45 skipped; models 1,655 passed / 8 skipped; optim 1,074 passed / 17 skipped; commands 4,364 passed / 9 skipped / 1 base-reproduced local failure; remaining 1,346 passed / 2 skipped / 1 base-reproduced local failure.
  • Commands exception: host OpenVINO discovery repopulated an intentionally empty registry and plugin registration missed onnxruntime_providers_shared.dll; exact base failed identically.
  • Remaining exception: host-network TLS HandshakeFailure while fetching PyPI metadata; exact base failed identically.
  • License-header check and Ruff passed. Editable whole-package mypy passed across 465 source files.
  • All exact-head hosted checks completed successfully on 11422d97..., including lint, all five test partitions, CodeQL, CLA, and policy checks.

Delta

Relative to the fresh base auto-config:

JSON pointer Base auto-config Shipped recipe/build config
/loader/task "text-classification" "reranking"
/loader/model_class "AutoModelForSequenceClassification" unchanged
/export/opset_version 17 unchanged
/export/input_tensors three [1,512] INT32 BERT inputs unchanged
/optim {"clamp_constant_values": true} {"gelu_fusion": true, "matmul_add_fusion": true}
/quant (fp32) null null
/quant/mode (fp16) absent "fp16"
/quant/fp16_keep_io_types (fp16) absent true

The contribution adds 24 owned paths: the two recipes; canonical reranking task resolution and registration; query/document pair inference; grouped/pairwise dataset normalization; RerankingEvaluator; RerankingMetric with true multi-positive Recall; evaluation utilities; a bounded, pinned-source MS MARCO fixture builder; and focused command/eval/inference/loader regressions. The implementation keeps the established AutoModelForSequenceClassification exporter rather than adding checkpoint-specific architecture code. The fp32 and fp16 recipes were each accepted only on their exact passing tuples. examples/recipes/README.md remains untouched.

Bug fix explanation

  1. Symptom and trigger: with default --queries 2, the earlier fixture selected IDs 1048578 and 1048579 before checking Top-1000 materializability; neither had usable Top-1000 rows, so zero groups were materialized even though later viable IDs existed.
  2. Root cause: query selection applied the requested count before joining HF rows/qrels with streamed Top-1000 evidence, so eligibility was mistaken for complete positive-plus-negative viability.
  3. Changed mechanism: the fixture builder now derives eligible IDs, streams Top-1000 to identify IDs with both a shared positive and an official negative, deterministically selects the first viable bounded set, then streams again to retain candidates only for that set.
  4. General rule: selection is driven by normalized source IDs, qrels, and observed Top-1000 positive/negative membership; it contains no checkpoint- or query-ID hardcoding.
  5. Compatibility and blast radius: the change is confined to bounded fixture materialization, preserves deterministic HF row order and authoritative Top-1000 candidate order, retains atomic/cache/checksum behavior, and does not change ordinary text-classification export behavior.
  6. Regression evidence: the absent-first-ID regression passes; pinned-source verification skips 1048578/1048579, selects 1048585 and 2, materializes exactly two groups/eight pairs, and matches independent metric recomputation. Focused compatibility reports 287/287 passing and every exact-head hosted check succeeds.

Analyze summary - component level and op level

ANALYZE-PARTIAL-SUCCESS. Static rule analysis is compatibility analysis, not runtime execution. The installed non-editable wheel omitted avaliable_providers.json and produced incomplete empty classifications; recovery used exact-candidate source plus an external current rules set and one bounded command per populated provider/device rule folder.

Component-level summary

Artifact Architecture coverage Mapping Actionable EP findings
fp32 Embeddings/mask; 6x attention and feed-forward blocks; pooler; classifier; dropout optimized away 194 mapped, 0 unmapped; 12 deterministic one-hop boundary assignments None across six rule-backed rows
fp16 Embeddings/mask; 6x attention and feed-forward blocks; pooler; classifier; dropout optimized away 195 mapped, 0 unmapped; 12 deterministic one-hop boundary assignments Two Cast nodes conservatively unknown: one in embeddings/mask and one in classifier

Op-level summary

Artifact Graph Dominant ops EP roll-up
fp32 194 ops / 15 types Reshape 60; Gemm 38; Transpose 24; Add 20; LayerNormalization 13; MatMul 12 All 15 types supported across NvTensorRTRTX GPU, OpenVINO CPU/GPU/NPU, and QNN GPU/NPU
fp16 195 ops / 15 types Reshape 60; Gemm 38; Transpose 24; Add 20; LayerNormalization 13; MatMul 12; Cast 2 14 types supported; Cast appears as both supported and unknown on all six rule-backed rows, so the two Cast nodes remain conservatively unknown

The fp32 recovery commands exited 0. The fp16 recovery commands exited 1 because of the conservative Cast unknowns. These static accelerator rows are not runtime-support claims; runtime testing in this report is CPUExecutionProvider only.

Reproduce commands

$OUT='temp/model-support-repro'
uv run winml build -c examples/recipes/cross-encoder_ms-marco-MiniLM-L6-v2/cpu/cpu/reranking_fp32_config.json -m cross-encoder/ms-marco-MiniLM-L6-v2 -o $OUT/fp32 --rebuild
uv run winml build -c examples/recipes/cross-encoder_ms-marco-MiniLM-L6-v2/cpu/cpu/reranking_fp16_config.json -m cross-encoder/ms-marco-MiniLM-L6-v2 -o $OUT/fp16 --precision fp16 --rebuild
uv run winml perf -m $OUT/fp32/model.onnx --ep cpu --device cpu
uv run winml perf -m $OUT/fp16/model.onnx --ep cpu --device cpu
uv run winml eval -m $OUT/fp32/model.onnx --model-id cross-encoder/ms-marco-MiniLM-L6-v2 --task reranking --dataset <fixture> --split dev --samples 2 --no-shuffle --column candidates_column=candidates --column max_candidates=4 --ep cpu --device cpu

@ssss141414 ssss141414 added the model-scale-by-skill Model support PR created or maintained by the adding-model-support skill label Aug 20, 2026
@ssss141414

Copy link
Copy Markdown
Contributor Author

REQUEST_CHANGES for head 207d9abcce3a21481511d34968d374fa82d87723

Scope and metadata re-check:

  • PR Add WinML reranking support for cross-encoder/ms-marco-MiniLM-L6-v2 #1322 is still Draft, base main, head producer/ms-marco-minilm-l6-v2-reranking, and the model-scale-by-skill label is present.
  • Exact-head GitHub checks for 207d9abcce3a21481511d34968d374fa82d87723 are not green yet: license/cla is COMPLETED/SUCCESS, while Analyze (Python), lint, test (analyze), test (models), test (optim), test (commands), and test (remaining) are still queued.
  • Public conversation/review-thread scrape at verdict time shows no external review comments or open threads.

Owner-tagged issues:

  1. PRODUCER: scripts/e2e_eval/datasets/build_msmarco_reranking_fixture.py biases grouped-eval tie handling by reordering candidates so positives always precede negatives. The metric explicitly says ties are broken by original candidate order, and RerankingMetric.update() implements that stable ordering in src/winml/modelkit/eval/metrics/ranking.py. But the fixture builder collects positives and negatives separately and then materializes candidates as (*positives, *negatives), while also preserving the original source ranks separately in candidate_ranks. That means an equal-score tie in grouped eval will prefer the positive passage even when the original authoritative top1000 order had negatives first, inflating MRR@K / Recall@K on the grouped fixture. Please preserve authoritative candidate order in the emitted candidates list, then add a regression test that proves grouped-inline ties do not gain a positive-first advantage from fixture construction.

Independent validation performed:

  • Local product checkout points at refs/heads/producer/ms-marco-minilm-l6-v2-reranking with remote origin=https://github.com/microsoft/winml-cli.git.
  • Focused loader regression slice passed: 2 passed in 19.63s.
  • Focused shared regression slice passed: 262 passed in 26.14s.
  • Changed-path Ruff passed.
  • uv lock --check evidence shows Resolved 256 packages in 6ms.
  • Final-SHA CPU fp32 L3 eval evidence reports MRR@10=1.0, Recall@1=1.0, Recall@10=1.0 on 2 grouped queries / 8 pairs, but the grouped fixture ordering issue above means the tie-handling claim is not yet trustworthy enough to approve.

Re-review gate:

  • After the fixture-order fix lands, rerun the focused reranking/loader regression slice and the bounded final-SHA CPU eval, then wait for every required exact-head GitHub check to reach COMPLETED/SUCCESS before requesting another reviewer pass.

Comment thread src/winml/modelkit/eval/reranking_evaluator.py Fixed
@ssss141414

Copy link
Copy Markdown
Contributor Author

Addressed on exact head 0286a7d6fcee4ffc103748aa9a85f7c93c407091.

Fix summary:

  • Changed files: scripts/e2e_eval/datasets/build_msmarco_reranking_fixture.py, tests/unit/eval/test_reranking_evaluator.py.
  • Behavior: grouped fixture selection/emission now preserves authoritative top1000 order (no positive-first materialization), while keeping deterministic bounded selection with required positive coverage.
  • Evidence details: both selected groups keep positives at emitted index 3 (original authoritative ranks 553 and 936), and model inference still ranks both positives first.

Validation:

  • Tie-order regressions: 3 passed.
  • Full reranking evaluator module: 10 passed.
  • Focused seven-path slice: 265 passed.
  • L3 CPU rerun on grouped fixture: MRR@10=1.0, Recall@1=1.0, Recall@10=1.0.
  • Manual metric audit: exact match with evaluator metrics and accounting.

Current review-thread snapshot:

Please re-review this exact head when convenient.

@ssss141414

Copy link
Copy Markdown
Contributor Author

REQUEST_CHANGES for head 0286a7d6fcee4ffc103748aa9a85f7c93c407091

Exact-head gate re-check:

  • PR Add WinML reranking support for cross-encoder/ms-marco-MiniLM-L6-v2 #1322 is still Draft, base main, head producer/ms-marco-minilm-l6-v2-reranking, and the model-scale-by-skill label is present.
  • Exact-head GitHub checks on 0286a7d6fcee4ffc103748aa9a85f7c93c407091: CodeQL, license/cla, Analyze (Python), test (commands), test (optim), test (remaining), test (analyze), and test (models) are COMPLETED/SUCCESS; lint is COMPLETED/FAILURE.
  • Current thread gate: user-provided state is unresolved threads = 0.

Owner-tagged issue:

  1. PRODUCER: the failing check is not Ruff. In workflow Lint run 32369963611, job 96428006621, step Type check (required) failed while running the exact required command uv run mypy -p winml.modelkit. In the same job, Check license headers passed and the Lint step (uv run ruff check src/ tests/) passed, so this is a candidate-owned type-check failure on the exact PR head rather than infrastructure. Please fix the mypy diagnostics on this exact head and rerun until lint is green.

Evidence used for this verdict:

  • Live PR API confirms head 0286a7d6fcee4ffc103748aa9a85f7c93c407091, draft state, and label.
  • Live check-runs API confirms only lint failed on this head.
  • Live job metadata API confirms the failing step is Type check (required).
  • .github/workflows/lint.yml at this exact head defines that step as uv run mypy -p winml.modelkit.
  • The public Actions surface available here did not expose the raw mypy lines from job 96428006621; the public job-logs endpoint returned HTTP 403, so the actionable owner route is to fix or surface the exact mypy diagnostics from the authenticated log and rerun the required gate.

@ssss141414

Copy link
Copy Markdown
Contributor Author

ssss141414 Thanks for the re-check on #issuecomment-5356216204.

Addressed on exact head 030e79fa6482acb08e51338ab4172ac4387b387d:

  • Fixed the two original mypy diagnostics in src/winml/modelkit/eval/reranking_evaluator.py.
  • Fixed the replacement-signature mypy diagnostic by aligning _RawRerankingPipeline.postprocess to the locked TextClassificationPipeline/Pipeline postprocess signature surface while preserving raw ModelOutput passthrough behavior.

Current evidence status on this exact head:

  • Clean-env changed-file mypy: PASS.
  • uv lock --check: PASS.
  • Full reranking evaluator tests: 10 passed.
  • Focused reranking regression slice: 265 passed.
  • Changed-path Ruff: PASS.
  • L2 parity: PASS.
  • L3 functional smoke eval: PASS.

Please re-review PR #1322 at this head.

Comment thread src/winml/modelkit/eval/reranking_evaluator.py
@ssss141414

Copy link
Copy Markdown
Contributor Author

REQUEST_CHANGES for head 030e79fa6482acb08e51338ab4172ac4387b387d

Exact-head status re-check:

  • PR Add WinML reranking support for cross-encoder/ms-marco-MiniLM-L6-v2 #1322 is still Draft, base main, head producer/ms-marco-minilm-l6-v2-reranking, and the model-scale-by-skill label is present.
  • Exact-head GitHub checks for 030e79fa6482acb08e51338ab4172ac4387b387d are all green: Analyze (Python), CodeQL, lint, test (analyze), test (commands), test (models), test (optim), test (remaining), and license/cla all reached COMPLETED/SUCCESS.
  • Focused local retained evidence also passed: changed-path Ruff, the 3 tie-order regression tests, the 265-test eval/loader/commands slice, the module-targeted mypy check for winml.modelkit.eval.reranking_evaluator, and uv lock --check.
  • The package-wide mypy -p winml.modelkit command fails on this host, but the same error family reproduces from the clean base-main-02179d185f0e snapshot under PYTHONPATH=<base-main>/src, so that is a pre-existing/environmental gate issue rather than a PR regression.

Owner-tagged issue:

  1. EXPLAINER (or PRODUCER if you prefer the cleanup path): fresh post-CI thread enumeration still shows 1 unresolved review thread on this exact head, which blocks approval by contract.
    • Thread: Add WinML reranking support for cross-encoder/ms-marco-MiniLM-L6-v2 #1322 (comment)
    • Claim re-verified: the cited TYPE_CHECKING import of torch in src/winml/modelkit/eval/reranking_evaluator.py is unused, while runtime behavior still comes from the local import torch inside _score_pair() and the remaining cast("torch.Tensor", logits) is only a string forward reference.
    • Required action: either remove the unused TYPE_CHECKING import in a follow-up commit, or post an explicit rationale reply for this new thread and resolve it. I cannot post APPROVE while reviewThreads.isResolved == false for any thread.

Independent verification completed on this head:

  • Final diff scope contains 22 tracked source/test/recipe files and no temp/scratch payloads or large binary artifacts in the PR diff.
  • The tie-order fix is present in scripts/e2e_eval/datasets/build_msmarco_reranking_fixture.py, and targeted tests prove grouped inline ties preserve authoritative candidate order.
  • Metric semantics are consistent: RerankingMetric still uses stable score ordering and excludes groups without positives from the scoring denominator; the retained manual metric audit matches evaluator output exactly.
  • Fixture provenance is pinned to the stated HF and official MS MARCO sources, and the retained validation evidence confirms the selected candidate IDs preserve authoritative top1000 order with positives not forced first.
  • The PR body claims inspected at verdict time are consistent with the verified CPU recipes, reranking evaluator support, and the retained final-SHA CPU fp32 functional-smoke evidence.

Re-review gate:

  • After the unresolved thread count reaches zero on head 030e79fa6482acb08e51338ab4172ac4387b387d, rerun the final GitHub thread enumeration and exact-head metadata check, then the PR is ready for an approval-only final pass.

@ssss141414

Copy link
Copy Markdown
Contributor Author

APPROVE for head 030e79fa6482acb08e51338ab4172ac4387b387d

Exact-head authoritative reconfirmation:

  • PR Add WinML reranking support for cross-encoder/ms-marco-MiniLM-L6-v2 #1322 metadata is unchanged and valid for shipment gating: OPEN, Draft=true, base main, head producer/ms-marco-minilm-l6-v2-reranking, label model-scale-by-skill present.
  • Remote branch head matches reviewed SHA exactly: refs/heads/producer/ms-marco-minilm-l6-v2-reranking -> 030e79fa6482acb08e51338ab4172ac4387b387d.
  • Checks gate is fully green on this exact SHA: 9/9 SUCCESS (Analyze (Python), CodeQL, lint, test (analyze), test (commands), test (models), test (optim), test (remaining), license/cla), with zero pending and zero failed.
  • Thread gate is closed: reviewThreads(first:100) reports totalCount=2, unresolved=0, with resolved thread IDs PRRT_kwDORaJc5s6azQYG and PRRT_kwDORaJc5s6a2T_d.
  • Latest existing issue comment remains the prior reviewer REQUEST_CHANGES for this same head; its blocking conditions are now satisfied by green exact-head CI and resolved review thread state.

Structured verification evidence:

  • CPU coverage confirmed for both required tuples:
    • CPUExecutionProvider/cpu/fp32: L0 build PASS and L1 perf PASS (mean 64.14 ms, p50 63.23 ms, 15.59 samples/sec, RAM +79.8 MB).
    • CPUExecutionProvider/cpu/fp16: L0 build PASS and L1 perf PASS (mean 92.11 ms, p50 91.65 ms, 10.86 samples/sec, RAM +88.4 MB).
  • L3 functional smoke evidence is present and consistent on bounded real grouped data: MRR@10=1.0, Recall@1=1.0, Recall@10=1.0 on exactly 2 groups / 8 pairs.
  • Tie-order and metric-semantics integrity is explicitly re-verified: stable tie ordering preserves authoritative candidate order, and manual metric audit matches evaluator output exactly.

Reviewer verdict:

  • APPROVE as the independent final authoritative pass for this head.
  • Posted as a normal PR comment only (no review API state mutation).

@ssss141414

Copy link
Copy Markdown
Contributor Author

REQUEST_CHANGES for head 207d9abcce3a21481511d34968d374fa82d87723

Scope and metadata re-check:

  • PR Add WinML reranking support for cross-encoder/ms-marco-MiniLM-L6-v2 #1322 is still Draft, base main, head producer/ms-marco-minilm-l6-v2-reranking, and the model-scale-by-skill label is present.
  • Exact-head GitHub checks for 207d9abcce3a21481511d34968d374fa82d87723 are not green yet: license/cla is COMPLETED/SUCCESS, while Analyze (Python), lint, test (analyze), test (models), test (optim), test (commands), and test (remaining) are still queued.
  • Public conversation/review-thread scrape at verdict time shows no external review comments or open threads.

Owner-tagged issues:

  1. PRODUCER: scripts/e2e_eval/datasets/build_msmarco_reranking_fixture.py biases grouped-eval tie handling by reordering candidates so positives always precede negatives. The metric explicitly says ties are broken by original candidate order, and RerankingMetric.update() implements that stable ordering in src/winml/modelkit/eval/metrics/ranking.py. But the fixture builder collects positives and negatives separately and then materializes candidates as (*positives, *negatives), while also preserving the original source ranks separately in candidate_ranks. That means an equal-score tie in grouped eval will prefer the positive passage even when the original authoritative top1000 order had negatives first, inflating MRR@K / Recall@K on the grouped fixture. Please preserve authoritative candidate order in the emitted candidates list, then add a regression test that proves grouped-inline ties do not gain a positive-first advantage from fixture construction.

Independent validation performed:

  • Local product checkout points at refs/heads/producer/ms-marco-minilm-l6-v2-reranking with remote origin=https://github.com/microsoft/winml-cli.git.
  • Focused loader regression slice passed: 2 passed in 19.63s.
  • Focused shared regression slice passed: 262 passed in 26.14s.
  • Changed-path Ruff passed.
  • uv lock --check evidence shows Resolved 256 packages in 6ms.
  • Final-SHA CPU fp32 L3 eval evidence reports MRR@10=1.0, Recall@1=1.0, Recall@10=1.0 on 2 grouped queries / 8 pairs, but the grouped fixture ordering issue above means the tie-handling claim is not yet trustworthy enough to approve.

Re-review gate:

  • After the fixture-order fix lands, rerun the focused reranking/loader regression slice and the bounded final-SHA CPU eval, then wait for every required exact-head GitHub check to reach COMPLETED/SUCCESS before requesting another reviewer pass.

@ssss141414

Copy link
Copy Markdown
Contributor Author

APPROVE: reviewed exact head 414e919. Grouped reranking now filters empty passages before capping and uses deterministic label-independent interleaving so tied scores do not receive positive-first bias. Independent detached validation passed 111 focused tests; Ruff and package-wide mypy passed; all 9 fresh GitHub checks are green; no active review threads remain.

@ssss141414
ssss141414 marked this pull request as ready for review September 2, 2026 16:34
@ssss141414
ssss141414 requested a review from a team as a code owner September 2, 2026 16:34

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reranking integration has several correctness issues in online inference, metric semantics, grouped-ID normalization, and the bounded fixture builder. Inline comments include concrete failure modes and fixes.

Comment thread src/winml/modelkit/inference/tasks.py Outdated
Comment thread src/winml/modelkit/eval/metrics/ranking.py Outdated
Comment thread src/winml/modelkit/eval/reranking_evaluator.py Outdated
Comment thread scripts/e2e_eval/datasets/build_msmarco_reranking_fixture.py
Comment thread scripts/e2e_eval/datasets/build_msmarco_reranking_fixture.py Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The five previous blockers are fixed, but the new bounded preselection makes the default pinned fixture build fail before it can find viable Top-1000 groups. See the inline comment.

Comment thread scripts/e2e_eval/datasets/build_msmarco_reranking_fixture.py Outdated
@ssss141414
ssss141414 force-pushed the producer/ms-marco-minilm-l6-v2-reranking branch from ca9aed3 to 11422d9 Compare September 23, 2026 04:05
@ssss141414

Copy link
Copy Markdown
Contributor Author

APPROVE: independent re-verification completed for exact head 11422d97cc9a31ffdf64fb98462976b13ad25c19 on base f4e7eafd4e2365016371686dc2d86fa03bfc475a.

  • Integrity: all role manifests pass; the tester seal covers 168 files / 1,533,996,711 bytes with zero missing, size, or SHA-256 mismatches.
  • Goal ladder: L0-L3 PASS. Full required coverage is CPUExecutionProvider/cpu fp32 and fp16; both pass build/perf, both pass named-input PyTorch/ONNX parity and complete ranking/top-1 agreement, and there are no deferred tuples.
  • True fp16: 108 FLOAT16 initializers, no FLOAT initializers, 45,494,814-byte artifact versus 90,921,926-byte fp32 (0.5003723 ratio); the build used --precision fp16 without --no-quant. Perf on the exact artifacts reports fp32 61.00/60.36 ms mean/P50 and fp16 85.02/84.02 ms, with memory retained.
  • L3: pinned HF revision plus official MS MARCO queries/qrels/Top-1000 checksums reverified. IDs 1048578/1048579 are nonviable; selected IDs are 1048585 and 2; authoritative order, 2 groups / 8 pairs / 0 skipped, and MRR@10/Recall@1/Recall@10 1.0 all independently reproduce. Functional smoke only, not benchmark accuracy.
  • Analyze: component mapping covers 194/194 fp32 and 195/195 fp16 nodes; op-level evidence has 15 unique types and six rule-backed EP/device rows per precision. fp16 conservatively retains two Cast nodes as unknown; no static classification is presented as runtime accelerator support.
  • Quality: license and Ruff pass; pinned-environment mypy passes 465 files; focused rerun passes 366 tests. All five local partitions were exercised. The two local failures reproduce identically on exact base (OpenVINO host registration and PyPI TLS), while all 10 exact-head hosted checks are COMPLETED/SUCCESS.
  • Feedback/state: 8 issue comments, 12 reviews, 16 review comments, and 8 review threads were fully paginated; unresolved threads: 0. PR is OPEN, base main, label present, exact head, and its pre-existing non-draft state was preserved; the explainer made no readiness change.

No blockers remain.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found a correctness issue in pairwise dataset evaluation: row-level sampling can silently evaluate incomplete query groups. Current hosted checks pass, but the existing pairwise tests bypass prepare_data and do not cover this path.


grouped: dict[str, list[_Candidate]] = {}
queries: dict[str, str] = {}
for row_index, sample in enumerate(self.data):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Preserve complete query groups before sampling pairwise rows. self.data has already been shuffled and truncated by inherited WinMLEvaluator.prepare_data (default samples=100, shuffle=True), so grouping here cannot recover discarded candidates. A query with a positive scored 0.1 followed by a negative scored 0.9 yields MRR@10=0.5 and Recall@1=0 on the complete group; with --samples 1 --no-shuffle, only the positive survives and this reports both metrics as 1.0. I reproduced the sampling and metric change using the actual prepare_data method and RerankingMetric. Larger pairwise datasets are likewise reduced to partial groups by the default row sampling, and shuffled rows also change tie order. Please sample/shuffle complete query groups for pairwise mode (or reject sampling that would split a group), and add an integration regression through prepare_data, including streaming input.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

model-scale-by-skill Model support PR created or maintained by the adding-model-support skill

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants