From d41bb7456ae95f1b3fe161211d2ba8f978205471 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Thu, 13 Aug 2026 14:49:00 +0000 Subject: [PATCH] test(SAMPLE-REASONING): pin the two qwen3 branches nothing caught, and correct a 2-core that was never possible (#605, #606) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FOLLOWING_AGENTS_PROTOCOL Three follow-ups accepted when the recipe-sweep PRs merged, all evidence repair rather than behaviour: no src/ or include/ file changes. 1. #630 LOW-1 -- qwen3.cpp:55's reasoning-reopen guard was load-bearing and unpinned. Deleting it left the focused suite FULLY GREEN (7 cases / 7 passed, 157 assertions / 0 failed, Status: SUCCESS!), because all four existing tool-call assertions put BEFORE the , where `ps > p` can never fire. Added the one input that reaches it. Mutation re-run with the assertion in place: 8/7 passed / 1 failed, 164/163 passed / 1 failed, Status: FAILURE!, and the single failure is `CHECK( p->is_reasoning_end("xy") == false )` -- values `true == false`. Head is the correct side: upstream walks the token ids backwards and tests the reasoning-start id at every index BEFORE the tool-call id (qwen3.py:263-268), so it meets first and returns False. 2. #630 LOW-2 -- the seed_oss refactor (now `class SeedOssParser(Qwen3Parser)` exactly as upstream) changed extract_reasoning on the thinking=false arm, and nothing pinned it. Judged worth a test: the change is invisible on the production path -- OpenAIServingChat::MakeParserEngine (serving_chat.cpp:580) takes the header default thinking=true, under which a bare engine and Qwen3Parser are byte-identical -- so the thinking-OFF arm is the ONLY observer there is, and an unpinned invariant that no production path exercises is exactly the one a later refactor silently drops. Driven through get_parser_engine("seed_oss") so it pins the routing too. Mutation: route seed_oss back to the pre-#630 bare ParserEngine and the new case reds alone (8/7 passed / 1 failed, 164/163 / 1 failed, Status: FAILURE!) on `CHECK( content == Opt(output) )`. Recorded as coverage BEYOND upstream: `test_qwen3_reasoning_parser.py` has NO qwen3 is_reasoning_end case at all (the only one in the suite is the base family's, test_base_thinking_reasoning_parser.py:111) and its THINKING_DISABLED_CASES run with tool parsing suppressed, so both overrides sit on branches upstream's own fixtures cannot reach. Written from scratch against the upstream SOURCE, disclosed in the test header and in the spec's Tests-to-port section with the mutation that proves each case. Filed there rather than porting-inventory §9: §9 is the register for IMPLEMENTATION deviations forced by the no-Python constraint, and the precedent for beyond-upstream TEST coverage is the row spec (kv-persistence-lmcache.md:537, kv-events.md:196). 3. #624 LOW-1 -- serve-recipe-args.md said "2-core CI runner" twice for a run that a 2-core box cannot produce: the case returns early at test_cpu_threadpool.cpp:501 when `cores < 4`, and `fits = cores / 2` (:512) reporting 2 threads with `over = cores + 1` (:513) reporting 5 pins it at exactly 4 -- which is what ubuntu-latest gives a public repo. Corrected both lines (:278 prose, :284 table) and added the derivation so the number carries its own proof next time. #631 was already retitled 4-core upstream of this and is untouched; this only makes the spec agree with it. The three test anchors were re-verified in the tree, not trusted from the finding (#632). Verification, all observed in this worktree at 983b4aa4c + this diff: focused RED (guard mutated, new assertion present) exit 1, Status: FAILURE! focused GREEN (tree restored byte-for-byte) exit 0, 8/8, 164/164 full serial ctest exit 0, 408/408, 0 failed scripts/agent-preflight.sh --staged exit 0, all gates green python3 scripts/check-windows-portability.py exit 0 Both mutated sources restored byte-for-byte and re-verified by md5 and an empty `git diff -- src/`. test_cpu_threadpool is green here (20-core box); its CI red is #631 and unrelated. Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code] --- .agents/specs/reasoning-parsers.md | 25 ++++++++ .agents/specs/serve-recipe-args.md | 11 +++- .../openai/reasoning_parsers/test_qwen3.cpp | 61 +++++++++++++++++++ 3 files changed, 95 insertions(+), 2 deletions(-) diff --git a/.agents/specs/reasoning-parsers.md b/.agents/specs/reasoning-parsers.md index f6d02798c..b848ebee6 100644 --- a/.agents/specs/reasoning-parsers.md +++ b/.agents/specs/reasoning-parsers.md @@ -142,6 +142,31 @@ The v4-alias sub-case is SKIPPED-with-reason in `test_deepseek_v3.cpp` input strings, non-streaming AND the per-delta streaming reconstruction, RED-first. +### Coverage BEYOND upstream in `test_qwen3.cpp` (from scratch, recorded) + +Upstream drops nothing here — there is nothing to drop. `test_qwen3_reasoning_parser.py` +has **no** `is_reasoning_end` case for qwen3 at all (the only one in the suite is +`test_base_thinking_reasoning_parser.py:111`, for the base family), and its +`THINKING_DISABLED_CASES` run through the adapter, which suppresses tool parsing. +Both `Qwen3Parser` overrides therefore sit on branches upstream's own fixtures +cannot reach, so three TEST_CASEs are written from scratch against the upstream +SOURCE (`vllm/parser/qwen3.py:247,256-275`, `vllm/parser/seed_oss.py:24-29`): + +| our TEST_CASE | pins | mutation that proves it | +|---|---|---| +| `engine thinking-off passthrough survives an unskipped tool` | `qwen3.py:247` on the ENGINE, tool parsing NOT suppressed | drop the `extract_reasoning` override (#630) | +| `is_reasoning_end (text form, incl. unpaired )` | `qwen3.py:256-275`, incl. the reasoning-REOPEN branch — a `` AFTER the tool-call marker, which upstream's backwards walk meets first | drop `qwen3.cpp:55` `ps > p`; only `is_reasoning_end("xy") == false` reds | +| `seed_oss inherits the qwen3 thinking-off passthrough` | `get_parser_engine("seed_oss")` builds the shared `Qwen3Parser`, mirroring `class SeedOssParser(Qwen3Parser)` | route seed_oss back to a bare `ParserEngine` | + +The reopen and seed_oss rows are follow-up to the #630 fresh review's two LOW +findings; before them, deleting `qwen3.cpp:55` left the focused suite fully green +(7/7, 157/157), because all four existing tool-call assertions put `` +BEFORE the ``, where the guard cannot fire. Only the thinking-OFF arm +can observe the seed_oss class change at all — the sole production call site, +`OpenAIServingChat::MakeParserEngine` (`serving_chat.cpp:580`), takes the header +default `thinking=true`, under which the refactor is byte-identical, so the +production path was never affected. + ## Gates - CORRECTNESS: doctest parity — each parser's `(reasoning, content)` over the diff --git a/.agents/specs/serve-recipe-args.md b/.agents/specs/serve-recipe-args.md index b54b90563..44086d666 100644 --- a/.agents/specs/serve-recipe-args.md +++ b/.agents/specs/serve-recipe-args.md @@ -275,16 +275,23 @@ tests/vt/test_cpu_threadpool.cpp:539: ERROR: CHECK( ratio < 100.0 ) is NOT corre Attributed rather than assumed, to the same standard as #584. The guard divides two wall-clock medians (`over_us / fits_us`) and compares the ratio to a fixed 100, so both ends are machine-shape dependent and the denominator is the problem: -on the 2-core runner `fits_us` collapsed to 0.48 us, small enough that ordinary +on the 4-core runner `fits_us` collapsed to 0.48 us, small enough that ordinary scheduler noise in the numerator moves the ratio by tens. Same commit, same code, three observations: | Box | `fits` | `over` | ratio | |---|---|---|---| -| 2-core CI runner | 2 threads, 0.48 us | 5 threads, 48.752 us | **101.567** RED | +| 4-core CI runner | 2 threads, 0.48 us | 5 threads, 48.752 us | **101.567** RED | | 20-core box (#631's table) | 10 threads, 7.213 us | 21 threads, 19.467 us | **2.699** GREEN | | 20-core box, this branch rebuilt at the reviewed head | 10 threads, 13.256 us | 21 threads, 13.135 us | **0.990872** GREEN | +The runner's core count is read off the failure itself, not assumed: the case +returns early at `test_cpu_threadpool.cpp:501` when `cores < 4`, so a box that +produced a ratio at all has at least 4; and `fits = cores / 2` (`:512`) reporting +2 threads with `over = cores + 1` (`:513`) reporting 5 pins it at exactly 4 — +which is what GitHub gives `ubuntu-latest` on a public repo. An earlier revision +of this section said 2-core, a number no run on that lane can produce. + The last row is the one measured while repairing this record — `test_cpu_threadpool` 9 cases / 9 passed, 19602 assertions / 0 failed, `Status: SUCCESS!`. That it sits 2.7x below the middle row on the *same class of box* is itself the finding: the diff --git a/tests/vllm/entrypoints/openai/reasoning_parsers/test_qwen3.cpp b/tests/vllm/entrypoints/openai/reasoning_parsers/test_qwen3.cpp index c32a1f594..df88a2fe8 100644 --- a/tests/vllm/entrypoints/openai/reasoning_parsers/test_qwen3.cpp +++ b/tests/vllm/entrypoints/openai/reasoning_parsers/test_qwen3.cpp @@ -17,6 +17,20 @@ // cannot carry request kwargs (same deviation as deepseek_v3, threading is // W4 in specs/reasoning-parsers.md), so the thinking flag is taken through // the public constructor — the same seam test_deepseek_v3.cpp uses. +// +// BEYOND UPSTREAM (written from scratch, no upstream case is dropped — there is +// none to drop). Upstream's reasoning suite has NO qwen3 `is_reasoning_end` +// test at all (only test_base_thinking_reasoning_parser.py:111 for the base +// family), and its thinking-disabled cases run with tool parsing suppressed. +// The last three TEST_CASEs are therefore ours, and cover branches upstream's +// fixtures cannot reach: +// - "engine thinking-off passthrough survives an unskipped tool" — the +// qwen3.py:247 override on the ENGINE, not the adapter. +// - "is_reasoning_end (text form, ...)" — the qwen3.py:256 override, +// including the reasoning-REOPEN branch (`` then ``). +// - "seed_oss inherits the qwen3 thinking-off passthrough" — the shared +// class, which only the thinking-off arm can observe. +// Recorded in .agents/specs/reasoning-parsers.md § Tests to port. #include #include @@ -28,6 +42,7 @@ #include "vllm/entrypoints/openai/reasoning_parsers/abstract.h" #include "vllm/entrypoints/openai/reasoning_parsers/parser_engine_adapter.h" #include "vllm/parser/engine/configs.h" +#include "vllm/parser/parser_manager.h" #include "vllm/parser/qwen3.h" using namespace vllm::entrypoints::openai; @@ -263,4 +278,50 @@ TEST_CASE("qwen3: is_reasoning_end (text form, incl. unpaired )") { CHECK(p->is_reasoning_end("thinking\n") == true); CHECK(p->is_reasoning_end("thinkingx") == false); CHECK(p->is_reasoning_end("ab") == true); + // The three above all put BEFORE the tool-call marker, where the + // re-open guard cannot fire. This one puts it AFTER: upstream's backwards walk + // (qwen3.py:263-268) tests the reasoning-start id at EVERY index before it + // tests the tool-call id, so it meets first and returns False — + // reasoning has re-opened and the earlier unpaired no longer ends + // it. It is the only assertion here that fails if `ps > p` (qwen3.cpp:55) is + // dropped. + CHECK(p->is_reasoning_end("xy") == false); +} + +// Upstream seed_oss is `class SeedOssParser(Qwen3Parser)` (seed_oss.py:24) +// overriding only the four wrapper token strings, so it INHERITS the +// thinking-off passthrough (qwen3.py:247). Since #630 (#605) our +// `get_parser_engine` mirrors that by building the same Qwen3Parser class over +// the `` spelling (parser_manager.cpp:28); before that, seed_oss got +// a bare ParserEngine and the thinking-off arm ran the state machine instead. +// +// Nothing else pins the inheritance. The only production call site, +// `OpenAIServingChat::MakeParserEngine` (serving_chat.cpp:580), takes the +// header default thinking=true, under which a bare engine and Qwen3Parser are +// byte-identical — so ONLY the thinking-off arm can tell the two apart, which +// is why this case exists. +TEST_CASE("seed_oss inherits the qwen3 thinking-off passthrough") { + namespace pe = vllm::parser::engine; + // seed_oss.py:26-29 — the qwen3 grammar with `seed:`-prefixed wrappers; + // ``/`` are byte-identical. + const std::string seed_tool_body = + "\n\n" + "\ncat /etc/hosts\n\n\n"; + const std::string output = "I need to read the file.\n\n" + seed_tool_body; + + auto off = vllm::parser::get_parser_engine("seed_oss", /*thinking=*/false); + REQUIRE(off != nullptr); + const auto [reasoning, content] = + off->extract_reasoning(output, pe::ParserRequest{}); + CHECK(reasoning == std::nullopt); + CHECK(content == Opt(output)); + + // Same name, thinking ON: the initial state is REASONING and the unpaired + // is an implicit reasoning end, so the content span is NOT + // the whole output. That difference is exactly what the override suppresses. + auto on = vllm::parser::get_parser_engine("seed_oss", /*thinking=*/true); + REQUIRE(on != nullptr); + const auto [r2, c2] = on->extract_reasoning(output, pe::ParserRequest{}); + CHECK(r2 == Opt("I need to read the file.\n\n")); + CHECK(c2 != Opt(output)); }