ci: test GPU workflows against unpinned wheel - #691
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:
WalkthroughThe GPU setup action now builds and installs a wheel in a clean virtual environment. New tasks separate environment preparation from GPU unit and E2E execution. The GPU workflow runs the new lanes and updates its timeout and documentation. ChangesGPU test pipeline
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR changes GPU CI to validate a freshly built wheel against dependencies resolved from live package indexes rather than the lockfile.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5d70fcbf-e47e-40bf-9d8f-828751564c98
📒 Files selected for processing (5)
.github/actions/setup-gpu-test-env/action.yml.github/workflows/README.md.github/workflows/gpu-tests.yml.mise/tasks/tests.tomltests/TESTING.md
📜 Review details
⏰ Context from checks skipped due to timeout. (11)
- GitHub Check: Unit Tests (3.12)
- GitHub Check: Unit Tests (3.13)
- GitHub Check: Unit Tests (3.11)
- GitHub Check: End-user Wheel Install
- GitHub Check: Smoke Tests
- GitHub Check: Greptile Review
- GitHub Check: Analyze (Python)
- GitHub Check: Typecheck
- GitHub Check: Format
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (11)
**/*.{md,markdown,py}
📄 CodeRabbit inference engine (.cursor/rules/agent-markdown-style.mdc)
**/*.{md,markdown,py}: Avoid decorative bold (**text**) in list items, body text, and docstrings; use structural cues (headers, list markers, colons, backticks) for emphasis instead
Use backticks for code identifiers, paths, and CLI commands in markdown and docstrings
Files:
tests/TESTING.md
**/*.{md,markdown}
📄 CodeRabbit inference engine (.cursor/rules/agent-markdown-style.mdc)
**/*.{md,markdown}: Bold is acceptable only in markdown tables where it's the conventional way to mark header-like cells in the body
Use##headers to segment markdown sections instead of bold text
Use--(em-dash) instead of-(hyphen) for asides in markdown
Files:
tests/TESTING.md
tests/**
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
tests/**: Mirrorsrc/directory structure intests/directory for test organization
Auto-mark tests by directory:tests/e2e/→e2e,tests/smoke/→smoke, otherwise default tounit
Files:
tests/TESTING.md
**/*.md
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Do not use decorative bold in Markdown body text, list items, or docstrings; use single backticks for code identifiers, paths, and commands.
Files:
tests/TESTING.md
**/*
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
**/*: Every source file requires the SPDX copyright and license header appropriate to its file format.
End files with a newline, remove trailing whitespace, use one space between sentences, and keep code, comments, and docstrings within 120 characters.
**/*: All contributions must use verified Git commits and DCO sign-off; unsigned or unsigned-off commits cannot be merged.
Branches other thanmainmust follow<author>/<description>, optionally including an issue ID or type; branch names must use lowercase alphanumeric characters and hyphens.
Commits merged tomainmust follow Conventional Commits, using a valid lowercase type and a description of at most 100 characters.
Files:
tests/TESTING.md
⚙️ CodeRabbit configuration file
**/*: Review as a senior maintainer for NeMo Safe Synthesizer. Prioritize issues that can change behavior, break user workflows, weaken privacy guarantees, hide failures, make tests unreliable, or create maintenance risk. Avoid generic style commentary unless it points to a concrete project convention that automated tools will not catch.
Comment only when the finding is actionable and tied to changed code. For each finding, state the impact, the condition that triggers it, and the smallest practical fix. Prefer one precise comment over broad advice. Do not ask for refactors outside the PR scope unless the changed code creates the problem.
Review type guidance: - Potential issue: use for correctness bugs, data loss, privacy leaks,
security risks, broken public APIs, invalid config behavior, missing
validation, hidden failures, nondeterministic tests, or CI breakage.
- Refactor suggestion: use for local maintainability problems introduced
by the diff when they have clear future cost, such as duplicated setup,
unclear boundaries, over-mocking, avoidable complexity, or opaque test
helpers.- Nitpick: avoid in chill mode. Do not emit formatting, import-order,
wording, or style-only comments unless automated tools cannot catch the
issue and it affects maintainability.Severity guidance: - Critical: security/privacy leaks, data loss, training/test/holdout
contamination, or broken release/package/core pipeline execution.
- Major: incorrect generation/training/evaluation behavior, broken
CLI/SDK public API, invalid config defaults or validators, or GPU/vLLM
cleanup and process-isolation bugs likely to fail CI or production
runs.- Minor: localized bugs, missing focused tests for changed behavior, or
bad test patterns that weaken regression coverage.- Trivial: small cleanup with no behavior impact. Usually suppress in
chill mode.- Info: context only. Avoid unless it helps reviewers understand risk.
Safe-Synthesizer-specific review focus: - Data ...
Files:
tests/TESTING.md
**/*.{py,sh,yaml,yml,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
All Python, shell, YAML, YML, and Markdown source files require SPDX copyright headers.
Files:
tests/TESTING.md
**/*.{py,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use the repository's documented Python and Markdown style conventions and validate changes with the pinned
miseformatting and checking tasks.
Files:
tests/TESTING.md
.github/workflows/*.yml
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Changes to GitHub Actions workflows must preserve the documented CI, release, signing, and deployment requirements.
Files:
.github/workflows/gpu-tests.yml
.github/**
⚙️ CodeRabbit configuration file
Review GitHub configuration for branch protection expectations, CODEOWNERS alignment, least privilege permissions, pinned actions where practical, and consistency with CONTRIBUTING.md.
Files:
.github/workflows/gpu-tests.yml.github/actions/setup-gpu-test-env/action.yml.github/workflows/README.md
.mise/tasks/**/*.toml
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Keep declarative Mise tasks under
.mise/tasks/, provide descriptions for public tasks, and add usage metadata where arguments need validation or help.
Files:
.mise/tasks/tests.toml
.mise/tasks/*
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Keep shared shell helpers in
.mise/tasks/_lib.shand make that file non-executable.
Files:
.mise/tasks/tests.toml
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: NVIDIA-NeMo/Safe-Synthesizer
Timestamp: 2026-08-03T17:39:40.076Z
Learning: Use the documented mise and pytest commands for the appropriate test scope; run individual tests with `uv run --frozen pytest tests/path/test_file.py::test_name -vvs -n0`.
🪛 LanguageTool
.github/workflows/README.md
[uncategorized] ~154-~154: The official name of this software platform is spelled with a capital “H”.
Context: ... workflow/action changes. GPU jobs use .github/actions/setup-gpu-test-env for shared ...
(GITHUB)
🔇 Additional comments (6)
.github/actions/setup-gpu-test-env/action.yml (1)
16-16: LGTM!Also applies to: 28-28, 42-42, 50-94
.mise/tasks/tests.toml (2)
89-115: LGTM!
13-16: 🩺 Stability & AvailabilityNo process-isolation change is needed.
The only matching test is
tests/benchmarks/test_generation_structured_methods.py. Each benchmark invokes the vLLM CLI throughsubprocess.run, so each generation runs in a separate process.> Likely an incorrect or invalid review comment.tests/TESTING.md (1)
22-22: LGTM!Also applies to: 32-32
.github/workflows/gpu-tests.yml (1)
88-88: LGTM!Also applies to: 165-165
.github/workflows/README.md (1)
154-154: LGTM!
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Signed-off-by: Yunfeng Zhang <yunzhang@nvidia.com>
d137792 to
a66b092
Compare
Signed-off-by: Yunfeng Zhang <yunzhang@nvidia.com>
Signed-off-by: Yunfeng Zhang <yunzhang@nvidia.com>
Signed-off-by: Yunfeng Zhang <yunzhang@nvidia.com>
Signed-off-by: Yunfeng Zhang <yunzhang@nvidia.com>
github-actions
Bot
added
area:dev-ex
|
our test semantics could use a little update in a followup pr |
Agree. |
Signed-off-by: Yunfeng Zhang <yunzhang@nvidia.com>
binaryaaron
left a comment
binaryaaron
left a comment
There was a problem hiding this comment.
approved; up to you if you want to address this in this pr or a followup. thanks!
…NeMo#705) # Summary Fixes GPU smoke + e2e failing on `main` with: ``` × No solution found when resolving dependencies: ╰─▶ Because there is no version of flashinfer-cubin{sys_platform == 'linux'}==0.6.14 ... conclude that your requirements are unsatisfiable. ``` ## Root cause `setup-gpu-test-env` infers which indexes to pass to `uv` by matching the CUDA extra against each index's name or URL: ```python if index["name"].endswith(f"-{cuda_extra}") or f"/{cuda_extra}" in index["url"] ``` When that inference landed (NVIDIA-NeMo#691, Aug 4 16:50 UTC) every index encoded its variant one of those two ways, so the rule held. NVIDIA-NeMo#664 landed 27 minutes later (17:17 UTC) and added the first index that doesn't: `flashinfer-cubin`, which serves a single variant-agnostic `https://flashinfer.ai/whl/` and is bound to packages only through `[tool.uv.sources]`. It matches neither branch of the rule, so it was silently dropped — and since the install also passes `--no-sources`, the sources table couldn't reach it either. Both PRs were green on their own base; the breakage exists only in the merge. That index is the only place `flashinfer-cubin==0.6.14` exists: | Index | max `flashinfer-cubin` | | ----- | ---------------------- | | `https://flashinfer.ai/whl/` | 0.6.16.post1 (has 0.6.14) | | PyPI | 0.6.13 | | `https://flashinfer.ai/whl/cu129` | does not serve the package | | `https://wheels.vllm.ai/0.26.0/cu129` | 0.6.13 | ## Fix Also collect indexes referenced by `[tool.uv.sources]` for the target extra. Before / after, run against `pyproject.toml` at `cu129`: ``` https://download.pytorch.org/whl/cu129 https://flashinfer.ai/whl/cu129 + https://flashinfer.ai/whl/ https://wheels.vllm.ai/0.26.0/cu129 ``` This matches the index set in the documented manual install command, which is why installing by hand works today while CI does not. ## Verification Resolution flips with exactly that one index, reproducing CI's error before and succeeding after: ``` $ uv pip install --dry-run --no-deps --default-index https://pypi.org/simple \ --index .../cu129 (x3) --index-strategy unsafe-best-match flashinfer-cubin==0.6.14 × No solution found ... there is no version of flashinfer-cubin==0.6.14 $ # same, plus --index https://flashinfer.ai/whl/ Resolved 1 package + flashinfer-cubin==0.6.14 ``` `mise run format-check` passes. No dependency versions change. ## Pre-Review Checklist - [x] `mise run format && mise run check` - [x] GPU CI status check passes -- this PR is the fix for it ## Other Notes CI-only change; no source or dependency changes. Worth noting the `index_count < 3` guard did not catch this — the count stayed at exactly 3 while the wrong index was dropped. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved GPU test environment setup to reliably discover all CUDA package indexes configured for the selected extra. * Added support for indexes declared through source configuration, alongside existing name- and URL-based matching. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Yunfeng Zhang <yunzhang@nvidia.com>
2 participants
Summary
uv.lockpyproject.tomlwith a tested stdlib-only helper, require at least three unique matches, and reuse it in CI wheel verification instead of hardcoding the vLLM indexTest plan
mise run format-checkmise run typecheckuv run --frozen pytest tests/tools -n0docker build --check --target wheel-install --file containers/Dockerfile.test_ci .Summary by CodeRabbit
Testing
Documentation