Skip to content

test: capture and surface tritonserver output on server-startup failure - #126

Merged
Vinya567 merged 3 commits into
mainfrom
vinyak/tri-1352-fix-ci-test-l0_triton_cli_test_trtllm-base-gpt2
Jun 10, 2026
Merged

test: capture and surface tritonserver output on server-startup failure#126
Vinya567 merged 3 commits into
mainfrom
vinyak/tri-1352-fix-ci-test-l0_triton_cli_test_trtllm-base-gpt2

Conversation

@Vinya567

@Vinya567 Vinya567 commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

What does the PR do?

ScopedTritonServer.wait_for_server_ready() previously raised a generic
"Server failed to start in time." which discarded the tritonserver
subprocess output entirely. That made CI failures effectively impossible
to triage without a local repro.

This change captures the subprocess's stdout/stderr to a temporary file
and includes the tail (with pid, exit code, elapsed time, and timeout)
in the RuntimeError raised on startup failure. The temp file is
unlinked in kill_server().

The two failure modes are now distinguished in the error message:

  • subprocess exited before becoming ready → exit code + log tail
  • subprocess still running but ready-check timed out → elapsed /
    timeout + log tail

This is a pure test-harness diagnostics change. No production code path
is affected; no public API change; happy-path test runs behave
identically.

Checklist

Checklist

  • PR title reflects the change and is of format <commit_type>: <Title>
  • Changes are described in the pull request.
  • Related issues are referenced.
  • Populated github labels field
  • Added test plan and verified test passes.
  • Verified that the PR passes existing CI.
  • Verified copyright is correct on all changed files.
  • Added succinct git squash message before merging ref.
  • All template sections are filled out.
  • Optional: Additional screenshots for behavior/output changes with before/after.

Commit Type:

Check the conventional commit type
box here and add the label to the github PR.

  • build
  • ci
  • docs
  • feat
  • fix
  • perf
  • refactor
  • revert
  • style
  • test

Related PRs:

  • ci: Fix L0_triton_cli_test_vllm--base #122 (ci: Fix L0_triton_cli_test_vllm--base) — same family of CI L0
    test fixes for this repo. Useful reviewer context on the failure
    pattern this diagnostic surfaces.
  • A companion CI-image fix is being shipped in our internal CI repo as
    a separate MR (tracked internally). This PR stands on its own — pure
    test-harness change — and can land independently.

Where should the reviewer start?

  • tests/utils.py — the only file changed.
    • ScopedTritonServer.run_server() — new temp-file redirect of the
      subprocess stdout/stderr via tempfile.NamedTemporaryFile and
      Popen(..., stdout=log_handle, stderr=STDOUT).
    • ScopedTritonServer._format_server_output() — new helper that
      returns the tail (default last 50 lines) of the captured log.
    • ScopedTritonServer.wait_for_server_ready() — error messages now
      include pid / exit code / elapsed / timeout / log tail.
    • ScopedTritonServer.kill_server() — restructured early-return
      into an if/else and unconditionally unlinks the temp log at the
      end, so nothing leaks on disk.

Test plan:

  • Re-ran the previously-failing L0_triton_cli_test_trtllm--base
    matrix entries with this diagnostics change in place. The new error
    output identified the actual missing binary in the test image
    immediately — confirming the diagnostic surface works end-to-end on
    a real failure.

  • Happy-path runs (server starts within timeout) behave identically;
    the temp log file is unlinked by kill_server() so nothing is left
    on disk.

  • No new tests added — this PR changes only an existing test-harness
    error path. Coverage of the harness itself is exercised every time
    an L0 test runs.

  • CI Pipeline ID: 54134005

Caveats:

  • The captured log lives in a process-scoped tempfile.NamedTemporaryFile
    under the system temp dir; kill_server() removes it. If a test
    process is SIGKILL'd before kill_server() runs, the temp file is
    left for the OS to reap (standard behavior, same as any Python
    tempfile).
  • Default tail size is 50 lines; sufficient for the common "server
    exited with traceback" case and bounded so the RuntimeError stays
    readable in CI log viewers.

Background

The previous error string discarded the actual tritonserver stderr,
so a missing binary, a missing Python module, a failed model load, and
a slow startup all produced the same useless one-liner in CI. Intent
of this PR is exactly what was asked for in review: surface the real
failure cause directly in the CI job log so future triage doesn't
require a local repro.

Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)

  • N/A (internally tracked; no public GitHub issue).

ScopedTritonServer previously raised a generic "Server failed to start in
time." which hid the actual cause. Redirect the triton subprocess's
stdout/stderr to a temp file and include the tail (plus pid, exit code,
elapsed time, and timeout) in the RuntimeError raised by
wait_for_server_ready. The log file is unlinked in kill_server.

Pure test-harness diagnostics change; no production code path affected.
Comment thread tests/utils.py Outdated
Co-authored-by: Yingge He <157551214+yinggeh@users.noreply.github.com>
Removed unused import statement for 'os'.

@whoisj whoisj left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Vinya567
Vinya567 merged commit 7f448b2 into main Jun 10, 2026
2 of 3 checks passed
@Vinya567
Vinya567 deleted the vinyak/tri-1352-fix-ci-test-l0_triton_cli_test_trtllm-base-gpt2 branch June 10, 2026 02:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants