DO NOT MERGE - #745
DO NOT MERGE#745jagtesh wants to merge 35 commits into
Conversation
Admit serialized requests after reclaiming idle caches, then fall back to uncached execution before returning a resource error. Bound optional CPU snapshots and preserve full-cache indexes when eviction creates holes. Add RAM-budget and cache-eviction tests.
Bound queuing and request deadlines, cancel abandoned generations, and recycle failed backends. Add configurable model/runtime paths, generic service and logging templates, deployment documentation, and eight gateway tests. Keep machine-specific configuration, weights, logs, and rollback artifacts untracked.
Admit serialized requests after reclaiming idle caches, then fall back to uncached execution before returning a resource error. Bound optional CPU snapshots and preserve full-cache indexes when eviction creates holes. Add RAM-budget and cache-eviction tests.
Bound queuing and request deadlines, cancel abandoned generations, and recycle failed backends. Add configurable model/runtime paths, generic service and logging templates, deployment documentation, and eight gateway tests. Keep machine-specific configuration, weights, logs, and rollback artifacts untracked.
There was a problem hiding this comment.
13 issues found across 253 files
Not reviewed (too large): benchmarks/qwen-swift-20260916/swift-controlled-v1/results.json (~7,766 lines), benchmarks/bonsai-vs-swift-20260917/results.json (~6,992 lines), benchmarks/bonsai-vs-swift-20260917/report-source/reviewed-data.json (~3,945 lines), benchmarks/bonsai-vs-swift-20260917/dflash-remaining/results.json (~2,798 lines), benchmarks/bonsai-vs-swift-20260917/dflash-followup/results.json (~1,400 lines), benchmarks/qwen-swift-20260916/unsloth-controlled-once/results.json (~1,384 lines) - if these are generated or fixture files, add them to ignored paths to exclude them from future reviews.
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/README.md">
<violation number="1" location="server/README.md:830">
P2: The CLI now rejects `--draft-residency` and `--lazy-draft`, but the repository's own launch configurations still pass them, so those paths fail at server startup instead of running without the feature. `server/scripts/entrypoint.sh` adds `--lazy-draft` to the server command when `DFLASH_LAZY=1` (and its comment still says the C++ server "silently drops" it), while the native `dflash_server` now exits with `[server] unknown option: --lazy-draft` (return code 2), which fails the container under `set -euo pipefail`. `harness/benchmarks/run_lucebox_vs_llamacpp.sh` defaults `EXTRA_SERVER_ARGS=--lazy-draft`, `harness/client_test_runner.py` passes `--lazy-draft` in the `rtx3090_dflash_long` and `rtx3090_pflash_32k` profiles, and `harness/benchmarks/concurrency/run_qwen36_canonical_concurrency.sh` passes `--draft-residency persistent` — all against the same `server/build/dflash_server` binary. Update these launch configs (drop the removed flags) as part of this change so the docs' migration instruction matches the repo state.</violation>
</file>
<file name="server/src/server/prefix_cache.h">
<violation number="1" location="server/src/server/prefix_cache.h:290">
P1: When session mode runs through the classic `HttpServer` path, every inline lookup and capture is rejected because the caller supplies no session ID. Route session-mode requests exclusively through the owner-aware scheduler or propagate `req.session_id` through this path.</violation>
</file>
<file name="server/deps/llama.cpp/ggml/src/ggml-cuda/vecdotq.cuh">
<violation number="1" location="server/deps/llama.cpp/ggml/src/ggml-cuda/vecdotq.cuh:166">
P1: When `GGML_USE_MUSA` is enabled, this Q2/PQ2 path enters the non-HIP branch and calls unsupported CUDA byte-permute intrinsics, so the MUSA backend cannot compile; the PTQ1 helper has the same issue. Guard these CUDA paths for MUSA and provide portable unpacking like the existing table lookup does.</violation>
</file>
<file name="server/src/common/model_backend.h">
<violation number="1" location="server/src/common/model_backend.h:439">
P2: When hybrid compression runs through DeepSeek4, Qwen3, or the legacy/base path, `CompressRequest::should_cancel` is silently dropped, so client disconnects and expired deadlines do not stop PFlash scoring. Thread `request.should_cancel` through every typed and legacy compression path, including the base `DaemonIO`, before exposing this callback in the common request contract.</violation>
</file>
<file name="server/deps/llama.cpp/ggml/src/ggml.c">
<violation number="1" location="server/deps/llama.cpp/ggml/src/ggml.c:1001">
P1: When any newly registered Q2_0, PQ2_0, or PTQ1_0 tensor is multiplied on the CPU with an F32 RHS, the CPU backend accepts the operation despite having no CPU trait entry, then calls a null dot-product function (or loops with zero row stride). Add CPU traits and kernels, or explicitly reject these types in the CPU backend before registering them as usable types.</violation>
</file>
<file name="server/src/qwen35/gguf_target_loader.cpp">
<violation number="1" location="server/src/qwen35/gguf_target_loader.cpp:1128">
P2: When `token_embd.weight` has a different width than `out.n_embd`, this lookup silently leaves `bonsai_signs` empty and `embed()` still processes `out.n_embd` values. Reject the model unless the embedding tensor width and its sign vector match `out.n_embd` before enabling inverse embedding.</violation>
</file>
<file name="server/src/server/hybrid_http.inc">
<violation number="1" location="server/src/server/hybrid_http.inc:258">
P2: When compression validation or scoring fails after this call, the cache has already evicted entries and may have promoted a source snapshot, but no finalizer restores that state. Validate before committing the plan or add rollback for every post-execution failure.</violation>
</file>
<file name="benchmarks/qwen-swift-20260916/swift-controlled-v1/frozen-template.jinja">
<violation number="1" location="benchmarks/qwen-swift-20260916/swift-controlled-v1/frozen-template.jinja:47">
P2: Requests selecting `low` or `medium` still receive the `xhigh` instruction. The Jinja renderer never adds `reasoning_effort` to its context, so `default('xhigh')` is always selected; pass the normalized effort into the context before evaluating this template.</violation>
<violation number="2" location="benchmarks/qwen-swift-20260916/swift-controlled-v1/frozen-template.jinja:121">
P1: After a restart or tool-memory eviction, a standard assistant `tool_calls` history reaches this template without `message.tool_calls`, so the assistant invocation is silently omitted from the prompt. Preserve tool calls in the Jinja input or reconstruct them from the request rather than relying only on in-memory `ToolMemory`.</violation>
</file>
<file name="benchmarks/qwen-swift-20260916/swift-mtp/evaluate-xhigh.py">
<violation number="1" location="benchmarks/qwen-swift-20260916/swift-mtp/evaluate-xhigh.py:20">
P2: When the model emits a correct first call plus another call, this parser ignores the extra call and marks the case passed. Require exactly one tool call before parsing its arguments.</violation>
<violation number="2" location="benchmarks/qwen-swift-20260916/swift-mtp/evaluate-xhigh.py:24">
P2: For `tool_call`, a matching argument object with `finish_reason` `stop` or another non-`length` value still passes, although the rubric requires `tool_calls`. Compare against `tool_calls` for this case and `stop` for normal JSON cases.</violation>
</file>
<file name="benchmarks/qwen-swift-20260916/swift-mtp/evaluate.py">
<violation number="1" location="benchmarks/qwen-swift-20260916/swift-mtp/evaluate.py:22">
P2: The evaluator counts fenced JSON and removed `<think>` wrappers as valid despite the JSON-only contract. Parse the raw assistant content so format violations fail the strict score.</violation>
<violation number="2" location="benchmarks/qwen-swift-20260916/swift-mtp/evaluate.py:24">
P2: A response such as `{"parts": 527.0}` passes the integer case because Python equality ignores the integer-versus-float distinction. Validate the recursive JSON shape and types before comparing expected values.</violation>
</file>
Note: This PR contains a large number of files. cubic selects up to 200 of the highest-priority eligible files for this review, so some files may not have been reviewed.
Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.
Re-trigger cubic
| else: | ||
| content=(msg.get('content') or '').strip();value=json.loads(content) | ||
| except (ValueError,KeyError,IndexError):pass | ||
| row={'name':name,'wall_seconds':elapsed,'passed':value==expected and choice['finish_reason']!='length','expected':expected,'response':response,'request':body};rows.append(row) |
There was a problem hiding this comment.
P2: For tool_call, a matching argument object with finish_reason stop or another non-length value still passes, although the rubric requires tool_calls. Compare against tool_calls for this case and stop for normal JSON cases.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At benchmarks/qwen-swift-20260916/swift-mtp/evaluate-xhigh.py, line 24:
<comment>For `tool_call`, a matching argument object with `finish_reason` `stop` or another non-`length` value still passes, although the rubric requires `tool_calls`. Compare against `tool_calls` for this case and `stop` for normal JSON cases.</comment>
<file context>
@@ -0,0 +1,26 @@
+ else:
+ content=(msg.get('content') or '').strip();value=json.loads(content)
+ except (ValueError,KeyError,IndexError):pass
+ row={'name':name,'wall_seconds':elapsed,'passed':value==expected and choice['finish_reason']!='length','expected':expected,'response':response,'request':body};rows.append(row)
+ (out/(label+'.json')).write_text(json.dumps(rows,indent=2));print(json.dumps({'name':name,'seconds':elapsed,'passed':row['passed'],'usage':response.get('usage'),'finish':choice['finish_reason']}),flush=True)
+print(json.dumps({'label':label,'passed':sum(x['passed'] for x in rows),'total':len(rows),'seconds':sum(x['wall_seconds'] for x in rows)}),flush=True)
</file context>
| if name=='tool_call': | ||
| call=msg['tool_calls'][0]['function'];value=json.loads(call['arguments']) if call['name']=='lookup_weather' else None | ||
| else: | ||
| content=re.sub(r'<think>.*?</think>','',msg.get('content') or '',flags=re.S).strip();content=re.sub(r'^```(?:json)?\s*|\s*```$','',content);value=json.loads(content) |
There was a problem hiding this comment.
P2: The evaluator counts fenced JSON and removed <think> wrappers as valid despite the JSON-only contract. Parse the raw assistant content so format violations fail the strict score.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At benchmarks/qwen-swift-20260916/swift-mtp/evaluate.py, line 22:
<comment>The evaluator counts fenced JSON and removed `<think>` wrappers as valid despite the JSON-only contract. Parse the raw assistant content so format violations fail the strict score.</comment>
<file context>
@@ -0,0 +1,26 @@
+ if name=='tool_call':
+ call=msg['tool_calls'][0]['function'];value=json.loads(call['arguments']) if call['name']=='lookup_weather' else None
+ else:
+ content=re.sub(r'<think>.*?</think>','',msg.get('content') or '',flags=re.S).strip();content=re.sub(r'^```(?:json)?\s*|\s*```$','',content);value=json.loads(content)
+ except (ValueError,KeyError,IndexError):pass
+ row={'name':name,'wall_seconds':elapsed,'passed':value==expected and choice['finish_reason']!='length','expected':expected,'response':response,'request':body};rows.append(row)
</file context>
| else: | ||
| content=re.sub(r'<think>.*?</think>','',msg.get('content') or '',flags=re.S).strip();content=re.sub(r'^```(?:json)?\s*|\s*```$','',content);value=json.loads(content) | ||
| except (ValueError,KeyError,IndexError):pass | ||
| row={'name':name,'wall_seconds':elapsed,'passed':value==expected and choice['finish_reason']!='length','expected':expected,'response':response,'request':body};rows.append(row) |
There was a problem hiding this comment.
P2: A response such as {"parts": 527.0} passes the integer case because Python equality ignores the integer-versus-float distinction. Validate the recursive JSON shape and types before comparing expected values.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At benchmarks/qwen-swift-20260916/swift-mtp/evaluate.py, line 24:
<comment>A response such as `{"parts": 527.0}` passes the integer case because Python equality ignores the integer-versus-float distinction. Validate the recursive JSON shape and types before comparing expected values.</comment>
<file context>
@@ -0,0 +1,26 @@
+ else:
+ content=re.sub(r'<think>.*?</think>','',msg.get('content') or '',flags=re.S).strip();content=re.sub(r'^```(?:json)?\s*|\s*```$','',content);value=json.loads(content)
+ except (ValueError,KeyError,IndexError):pass
+ row={'name':name,'wall_seconds':elapsed,'passed':value==expected and choice['finish_reason']!='length','expected':expected,'response':response,'request':body};rows.append(row)
+ (out/(label+'.json')).write_text(json.dumps(rows,indent=2));print(json.dumps({'name':name,'seconds':elapsed,'passed':row['passed'],'usage':response.get('usage'),'finish':choice['finish_reason']}),flush=True)
+print(json.dumps({'label':label,'passed':sum(x['passed'] for x in rows),'total':len(rows),'seconds':sum(x['wall_seconds'] for x in rows)}),flush=True)
</file context>
| } | ||
| if (out.bonsai->meta.inverse_embedding) { | ||
| out.embedder.bonsai_block = out.bonsai->meta.block; | ||
| const auto signs = out.bonsai->meta.signs.find(out.n_embd); |
There was a problem hiding this comment.
P2: When token_embd.weight has a different width than out.n_embd, this lookup silently leaves bonsai_signs empty and embed() still processes out.n_embd values. Reject the model unless the embedding tensor width and its sign vector match out.n_embd before enabling inverse embedding.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/qwen35/gguf_target_loader.cpp, line 1128:
<comment>When `token_embd.weight` has a different width than `out.n_embd`, this lookup silently leaves `bonsai_signs` empty and `embed()` still processes `out.n_embd` values. Reject the model unless the embedding tensor width and its sign vector match `out.n_embd` before enabling inverse embedding.</comment>
<file context>
@@ -1103,6 +1117,19 @@ bool load_target_gguf_partial(const std::string & path,
+ }
+ if (out.bonsai->meta.inverse_embedding) {
+ out.embedder.bonsai_block = out.bonsai->meta.block;
+ const auto signs = out.bonsai->meta.signs.find(out.n_embd);
+ if (signs != out.bonsai->meta.signs.end()) out.embedder.bonsai_signs = signs->second;
+ }
</file context>
Co-authored-by: OpenAI Codex <noreply@openai.com>
0fa3c8e to
5d995bc
Compare
Uh oh!
There was an error while loading. Please reload this page.