feat(skills): add DeepSeek V4 online SA_PROFILE perf skill - #121
feat(skills): add DeepSeek V4 online SA_PROFILE perf skill#121sunghajung6688 wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdds a skill guide for profiling DeepSeek V4 online serving on pypto-serving, covering eight-NPU setup, benchmark execution, trace merging, kernel analysis, troubleshooting, and metric reporting. ChangesDeepSeek V4 online profiling
Estimated code review effort: 1 (Trivial) | ~5 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
61a09f8 to
e066f7b
Compare
DeepSeek has no offline generation entry, so this is the online-only counterpart of qwen3-14b-online-perf-test. It profiles the 8-NPU overlapped DP=8/EP=8 serving run with SA_PROFILE and documents the DeepSeek-specific prefill sub-phase spans plus MTP decode interpretation. Default workload 128/128/16 capped at --max-model-len 512.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.agents/skills/deepseek-v4-online-perf-test/SKILL.md (1)
165-166: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winInclude the mean in the aggregator output.
The checklist and interpretation sections require per-kernel mean timing, but the compact script prints only total duration and count. Add
d / nto the output or explicitly document that readers must calculate it manually.Also applies to: 205-205
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.agents/skills/deepseek-v4-online-perf-test/SKILL.md around lines 165 - 166, Update the compact aggregator output in the sorted tot reporting loop to include each kernel’s mean timing, calculated as d / n, alongside the existing total duration and count. Apply the same change to the corresponding output at the additionally referenced location, preserving the current ordering and formatting.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agents/skills/deepseek-v4-online-perf-test/SKILL.md:
- Around line 102-107: The benchmark instructions around tests/bench_serving.py
must pass the served model name in every workload request. Update the documented
command or payload configuration to include the model argument used by the
serving endpoint, or explicitly document and verify that the endpoint supplies a
default model; ensure requests no longer fail model validation before metrics
are collected.
- Around line 118-121: Update the trace-loading logic, particularly events(), to
select the merged trace.json when it exists instead of also reading retained
fragments; only fall back to fragments/trace.*.jsonl when no merged file is
available. Preserve fragment-only aggregation for ungraceful shutdowns or
interim reads while the server is still running, without yielding the same
events from both sources.
- Around line 110-112: Revise the benchmark documentation around the --stream
guidance and the referenced MTP throughput/TPOT statements to describe tok/s,
decode intervals, and related measurements as streamed-chunk metrics rather than
accepted-token metrics. Remove or qualify claims that these values represent
actual tokens or user-perceived MTP throughput; do not change benchmark code
unless implementing real token counting.
---
Nitpick comments:
In @.agents/skills/deepseek-v4-online-perf-test/SKILL.md:
- Around line 165-166: Update the compact aggregator output in the sorted tot
reporting loop to include each kernel’s mean timing, calculated as d / n,
alongside the existing total duration and count. Apply the same change to the
corresponding output at the additionally referenced location, preserving the
current ordering and formatting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1cc59173-5182-44b3-afcf-3e24bd33814c
📒 Files selected for processing (1)
.agents/skills/deepseek-v4-online-perf-test/SKILL.md
| Drive the workload with **`tests/bench_serving.py` (default — no install)**. The repo's own async benchmark drives the endpoint with aiohttp (already in the env) and prints TTFT, per-token decode interval, throughput (req/s, tok/s), and latency p50/p99. Run the config: | ||
|
|
||
| ```bash | ||
| PORT=<your-port> | ||
| python tests/bench_serving.py --host localhost --port "$PORT" \ | ||
| --input-len 128 --max-tokens 128 -n 16 -c 16 --stream |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Pass the served model to the benchmark requests.
The supplied tests/bench_serving.py payload contains prompt, max_tokens, temperature, and stream, but no model. Since the readiness request includes model and the troubleshooting section identifies model validation as a 422 cause, this workload can fail every request before collecting metrics. Update the benchmark CLI/payload to send the served model name, or document and verify an endpoint default.
Also applies to: 195-195
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.agents/skills/deepseek-v4-online-perf-test/SKILL.md around lines 102 - 107,
The benchmark instructions around tests/bench_serving.py must pass the served
model name in every workload request. Update the documented command or payload
configuration to include the model argument used by the serving endpoint, or
explicitly document and verify that the endpoint supplies a default model;
ensure requests no longer fail model validation before metrics are collected.
| `--stream` enables TTFT + per-token decode measurement. `-n` is the request count, `-c` the concurrency, `--input-len` builds a fixed-length synthetic prompt (approximate; the exact count shows in the server log and the SA_PROFILE `prefill_fwd` span). | ||
|
|
||
| Under MTP, `bench_serving.py`'s tok/s reflects **accepted** tokens (main + committed draft), which is the throughput the user perceives — it does not need MTP-aware correction. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not describe streamed chunks as token-level metrics.
tests/bench_serving.py records one timestamp per non-empty SSE chunk and sets num_tokens = len(token_times); it neither tokenizes delta nor reads accepted/draft token metadata. Therefore tok/s, decode intervals, and the MTP TPOT guidance are chunk metrics, not necessarily accepted-token metrics. Update the benchmark to count actual tokens, or weaken these claims.
Also applies to: 174-174, 205-205
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.agents/skills/deepseek-v4-online-perf-test/SKILL.md around lines 110 - 112,
Revise the benchmark documentation around the --stream guidance and the
referenced MTP throughput/TPOT statements to describe tok/s, decode intervals,
and related measurements as streamed-chunk metrics rather than accepted-token
metrics. Remove or qualify claims that these values represent actual tokens or
user-perceived MTP throughput; do not change benchmark code unless implementing
real token counting.
| - **Preferred:** stop the server gracefully (`SIGTERM`/`SIGINT` to the `pypto-serving` process). The application-shutdown path waits for the workers and calls `merge_profile()`, producing `<SA_PROFILE_OUTPUT>/trace.json`. | ||
| - **If the server was killed ungracefully:** run `./scripts/merge_profile.sh <SA_PROFILE_OUTPUT>` (or `SA_PROFILE_OUTPUT=<dir> ./scripts/merge_profile.sh`). Stop all profiled processes first so buffered events are flushed. | ||
|
|
||
| Fragments are retained after merging, so aggregation also works directly on `fragments/trace.*.jsonl` without a merged file — useful for an interim read while the server is still running (some recent events may still be buffered). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Avoid double-counting merged trace events.
After graceful shutdown, trace.json contains the merged events while the fragments are intentionally retained. events() then yields both sources, so every event is counted twice and total duration, counts, and device time are inflated.
Proposed fix
def events():
m=os.path.join(D,"trace.json")
if os.path.isfile(m):
for e in json.load(open(m)).get("traceEvents",[]): yield e
+ return
for f in sorted(glob.glob(os.path.join(D,"fragments","trace.*.jsonl"))):Also applies to: 149-159
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.agents/skills/deepseek-v4-online-perf-test/SKILL.md around lines 118 - 121,
Update the trace-loading logic, particularly events(), to select the merged
trace.json when it exists instead of also reading retained fragments; only fall
back to fragments/trace.*.jsonl when no merged file is available. Preserve
fragment-only aggregation for ungraceful shutdowns or interim reads while the
server is still running, without yielding the same events from both sources.
1 participant
DeepSeek has no offline generation entry, so this is the online-only counterpart of qwen3-14b-online-perf-test. It profiles the 8-NPU overlapped DP=8/EP=8 serving run with SA_PROFILE and documents the DeepSeek-specific prefill sub-phase spans plus MTP decode interpretation. Default workload 128/128/16 capped at --max-model-len 512.