ci: run GPU tests and notebooks per PR, and fix the GPU lane - #68
Open
ramakrishnap-nv wants to merge 5 commits into
Open
ci: run GPU tests and notebooks per PR, and fix the GPU lane#68ramakrishnap-nv wants to merge 5 commits into
ramakrishnap-nv wants to merge 5 commits into
Conversation
Add a root VERSION file and reconcile the three-way version drift between pyproject.toml (26.6), src/__init__.py (26.4), and the plugin manifests (25.10.00), which had no common value. The drift was possible because the guard was inert: every version check in validate_skills.sh was gated on `[[ -f VERSION ]]`, and no VERSION file existed, so all of them silently passed. Versioning: - Add VERSION (26.6) as the single source of truth. - Extend sync_skills_version.sh to also propagate to pyproject.toml and src/__init__.py; add matching checks to validate_skills.sh. - validate_skills.sh now hard-fails when VERSION is absent or empty. - Enable the sync-skills-version pre-commit hook and add `version:` to SKILL.md frontmatter. - Fix the validate-skills hook trigger: `^(...|skills/|...)$` anchors the alternation, so the `skills/`, `.claude-plugin/` and `.cursor-plugin/` prefixes only ever matched those literal strings. The hook had never fired on SKILL.md, marketplace.json, or plugin.json edits. Its trigger list is now a strict superset of sync-skills-version's. Packaging: - Declare matplotlib>=3.8. src/ imports it directly but it was only present transitively via seaborn. - Cap requires-python at <3.14: on 3.14 uv falls back to building pandas from sdist and the Meson build fails. 3.11-3.13 all pass. - Add Python 3.13 to the PR test matrix. CI hardening: - Add `permissions: contents: read` to main.yml, pr.yaml and checks.yml, and `persist-credentials: false` to every checkout except the Git LFS one, which needs the credential helper. - Lower the zizmor gate from --min-severity high to medium, which is what surfaced the two findings above. - Remove release-2512.yml: a stale duplicate of main.yml (same workflow `name:`) that ran on every push to main, consuming a second GPU runner while installing no CUDA extra. - Add SPDX headers to the six workflow and issue-template files missing them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
GPU coverage only ran post-merge on main, so a PR could break the GPU
paths and only be caught after landing. Add a blocking per-PR GPU lane.
The existing post-merge GPU lane is red, which had to be fixed first:
main.yml installs `--extra cuda13` (cuOpt 26.04) and then runs
`pytest -m gpu`, which includes the SOCP variance-cap tests. cuOpt gained
QCQP/SOCP support in 26.06, so 26.04 raises
Exception: Quadratic constraints not supported
from QuadraticExpression.__le__. The tests guarded only on
`importorskip("cuopt")`, with no version check. Verified locally against
both cuOpt 26.04 (now skips with a reason) and 26.06 (still passes, so
the guard does not silently disable coverage).
Test fixes:
- Add tests/conftest.py with a `require_cuopt_socp` fixture that skips
when the installed cuOpt predates 26.06.
- Apply it to test_cuopt_python_var_limit_solves_socp and
test_socp_variance_limit_solves.
Per-PR GPU lane (pr.yaml):
- New `changes` job diffs the PR range and sets an output when
src/, tests/, notebooks/, the GPU scripts, pyproject.toml, uv.lock or
pr.yaml changed. Path filtering lives in a job rather than in
`on.pull_request.paths` because a workflow-level filter makes the check
never report, leaving a required check pending forever.
- New `gpu-tests` job runs the GPU pytest suite and executes the five
notebooks, uploading results as artifacts.
- Guarded with `head.repo.full_name == github.repository` so the
self-hosted GPU runner is never exposed to code from a fork.
- pr-builder now gates on it, accepting `skipped` (no GPU-relevant change,
or a fork PR) but blocking on failure.
Deduplication:
- Extract the inline notebook and GPU-pytest bash from main.yml into
ci/utils/run_notebooks.sh and ci/utils/run_gpu_tests.sh, now called by
both lanes. Duplicating this per workflow is what let release-2512.yml
silently drift. The launchable -> launchable_brev_result output name is
preserved, since the artifact and QA verification steps depend on it.
- run_gpu_tests.sh prints solver versions and passes -rs, so a lane that
skips everything (wrong extra, missing cuML) is visible in the log
instead of reading green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
ramakrishnap-nv
requested review from
jgoldberg-nvidia and
tmckayus
and removed request for
a team
August 3, 2026 22:24
ramakrishnap-nv
changed the base branch from
chore/version-single-source-of-truth
to
main
August 3, 2026 22:25
The notebook script invokes `papermill` and `jupyter nbconvert` as bare
commands, so they must be on PATH. `uv pip install` puts them in .venv,
whose bin/ is not on PATH, so every notebook failed with
ci/utils/run_notebooks.sh: line 69: papermill: command not found
mean_variance_basic.ipynb,FAIL(127)
Restores the container-level `pip install` the inline main.yml block used
before the extraction. The notebooks still run against the uv environment
through the portfolio-opt kernel.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
The notebook toolchain was the one part of the pipeline outside uv. The
inline main.yml block ran
pip install jupyter jupyterlab ipykernel papermill nbconvert
which installs into the container's system Python, unpinned, resolving
whatever is newest on PyPI on each run. Everything else in this repo
builds and tests from uv.lock, so a papermill or nbconvert release could
break CI with no change to the repo.
- Add a `notebooks` dependency group (papermill, nbconvert, ipykernel)
so the versions resolve into uv.lock: papermill 2.7.0, nbconvert
7.17.1, ipykernel 7.1.0.
- run_notebooks.sh syncs `--group notebooks` and invokes every tool via
`uv run`, which resolves from the project environment. This also fixes
the "papermill: command not found" failure: `uv pip install` puts the
entry points in .venv/bin, which is not on PATH, so a bare `papermill`
cannot be used.
- Drop jupyter/jupyterlab from the CI install. No lab server is started;
nbconvert supplies the `jupyter` entry point the script needs.
Verified papermill and nbconvert resolve through `uv run` from both the
repo root and the notebooks/ subdirectory the script runs in, and that
`uv lock --locked` is clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
Both GPU lanes install --extra cuda13, which tracks cuOpt 26.04. That build has no QCQP/SOCP support, so the variance-cap tests skip and the Mean-Variance SOCP path added in PR #50 had no CI coverage at all -- visible in the log thanks to -rs, but still untested. - Add a "Run SOCP tests (cuOpt 26.06)" step to both the per-PR lane and the post-merge lane, invoking run_gpu_tests.sh with the cuOpt-only cuda13-socp extra. Ordered last, since it swaps the environment and the preceding steps both want cuda13. - Assert in run_gpu_tests.sh that a SOCP-capable extra actually resolved cuOpt >= 26.06, failing loudly otherwise. Without it a misresolved environment would skip every SOCP test and still exit 0, so the lane would report green while testing nothing -- the exact failure mode this step exists to prevent. Verified locally on GPU: the assertion passes on cuOpt 26.06 and the SOCP test PASSES rather than skipping; the guard rejects 26.04. Remove this step once cuml-cu13 26.06 ships and cuda13 covers both paths in a single pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
Collaborator
Author
|
@jgoldberg-nvidia May I get review on this PR |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
GPU coverage only ran post-merge on
main, so a PR could break the GPU paths and only be caught after landing.The existing GPU lane is red, and had to be fixed first
main.ymlinstalls--extra cuda13(cuOpt 26.04) and then runspytest -m gpu, which includes the SOCP variance-cap tests. cuOpt gained QCQP/SOCP support in 26.06, so 26.04 raises:The tests guarded only on
importorskip("cuopt")— no version check. Reproduced locally on a Quadro RTX 8000 / CUDA 13:cuda13cuda13-socpBoth directions verified, so the guard reports honestly rather than silently disabling coverage.
Test fixes
tests/conftest.pywith arequire_cuopt_socpfixture that skips when cuOpt predates 26.06.test_cuopt_python_var_limit_solves_socpandtest_socp_variance_limit_solves.Per-PR GPU lane
changesjob diffs the PR range and sets an output whensrc/,tests/,notebooks/, the GPU scripts,pyproject.toml,uv.lockorpr.yamlchanged. Path filtering deliberately lives in a job, noton.pull_request.paths— a workflow-level filter makes the check never report, which leaves a required check pending forever.gpu-testsjob runs the GPU pytest suite and executes all five notebooks, uploading HTML/logs as artifacts (7-day retention).head.repo.full_name == github.repository, so the self-hostedarc-runners-org-nvidia-ai-bp-1-gpurunner is never exposed to code from a fork.pr-buildernow gates on it, acceptingskipped(no GPU-relevant change, or a fork PR) but failing on an actual failure.Path filter dry-run:
src/portfolio.py,tests/conftest.py,notebooks/cvar_basic.ipynb,pyproject.tomlREADME.md,CONTRIBUTING.md,docs/arch_diagram.png,ci/utils/validate_skills.shDeduplication
The inline notebook and GPU-pytest bash moved out of
main.ymlintoci/utils/run_notebooks.shandci/utils/run_gpu_tests.sh, now called by both lanes. Duplicating ~100 lines of this per workflow is exactly what letrelease-2512.ymlsilently drift.main.ymlshrinks by ~4.3 KB with no behaviour change — thelaunchable→launchable_brev_resultoutput name is preserved, since the artifact upload and QA verification steps depend on that exact filename.run_gpu_tests.shalso prints solver versions and passes-rs, so a lane that skips everything (wrong extra, missing cuML) is visible in the log instead of reading green.Verification
shellcheck,yamllint,zizmorand the fullpre-commit run --all-filespass.Not verified locally
The notebook execution and the 6 skill-benchmark tests need cuML, which needs
libnvrtc.so.13; my box has the CUDA 13 driver but not the runtime libs, so those error locally withCuPy failed to load libnvrtc.so.13. That is a limitation of my environment, not the repo — thenvcr.io/nvidia/pytorch:25.08-py3container ships those libs. This PR's owngpu-testsrun is the first real end-to-end check, since it touchestests/andpyproject.tomland therefore trips its own path filter.🤖 Generated with Claude Code