Repository navigation
CI: adapt DFX smokes to allocated device count - #1753
ChaoZheng109 merged 1 commit into
Conversation
|
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:
📝 WalkthroughWalkthroughAdds a shared composite action for A2A3 and A5 onboard DFX smokes. The action validates devices, schedules four feature jobs with per-device serialization, captures results, and reports failures. Workflows, CI documentation, and A5 scheduling tests are updated. ChangesAdaptive DFX smoke orchestration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Workflow
participant DFXAction as run-onboard-dfx-smokes
participant Devices
participant Pytest
Workflow->>DFXAction: invoke for a2a3 or a5
DFXAction->>Devices: validate and assign devices
DFXAction->>Pytest: run serialized smoke jobs
Pytest-->>DFXAction: return logs and exit codes
DFXAction-->>Workflow: report grouped results
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/ut/py/test_run_onboard_dfx_smokes_action.py (1)
126-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the documented round-robin distribution.
These assertions prove device coverage, but they do not prove round-robin assignment. For three devices, an assignment of
7, 8, 9, 8passes but violates the action contract.Assert the expected invocation count for each device.
Proposed test update
assert len(invocations) == 4 for device in range(7, 7 + device_count): - assert any(f"--device {device}" in invocation for invocation in invocations) + assert sum(f"--device {device}" in invocation for invocation in invocations) == ( + 4 // device_count + (device - 7 < 4 % device_count) + ) assert (state_dir / "parallel").exists() == (device_count > 1)🤖 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 `@tests/ut/py/test_run_onboard_dfx_smokes_action.py` around lines 126 - 130, Update the assertions in the onboarding smoke test around invocations to verify the documented round-robin distribution, not just device coverage: count occurrences of each device in the invocation list and assert the expected per-device counts for the configured invocation total and device_count. Preserve the existing parallel-state assertion and use the test’s existing invocation/device-count symbols.
🤖 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 `@docs/ci.md`:
- Around line 265-267: Update the DFX smoke allocation documentation to state
that the A5 schedule has four smoke groups, while A2A3 additionally schedules
the dep_gen (host_build_graph) smoke when platform is a2a3. Clarify the capacity
impact of this fifth A2A3-only smoke in the section describing
run-onboard-dfx-smokes.
---
Nitpick comments:
In `@tests/ut/py/test_run_onboard_dfx_smokes_action.py`:
- Around line 126-130: Update the assertions in the onboarding smoke test around
invocations to verify the documented round-robin distribution, not just device
coverage: count occurrences of each device in the invocation list and assert the
expected per-device counts for the configured invocation total and device_count.
Preserve the existing parallel-state assertion and use the test’s existing
invocation/device-count symbols.
🪄 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: 6cc3bd60-acc8-4eb8-a5e4-9b0a68e80f99
📒 Files selected for processing (5)
.github/actions/run-onboard-dfx-smokes/action.yml.github/workflows/_st-npu-a2a3.yml.github/workflows/_st-npu-a5.ymldocs/ci.mdtests/ut/py/test_run_onboard_dfx_smokes_action.py
e60ac84 to
86d61d0
Compare
- Share one composite action between a2a3 and a5 onboard workflows. - Serialize each device lane while independent lanes run concurrently. - Document the fifth a2a3 smoke and round-robin device reuse. - Cover one through four allocations and their distribution in a hermetic test.
|
@coderabbitai Addressed:
|
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
Summary
Testing
python -m pytest tests/ut -q(1295 passed, 19 skipped)