feat: Debugging tools: backtrace collection for hangs and nvidia-smi monitoring#159
feat: Debugging tools: backtrace collection for hangs and nvidia-smi monitoring#159trevor-m wants to merge 11 commits into
Conversation
📝 WalkthroughWalkthroughThis pull request introduces automated hang debugging and GPU monitoring capabilities to the SGLang infrastructure. It adds new configuration classes for debug and NVIDIA SMI settings, implements a bash script to collect Python and CUDA backtraces from worker processes, integrates debugging into worker startup, and provides comprehensive documentation. Container name support is added to SLURM commands to facilitate debugging within containers. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/srtctl/cli/mixins/worker_stage.py (1)
293-306:⚠️ Potential issue | 🟡 MinorMissing
container_nameinstart_endpoint_workersrun call.
start_worker(line 196) passescontainer_name=self.runtime.container_name, butstart_endpoint_workerdoes not. This means workers launched with MPI-style (used by TRTLLM) won't get the--container-nameflag, preventing debugging tools from attaching to those containers via srun.
🤖 Fix all issues with AI agents
In `@docs/debugging.md`:
- Around line 89-91: Update the documentation to match the actual filenames
produced by the script: replace references to `pyspy_{hostname}_{pid}.txt` and
`cudagdb_{hostname}_{pid}.txt` with the patterns used in collect_backtraces.sh
(e.g. `{node}_{mode}_w{index}_pyspy[_p{n}].txt` and
`{node}_{mode}_w{index}_cudagdb[_p{n}].txt`), and add a short note explaining
optional `_p{n}` suffix for per-process files so the docs reflect the real
naming in collect_backtraces.sh.
- Line 72: The docs currently claim containers are named
`sglang_{job_id}_{node}`, but the actual implementation
(RuntimeContext.container_name in src/srtctl/core/runtime.py) returns
`srtctl_{job_id}`; update the documentation entry in docs/debugging.md to
reflect the actual format `srtctl_{job_id}` (or, if the intended canonical name
is `sglang_{job_id}_{node}`, change RuntimeContext.container_name to build that
format); ensure the doc string and RuntimeContext.container_name agree and
reference the single source of truth you choose.
In `@src/srtctl/benchmarks/scripts/debug/collect_backtraces.sh`:
- Around line 126-143: The cuda-gdb invocation inside the background subshell
that references PID and CUDAGDB_FILE can hang indefinitely; wrap the cuda-gdb
call with the timeout command (60s as per docs) so the child is killed after the
timeout, preserve the existing -batch, -ex args and redirection, and ensure the
existing failure handler still runs when timeout exits non‑zero (i.e., keep the
"|| { echo ...; echo 'ERROR: cuda-gdb failed' >> \"${CUDAGDB_FILE}\"; }"
behavior). Also make sure the timeout exit code is treated as failure so the
warning/log message is emitted if cuda-gdb is terminated by timeout.
In `@src/srtctl/cli/mixins/worker_stage.py`:
- Around line 86-97: The code in the worker_stage mixin ignores the configured
debug output path by hardcoding output_dir = "/logs/backtraces"; change it to
read the user-configured path (self.config.debug.output_dir) with a sensible
fallback to "/logs/backtraces", and ensure the same output_dir variable is used
for both mkdir -p and the collect_backtraces.sh invocation (the block around
debug_script, wait_seconds, output_dir, node, mode, index and log_file). Update
any tests that assume the hardcoded path to use the configured value if
necessary.
In `@src/srtctl/core/runtime.py`:
- Around line 277-284: The docstring for the container_name property is
inconsistent with the returned value: update either the docstring or the
returned string so they match; specifically, in the container_name property
(method name container_name) ensure the docstring's example format matches the
actual return value f"srtctl_{self.job_id}" (or change the return to
f"sglang_{self.job_id}" if that's the intended name) and also update the
matching mention in docs/debugging.md (around the referenced line) so all
references consistently use the same prefix.
🧹 Nitpick comments (2)
docs/debugging.md (1)
99-99: Add language identifiers to fenced code blocks.Several fenced code blocks (lines 99, 120, 137, 145, 152, 169) lack a language identifier, which triggers markdownlint MD040 warnings. Consider using
textorplaintextfor example output blocks.src/srtctl/benchmarks/scripts/debug/collect_backtraces.sh (1)
86-95: Consider adding a timeout to py-spy as well.While py-spy is described as non-invasive, it can still hang if ptrace is blocked or the process is in an uninterruptible state. A
timeoutguard (e.g.,timeout 30 py-spy dump ...) would prevent the script from stalling in Phase 1 and never reaching Phase 2.
|
|
||
| ## How It Works | ||
|
|
||
| 1. **Named Containers**: Workers are launched with named containers (`sglang_{job_id}_{node}`) using pyxis `--container-name` |
There was a problem hiding this comment.
Documentation inconsistency: container name format doesn't match code.
The docs say containers are named sglang_{job_id}_{node}, but RuntimeContext.container_name in src/srtctl/core/runtime.py (line 284) returns srtctl_{job_id} — no sglang prefix and no _{node} suffix.
📝 Suggested fix
-2. **Named Containers**: Workers are launched with named containers (`sglang_{job_id}_{node}`) using pyxis `--container-name`
+2. **Named Containers**: Workers are launched with named containers (`srtctl_{job_id}`) using pyxis `--container-name`🤖 Prompt for AI Agents
In `@docs/debugging.md` at line 72, The docs currently claim containers are named
`sglang_{job_id}_{node}`, but the actual implementation
(RuntimeContext.container_name in src/srtctl/core/runtime.py) returns
`srtctl_{job_id}`; update the documentation entry in docs/debugging.md to
reflect the actual format `srtctl_{job_id}` (or, if the intended canonical name
is `sglang_{job_id}_{node}`, change RuntimeContext.container_name to build that
format); ensure the doc string and RuntimeContext.container_name agree and
reference the single source of truth you choose.
| 6. **Output**: Two files per process: | ||
| - `{output_dir}/pyspy_{hostname}_{pid}.txt` - Python stack traces | ||
| - `{output_dir}/cudagdb_{hostname}_{pid}.txt` - CUDA kernels and native backtraces |
There was a problem hiding this comment.
Documentation inconsistency: output file naming doesn't match script.
The docs describe output files as pyspy_{hostname}_{pid}.txt and cudagdb_{hostname}_{pid}.txt, but collect_backtraces.sh uses the pattern {node}_{mode}_w{index}_pyspy[_p{n}].txt and {node}_{mode}_w{index}_cudagdb[_p{n}].txt. Please update to reflect the actual naming convention.
🤖 Prompt for AI Agents
In `@docs/debugging.md` around lines 89 - 91, Update the documentation to match
the actual filenames produced by the script: replace references to
`pyspy_{hostname}_{pid}.txt` and `cudagdb_{hostname}_{pid}.txt` with the
patterns used in collect_backtraces.sh (e.g.
`{node}_{mode}_w{index}_pyspy[_p{n}].txt` and
`{node}_{mode}_w{index}_cudagdb[_p{n}].txt`), and add a short note explaining
optional `_p{n}` suffix for per-process files so the docs reflect the real
naming in collect_backtraces.sh.
| ( | ||
| echo "[$(date)] Collecting cuda-gdb backtrace for PID ${PID}..." | ||
| cuda-gdb \ | ||
| -batch \ | ||
| -ex "set logging redirect on" \ | ||
| -ex "set logging file ${CUDAGDB_FILE}" \ | ||
| -ex "set logging enabled on" \ | ||
| -ex "info cuda kernels" \ | ||
| -ex "thread apply all bt" \ | ||
| -ex quit \ | ||
| -p "${PID}" 2>&1 || { | ||
| echo "[$(date)] WARNING: cuda-gdb failed for PID ${PID}" | ||
| echo "ERROR: cuda-gdb failed" >> "${CUDAGDB_FILE}" | ||
| } | ||
| if [ -f "${CUDAGDB_FILE}" ] && [ -s "${CUDAGDB_FILE}" ]; then | ||
| echo "[$(date)] cuda-gdb output saved to ${CUDAGDB_FILE}" | ||
| fi | ||
| ) & |
There was a problem hiding this comment.
Missing timeout for cuda-gdb — script may hang indefinitely.
The documentation (docs/debugging.md line 199) states "The script uses a 60-second timeout per process", but no timeout command wraps the cuda-gdb invocation. If cuda-gdb deadlocks (acknowledged in the docs as a known cause), this background job will never return and wait on line 149 will block forever.
🔧 Suggested fix — wrap with `timeout`
(
echo "[$(date)] Collecting cuda-gdb backtrace for PID ${PID}..."
- cuda-gdb \
+ timeout 60 cuda-gdb \
-batch \
-ex "set logging redirect on" \
-ex "set logging file ${CUDAGDB_FILE}" \📝 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.
| ( | |
| echo "[$(date)] Collecting cuda-gdb backtrace for PID ${PID}..." | |
| cuda-gdb \ | |
| -batch \ | |
| -ex "set logging redirect on" \ | |
| -ex "set logging file ${CUDAGDB_FILE}" \ | |
| -ex "set logging enabled on" \ | |
| -ex "info cuda kernels" \ | |
| -ex "thread apply all bt" \ | |
| -ex quit \ | |
| -p "${PID}" 2>&1 || { | |
| echo "[$(date)] WARNING: cuda-gdb failed for PID ${PID}" | |
| echo "ERROR: cuda-gdb failed" >> "${CUDAGDB_FILE}" | |
| } | |
| if [ -f "${CUDAGDB_FILE}" ] && [ -s "${CUDAGDB_FILE}" ]; then | |
| echo "[$(date)] cuda-gdb output saved to ${CUDAGDB_FILE}" | |
| fi | |
| ) & | |
| ( | |
| echo "[$(date)] Collecting cuda-gdb backtrace for PID ${PID}..." | |
| timeout 60 cuda-gdb \ | |
| -batch \ | |
| -ex "set logging redirect on" \ | |
| -ex "set logging file ${CUDAGDB_FILE}" \ | |
| -ex "set logging enabled on" \ | |
| -ex "info cuda kernels" \ | |
| -ex "thread apply all bt" \ | |
| -ex quit \ | |
| -p "${PID}" 2>&1 || { | |
| echo "[$(date)] WARNING: cuda-gdb failed for PID ${PID}" | |
| echo "ERROR: cuda-gdb failed" >> "${CUDAGDB_FILE}" | |
| } | |
| if [ -f "${CUDAGDB_FILE}" ] && [ -s "${CUDAGDB_FILE}" ]; then | |
| echo "[$(date)] cuda-gdb output saved to ${CUDAGDB_FILE}" | |
| fi | |
| ) & |
🤖 Prompt for AI Agents
In `@src/srtctl/benchmarks/scripts/debug/collect_backtraces.sh` around lines 126 -
143, The cuda-gdb invocation inside the background subshell that references PID
and CUDAGDB_FILE can hang indefinitely; wrap the cuda-gdb call with the timeout
command (60s as per docs) so the child is killed after the timeout, preserve the
existing -batch, -ex args and redirection, and ensure the existing failure
handler still runs when timeout exits non‑zero (i.e., keep the "|| { echo ...;
echo 'ERROR: cuda-gdb failed' >> \"${CUDAGDB_FILE}\"; }" behavior). Also make
sure the timeout exit code is treated as failure so the warning/log message is
emitted if cuda-gdb is terminated by timeout.
| # 3. Hang debugger (runs in background, waits then collects backtraces) | ||
| # Use subshell so the & doesn't break && chaining | ||
| if self.config.debug.enabled and node and mode is not None and index is not None: | ||
| debug_script = "/srtctl-benchmarks/debug/collect_backtraces.sh" | ||
| wait_seconds = self.config.debug.wait_seconds | ||
| output_dir = "/logs/backtraces" | ||
| # Pass node/mode/index for consistent backtrace file naming | ||
| log_file = f"/logs/{node}_hang_debug.log" | ||
| parts.append( | ||
| f"mkdir -p {output_dir} && " | ||
| f"(nohup bash {debug_script} {wait_seconds} {output_dir} {node} {mode} {index} > {log_file} 2>&1 &)" | ||
| ) |
There was a problem hiding this comment.
Bug: config.debug.output_dir is never used — custom output directory is silently ignored.
Line 91 hardcodes output_dir = "/logs/backtraces" regardless of self.config.debug.output_dir. Users who set output_dir: /custom/path/backtraces in their config will have it silently ignored. The schema supports it, the docs describe it, and test_debug_config_enabled asserts the value is stored — but the preamble never reads it.
Proposed fix
if self.config.debug.enabled and node and mode is not None and index is not None:
debug_script = "/srtctl-benchmarks/debug/collect_backtraces.sh"
wait_seconds = self.config.debug.wait_seconds
- output_dir = "/logs/backtraces"
+ output_dir = self.config.debug.output_dir or "/logs/backtraces"
# Pass node/mode/index for consistent backtrace file naming
log_file = f"/logs/{node}_hang_debug.log"📝 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.
| # 3. Hang debugger (runs in background, waits then collects backtraces) | |
| # Use subshell so the & doesn't break && chaining | |
| if self.config.debug.enabled and node and mode is not None and index is not None: | |
| debug_script = "/srtctl-benchmarks/debug/collect_backtraces.sh" | |
| wait_seconds = self.config.debug.wait_seconds | |
| output_dir = "/logs/backtraces" | |
| # Pass node/mode/index for consistent backtrace file naming | |
| log_file = f"/logs/{node}_hang_debug.log" | |
| parts.append( | |
| f"mkdir -p {output_dir} && " | |
| f"(nohup bash {debug_script} {wait_seconds} {output_dir} {node} {mode} {index} > {log_file} 2>&1 &)" | |
| ) | |
| # 3. Hang debugger (runs in background, waits then collects backtraces) | |
| # Use subshell so the & doesn't break && chaining | |
| if self.config.debug.enabled and node and mode is not None and index is not None: | |
| debug_script = "/srtctl-benchmarks/debug/collect_backtraces.sh" | |
| wait_seconds = self.config.debug.wait_seconds | |
| output_dir = self.config.debug.output_dir or "/logs/backtraces" | |
| # Pass node/mode/index for consistent backtrace file naming | |
| log_file = f"/logs/{node}_hang_debug.log" | |
| parts.append( | |
| f"mkdir -p {output_dir} && " | |
| f"(nohup bash {debug_script} {wait_seconds} {output_dir} {node} {mode} {index} > {log_file} 2>&1 &)" | |
| ) |
🤖 Prompt for AI Agents
In `@src/srtctl/cli/mixins/worker_stage.py` around lines 86 - 97, The code in the
worker_stage mixin ignores the configured debug output path by hardcoding
output_dir = "/logs/backtraces"; change it to read the user-configured path
(self.config.debug.output_dir) with a sensible fallback to "/logs/backtraces",
and ensure the same output_dir variable is used for both mkdir -p and the
collect_backtraces.sh invocation (the block around debug_script, wait_seconds,
output_dir, node, mode, index and log_file). Update any tests that assume the
hardcoded path to use the configured value if necessary.
| @property | ||
| def container_name(self) -> str: | ||
| """Container name for this job. | ||
|
|
||
| Returns: | ||
| Container name in format: sglang_{job_id} | ||
| """ | ||
| return f"srtctl_{self.job_id}" |
There was a problem hiding this comment.
Docstring says sglang_{job_id} but code returns srtctl_{job_id}.
The docstring on line 282 says the format is sglang_{job_id}, but line 284 actually returns f"srtctl_{self.job_id}". This same mismatch propagated to docs/debugging.md line 72.
📝 Suggested fix
`@property`
def container_name(self) -> str:
"""Container name for this job.
Returns:
- Container name in format: sglang_{job_id}
+ Container name in format: srtctl_{job_id}
"""
return f"srtctl_{self.job_id}"🤖 Prompt for AI Agents
In `@src/srtctl/core/runtime.py` around lines 277 - 284, The docstring for the
container_name property is inconsistent with the returned value: update either
the docstring or the returned string so they match; specifically, in the
container_name property (method name container_name) ensure the docstring's
example format matches the actual return value f"srtctl_{self.job_id}" (or
change the return to f"sglang_{self.job_id}" if that's the intended name) and
also update the matching mention in docs/debugging.md (around the referenced
line) so all references consistently use the same prefix.
1 participant
Two new debugging features:
Backtrace Collection for Hangs
When enabled, py-spy and cuda-gdb backtraces will be collected to the logs/backtraces dir after waiting for a user-specified amount of time. Useful for seeing the state of execution when a hang occurs.
Nvidia-smi monitoring
Will log nvidia-smi to logs/{node}_nvidia_smi.log at the specific interval in seconds. Useful for seeing memory usage and other stats.
Container name
Srun commands to connect to a worker now use the container-name so that they join the same container as the server which allows debugging tools to attach to the worker.
Summary by CodeRabbit
Release Notes
New Features
debugandnvidia_smiconfiguration options with customizable parametersDocumentation
debugandnvidia_smisectionsBug Fixes
Tests