Add DeepSeek persistent L3 profiling - #111
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughDeepSeek V4 compilation and serving paths now emit profiling spans with execution timings and metadata across prefill, decode, MTP, L3, buffer, weight, and worker operations. Accuracy-test process shutdown now uses staged signal escalation. ChangesDeepSeek V4 profiling
Accuracy-test process shutdown
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant DeepSeekV4ModelRunner
participant profile_span
participant _run_l3
participant DistributedWorker
DeepSeekV4ModelRunner->>profile_span: trace serving preparation and logits
DeepSeekV4ModelRunner->>_run_l3: dispatch L3 callable
_run_l3->>profile_span: create outer and worker spans
_run_l3->>DistributedWorker: execute worker callable
DistributedWorker-->>_run_l3: provide execution timing
Possibly related PRs
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 |
![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
🤖 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 `@pypto_serving/model/deepseek/npu_runner.py`:
- Around line 2101-2111: Update the profile_span metadata in the decode MTP
advance block to represent acceptance across the entire batch, replacing the
request-specific len(accepted[0]) value with the per-request accepted-count
tuple or an explicitly documented aggregate. Keep the existing
_advance_mtp_drafts call and acceptance behavior unchanged.
- Around line 945-946: Update the DeepSeekV4L3Callable construction in _run_l3
to populate block_dim and aicpu_thread_num from the applicable compile/runtime
configuration instead of relying on dataclass defaults; if either value is
unavailable, omit it or preserve an explicitly unknown value rather than
recording block_dim=None and aicpu_thread_num=4.
In `@tests/test_deepseek_v4_accuracy.py`:
- Line 216: In the process-wait exception handlers around the affected test
cleanup logic, replace the broad Exception catches with only the expected
OSError and supported subprocess exception types. Update all three handlers at
the referenced locations while preserving their existing suppression behavior
and conforming to the repository’s Ruff configuration.
🪄 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: b6d3b192-6116-42ab-90bb-15295d2503cb
📒 Files selected for processing (4)
pypto_serving/model/deepseek/npu_executor.pypypto_serving/model/deepseek/npu_runner.pytests/test_deepseek_v4.pytests/test_deepseek_v4_accuracy.py
| block_dim: int | None = None | ||
| aicpu_thread_num: int = 4 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate files:"
fd -a 'npu_(runner|executor)\.py$' . || true
echo
echo "Check relevant symbols/usages:"
rg -n "class DeepSeekV4L3Callable|def _compile_l3_callable|compile_l3|DeepSeekV4L3Callable\(" -S .
echo
echo "Inspect relevant lines:"
for f in $(fd 'npu_(runner|executor)\.py$' .); do
echo "===== $f ====="
wc -l "$f"
doneRepository: hw-native-sys/pypto-serving
Length of output: 2379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== runner callable metadata/default lines ====="
sed -n '920,970p' pypto_s serving/model/deepseek/npu_runner.py 2>/dev/null || sed -n '920,970p' pypto_serving/model/deepseek/npu_runner.py
echo
echo "===== _compile_l3_callable implementation ====="
sed -n '480,550p' pypto_serving/model/deepseek/npu_executor.py
echo
echo "===== compilation args and callables around l3 compile calls ====="
sed -n '360,435p' pypto_serving/model/deepseek/npu_executor.py
echo
echo "===== search for aicpu_thread_num/block_dim in runner/executor ====="
rg -n "aicpu_thread_num|block_dim" pypto_serving/model/deepseek/npu_runner.py pypto_serving/model/deepseek/npu_executor.py -C 2Repository: hw-native-sys/pypto-serving
Length of output: 9535
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== call sites for DeepSeekV4L3Callable/span metadata ====="
rg -n "DeepSeekV4L3Callable|profile_span\(.*kernel|block_dim|aicpu_thread_num" pypto_serving/model pypto_serving -S --glob '*.py' -C 2
echo
echo "===== read-only verifier: construction call sites and defaults ====="
python3 - <<'PY'
from pathlib import Path
import re
runner = Path("pypto_serving/model/deepseek/npu_runner.py").read_text()
executor = Path("pypto_serving/model/deepseek/npu_executor.py").read_text()
constructor = re.search(r"return DeepSeekV4L3Callable\((?P<body>.*?)(?:\n[A-Za-z_][A-Za-z0-9_]*|$\s*$)", executor, re.S)
print("constructor_text:", re.sub(r"\s+", " ", constructor.group(0)))
print("passes_block_dim:", "block_dim" in executor)
print("passes_aicpu_thread_num:", "aicpu_thread_num" in executor)
print("callable class fields:")
class_match = re.search(r"`@dataclass`\n\nclass DeepSeekV4L3Callable:\n(?P<body>.*?\n\nclass )", runner, re.S | re.M)
for line in class_match.group(0).splitlines():
if re.match(r'\s+(compiled|name|block_dim|aicpu_thread_num)\s*:', line):
print(line.strip())
print("span args include callable metadata:", "span_args = {" in runner and '"block_dim": callable_spec.block_dim' in runner)
PYRepository: hw-native-sys/pypto-serving
Length of output: 37820
Populate L3 callable metadata in production compilation.
DeepSeekV4L3Callable(compiled=compiled, name=name) relies on the dataclass defaults, so kernels traced by _run_l3 always record block_dim=None and aicpu_thread_num=4. Populate these fields from the compile/runtime configuration, or omit them until their values are known.
🤖 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 `@pypto_serving/model/deepseek/npu_runner.py` around lines 945 - 946, Update
the DeepSeekV4L3Callable construction in _run_l3 to populate block_dim and
aicpu_thread_num from the applicable compile/runtime configuration instead of
relying on dataclass defaults; if either value is unavailable, omit it or
preserve an explicitly unknown value rather than recording block_dim=None and
aicpu_thread_num=4.
| with profile_span( | ||
| "DeepSeekV4ModelRunner.decode.mtp_advance", | ||
| cat="executor", | ||
| args={"accepted_count": len(accepted[0]) if accepted else 0}, | ||
| ): | ||
| self._advance_mtp_drafts( | ||
| inputs, | ||
| main_ids, | ||
| output.pre_hc_hidden, | ||
| accepted_counts=tuple(len(tokens) for tokens in accepted), | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Record MTP acceptance for the whole batch.
accepted is request-specific, but the span records only len(accepted[0]); mixed batches therefore expose misleading metadata. Record the per-request tuple or an explicitly documented aggregate.
Suggested adjustment
- args={"accepted_count": len(accepted[0]) if accepted else 0},
+ args={"accepted_counts": tuple(len(tokens) for tokens in accepted)},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with profile_span( | |
| "DeepSeekV4ModelRunner.decode.mtp_advance", | |
| cat="executor", | |
| args={"accepted_count": len(accepted[0]) if accepted else 0}, | |
| ): | |
| self._advance_mtp_drafts( | |
| inputs, | |
| main_ids, | |
| output.pre_hc_hidden, | |
| accepted_counts=tuple(len(tokens) for tokens in accepted), | |
| ) | |
| with profile_span( | |
| "DeepSeekV4ModelRunner.decode.mtp_advance", | |
| cat="executor", | |
| args={"accepted_counts": tuple(len(tokens) for tokens in accepted)}, | |
| ): | |
| self._advance_mtp_drafts( | |
| inputs, | |
| main_ids, | |
| output.pre_hc_hidden, | |
| accepted_counts=tuple(len(tokens) for tokens in accepted), | |
| ) |
🤖 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 `@pypto_serving/model/deepseek/npu_runner.py` around lines 2101 - 2111, Update
the profile_span metadata in the decode MTP advance block to represent
acceptance across the entire batch, replacing the request-specific
len(accepted[0]) value with the per-request accepted-count tuple or an
explicitly documented aggregate. Keep the existing _advance_mtp_drafts call and
acceptance behavior unchanged.
| process.wait(timeout=10) | ||
| except subprocess.TimeoutExpired: | ||
| print(f"WARNING: process group {process.pid} still alive after SIGKILL", flush=True) | ||
| except Exception as exc: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace blind Exception handlers with expected process-wait errors.
Lines 216, 218, and 221 suppress unrelated failures and trigger Ruff BLE001. Catch OSError and explicitly supported subprocess exceptions instead.
Proposed fix
- except Exception as exc:
+ except OSError as exc:
print(f"WARNING: failed to reap process group {process.pid}: {exc}", flush=True)
...
- except Exception as exc:
+ except OSError as exc:
print(f"WARNING: failed to reap process group {process.pid}: {exc}", flush=True)
...
- except Exception as exc:
+ except OSError as exc:
print(f"WARNING: failed to wait for process group {process.pid}: {exc}", flush=True)As per coding guidelines, follow the repository’s Ruff configuration.
Also applies to: 218-218, 221-221
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 216-216: Do not catch blind exception: Exception
(BLE001)
🤖 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 `@tests/test_deepseek_v4_accuracy.py` at line 216, In the process-wait
exception handlers around the affected test cleanup logic, replace the broad
Exception catches with only the expected OSError and supported subprocess
exception types. Update all three handlers at the referenced locations while
preserving their existing suppression behavior and conforming to the
repository’s Ruff configuration.
Sources: Coding guidelines, Linters/SAST tools
2 participants
What changed
DistributedWorkerso communication domains and worker state are reused across requests.Why
DeepSeek serving previously had persistent-L3 support at the model runner boundary but lacked complete framework-level timing coverage. The accuracy harness also terminated the whole process group immediately, which could leave the serving worker or profiling merge incomplete.
Validation
python -m pytest tests/test_deepseek_v4.py -q— 31 passedpython -m pytest tests/test_deepseek_v4_accuracy.py -q -k 'not deepseek_v4_http_completion_matches_expected_text'— 3 passedgit diff --checkpassed