Repository navigation
ci(e2e): upload worker logs as artifacts on test failure - #963
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds CI-driven E2E log collection and diagnostics, a pytest hook to print worker log summaries on test failures, environment-driven log_dir handling with worker start-failure tracking, and changes to worker process log routing and vLLM launch args. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI Workflow
participant Pytest as Test Runner
participant Fixture as setup_backend fixture
participant Worker as Worker Process
participant Artifact as Artifact Storage
CI->>Pytest: set E2E_LOG_DIR and start E2E job
Pytest->>Fixture: initialize backend (reads E2E_LOG_DIR)
Fixture->>Worker: spawn worker (log_dir from env/config)
Worker-->>Fixture: start result or raise TimeoutError/RuntimeError
alt worker start failure
Fixture->>Fixture: increment _worker_start_failures
Fixture-->>Pytest: raise/fail test
end
Pytest-->Pytest: on test failure -> pytest_runtest_makereport reads E2E_LOG_DIR, prints log summary
Pytest->>CI: job fails/cancels
CI->>Artifact: upload e2e-logs/ as artifact (conditional)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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 docstrings
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces failure diagnostics for E2E tests by adding a pytest hook that lists worker logs on failure. It also modifies the worker spawning logic to ensure logs are always captured to a file, defaulting to a temporary directory if needed. Feedback focuses on improving code consistency by using pathlib in conftest.py and refactoring the log path construction in worker.py to reduce redundant checks.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9a9038d92
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 737331dc03
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f812b7be66
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e_test/conftest.py`:
- Around line 132-136: The failure-summary logic currently only checks the
E2E_LOG_DIR env var, but setup_backend can resolve the worker log directory from
either E2E_LOG_DIR or gateway(log_dir=...), so you must read the same resolved
path instead of directly reading the env var. Modify the code that computes
log_dir in setup_backend (or stash the resolved path there) so it exposes the
final worker log_dir (e.g., by returning it, storing it in a module-level
variable, or attaching it to the pytest config object), then update the summary
reader to use that resolved log_dir rather than os.environ.get("E2E_LOG_DIR") so
logs written via the marker-backed gateway path are discovered and printed.
In `@e2e_test/infra/worker.py`:
- Around line 256-257: The hard-coded, intentionally invalid vLLM flag value
"1.1" for the "--gpu-memory-utilization" argument is left in the default worker
startup and will break vLLM backends; remove this test scaffolding or gate it
behind a test-only switch. Locate the argument list that contains the
"--gpu-memory-utilization" entry and either delete the "1.1" element (and the
TEMP comment) or wrap that element in a conditional (e.g., controlled by a
TEST_MODE or env var) so production runs never pass an out-of-range value;
alternatively replace "1.1" with a valid default in-range value (<= "1.0") if
that matches intended behavior. Ensure the change targets the code building the
vLLM CLI args where "--gpu-memory-utilization" is added.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: ff1d6fcb-76e1-4f2f-ad4a-05d89e9e36a1
📒 Files selected for processing (2)
e2e_test/conftest.pye2e_test/infra/worker.py
f812b7b to
498af02
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 498af029e7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
♻️ Duplicate comments (1)
e2e_test/infra/worker.py (1)
256-257:⚠️ Potential issue | 🔴 CriticalRemove the temporary invalid vLLM flag before merge.
The
--gpu-memory-utilization 1.1value exceeds vLLM's accepted range of (0, 1] and will cause all vLLM-backed worker startup to fail. The comment indicates this is intentional test scaffolding for verifying log artifact upload, but it should not be merged into main.Proposed fix
"--gpu-memory-utilization", - "1.1", # TEMP: intentionally invalid to test log artifact upload + "0.9",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/infra/worker.py` around lines 256 - 257, Remove the temporary invalid vLLM flag value that sets "--gpu-memory-utilization" to "1.1" in e2e_test/infra/worker.py; locate the CLI args list where "--gpu-memory-utilization" is paired with the string "1.1" and either delete that flag entry or replace the value with a valid number in (0,1] such as "1.0" (or remove the flag entirely) so vLLM-backed worker startup no longer fails; ensure any test-only scaffolding comment is also removed or guarded behind a test-only conditional.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@e2e_test/infra/worker.py`:
- Around line 256-257: Remove the temporary invalid vLLM flag value that sets
"--gpu-memory-utilization" to "1.1" in e2e_test/infra/worker.py; locate the CLI
args list where "--gpu-memory-utilization" is paired with the string "1.1" and
either delete that flag entry or replace the value with a valid number in (0,1]
such as "1.0" (or remove the flag entirely) so vLLM-backed worker startup no
longer fails; ensure any test-only scaffolding comment is also removed or
guarded behind a test-only conditional.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a4b83d67-57fb-45f0-8304-7f65287ac311
📒 Files selected for processing (4)
.github/workflows/e2e-gpu-job.ymle2e_test/conftest.pye2e_test/fixtures/setup_backend.pye2e_test/infra/worker.py
github-actions
Bot
added
documentation
Signed-off-by: key4ng <rukeyang@gmail.com>
0d7ee21 to
50eb577
Compare
key4ng
removed
documentation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50eb577c51
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| worker_key = (model_id, engine) | ||
| if worker_key in _failed_worker_starts: | ||
| pytest.skip(f"Worker {model_id}/{engine} already failed to start — skipping") |
There was a problem hiding this comment.
setup_backend now skips all future startups after any (model_id, engine) failure, but worker startup paths differ by backend mode/topology (for example, HTTP vs gRPC use different launch entrypoints and PD mode uses different worker roles). A failure in one mode therefore causes unrelated configurations for the same model/engine to be skipped without even attempting startup, which can mask real regressions and silently reduce E2E coverage. Include at least connection_mode (and PD/non-PD shape) in the cache key, or scope this skip to identical worker configs only.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
e2e_test/conftest.py (1)
132-134: 🧹 Nitpick | 🔵 TrivialLog discovery only checks
E2E_LOG_DIR, but workers may write to tempfile fallback.When
E2E_LOG_DIRis unset (local development),worker.pywrites logs totempfile.gettempdir(). These logs won't be discovered by this hook since it only checks the env var path. Consider also checking the tempfile location whenE2E_LOG_DIRis not set, or document that this diagnostic is CI-only.♻️ Proposed fix to support both paths
log_dir = os.environ.get("E2E_LOG_DIR") - if not log_dir or not Path(log_dir).is_dir(): - return + if not log_dir: + # Fallback to tempfile location used by worker.py when E2E_LOG_DIR is unset + import tempfile + log_dir = tempfile.gettempdir() + if not Path(log_dir).is_dir(): + return - logs = sorted(p.name for p in Path(log_dir).glob("*.log") if p.is_file()) + # Match the naming pattern from worker.py + pattern = "smg-worker-*.log" if "E2E_LOG_DIR" not in os.environ else "*.log" + logs = sorted(p.name for p in Path(log_dir).glob(pattern) if p.is_file())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/conftest.py` around lines 132 - 134, The hook currently exits if E2E_LOG_DIR is unset (log_dir) and so misses logs written to the tempfile fallback used by worker.py; update the discovery to also check tempfile.gettempdir() (or document CI-only behavior). Modify the logic around log_dir/Path(...) to: if E2E_LOG_DIR is set use that path, otherwise probe tempfile.gettempdir() for an existing log directory (and fall back to returning only if neither exists), referencing the log_dir variable and tempfile.gettempdir() so both possible locations are scanned.e2e_test/infra/worker.py (1)
256-257:⚠️ Potential issue | 🔴 CriticalRemove the intentionally invalid vLLM flag before merge.
--gpu-memory-utilization 1.1exceeds vLLM's valid range of (0, 1] and will cause all vLLM-backed E2E tests to fail at worker startup. This test scaffolding should not be in the merge target.🐛 Proposed fix
"--gpu-memory-utilization", - "1.1", # TEMP: intentionally invalid to test log artifact upload + "0.9",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/infra/worker.py` around lines 256 - 257, Remove the temporary invalid vLLM flag "--gpu-memory-utilization" set to "1.1" (or change it to a valid value ≤ 1.0) in the worker startup arguments so vLLM-backed E2E tests do not fail; locate the argument list where "--gpu-memory-utilization", "1.1" is added (search for that exact string or the flags list in e2e_test/infra/worker.py) and either delete those two entries or replace "1.1" with a valid value like "1.0".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/e2e-gpu-job.yml:
- Around line 130-137: The artifact name in the "Upload worker logs" step uses
inputs.test_dirs which can contain '/' and will fail validation; fix by adding a
preceding step (e.g., id: sanitize) that takes ${{ inputs.test_dirs }}, replaces
'/' with '-' (or otherwise sanitizes unsafe characters) and writes the result to
GITHUB_OUTPUT (e.g., artifact_suffix), then change the "Upload worker logs" step
to use that sanitized output (steps.sanitize.outputs.artifact_suffix) in the
name instead of raw inputs.test_dirs so the artifact name contains no path
separators.
---
Duplicate comments:
In `@e2e_test/conftest.py`:
- Around line 132-134: The hook currently exits if E2E_LOG_DIR is unset
(log_dir) and so misses logs written to the tempfile fallback used by worker.py;
update the discovery to also check tempfile.gettempdir() (or document CI-only
behavior). Modify the logic around log_dir/Path(...) to: if E2E_LOG_DIR is set
use that path, otherwise probe tempfile.gettempdir() for an existing log
directory (and fall back to returning only if neither exists), referencing the
log_dir variable and tempfile.gettempdir() so both possible locations are
scanned.
In `@e2e_test/infra/worker.py`:
- Around line 256-257: Remove the temporary invalid vLLM flag
"--gpu-memory-utilization" set to "1.1" (or change it to a valid value ≤ 1.0) in
the worker startup arguments so vLLM-backed E2E tests do not fail; locate the
argument list where "--gpu-memory-utilization", "1.1" is added (search for that
exact string or the flags list in e2e_test/infra/worker.py) and either delete
those two entries or replace "1.1" with a valid value like "1.0".
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 17c2b9ec-c9be-4a17-9020-70361bf07600
📒 Files selected for processing (4)
.github/workflows/e2e-gpu-job.ymle2e_test/conftest.pye2e_test/fixtures/setup_backend.pye2e_test/infra/worker.py
key4ng
changed the title
… skipping Use pytest.fail() instead of pytest.skip() so CI still reports failure. Allow reruns to retry before giving up (threshold: 3 failures). Signed-off-by: key4ng <rukeyang@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0dc1ca264f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if fail_count >= _MAX_WORKER_START_FAILURES: | ||
| pytest.fail(f"Worker {model_id}/{engine} failed to start {fail_count} times — giving up") |
There was a problem hiding this comment.
setup_backend now hard-fails once _worker_start_failures[(model_id, engine)] >= 3, but this counter is only incremented in exception paths and never reset after a successful startup. That makes the budget cumulative for the whole test session, so three intermittent startup flakes (even if separated by many successful classes) permanently force later classes for the same model/engine to fail immediately, which can mask real test outcomes and materially reduce E2E coverage.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e_test/fixtures/setup_backend.py`:
- Around line 170-182: Extract the duplicate failure-tracking logic into a small
helper (e.g. _track_worker_failure) that increments
_worker_start_failures[(model_id, engine)] and return None; then replace the
repeated except (TimeoutError, RuntimeError) blocks around start_workers calls
with a call to that helper and re-raise the exception (keep the try/except
structure but delegate the counter increment to _track_worker_failure). Update
all three locations that currently catch (TimeoutError, RuntimeError) (the
blocks surrounding start_workers) to call the new helper so the increment logic
is centralized and duplicated code removed.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 2139dd6e-2d28-45af-a1b4-09f30bbe66be
📒 Files selected for processing (1)
e2e_test/fixtures/setup_backend.py
Signed-off-by: key4ng <rukeyang@gmail.com>
When the engine itself is broken (e.g., bad gpu-memory-utilization), every model fails the same way. Track by engine so all models fail fast after the first 3 failures, instead of each model burning 3 attempts. Signed-off-by: key4ng <rukeyang@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c23151e36e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _WORKER_DEFAULTS = {"count": 1, "prefill": None, "decode": None} | ||
|
|
||
| # Track worker startup failures — fail fast after repeated failures | ||
| _worker_start_failures: dict[str, int] = {} # engine -> count |
There was a problem hiding this comment.
The new fail-fast cache is global per engine and only increases, then setup_backend aborts once it reaches 3; this means a few transient startup failures in one class can cause all later classes on that engine (including different models/modes/topologies) to fail without even attempting startup. This can silently drop large portions of E2E coverage and mask unrelated regressions.
Useful? React with 👍 / 👎.
pytest.fail() gets retried by --reruns, causing hundreds of ERROR/RERUN cycles with 5s delays each. pytest.exit() stops the entire session immediately with returncode=1. Signed-off-by: key4ng <rukeyang@gmail.com>
|
Test did:
|
…ation Signed-off-by: key4ng <rukeyang@gmail.com>
Signed-off-by: key4ng <rukeyang@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5d54f92dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not log_dir or not Path(log_dir).is_dir(): | ||
| return | ||
|
|
||
| logs = sorted(p.name for p in Path(log_dir).glob("*.log") if p.is_file()) |
There was a problem hiding this comment.
The new pytest_runtest_makereport hook claims to show the “most recent” worker logs, but it sorts by filename (sorted(p.name ...)) and then takes logs[-5:]. Filename order is not chronological (it is dominated by model/engine/port), so when multiple log files exist this summary can hide the logs from the actual failing run and surface stale ones instead, weakening the diagnostics this change is meant to provide.
Useful? React with 👍 / 👎.
Signed-off-by: key4ng <rukeyang@gmail.com>
key4ng
changed the title
…#963) Signed-off-by: key4ng <rukeyang@gmail.com>
1 participant
Description
Problem
E2E CI tests set
SHOW_WORKER_LOGS=0with nolog_dir, so worker logs (sglang/vllm/trtllm) are sent to/dev/null. When a test fails, there is no way to see what happened on the worker side. Developers must re-run the entire CI job withSHOW_WORKER_LOGS=1which floods the console with noise for all tests, not just the failing one.Solution
Always capture worker logs to files. Upload them as GitHub Actions artifacts on failure (7-day retention). Print a short console summary listing available log files when a test fails.
Changes
e2e_test/infra/worker.py— Write to tempfile instead of/dev/nullwhen logs are suppressed and nolog_diris sete2e_test/fixtures/setup_backend.py— ReadE2E_LOG_DIRenv var to set worker log directorye2e_test/conftest.py— Addpytest_runtest_makereporthook that lists log files on test failure.github/workflows/e2e-gpu-job.yml— SetE2E_LOG_DIR: e2e-logsand addupload-artifactstep withif: failure()Test Plan
e2e-logs/directory during CIChecklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit
Tests
Bug Fixes