Skip to content

[CI] fail the benchmark step when a bench script crashes, don't wait … - #443

Open
mingfeima wants to merge 3 commits into
sgl-project:mainfrom
mingfeima:ci/honest-benchmark-step-failure
Open

mingfeima wants to merge 3 commits into
sgl-project:mainfrom
mingfeima:ci/honest-benchmark-step-failure

Conversation

@mingfeima

Copy link
Copy Markdown
Collaborator

Summary

  • Run Sglang Kernel Benchmarks used a long cmd | tee log && ... chain. Two missing && turned later benches into a new bash statement, so a crashing bench (e.g. bench_flash_attn.py in [Attention] support softmax lse #330) did not fail that step.
  • The job stayed green until Copy logs from container (docker cp fused_moe.log), which looks like infra rather than a kernel regression.
  • Run benches sequentially under set -eo pipefail so a non-zero python3 bench_*.py | tee fails this step. Adding a new bench without && cannot hide a failure again.

Copilot AI lite review requested due to automatic review settings August 26, 2026 02:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the XPU PR CI workflow to ensure the benchmark step fails immediately when any benchmark script exits non-zero (including crashes), rather than allowing later commands to mask failures and only erroring at a subsequent docker cp step.

Changes:

  • Replaces the long cmd && cmd && ... chain with sequential commands executed under set -eo pipefail.
  • Ensures python3 bench_*.py 2>&1 | tee ... propagates failures correctly within the benchmark step.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

…util copy logs

A missing && in the long pipefail chain let later benches still run,
so the step stayed green and the job only failed on docker cp of a
log that was never written.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants