Automate Puzzletron GPU lifecycle tests - #2167
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (3)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThe PR adds a pinned CUDA-based Puzzletron CI image, validates its image and environment contracts, updates Nox GPU verification, and adds a gated GitHub Actions workflow for GPU lifecycle tests. ChangesPuzzletron GPU CI
Estimated code review effort: 3 (Moderate) | ~25 minutes Mergeability Score: 🔵 Low · up to The new GPU test image currently runs its test process as root, leaving a bounded container-isolation risk that should remain explicitly owned and followed up; this is mergeable without blocking the PR. Sequence Diagram(s)sequenceDiagram
participant PullRequest
participant PRGate
participant ImageResolver
participant GPUContainer
participant RequiredCheck
PullRequest->>PRGate: evaluate changed-file scope
PRGate->>ImageResolver: resolve pinned CI image
ImageResolver->>GPUContainer: provide image and digest cache key
GPUContainer->>RequiredCheck: report GPU test result
PRGate->>RequiredCheck: report gate result
ImageResolver->>RequiredCheck: report image result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 4
🤖 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 @.github/workflows/puzzletron_gpu_tests.yml:
- Around line 19-23: Remove secrets: inherit from the pr-gate reusable workflow
invocation in puzzletron_gpu_tests.yml. Preserve the existing permissions and
uses configuration; if pr-gate requires a secret, pass only the specific named
secret through an explicit secrets mapping.
In `@examples/puzzletron/ci/Dockerfile`:
- Around line 20-31: Create a non-root runtime user in the Dockerfile, assign
ownership of /venv and /workspace/modelopt to that user, and add the
corresponding USER instruction before WORKDIR so GPU jobs execute checked-out
code without root privileges.
In `@examples/puzzletron/ci/resolve_ci_image.py`:
- Around line 18-22: Add the module-level __all__ declaration immediately after
the imports in resolve_ci_image.py, listing resolve_image_reference and
validate_repository_contract so both functions are explicitly exposed as the
resolver module’s public API.
In `@tests/unit/torch/puzzletron/test_ci_image_contract.py`:
- Line 87: Replace yaml.load with yaml.safe_load in the workflow parsing setup,
then update the related scalar assertions to expect PyYAML’s native values,
including True for “on” and integer 50 for “timeout-minutes.”
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ab0acb2b-83ac-4994-9a09-dc95f1ef40f2
📒 Files selected for processing (8)
.github/workflows/puzzletron_gpu_tests.ymlexamples/puzzletron/ci/Dockerfileexamples/puzzletron/ci/README.mdexamples/puzzletron/ci/resolve_ci_image.pyexamples/puzzletron/ci/setup_env.shexamples/puzzletron/ci_environment.jsonnoxfile.pytests/unit/torch/puzzletron/test_ci_image_contract.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## jrausch/puzzletron-gpu-quality-baseline-v4 #2167 +/- ##
===========================================================================
Coverage 63.03% 63.04%
===========================================================================
Files 709 709
Lines 91929 91929
===========================================================================
+ Hits 57948 57954 +6
+ Misses 33981 33975 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
55478b8 to
de15733
Compare
c2b81f3 to
1b5940f
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 1
🤖 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 @.github/workflows/puzzletron_gpu_tests.yml:
- Around line 69-71: Remove the privileged NGC_API_KEY use from the
push-triggered GPU workflow around the credentials block. Move the GPU job and
secret access into a maintainer-controlled workflow revision, and require an
explicit trusted workflow_dispatch or maintainer approval before executing
copied PR content.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 402d3d89-0192-4214-a74a-3b6b28b3c9a9
📒 Files selected for processing (8)
.github/workflows/puzzletron_gpu_tests.ymlexamples/puzzletron/ci/Dockerfileexamples/puzzletron/ci/README.mdexamples/puzzletron/ci/resolve_ci_image.pyexamples/puzzletron/ci/setup_env.shexamples/puzzletron/ci_environment.jsonnoxfile.pytests/unit/torch/puzzletron/test_ci_image_contract.py
e013fe8 to
e3f78c2
Compare
1b5940f to
3274cb6
Compare
6a6b3d0 to
79eb6cd
Compare
3274cb6 to
278e060
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
79eb6cd to
a34b939
Compare
278e060 to
20988e6
Compare
a34b939 to
57c87e1
Compare
20988e6 to
5fdeeb2
Compare
5fdeeb2 to
f022768
Compare
|
/claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude review — Puzzletron GPU CI image + workflow
Findings: CRITICAL: 0 · IMPORTANT: 2 · SUGGESTION: 2
Scope: all 9 changed files reviewed (547 additions). This PR touches no modelopt/ runtime code — it is CI infrastructure (workflow, Dockerfile, provisioning script, resolver, noxfile session, contract test), so the review focused on CI correctness, gate coverage, and the pinning contract rather than algorithm/mode/export concerns. I traced setup_env.sh end-to-end against noxfile.py::gpu_puzzletron, _pr_gate.yml's run_tests output, _wait_for_checks.yml's secret usage, pyproject.toml's setuptools_scm config, and the targeted GPU test's fixture (build_tiny_qwen_campaign).
Most impactful findings
1. [IMPORTANT] Unconfigured PUZZLETRON_GPU_CI_IMAGE becomes a red required check on many PRs (puzzletron_gpu_tests.yml:57-61). With the variable unset — the state this PR merges in, per its own description — resolve-image fails, gpu-puzzletron is skipped, and gpu-puzzletron-required-check exits 1. The gate list includes pyproject.toml, noxfile.py, and tests/conftest.py, so this lands on a large share of PRs plus every nightly, and the error text (must be an immutable nvcr.io digest) reads as malformed value rather than variable never set. Suggested: distinguish unset from invalid in resolve_ci_image.py::main, and either land after publishing or make the unset case an explained skip while keeping invalid values a hard failure.
2. [IMPORTANT] The new CPU contract test can't be triggered by the file it guards (unit_tests.yml:87). The puzzletron_changed gate matches Puzzletron paths by extension only (**/*.py, **/*.sh, **/*.json, …), and examples/puzzletron/ci/Dockerfile has none — so a Dockerfile-only change (bumped FROM digest, dropped --verify hook, changed extras) skips the puzzletron_v2 job and test_gpu_image_uses_the_recorded_immutable_base never runs. The workflow-level paths: filter does include examples/puzzletron/**, which makes the inner skip easy to miss. Adding examples/puzzletron/ci/** (as puzzletron_gpu_tests.yml:31 already does) closes it. Note the compensating validate_repository_contract check only runs once the image variable is configured, so right now nothing catches this drift.
Suggestions
puzzletron_gpu_tests.yml:34— gate usesmodelopt/torch/puzzletron/**, narrower than the CPU Puzzletron gate (modelopt/torch/**) andgpu_tests.yml(modelopt/**), while the lifecycle test exercisesnas/prune/opt. Defensible as a runner-cost tradeoff (nightly covers it), but worth widening or annotating.setup_env.sh:23— the Dockerfile'sENV PUZZLETRON_CI_ENVIRONMENT=/opt/puzzletron/...means the runtime--verifycompares the image against its own baked pins, not the checked-out ones the README describes. Drift is still caught, but by_verify_puzzletron_v2_environmentone step later, as a raw version-mismatch dict rather than "rebuild the image."
Things I checked that are correct
pr-gatewithoutsecrets: inheritis safe:_pr_gate.ymlreferences no secrets, and the nested_wait_for_checks.ymluses only the automaticsecrets.GITHUB_TOKEN.permissions: {checks: read}matchesgpu_tests.yml/example_tests.yml.run_testsis populated for PR pushes even withoutskip_puzzletron_only—scope-prruns unconditionally onpull-request/*and defaultsrun_tests="$ANY_CHANGED", so the gate is not accidentally always-false.--no-depsat runtime does not silently hide new base dependencies: thepip checkimmediately after the editable install reports them (extras-only additions still escape).- The Dockerfile places
setup_env.sh --verifyin the sameRUNas the dependency install, so a[hf]-extra resolution that movestransformersoff5.8.1fails the build rather than shipping a mispinned image. venv_backend="none"keepsVIRTUAL_ENV=/venvintact, soinstall_modelopt'ssys.prefixguard passes in-image and fails loudly outside it; the missing-.git/shallow-checkout case is covered byfallback_version = "0.0.0"."on":is quoted, soyaml.safe_loadyields the string key the test indexes; the targeted GPU test builds its model and dataset locally, so the deliberate absence ofHF_TOKENis fine.
Risk assessment
Low-to-moderate, and confined to CI. No runtime, mode-registration, state, or export surface is touched, and the pinning design (digest-pinned base, recorded contract, build-time and pre-flight verification, no registry credentials on copied-PR branches) is sound. Both IMPORTANT findings are about when checks run rather than what they assert: one makes a new check fail broadly before the image exists, the other leaves the guarded Dockerfile ungated. Both are small, localized fixes.
Pin the execution image and dependency contract so the focused lifecycle workflow runs against a reproducible one-GPU environment. Signed-off-by: Johannes Rausch <jrausch@nvidia.com>
Signed-off-by: Johannes Rausch <jrausch@nvidia.com>
Require the immutable execution image to support anonymous pulls so copied pull-request branches never receive registry credentials. Signed-off-by: Johannes Rausch <jrausch@nvidia.com>
Signed-off-by: Johannes Rausch <jrausch@nvidia.com>
f022768 to
9abfd3f
Compare
1 participant
What does this PR do?
Type of change: New tests.
The existing
gpu_puzzletronsession proves the current Puzzletron lifecycle but is only run manually. This change adds a digest-pinned repository-owned CUDA image recipe and a dedicated copied-PR, nightly, and manual workflow that runs that focused session against the reusable tiny-Qwen lifecycle test.The workflow accepts only immutable
nvcr.ioimage references, verifies the checked-out image contract before allocating a GPU, and installs the checked-out ModelOpt source without resolving dependencies at runtime. The image build preinstalls the declared ModelOpt runtime and test dependencies and verifies the pinned CUDA, Torch, Transformers, LMMS-Eval, AutoModel, AIPerf, and Nox environment.Testing
Adds CPU contract coverage for the recorded base-image digest, image-reference validation, Docker setup contract, workflow permissions and routing, and dependency-isolated runtime installation. The dedicated GPU workflow runs the existing one-GPU Puzzletron lifecycle test through
nox -s gpu_puzzletron.Additional Information
This PR is stacked on #2166. The registry image must be built and published separately, then
PUZZLETRON_GPU_CI_IMAGEmust be configured with its immutable digest before the workflow can run.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests