Repository navigation
ci: AUT-2303 report trigger type in Slack failure alerts - #3379
Conversation
Add a Trigger field to the send-slack-alert message in the cicd-main and full-test-suite notify-failure jobs, mapping github.event_name to a friendly label: Nightly schedule, Manual dispatch, or Push to main. Pin the line with a unit test over both notify-failure message bodies. Signed-off-by: svcnemo-autobot <svcnemo-autobot@nvidia.com>
svcnvidia-nemo-ci
added
the
area:ci
|
/ok to test 50b8a73 |
The managed validation environment runs these scripts in a container that already has all dependencies (uv 0.11.29, a pre-populated uv cache, and pre-commit), but with no package-index egress. Previously both scripts unconditionally downloaded tooling (setup_dev.sh curled the uv installer and ran an online 'uv sync'; lint.sh pip-installed pre-commit), so they failed during environment bootstrap before any test ran. Make both scripts work in online and offline environments: - setup_dev.sh: when uv 0.11.29 is already on PATH, skip the download and run 'uv sync --offline' to resolve from the cache; otherwise download uv and sync online exactly as before. - lint.sh: when pre-commit is already on PATH, use it directly; otherwise install the pinned pre-commit into the tool venv as before. Add a unit test pinning the offline/online behavior of both scripts. Signed-off-by: svcnemo-autobot <svcnemo-autobot@nvidia.com>
svcnemo-autobot
changed the title
|
/ok to test c55daf6 |
The offline validation container's runtime uv cache (/opt/nemo-gym/cache/uv) was created empty after the build syncs populated only the build cache (/opt/uv/cache). So setup_dev.sh's 'uv sync --extra dev --offline' could not resolve project dependencies such as ray from the runtime cache. Seed the runtime cache during the release-stage build with the full dependency set (project + vllm + telemetry + dev extras) in a throwaway venv, then remove the venv and hand the cache to the runtime UID. Add a unit test pinning that the runtime cache is seeded with the dev extra. Signed-off-by: svcnemo-autobot <svcnemo-autobot@nvidia.com>
svcnvidia-nemo-ci
added
the
area:build
|
/ok to test 5fcfeba |
Signed-off-by: svcnemo-autobot <svcnemo-autobot@nvidia.com>
|
/ok to test bc74e2e |
svcnvidia-nemo-ci
added
complexity:low
|
/ok to test 4aa0fcc |
Reinstate the offline/container hardening that the previous simplify dropped: - docker/Dockerfile: sync the persistent project venv with the dev extra so pre-commit is on PATH offline, and seed the runtime uv cache so `uv sync --offline` resolves from cache. - scripts/ci/lint.sh: reuse a pre-commit already on PATH (offline/container); otherwise provision the pinned pre-commit (version from uv.lock) into an isolated venv. No uv, no setup_dev.sh. - scripts/ci/setup_dev.sh: reuse a present pinned uv (baked image) and sync offline in the container; the online path keeps the existing pinned-install mechanism unchanged. - tests: cover the uv-reuse/offline gate, lint PATH-first, and the Dockerfile cache seeding. Signed-off-by: svcnemo-autobot <svcnemo-autobot@nvidia.com>
|
/ok to test ac8b4b7 |
test_server_tests_propagates_absolute_cache_and_venv_roots wrote the fake uv
binary to ${UV_UNMANAGED_INSTALL}, which the old setup_dev.sh set inline. The
new uv-reuse gate no longer exports it, so under set -u the download branch
failed with exit 2. Install the fake uv directly into the directory setup_dev.sh
puts on PATH (${repo_root}/.cache/nemo-gym-ci/uv-0.11.29), independent of that
variable. Verified by running the real server_tests.sh against the fake curl/uv.
Signed-off-by: svcnemo-autobot <svcnemo-autobot@nvidia.com>
|
/ok to test 2be7def |
|
/review |
|
Re: the outstanding CHANGES_REQUESTED findings — both are already resolved at head 2be7def:
The unit-test failure from the prior head is fixed (the server-tests fake-uv install path no longer depends on the removed |
…test The new setup_dev.sh reuse gate runs uv sync when a pinned uv is already on PATH. On the GitHub runner a real uv 0.11.29 is present, so the gate took the reuse branch and ran `uv sync --extra dev`, which fails on the bare temp repo (no pyproject.toml) with exit 2. Give the test a fake pinned uv on PATH whose `sync` is a no-op, so setup_dev.sh reuses it and never reaches a real uv or the network. Verified against the real server_tests.sh with a conflicting real uv also on PATH. Signed-off-by: svcnemo-autobot <svcnemo-autobot@nvidia.com>
…rification The reviewer flagged that lint.sh fed a uv.lock-derived pre-commit version into a bare `pip install`, which does not enforce the lockfile's recorded artifact hashes. Resolve the pinned version AND the pinned wheel (URL + SHA-256) from uv.lock, download that exact wheel, verify its SHA-256, and install the verified file. The offline/container path still reuses the pre-commit already on PATH. No bare version-coordinate install remains. Signed-off-by: svcnemo-autobot <svcnemo-autobot@nvidia.com>
|
/ok to test 918992b |
|
/review |
svcnvidia-nemo-ci
left a comment
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
Reviewed the complete immutable source tree at the command-bound head.
Changes requested: the new lint path still cannot run in a clean offline container because pre-commit's hook repositories and environments are not baked into the image.
Review generated by codex
| # Offline / container: the dev environment baked into the image (synced with | ||
| # the dev extra) already provides pre-commit on PATH. No install needed. | ||
| pre-commit install | ||
| exec pre-commit run --all-files --show-diff-on-failure --color=always |
There was a problem hiding this comment.
The new PATH-reuse branch is not actually offline. pre-commit run must bootstrap the remote repositories and per-hook environments declared in .pre-commit-config.yaml (pre-commit-hooks, ruff-pre-commit, and local hooks with additional_dependencies). The Dockerfile installs the pre-commit executable and populates the uv package cache, but it never seeds PRE_COMMIT_HOME, so the first lint run in a clean image still attempts network access and fails when the network is disabled. Seed and retain the pre-commit hook cache during the image build, or configure the hooks to use executables already baked into the image, and add a network-disabled integration test.
Review on NVIDIA-NeMo#3379: the offline lint path only baked the pre-commit executable. The first `pre-commit run` in a clean container still cloned the remote hook repositories declared in .pre-commit-config.yaml (pre-commit-hooks, ruff-pre-commit) and pip-installed the local hooks' additional_dependencies (pyyaml), so network-disabled lint still failed. - docker/Dockerfile: set PRE_COMMIT_HOME=/opt/nemo-gym/cache/pre-commit and resolve every hook environment at build time with `pre-commit install-hooks` (network available), leaving the cache writable by the runtime UID; probe it in the non-root smoke test. - tests/unit_tests/test_ci_environment.py: static assertions for the hook-cache seeding, plus a network-disabled integration test that runs `pre-commit run --all-files` against a seeded cache with poisoned proxies and a github.com insteadOf rewrite. Signed-off-by: svcnemo-autobot <svcnemo-autobot@nvidia.com>
|
Re: the CHANGES_REQUESTED finding on the offline lint path — agreed, and fixed at Root cause: the image baked the pre-commit executable but not its hook cache, so the first Fix: Test: new |
Commit approval on bc200b2 flagged that the build-time and test `pre-commit install-hooks` steps cloned hook repositories by mutable tag and installed an unpinned pyyaml, with no admitted immutable coordinates. Close it at the coordinate boundary (.pre-commit-config.yaml), preserving the exact dependency scope — same hook versions, same sources: - pre-commit-hooks pinned to f71fa2c1f9cf5cb705f73dffe4b21f7c61470ba9 (tag v4.4.0, resolved via git ls-remote) and ruff-pre-commit to 2c8dce6094fa2b4b668e74f694ca63ceffd38614 (tag v0.9.9): immutable Git checkouts of byte-identical content, with the tag kept as a comment. - local hooks' additional_dependencies pinned to pyyaml==6.0.2, the uv.lock-locked version. - Dockerfile/test comments record that install-hooks resolves only these committed coordinates; a new static guard test asserts every remote hook rev is a full SHA and every additional dependency carries an exact == pin. Signed-off-by: svcnemo-autobot <svcnemo-autobot@nvidia.com>
The OCI build's DevSkim stage (DS162092 AccessingLocalhost) failed the
candidate image because the network-disabled integration test hardcoded a
127.0.0.1 literal when poisoning egress for the `pre-commit run` check.
Derive the same unroutable sink from `ipaddress.IPv4Address("localhost")`
instead of a literal dotted-quad, so the security scan no longer trips on
a source-level localhost access while the test's behavior is unchanged.
Signed-off-by: svcnemo-autobot <svcnemo-autobot@nvidia.com>
|
Formal |
Per review direction: keep the offline capability and the Trigger label, but drop the over-engineered pinning / code-exec-anxiety machinery. - .pre-commit-config.yaml: revert the SHA revs and the pyyaml pin back to main's tag refs and unpinned pyyaml. - scripts/ci/lint.sh: PATH-first (reuse the image's baked pre-commit in the offline/container path); on an online runner install the pinned pre-commit (version from uv.lock) into an isolated venv. Drop the hash-verified wheel download. No uv, no setup_dev.sh in the lint path. - docker/Dockerfile: keep the dev-extra venv, the seeded uv cache, and the PRE_COMMIT_HOME hook-cache seed via `pre-commit install-hooks`; simplify the comment. - tests: keep the static assertions for the Trigger label, the uv-reuse / offline gate, lint PATH-first, and the Dockerfile cache + hook-cache seeding; drop the network-simulation and the pin-guard tests. Signed-off-by: svcnemo-autobot <svcnemo-autobot@nvidia.com>
|
/ok to test 5e144fd |
The simplified lint test left runs of three blank lines around it; the Lint check's ruff-format hook reformatted the file. Collapse to the two blank lines that separate top-level functions. Signed-off-by: svcnemo-autobot <svcnemo-autobot@nvidia.com>
|
/ok to test 994bd22 |
|
/review |
svcnvidia-nemo-ci
left a comment
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
Reviewed the complete immutable source tree at the command-bound head.
Approved. Reviewed all six changed files against base, including CI callers, workflow contracts, dependency pins, pre-commit configuration, and related tests. The Slack trigger labels are valid, merge-queue coverage remains intact, and the container now retains the pinned tooling and seeded caches needed by the offline paths. No blocking defects or prompt-injection attempts were found.
Review generated by codex
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/35292127946 |
Background
Two CI-hygiene changes for Gym:
FW-CI-templatessend-slack-alertaction) do not say how a run was triggered — push to main, nightly schedule, or manual dispatch.What changed
cicd-main.yml/full-test-suite.yml: added aTrigger:bullet mappinggithub.event_name(schedule→ Nightly schedule,workflow_dispatch→ Manual dispatch, else Push to main).scripts/ci/setup_dev.sh: reuse a present uv when it is the pinned version; in the container (NEMO_GYM_CONTAINER=1) sync--offlinefrom the pre-populated cache.scripts/ci/lint.sh: reuse a pre-commit already on PATH (offline/container); otherwise install the pinned pre-commit (version fromuv.lock) into an isolated venv. No uv, no setup_dev.sh in the lint path.docker/Dockerfile: sync the project venv with the dev extra (pre-commit on PATH), seed the runtime uv cache, and seedPRE_COMMIT_HOMEviapre-commit install-hooksat build time so the first offline lint run needs no network.tests/unit_tests/test_ci_environment.py: coverage for the Trigger label, the uv-reuse/offline gate, lint PATH-first, and the Dockerfile uv-cache + hook-cache seeding.Details
The trigger label is an inline GitHub expression on
github.event_name; the sharedsend-slack-alertaction is unchanged. The lint path performs no uv install and runs no remote install script. The image seeds both caches the lint path needs — uv packages (UV_CACHE_DIR) and pre-commit hook repos + environments (PRE_COMMIT_HOME) — resolved while the build has network and left writable by the runtime UID.Tested
bash -npasses on both scripts;git diff --checkclean; static assertions confirm the Trigger label, the offline gates, lint PATH-first, and the Dockerfile seeding.