CI: Collect L4 Pod Tests through pytest - #1758
Conversation
- Add pod pytest markers, fixtures, and L4 wrappers so pod CI can collect examples and tests/st with one marker expression - Replace the hand-enumerated pod example action with a pytest action while keeping manual run_parent.sh entry points - Exclude pod tests from ordinary ST lanes and update docs to match the new CI shape
|
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:
📝 WalkthroughWalkthroughThe pod CI path now uses one pytest action for marked L4 tests. It adds peer and remote-device fixtures, refactors examples for direct invocation, updates test selection, and replaces per-example workflow execution and documentation. ChangesPod pytest execution
Estimated code review effort: 4 (Complex) | ~60 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
examples/workers/README.md (2)
63-72: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe "What a new L4 example needs" checklist omits the
test_*.pywrapper.Lines 152-155 now state that each L4 example ships a
test_*.pywrapper and that CI uses it. The file tree at lines 66-72 still lists onlymain.pyandrun_parent.sh. A contributor who copies that tree produces an example the pod job never collects.📝 Proposed fix
l4/<your_example>/ README.md kernels/aiv/*.cpp kernels/orchestration/*.cpp main.py # entry point: argparse + main() delegating to run() + test_<your_example>.py # `@pytest.mark.pod` wrapper; this is what the pod job collects run_parent.sh # maps environment variables onto main.py's flags🤖 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 `@examples/workers/README.md` around lines 63 - 72, Update the “What a new L4 example needs” file tree to include the required test_*.py wrapper alongside main.py and run_parent.sh, matching the requirement documented later in the README.
105-112: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe "Running it in CI" section still tells contributors to edit the pod workflow.
The text says adding an example to
st-pod-onboard-a2a3is "one block plus one line". This PR removes that path. The updateddocs/ci.mdstates that adding an L4 pod example means adding atest_*.pywrapper with@pytest.mark.pod, not editing_st-pod.yml. The warning about "why a step that reports green there may still have failed" also no longer applies, because the per-examplecontinue-on-errorsteps are gone.Rewrite this section to describe the marker-based path.
📝 Proposed rewrite
### Running it in CI The `st-pod-onboard-a2a3` job runs L4 examples across a pair of a2a3 machines. -Adding yours to it is one block plus one line, and the wiring, the log -artifact, and the failure semantics are described in -[`docs/ci.md`](../../docs/ci.md#multi-machine-pod-jobs). Read that before adding the -block — in particular why a step that reports green there may still have -failed. +The job runs one `pytest examples tests/st -m pod` sweep, so adding yours means +adding a `test_*.py` wrapper carrying `@pytest.mark.pod`. Do not edit +`_st-pod.yml`. The wiring and the log artifact are described in +[`docs/ci.md`](../../docs/ci.md#multi-machine-pod-jobs).🤖 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 `@examples/workers/README.md` around lines 105 - 112, Rewrite the “Running it in CI” section in examples/workers/README.md to describe adding a test_*.py wrapper decorated with `@pytest.mark.pod`, referring contributors to the updated multi-machine pod-job guidance in docs/ci.md. Remove the obsolete instructions about editing st-pod-onboard-a2a3, adding a workflow block and line, and the warning about green steps masking failures.
🧹 Nitpick comments (2)
conftest.py (1)
1281-1290: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRemote device allocation always returns the same prefix, so it depends on
--max-parallel 1.
st_pod_remote_device_idsreturnsremote_device_ids[:n]for every test. Local ids come from the framework allocator, but remote ids do not._st-pod.ymlpasses--max-parallel 1, so no two pod tests run at once today and no collision occurs. If that value is ever raised, two concurrent pod tests claim the same peer device.Record the dependency in the fixture docstring, or derive the slice from the allocated local worker index.
♻️ Proposed docstring change
def st_pod_remote_device_ids(request, st_pod_peer): - """Allocate remote device IDs from the pod peer's default device pool.""" + """Allocate remote device IDs from the pod peer's default device pool. + + Every test receives the same leading slice, so this is only collision-free + while the pod job serializes with ``--max-parallel 1``. Raising that value + requires deriving the slice from the allocated local worker index. + """🤖 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 `@conftest.py` around lines 1281 - 1290, Update the st_pod_remote_device_ids fixture docstring to explicitly document that it always allocates the prefix of the peer’s remote_device_ids and therefore requires --max-parallel 1 to avoid concurrent test collisions. Do not change allocation behavior unless implementing it via the existing local worker index..github/actions/pod-run-pytest/action.yml (1)
97-110: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winOne
POD_SMOKE_TIMEOUT_Snow covers the whole sweep instead of one example.Before this change the same budget applied per pod example. Now it bounds the full
-m podcollection, which currently holds four tests plus per-test kernel compilation. With the default of 1800 s the last test can be killed bytimeouteven when every test is healthy.Consider scaling the value in
_st-pod.yml, or replacing the outertimeoutwith the existing--pto-session-timeoutbudget so the failure names the test that hung.🤖 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 @.github/actions/pod-run-pytest/action.yml around lines 97 - 110, The Run pod pytest step currently applies POD_SMOKE_TIMEOUT_S to the entire pytest sweep; update the timeout handling around “Run pod pytest” to preserve a per-test budget or use the existing --pto-session-timeout mechanism, and adjust the _st-pod.yml configuration if scaling the shared value is the chosen approach. Ensure healthy multi-test pod runs are not terminated solely because the aggregate duration exceeds the single-test budget, while hung tests still report which test timed out.
🤖 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 `@examples/README.md`:
- Line 20: Update the trailing comments for the hardware pytest commands in
examples/README.md lines 20-20 and docs/user/reference/cli.md lines 9-9 to name
both SDMA and pod as quarantined markers and state that pod runs in the
two-machine pod job; keep the command text and comments identical at both sites.
In `@examples/workers/l4/compute_then_tload_mixed_l3/main.py`:
- Around line 248-258: Suppress Ruff S104 for the intentional 0.0.0.0 defaults
and document the peer-callback rationale at both affected sites: update the
session_listen_host parameter in
examples/workers/l4/compute_then_tload_mixed_l3/main.py (lines 248-258) and the
os.environ.get default in conftest.py (lines 1276-1278) with the same noqa
annotation and one-line explanation; both sites require direct changes.
- Around line 267-284: Widen the try/finally cleanup in the run flow to include
local_l3 creation, registration, Worker construction, and add_worker. Ensure the
finally block closes local_l3 when it was created but never successfully
attached to worker, while preserving existing cleanup for attached workers.
In `@examples/workers/l4/compute_then_tload_mixed_l3/README.md`:
- Around line 86-88: Update the CI/manual-entry documentation in
examples/workers/l4/compute_then_tload_mixed_l3/README.md:86-88 and
examples/workers/l4/global_tload_mixed_l3/README.md:84-85 so it no longer claims
pytest uses the same variables as run_parent.sh; name each respective test
wrapper, state that CI supplies POD_REMOTE_ENDPOINT, POD_REMOTE_DEVICES,
POD_L3_SESSION_TIMEOUT_S, POD_L3_SESSION_LISTEN_HOST, and the
--platform/--device options instead of the corresponding SIMPLER_* variables,
and retain run_parent.sh with the existing table as the manual entry point.
In `@examples/workers/l4/global_tload_mixed_l3/main.py`:
- Line 163: Resolve Ruff S104 for session_listen_host in
examples/workers/l4/global_tload_mixed_l3/main.py:163-163 and
examples/workers/l4/vector_add_mixed_l3/main.py:254-254 by using a configured
peer-reachable interface, or, if wildcard binding is required for pod peer
sessions, add a scoped # noqa: S104 with a brief reason at both sites.
---
Outside diff comments:
In `@examples/workers/README.md`:
- Around line 63-72: Update the “What a new L4 example needs” file tree to
include the required test_*.py wrapper alongside main.py and run_parent.sh,
matching the requirement documented later in the README.
- Around line 105-112: Rewrite the “Running it in CI” section in
examples/workers/README.md to describe adding a test_*.py wrapper decorated with
`@pytest.mark.pod`, referring contributors to the updated multi-machine pod-job
guidance in docs/ci.md. Remove the obsolete instructions about editing
st-pod-onboard-a2a3, adding a workflow block and line, and the warning about
green steps masking failures.
---
Nitpick comments:
In @.github/actions/pod-run-pytest/action.yml:
- Around line 97-110: The Run pod pytest step currently applies
POD_SMOKE_TIMEOUT_S to the entire pytest sweep; update the timeout handling
around “Run pod pytest” to preserve a per-test budget or use the existing
--pto-session-timeout mechanism, and adjust the _st-pod.yml configuration if
scaling the shared value is the chosen approach. Ensure healthy multi-test pod
runs are not terminated solely because the aggregate duration exceeds the
single-test budget, while hung tests still report which test timed out.
In `@conftest.py`:
- Around line 1281-1290: Update the st_pod_remote_device_ids fixture docstring
to explicitly document that it always allocates the prefix of the peer’s
remote_device_ids and therefore requires --max-parallel 1 to avoid concurrent
test collisions. Do not change allocation behavior unless implementing it via
the existing local worker index.
🪄 Autofix
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: f5488a2b-01b7-457e-ad76-48c8050b3b40
📒 Files selected for processing (23)
.github/actions/pod-run-example/action.yml.github/actions/pod-run-pytest/action.yml.github/actions/pod-teardown/action.yml.github/workflows/_st-npu-a2a3.yml.github/workflows/_st-npu-a5.yml.github/workflows/_st-pod.ymlconftest.pydocs/ci.mddocs/comm-domain.mddocs/testing.mddocs/troubleshooting/a2a3-507899-aicpu-shared-so-fault.mddocs/user/reference/cli.mdexamples/README.mdexamples/workers/README.mdexamples/workers/l4/compute_then_tload_mixed_l3/README.mdexamples/workers/l4/compute_then_tload_mixed_l3/main.pyexamples/workers/l4/compute_then_tload_mixed_l3/test_compute_then_tload_mixed_l3.pyexamples/workers/l4/global_tload_mixed_l3/README.mdexamples/workers/l4/global_tload_mixed_l3/main.pyexamples/workers/l4/global_tload_mixed_l3/test_global_tload_mixed_l3.pyexamples/workers/l4/vector_add_mixed_l3/main.pyexamples/workers/l4/vector_add_mixed_l3/test_vector_add_mixed_l3.pytests/st/a2a3/tensormap_and_ringbuffer/l4_pod/test_global_tload_mixed_l3_pod.py
💤 Files with no reviewable changes (1)
- .github/actions/pod-run-example/action.yml
- Skip pod tests when the pod runner environment is absent - Document serialized remote-device allocation and marker-based L4 CI - Close partially-created L4 local workers on setup failures - Increase the pod pytest sweep timeout budget
2 participants
Summary
-m podinstead of hand-enumerating L4 examples in_st-pod.ymltests/stpod wrapper to prove pod tests undertests/stare collected by the pod lanepod-run-examplewithpod-run-pytestand keeprun_parent.shas a manual entry pointFixes #1740