Add on-demand serving profile control - #127
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:
📝 WalkthroughWalkthroughOn-demand profiling now uses CLI configuration and HTTP start/stop endpoints. Recorder activation is delayed until requested, worker processes acknowledge profile commands through IPC, engines coordinate replicas, and stopping merges fragments into the configured trace output. ChangesHTTP profiling control
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.agents/skills/qwen3-14b-online-perf-test/SKILL.md (1)
151-161: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAvoid double-counting merged traces and retained fragments.
When
trace.jsonexists,events()reads it and then unconditionally reads every retained fragment. Since the documentation states that fragments remain after merging, normal aggregation counts each event twice. Read fragments only when the merged file is absent, or select exactly one source.Suggested fix
if os.path.isfile(m): for e in json.load(open(m)).get("traceEvents",[]): yield e - for f in sorted(glob.glob(os.path.join(D,"fragments","trace.*.jsonl"))): + return + for f in sorted(glob.glob(os.path.join(D,"fragments","trace.*.jsonl"))):🤖 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/qwen3-14b-online-perf-test/SKILL.md around lines 151 - 161, The events() generator currently reads both the merged trace.json and retained fragments, double-counting events; update it to read fragments only when trace.json is absent, while preserving the existing parsing and error-skipping behavior.
🧹 Nitpick comments (1)
pypto_serving/serving/server/serving_worker.py (1)
537-540: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant branch:
profile_output_queuealready defaults toNone.
WorkerProcess.__init__already acceptsprofile_output_queue: mp.Queue | None = None, so this conditional can collapse to a single call.♻️ Simplify to a single call
- if profile_output_queue is None: - worker = WorkerProcess(config, input_queue, output_queue) - else: - worker = WorkerProcess(config, input_queue, output_queue, profile_output_queue) + worker = WorkerProcess(config, input_queue, output_queue, profile_output_queue)🤖 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/serving/server/serving_worker.py` around lines 537 - 540, Remove the redundant conditional around WorkerProcess construction and always instantiate WorkerProcess with config, input_queue, output_queue, and profile_output_queue; rely on WorkerProcess.__init__’s existing None default and preserve the current behavior.
🤖 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/qwen3-14b-online-perf-test/SKILL.md:
- Around line 64-68: Move the /start_profile capture request after the /health,
/v1/models, and completion smoke-test readiness checks in the documented
workflow. Keep the existing startup-log readiness requirements, and ensure
profiling begins only after checklist steps 3–4 complete so those requests are
excluded from the trace.
In `@pypto_serving/serving/server/server.py`:
- Around line 160-171: Update _stop_profile so merge_profile() is always called
after engine.stop_profile(), including when it raises after partially stopping
replicas. Preserve the original exception by performing the merge in the failure
path before re-raising, and avoid merging twice on successful stops.
---
Outside diff comments:
In @.agents/skills/qwen3-14b-online-perf-test/SKILL.md:
- Around line 151-161: The events() generator currently reads both the merged
trace.json and retained fragments, double-counting events; update it to read
fragments only when trace.json is absent, while preserving the existing parsing
and error-skipping behavior.
---
Nitpick comments:
In `@pypto_serving/serving/server/serving_worker.py`:
- Around line 537-540: Remove the redundant conditional around WorkerProcess
construction and always instantiate WorkerProcess with config, input_queue,
output_queue, and profile_output_queue; rely on WorkerProcess.__init__’s
existing None default and preserve the current behavior.
🪄 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: a52e8fd7-6021-412e-9d7e-ec2ebbe451cb
📒 Files selected for processing (11)
.agents/skills/qwen3-14b-online-perf-test/SKILL.mddocs/dev/profile.mdpypto_serving/cli/main.pypypto_serving/serving/engine/async_engine.pypypto_serving/serving/server/ipc.pypypto_serving/serving/server/server.pypypto_serving/serving/server/serving_worker.pypypto_serving/tools/profile/__init__.pypypto_serving/tools/profile/env.pypypto_serving/tools/profile/recorder.pytests/test_profile_control.py
| Wait for `INFO: Application startup complete.` / `Uvicorn running on http://0.0.0.0:<port>` before sending traffic. The worker prints `Worker entering busy loop` and the engine prints `Engine loop started` once the model and KV cache are ready. Then start the capture: | ||
|
|
||
| ```bash | ||
| curl --noproxy "*" -sf -X POST http://localhost:$PORT/start_profile | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Run readiness checks before starting the capture.
/start_profile is called before the subsequent /health, /v1/models, and completion smoke test, so those readiness requests contaminate the profiling trace. Move this call after the checks, matching checklist steps 3–4.
🤖 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/qwen3-14b-online-perf-test/SKILL.md around lines 64 - 68,
Move the /start_profile capture request after the /health, /v1/models, and
completion smoke-test readiness checks in the documented workflow. Keep the
existing startup-log readiness requirements, and ensure profiling begins only
after checklist steps 3–4 complete so those requests are excluded from the
trace.
| async def _stop_profile(self) -> Response: | ||
| async with self._profile_lock: | ||
| logger.info("Stopping SA profiler...") | ||
| try: | ||
| await self.engine.stop_profile() | ||
| except Exception: | ||
| stop_sa_profile() | ||
| raise | ||
| stop_sa_profile() | ||
| event_count = merge_profile() | ||
| logger.info("SA profiler stopped; merged %d events", event_count) | ||
| return Response(status_code=200) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Merge is skipped when engine.stop_profile() partially fails, stranding already-flushed fragments.
AsyncLLMEngine.stop_profile() always attempts core.stop_profile() on every replica (via gather(return_exceptions=True)) before raising, so most/all workers will have already flushed their fragment files even when this raises. But the except branch here re-raises without calling merge_profile(), so a partial failure leaves flushed data unmerged until the caller happens to retry /stop_profile.
🛠️ Always merge on stop, even if a worker's ack failed
async def _stop_profile(self) -> Response:
async with self._profile_lock:
logger.info("Stopping SA profiler...")
try:
await self.engine.stop_profile()
- except Exception:
- stop_sa_profile()
- raise
- stop_sa_profile()
- event_count = merge_profile()
- logger.info("SA profiler stopped; merged %d events", event_count)
+ finally:
+ stop_sa_profile()
+ event_count = merge_profile()
+ logger.info("SA profiler stopped; merged %d events", event_count)
return Response(status_code=200)📝 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.
| async def _stop_profile(self) -> Response: | |
| async with self._profile_lock: | |
| logger.info("Stopping SA profiler...") | |
| try: | |
| await self.engine.stop_profile() | |
| except Exception: | |
| stop_sa_profile() | |
| raise | |
| stop_sa_profile() | |
| event_count = merge_profile() | |
| logger.info("SA profiler stopped; merged %d events", event_count) | |
| return Response(status_code=200) | |
| async def _stop_profile(self) -> Response: | |
| async with self._profile_lock: | |
| logger.info("Stopping SA profiler...") | |
| try: | |
| await self.engine.stop_profile() | |
| finally: | |
| stop_sa_profile() | |
| event_count = merge_profile() | |
| logger.info("SA profiler stopped; merged %d events", event_count) | |
| return Response(status_code=200) |
🤖 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/serving/server/server.py` around lines 160 - 171, Update
_stop_profile so merge_profile() is always called after engine.stop_profile(),
including when it raises after partially stopping replicas. Preserve the
original exception by performing the merge in the failure path before
re-raising, and avoid merging twice on successful stops.
|
HTTP serving 检测到 SA_PROFILE_* 但未传 --profile 时给个deprecated 的 warning？ 避免用户按旧方式启动以为 profiling 开了实际没开 |
2 participants
Summary
Add opt-in, on-demand profiling control for HTTP serving. Profiling can now be
started and stopped without restarting the server, following the vLLM-style
/start_profileand/stop_profileworkflow.Interface changes
Serving CLI
HTTP serving profiling is now configured with command-line options:
--profile--profile-output PATH.jsontrace path. Defaults to./profile_out.--profile-level LEVELSe2e,kernelorverbose. Defaults toe2e,kernel.--profile-outputand--profile-levelrequire--profile.This replaces
SA_PROFILE_OUTPUTandSA_PROFILE_LEVELfor the HTTP servingentry point. The environment variables remain supported by offline generation,
so the offline profiling interface is unchanged.
HTTP API
The following endpoints are registered when the server is launched with
--profile:POST/start_profilePOST/stop_profileBoth endpoints return HTTP 200 with an empty response body on success. Repeated
start or stop requests are safe.
Profile commands are sent to every worker through IPC. The API waits for worker
acknowledgements before completing the request, so
/stop_profileonly returnsafter worker-local fragments have been flushed and the merged trace has been
written.
Usage
The merged trace is written to
/data/profile/trace.json. Per-process JSONLfragments remain under
/data/profile/fragments/.Validation
and
git diff --checkpass.