feat(qwen36): complete concurrent serving implementation with speculation - #605
Draft
Graffioh wants to merge 16 commits into
Draft
feat(qwen36): complete concurrent serving implementation with speculation#605Graffioh wants to merge 16 commits into
Graffioh wants to merge 16 commits into
Conversation
Graffioh
changed the title
Graffioh
changed the title
Graffioh
force-pushed
the
codex/qwen36-concurrent-features
branch
from
August 14, 2026 07:12
7b0debd to
352b308
Compare
Contributor
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
28 issues found across 109 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="server/src/qwen35/qwen35_backend.cpp">
<violation number="1" location="server/src/qwen35/qwen35_backend.cpp:405">
P2: When the local draft runs on another GPU or the target uses tensor parallelism, this condition still advertises concurrent DDTree as enabled, but the sequence engine can only run packed AR. Gate this flag on same-device, non-split placement (or reject the configuration) so startup behavior and resource allocation match the actual feature activation.</violation>
</file>
<file name="server/tests/test_server_parallel.py">
<violation number="1" location="server/tests/test_server_parallel.py:330">
P2: In test_parallel_isolation, the cross-stream exclusion check (`not contains_number(combined, answer)`) falsely fails for --max-concurrency >= 38. make_math_prompts defines each stream's operands as s//2 and s-(s//2), so once count reaches 38 the sum hits 222 and the prompt becomes "What is 111+111?" — where 111 is stream 0's answer. A greedy Qwen run echoes the operands in its reasoning, so the "excludes stream j's answer" check flags 111 as a leak for every such stream. The docstring's invariant (operands stay 2-digit while answers are 3-digit, so an answer never collides with an echoed operand) only holds for small counts and breaks at n>=38, even though main() permits --max-concurrency up to 64. This makes an over-subscription-style isolation run produce misleading false failures. Derive prompts from a fixed 3-digit answer A_i with operands kept in a disjoint 2-digit range (e.g. a_i in [10,90], b_i = A_i - a_i <= 99) so an operand can never equal any answer regardless of count.</violation>
</file>
<file name="harness/benchmarks/concurrency/summarize_feature_matrix.py">
<violation number="1" location="harness/benchmarks/concurrency/summarize_feature_matrix.py:115">
P3: When a feature-only run omits the AR control, this reports `—` instead of showing that the comparison is unavailable. Initialize non-AR rows to `n/a` and retain `—` only for the AR row.</violation>
</file>
<file name="harness/benchmarks/concurrency/test_feature_tools.py">
<violation number="1" location="harness/benchmarks/concurrency/test_feature_tools.py:89">
P3: The test name promises disjointness, but it only checks that the 29 compression prompts are unique among themselves and never verifies the kv-pressure count (also 29) or that the two profiles' prompts are mutually disjoint. Add an assertion that the compression and kv-pressure prompt sets do not overlap (and assert len(pressure)==29) so the test matches its stated intent.</violation>
</file>
<file name="harness/benchmarks/concurrency/run_qwen36_feature_matrix.sh">
<violation number="1" location="harness/benchmarks/concurrency/run_qwen36_feature_matrix.sh:154">
P2: SIGINT/SIGTERM only runs `stop_server` and then returns, so the runner continues after interruption instead of terminating. Keep cleanup on `EXIT` and use an exiting INT/TERM trap.</violation>
</file>
<file name="server/src/qwen35/concurrency/qwen35_seq_engine.cpp">
<violation number="1" location="server/src/qwen35/concurrency/qwen35_seq_engine.cpp:658">
P3: `upload_block_table_snapshot` is introduced but never called, so its block-table upload path cannot execute or receive coverage. Remove this dead helper, or wire it into the residency update path if it is required.</violation>
</file>
<file name="harness/benchmarks/concurrency/summarize_concurrency.py">
<violation number="1" location="harness/benchmarks/concurrency/summarize_concurrency.py:89">
P2: When two or more reports omit `selected_output_set_sha256`, this summary prints `Stable output = yes` because all missing values collapse to `{None}`. Treat missing or invalid digests as unavailable, or reject those reports before declaring output stability.</violation>
</file>
<file name="harness/benchmarks/concurrency/test_concurrent_benchmark.py">
<violation number="1" location="harness/benchmarks/concurrency/test_concurrent_benchmark.py:60">
P3: The assertions on output_window_tok_s and prompt_tokens_per_s_to_first_token are circular and pass regardless of correctness. The source computes output_window_tok_s as sum(completion_counts)/output_window (16/output_window_s), and the test asserts it equals 16 / level["output_window_s"] — the exact same division. Same for prompt_tokens_per_s_to_first_token (40/first_window) against 40 / level["prompt_to_first_token_s"]. A regression that used the wrong window (e.g. wall time instead of the first-token window) or wrong numerator would not be caught. Assert against independently computed expectations (e.g. the known 0.5 s first-token window and 1.0 s end time from the fake_request records) rather than the level's own values.</violation>
</file>
<file name="server/src/common/concurrency/paged_kv_pool.cpp">
<violation number="1" location="server/src/common/concurrency/paged_kv_pool.cpp:288">
P1: When the asynchronous copy-out fails at synchronization, `page_out_block()` has already recycled the physical page and left the logical entry cold. Keep the old mapping until the copy-out succeeds, or roll back both the cold mapping and host-valid state when synchronization fails.</violation>
</file>
<file name="server/src/server/http_server.cpp">
<violation number="1" location="server/src/server/http_server.cpp:4095">
P2: When a nonblocking SSE socket is temporarily unwritable, this branch marks a live slow client disconnected. Distinguish transient backpressure and interrupted/partial writes from hard socket errors, then retry the heartbeat on a later monitor tick.</violation>
</file>
<file name="harness/benchmarks/concurrency/concurrent_benchmark.py">
<violation number="1" location="harness/benchmarks/concurrency/concurrent_benchmark.py:330">
P2: Publication reports omit the client command and client-source hash, so identical server and prompt metadata cannot distinguish different metric implementations. Record the invoked client argv and script hash in each report.</violation>
</file>
<file name="server/src/server/server_status.h">
<violation number="1" location="server/src/server/server_status.h:127">
P2: During concurrent admission, `/status` reports `decode` before any prefill has run. Track the aggregate phase separately and publish `prefill` until the active slots have entered decode, handling mixed prefill/decode state explicitly.</violation>
</file>
<file name="harness/benchmarks/concurrency/verify_feature_metrics.py">
<violation number="1" location="harness/benchmarks/concurrency/verify_feature_metrics.py:52">
P2: When the log contains duplicate terminal markers for one request, `aggregate_rows` merges them instead of rejecting the duplicate. Reject repeated telemetry request IDs before accumulating counters so the proof cannot combine partial records.</violation>
<violation number="2" location="harness/benchmarks/concurrency/verify_feature_metrics.py:86">
P2: When telemetry uses JSON `true` for a counter or resident-block count, `isinstance(..., int)` accepts it, so malformed telemetry can satisfy positive feature checks. Require exact integer types for telemetry, wire values, and metadata thresholds and pool values.</violation>
</file>
<file name="harness/benchmarks/concurrency/run_qwen36_concurrency.sh">
<violation number="1" location="harness/benchmarks/concurrency/run_qwen36_concurrency.sh:164">
P2: The recorded Lucebox command cannot reproduce the measured scheduler because the launch-only `DFLASH_*` environment is omitted. Include those assignments in `server-command.txt` before calling `write_metadata`.</violation>
</file>
<file name="harness/benchmarks/concurrency/generate_feature_prompts.py">
<violation number="1" location="harness/benchmarks/concurrency/generate_feature_prompts.py:27">
P3: `build_records()` and `main()` here duplicate the same functions in `generate_ragged_prompts.py` almost verbatim, differing only by the added `activation_target` field. The shared `PROFILES`/`prompt_text` are already imported from the base file, so future changes to the record-building logic (e.g., cohort layout or id scheme) must be made in two places and will silently drift. Factor the record builder/writer so the feature variant only supplies its profiles and the extra field, and reuse the base implementation for the rest.</violation>
</file>
<file name="server/src/common/feature_gate.cpp">
<violation number="1" location="server/src/common/feature_gate.cpp:261">
P2: When an explicit pool is used with concurrent DDTree, the gate reserves only one scratch block, but the backend reserves one tree scratch stride per slot. A value near the advertised maximum therefore passes this gate and then fails initialization with `paged KV pool exceeds INT32_MAX`; cap the pool after reserving the full concurrent DDTree scratch allocation and account for block-rounding.</violation>
</file>
<file name="harness/benchmarks/concurrency/write_feature_metadata.py">
<violation number="1" location="harness/benchmarks/concurrency/write_feature_metadata.py:59">
P2: When `ldd` cannot inspect the supplied executable, this unchecked call leaves `resolved_shared_library_sha256` empty and still writes successful metadata. Use `check=True` so failed dependency discovery aborts the case.</violation>
<violation number="2" location="harness/benchmarks/concurrency/write_feature_metadata.py:68">
P2: When `--repo` is not a Git worktree, `git rev-parse` fails but this code converts empty stdout to `None` and still emits metadata. Check the subprocess return code so provenance failures abort instead of producing an unversioned result.</violation>
</file>
<file name="server/deps/llama.cpp/ggml/src/ggml.c">
<violation number="1" location="server/deps/llama.cpp/ggml/src/ggml.c:4067">
P1: When this graph runs on the CPU backend, negative padding IDs are handled as ordinary destination rows and can write before the destination buffer. Give masked rows a backend-aware implementation, or reject masked `SET_ROWS` on backends that do not implement the flag.</violation>
<violation number="2" location="server/deps/llama.cpp/ggml/src/ggml.c:5655">
P2: A non-contiguous I32 `active_slot_ids` passes this constructor but fails during CUDA graph execution. Validate contiguity here, or materialize the mapping before creating the operation.</violation>
</file>
<file name="server/test/test_client_send_buffer.cpp">
<violation number="1" location="server/test/test_client_send_buffer.cpp:11">
P3: This test uses socketpair/recv/AF_UNIX/SOCK_STREAM/SOL_SOCKET/SO_SNDBUF/MSG_DONTWAIT directly but never includes <sys/socket.h> (and <sys/un.h> for AF_UNIX in this case source). It currently compiles only because client_send_buffer.h transitively pulls in <sys/socket.h>. Relying on a transitive include makes the test fragile to unrelated edits in the header; add the explicit system includes the test's own calls depend on.</violation>
</file>
<file name="harness/benchmarks/concurrency/README.md">
<violation number="1" location="harness/benchmarks/concurrency/README.md:39">
P3: The README says the short/medium/long profiles target approximately 400, 1,000, and 3,000 "input tokens per request," but `generate_ragged_prompts.py` targets word counts, not tokens: `PROFILES` are `(250,350,450,550)` / `(650,850,1150,1350)` / `(2000,2600,3400,4000)` in `target_words`, and `prompt_text` builds `words` up to that size. For the Qwen3.6 tokenizer the real token counts exceed the word counts (longer words like "reproducibility"/"implementation" tokenize to multiple tokens), so the 400/1,000/3,000 figures understate actual input tokens. Wording it as "words (≈ tokens)" or clarifying that tokens are server-observed would keep the protocol description accurate.</violation>
</file>
<file name="server/src/server/scheduler.cpp">
<violation number="1" location="server/src/server/scheduler.cpp:193">
P2: When the first generated token is skipped or has empty text, this line disables heartbeats even though no SSE frame was buffered. The monitor then stops read-close probes, so a disconnected client can retain its slot until generation ends; disable heartbeats only after buffering an actual frame.</violation>
</file>
<file name="server/src/common/concurrency/paged_kv_residency.cpp">
<violation number="1" location="server/src/common/concurrency/paged_kv_residency.cpp:246">
P1: When a cold partial head needs restoration and the append also needs a new block, this call can evict the head that was just restored. `pool.append()` then needs another remap allocation and fails despite enough total capacity; reserve room for the restoration and new blocks together, or protect the restored head until append completes.</violation>
<violation number="2" location="server/src/common/concurrency/paged_kv_residency.cpp:532">
P2: If the copy-stream synchronization fails after eviction is queued, this flag claims the host image is valid even though the D2H transfer was not confirmed. Invalidate the host copy on synchronization failure and only make it valid after a successful barrier before allowing restoration.</violation>
<violation number="3" location="server/src/common/concurrency/paged_kv_residency.cpp:603">
P2: When several requested cold blocks compete for the resident budget, a later iteration can evict an earlier requested block. Restore the requested set as a protected batch, or fail when the requested set cannot remain resident, instead of returning `Ok` with a requested block cold.</violation>
</file>
<file name="server/src/qwen35/qwen35_target_graph.cpp">
<violation number="1" location="server/src/qwen35/qwen35_target_graph.cpp:951">
P2: When `capture_qk` is enabled for a paged tree or ragged step, `cache.q_cap` is not updated because those branches never set `q_fa_out`. Set `*q_fa_out = Qfa` in both new paged branches before the attention call.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } | ||
|
|
||
| sequence.block_table[logical_block] = PAGED_KV_COLD_BLOCK; | ||
| give_back(free_blocks_, physical_block); |
Contributor
There was a problem hiding this comment.
P1: When the asynchronous copy-out fails at synchronization, page_out_block() has already recycled the physical page and left the logical entry cold. Keep the old mapping until the copy-out succeeds, or roll back both the cold mapping and host-valid state when synchronization fails.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/common/concurrency/paged_kv_pool.cpp, line 288:
<comment>When the asynchronous copy-out fails at synchronization, `page_out_block()` has already recycled the physical page and left the logical entry cold. Keep the old mapping until the copy-out succeeds, or roll back both the cold mapping and host-valid state when synchronization fails.</comment>
<file context>
@@ -176,10 +249,82 @@ PagedKvStatus PagedKvPool::sequence(
+ }
+
+ sequence.block_table[logical_block] = PAGED_KV_COLD_BLOCK;
+ give_back(free_blocks_, physical_block);
+ out_physical_block = physical_block;
+ return PagedKvStatus::Ok;
</file context>
| struct ggml_tensor * b, | ||
| struct ggml_tensor * c) { | ||
| struct ggml_tensor * result = ggml_set_rows(ctx, a, b, c); | ||
| ggml_set_op_params_i32(result, 0, 1); |
Contributor
There was a problem hiding this comment.
P1: When this graph runs on the CPU backend, negative padding IDs are handled as ordinary destination rows and can write before the destination buffer. Give masked rows a backend-aware implementation, or reject masked SET_ROWS on backends that do not implement the flag.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/deps/llama.cpp/ggml/src/ggml.c, line 4067:
<comment>When this graph runs on the CPU backend, negative padding IDs are handled as ordinary destination rows and can write before the destination buffer. Give masked rows a backend-aware implementation, or reject masked `SET_ROWS` on backends that do not implement the flag.</comment>
<file context>
@@ -4058,6 +4058,16 @@ struct ggml_tensor * ggml_set_rows(
+ struct ggml_tensor * b,
+ struct ggml_tensor * c) {
+ struct ggml_tensor * result = ggml_set_rows(ctx, a, b, c);
+ ggml_set_op_params_i32(result, 0, 1);
+ return result;
+}
</file context>
| const uint32_t globally_needed = additional_blocks > | ||
| snapshot.reserved_block_count | ||
| ? additional_blocks - snapshot.reserved_block_count : 0; | ||
| const auto room = make_room( |
Contributor
There was a problem hiding this comment.
P1: When a cold partial head needs restoration and the append also needs a new block, this call can evict the head that was just restored. pool.append() then needs another remap allocation and fails despite enough total capacity; reserve room for the restoration and new blocks together, or protect the restored head until append completes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/common/concurrency/paged_kv_residency.cpp, line 246:
<comment>When a cold partial head needs restoration and the append also needs a new block, this call can evict the head that was just restored. `pool.append()` then needs another remap allocation and fails despite enough total capacity; reserve room for the restoration and new blocks together, or protect the restored head until append completes.</comment>
<file context>
@@ -0,0 +1,750 @@
+ const uint32_t globally_needed = additional_blocks >
+ snapshot.reserved_block_count
+ ? additional_blocks - snapshot.reserved_block_count : 0;
+ const auto room = make_room(
+ handle, globally_needed, additional_blocks);
+ return finish_transfers(room);
</file context>
Comment on lines
+405
to
+407
| const bool concurrent_local_ddtree = | ||
| n_slots > 1 && cfg_.ddtree_mode && cfg_.draft_path && | ||
| !use_remote_draft; |
Contributor
There was a problem hiding this comment.
P2: When the local draft runs on another GPU or the target uses tensor parallelism, this condition still advertises concurrent DDTree as enabled, but the sequence engine can only run packed AR. Gate this flag on same-device, non-split placement (or reject the configuration) so startup behavior and resource allocation match the actual feature activation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/qwen35/qwen35_backend.cpp, line 405:
<comment>When the local draft runs on another GPU or the target uses tensor parallelism, this condition still advertises concurrent DDTree as enabled, but the sequence engine can only run packed AR. Gate this flag on same-device, non-split placement (or reject the configuration) so startup behavior and resource allocation match the actual feature activation.</comment>
<file context>
@@ -298,21 +377,105 @@ bool Qwen35Backend::init() {
+ set_last_error("--max-concurrency requires --paged-attention");
+ return false;
+ }
+ const bool concurrent_local_ddtree =
+ n_slots > 1 && cfg_.ddtree_mode && cfg_.draft_path &&
+ !use_remote_draft;
</file context>
Suggested change
| const bool concurrent_local_ddtree = | |
| n_slots > 1 && cfg_.ddtree_mode && cfg_.draft_path && | |
| !use_remote_draft; | |
| const bool concurrent_local_ddtree = | |
| n_slots > 1 && cfg_.ddtree_mode && cfg_.draft_path && | |
| !use_remote_draft && !split_gpus_ && | |
| cfg_.draft_gpu == cfg_.device.gpu; |
| continue | ||
| self._check( | ||
| f"stream {i+1} excludes stream {j+1}'s answer {answer}", | ||
| not contains_number(combined, answer), |
Contributor
There was a problem hiding this comment.
P2: In test_parallel_isolation, the cross-stream exclusion check (not contains_number(combined, answer)) falsely fails for --max-concurrency >= 38. make_math_prompts defines each stream's operands as s//2 and s-(s//2), so once count reaches 38 the sum hits 222 and the prompt becomes "What is 111+111?" — where 111 is stream 0's answer. A greedy Qwen run echoes the operands in its reasoning, so the "excludes stream j's answer" check flags 111 as a leak for every such stream. The docstring's invariant (operands stay 2-digit while answers are 3-digit, so an answer never collides with an echoed operand) only holds for small counts and breaks at n>=38, even though main() permits --max-concurrency up to 64. This makes an over-subscription-style isolation run produce misleading false failures. Derive prompts from a fixed 3-digit answer A_i with operands kept in a disjoint 2-digit range (e.g. a_i in [10,90], b_i = A_i - a_i <= 99) so an operand can never equal any answer regardless of count.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/tests/test_server_parallel.py, line 330:
<comment>In test_parallel_isolation, the cross-stream exclusion check (`not contains_number(combined, answer)`) falsely fails for --max-concurrency >= 38. make_math_prompts defines each stream's operands as s//2 and s-(s//2), so once count reaches 38 the sum hits 222 and the prompt becomes "What is 111+111?" — where 111 is stream 0's answer. A greedy Qwen run echoes the operands in its reasoning, so the "excludes stream j's answer" check flags 111 as a leak for every such stream. The docstring's invariant (operands stay 2-digit while answers are 3-digit, so an answer never collides with an echoed operand) only holds for small counts and breaks at n>=38, even though main() permits --max-concurrency up to 64. This makes an over-subscription-style isolation run produce misleading false failures. Derive prompts from a fixed 3-digit answer A_i with operands kept in a disjoint 2-digit range (e.g. a_i in [10,90], b_i = A_i - a_i <= 99) so an operand can never equal any answer regardless of count.</comment>
<file context>
@@ -0,0 +1,608 @@
+ continue
+ self._check(
+ f"stream {i+1} excludes stream {j+1}'s answer {answer}",
+ not contains_number(combined, answer),
+ f"content={r['content']!r} "
+ f"reasoning={r['reasoning'][:200]!r}")
</file context>
| return true; | ||
| } | ||
|
|
||
| bool Qwen35SeqEngine::upload_block_table_snapshot( |
Contributor
There was a problem hiding this comment.
P3: upload_block_table_snapshot is introduced but never called, so its block-table upload path cannot execute or receive coverage. Remove this dead helper, or wire it into the residency update path if it is required.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/qwen35/concurrency/qwen35_seq_engine.cpp, line 658:
<comment>`upload_block_table_snapshot` is introduced but never called, so its block-table upload path cannot execute or receive coverage. Remove this dead helper, or wire it into the residency update path if it is required.</comment>
<file context>
@@ -0,0 +1,1239 @@
+ return true;
+}
+
+bool Qwen35SeqEngine::upload_block_table_snapshot(
+ int slot, const std::vector<int32_t> & blocks) {
+ if (blocks.empty()) return true;
</file context>
| self.assertEqual(level["completion_tokens_total"], 16) | ||
| self.assertEqual(level["prompt_tokens_total"], 40) | ||
| self.assertTrue(level["fixed_token_workload_valid"]) | ||
| self.assertAlmostEqual( |
Contributor
There was a problem hiding this comment.
P3: The assertions on output_window_tok_s and prompt_tokens_per_s_to_first_token are circular and pass regardless of correctness. The source computes output_window_tok_s as sum(completion_counts)/output_window (16/output_window_s), and the test asserts it equals 16 / level["output_window_s"] — the exact same division. Same for prompt_tokens_per_s_to_first_token (40/first_window) against 40 / level["prompt_to_first_token_s"]. A regression that used the wrong window (e.g. wall time instead of the first-token window) or wrong numerator would not be caught. Assert against independently computed expectations (e.g. the known 0.5 s first-token window and 1.0 s end time from the fake_request records) rather than the level's own values.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At harness/benchmarks/concurrency/test_concurrent_benchmark.py, line 60:
<comment>The assertions on output_window_tok_s and prompt_tokens_per_s_to_first_token are circular and pass regardless of correctness. The source computes output_window_tok_s as sum(completion_counts)/output_window (16/output_window_s), and the test asserts it equals 16 / level["output_window_s"] — the exact same division. Same for prompt_tokens_per_s_to_first_token (40/first_window) against 40 / level["prompt_to_first_token_s"]. A regression that used the wrong window (e.g. wall time instead of the first-token window) or wrong numerator would not be caught. Assert against independently computed expectations (e.g. the known 0.5 s first-token window and 1.0 s end time from the fake_request records) rather than the level's own values.</comment>
<file context>
@@ -0,0 +1,126 @@
+ self.assertEqual(level["completion_tokens_total"], 16)
+ self.assertEqual(level["prompt_tokens_total"], 40)
+ self.assertTrue(level["fixed_token_workload_valid"])
+ self.assertAlmostEqual(
+ level["output_window_tok_s"],
+ 16 / level["output_window_s"],
</file context>
| } | ||
|
|
||
|
|
||
| def build_records(profile: str) -> list[dict[str, object]]: |
Contributor
There was a problem hiding this comment.
P3: build_records() and main() here duplicate the same functions in generate_ragged_prompts.py almost verbatim, differing only by the added activation_target field. The shared PROFILES/prompt_text are already imported from the base file, so future changes to the record-building logic (e.g., cohort layout or id scheme) must be made in two places and will silently drift. Factor the record builder/writer so the feature variant only supplies its profiles and the extra field, and reuse the base implementation for the rest.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At harness/benchmarks/concurrency/generate_feature_prompts.py, line 27:
<comment>`build_records()` and `main()` here duplicate the same functions in `generate_ragged_prompts.py` almost verbatim, differing only by the added `activation_target` field. The shared `PROFILES`/`prompt_text` are already imported from the base file, so future changes to the record-building logic (e.g., cohort layout or id scheme) must be made in two places and will silently drift. Factor the record builder/writer so the feature variant only supplies its profiles and the extra field, and reuse the base implementation for the rest.</comment>
<file context>
@@ -0,0 +1,72 @@
+}
+
+
+def build_records(profile: str) -> list[dict[str, object]]:
+ strata = PROFILES[profile]
+ layout = [
</file context>
|
|
||
| #include <cstdio> | ||
| #include <cstdlib> | ||
| #include <cstring> |
Contributor
There was a problem hiding this comment.
P3: This test uses socketpair/recv/AF_UNIX/SOCK_STREAM/SOL_SOCKET/SO_SNDBUF/MSG_DONTWAIT directly but never includes <sys/socket.h> (and <sys/un.h> for AF_UNIX in this case source). It currently compiles only because client_send_buffer.h transitively pulls in <sys/socket.h>. Relying on a transitive include makes the test fragile to unrelated edits in the header; add the explicit system includes the test's own calls depend on.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/test/test_client_send_buffer.cpp, line 11:
<comment>This test uses socketpair/recv/AF_UNIX/SOCK_STREAM/SOL_SOCKET/SO_SNDBUF/MSG_DONTWAIT directly but never includes <sys/socket.h> (and <sys/un.h> for AF_UNIX in this case source). It currently compiles only because client_send_buffer.h transitively pulls in <sys/socket.h>. Relying on a transitive include makes the test fragile to unrelated edits in the header; add the explicit system includes the test's own calls depend on.</comment>
<file context>
@@ -0,0 +1,149 @@
+
+#include <cstdio>
+#include <cstdlib>
+#include <cstring>
+#include <fcntl.h>
+#include <string>
</file context>
| The 29 generated prompts are disjoint cohorts for C1/C4/C8/C16. C4 and above | ||
| contain four substantial length strata while holding the mean target length | ||
| constant. The default short, medium, and long profiles target approximately | ||
| 400, 1,000, and 3,000 input tokens per request. The client refuses to wrap or |
Contributor
There was a problem hiding this comment.
P3: The README says the short/medium/long profiles target approximately 400, 1,000, and 3,000 "input tokens per request," but generate_ragged_prompts.py targets word counts, not tokens: PROFILES are (250,350,450,550) / (650,850,1150,1350) / (2000,2600,3400,4000) in target_words, and prompt_text builds words up to that size. For the Qwen3.6 tokenizer the real token counts exceed the word counts (longer words like "reproducibility"/"implementation" tokenize to multiple tokens), so the 400/1,000/3,000 figures understate actual input tokens. Wording it as "words (≈ tokens)" or clarifying that tokens are server-observed would keep the protocol description accurate.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At harness/benchmarks/concurrency/README.md, line 39:
<comment>The README says the short/medium/long profiles target approximately 400, 1,000, and 3,000 "input tokens per request," but `generate_ragged_prompts.py` targets word counts, not tokens: `PROFILES` are `(250,350,450,550)` / `(650,850,1150,1350)` / `(2000,2600,3400,4000)` in `target_words`, and `prompt_text` builds `words` up to that size. For the Qwen3.6 tokenizer the real token counts exceed the word counts (longer words like "reproducibility"/"implementation" tokenize to multiple tokens), so the 400/1,000/3,000 figures understate actual input tokens. Wording it as "words (≈ tokens)" or clarifying that tokens are server-observed would keep the protocol description accurate.</comment>
<file context>
@@ -0,0 +1,69 @@
+The 29 generated prompts are disjoint cohorts for C1/C4/C8/C16. C4 and above
+contain four substantial length strata while holding the mean target length
+constant. The default short, medium, and long profiles target approximately
+400, 1,000, and 3,000 input tokens per request. The client refuses to wrap or
+reuse a prompt; reports retain the exact server-observed token counts.
+
</file context>
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.
Summary
Depends on #595 at
a90ffe45. This PR completes concurrent Qwen3.6-27B serving for the requested Strix Halo configuration:Concurrent speculation uses packed verify-and-replay; standalone fast rollback remains a single-sequence path.
Strix Halo measurements
AMD Ryzen AI MAX+ 395 / Radeon 8060S (
gfx1151), ROCm 7.2.4, Release HIP. Full commands, hashes, prompt ranges, and caveats are in STRIX_HALO_RESULTS.md and #596.Recommended routing
Blog-aligned setup: Qwen3.6-27B Q4_K_M target, Q8_0 draft, SWA 2048, DDTree budget 22, fast rollback flag, paged attention, and Q4_0 paged KV.
Default policy: use fixed blog-aligned DDTree for C=1–3; use AR for C>3 and mixed or unknown traffic. Math500's small C=4 DDTree win is a workload-specific exception.
The earlier adaptive-DDTree screen used a weak-yield prompt cohort and measures fallback overhead; it does not contradict the canonical workload crossover above.
AR prefill tuning at C=16
Three paired observations per band, 16/16 successful requests per case, 64 output tokens. The selected policy keeps packed width at 8 and raises the mixed prefill budget from 2,048 to 4,096 only while an admitted prompt has at least 768 tokens.
Widths 12/16, an 8K idle budget, and a 1,024-token allocation quantum did not survive repeated testing. Short prompts retain the 2K mixed budget.
Complete DDTree + PFlash + KVFlash screen
One activation repeat per level, 64 output tokens, 38K–45K raw prompts compressed to roughly 4.9%, and an 8,192-token resident KV pool:
All rows proved DDTree, PFlash, and KVFlash active. C8 was the peak; C16 became paging-bound.
Validation
Constraints
NoEvictableBlockduring selected prefill still fails instead of deferring.