Skip to content

fix: nss image, remove ffmpeg, force wandb version - #463

Merged
mckornfield merged 2 commits into
mainfrom
cve-fixes-nss/mck
Jun 25, 2026
Merged

fix: nss image, remove ffmpeg, force wandb version#463
mckornfield merged 2 commits into
mainfrom
cve-fixes-nss/mck

Conversation

@mckornfield

@mckornfield mckornfield commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • Bug Fixes

    • Updated container builds to use a newer Python base image and applied a consistent package pin to improve build reliability.
    • Adjusted cleanup steps to remove outdated bundled binaries more safely across environments.
  • Chores

    • Added an explicit version pin for a key package in multiple build configurations to keep dependency behavior consistent.

@mckornfield
mckornfield requested review from a team as code owners June 25, 2026 17:45
Comment thread docker/Dockerfile.safe-synthesizer-tasks
@mckornfield
mckornfield requested a review from ironcommit June 25, 2026 17:50
@github-actions github-actions Bot added the fix label Jun 25, 2026
@coderabbitai

coderabbitai Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 23da9117-750d-4930-80d7-2fd7b53df91c

📥 Commits

Reviewing files that changed from the base of the PR and between efe42ec and b1c365d.

📒 Files selected for processing (5)
  • docker/Dockerfile.nmp-automodel-base
  • docker/Dockerfile.safe-synthesizer-tasks
  • docker/automodel/no_override_requirements.txt
  • docker/scripts/cve-cleanup.sh
  • docker/unsloth/no_override_requirements.txt
✅ Files skipped from review due to trivial changes (1)
  • docker/automodel/no_override_requirements.txt
🚧 Files skipped from review as they are similar to previous changes (3)
  • docker/unsloth/no_override_requirements.txt
  • docker/Dockerfile.safe-synthesizer-tasks
  • docker/scripts/cve-cleanup.sh

📝 Walkthrough

Walkthrough

Pins wandb==0.27.2 across automodel, unsloth, and Safe Synthesizer build paths, switches the Safe Synthesizer base image to slim-trixie, and updates the CVE cleanup script to remove ffmpeg binaries from venvs instead of wandb artifacts.

Changes

Docker dependency and cleanup alignment

Layer / File(s) Summary
Automodel and Unsloth wandb pins
docker/Dockerfile.nmp-automodel-base, docker/automodel/no_override_requirements.txt, docker/unsloth/no_override_requirements.txt
The automodel builder install and both no-override requirements lists pin wandb==0.27.2 with matching comments.
Safe Synthesizer base and overrides
docker/Dockerfile.safe-synthesizer-tasks
The Safe Synthesizer Dockerfile switches to the slim-trixie Python image, writes a wandb==0.27.2 override file, and applies it across the runtime uv pip install layers.
CVE cleanup targets
docker/scripts/cve-cleanup.sh
The cleanup script replaces wandb-core and wandb removals with ffmpeg deletion inside existing venvs.

Possibly related PRs

  • NVIDIA-NeMo/nemo-platform#464: Also changes Docker wandb handling in docker/Dockerfile.nmp-automodel-base and cleanup behavior around bundled artifacts.

Suggested reviewers

  • gabwow
  • ironcommit
  • philipmattingly
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed Title matches the main changes: base image update, ffmpeg cleanup, and pinning wandb.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch cve-fixes-nss/mck

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor
Suite Lines Covered Line Rate Branch Rate
Unit Tests 20911/27483 76.1% 61.2%
Integration Tests 12109/26252 46.1% 19.5%

@mckornfield
mckornfield force-pushed the cve-fixes-nss/mck branch 2 times, most recently from 8654d42 to efe42ec Compare June 25, 2026 18:35

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
docker/Dockerfile.nmp-unsloth-training (1)

126-127: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Stale toolkit reference in commented block. Base is now 26.05 / CUDA 13.2, but this TODO still says NGC 26.02 and /usr/local/cuda-13.1. Update so the future flash-attn re-enable doesn't target the wrong toolkit.

🤖 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 `@docker/Dockerfile.nmp-unsloth-training` around lines 126 - 127, The commented
Flash Attention 2 TODO is stale and references the old NGC 26.02 torch and
/usr/local/cuda-13.1, which no longer matches the current base image. Update the
comment in the Dockerfile.nmp-unsloth-training block to reflect the current NGC
26.05 / CUDA 13.2 toolchain so the future flash-attn rebuild points to the
correct toolkit. Use the existing Step 1d Flash Attention 2 comment as the
anchor when editing.
🤖 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 `@pyproject.toml`:
- Line 299: The pydantic dependency entry is still unbounded on the upper end,
which lets lock refreshes pull in newer releases that break stainless
generation. Update the dependency declaration in the pyproject.toml package list
to add an upper bound or exact pin for pydantic, keeping the existing minimum
while preventing newer incompatible versions from being selected.

---

Nitpick comments:
In `@docker/Dockerfile.nmp-unsloth-training`:
- Around line 126-127: The commented Flash Attention 2 TODO is stale and
references the old NGC 26.02 torch and /usr/local/cuda-13.1, which no longer
matches the current base image. Update the comment in the
Dockerfile.nmp-unsloth-training block to reflect the current NGC 26.05 / CUDA
13.2 toolchain so the future flash-attn rebuild points to the correct toolkit.
Use the existing Step 1d Flash Attention 2 comment as the anchor when editing.
🪄 Autofix (Beta)

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: 73397f58-06ba-4f1e-bb28-9d93f0f275bb

📥 Commits

Reviewing files that changed from the base of the PR and between 87af3ec and 8654d42.

⛔ Files ignored due to path filters (1)
  • uv.lock is excluded by !**/*.lock
📒 Files selected for processing (8)
  • docker-bake.hcl
  • docker/Dockerfile.nmp-automodel-base
  • docker/Dockerfile.nmp-unsloth-training
  • docker/Dockerfile.safe-synthesizer-tasks
  • docker/automodel/no_override_requirements.txt
  • docker/scripts/cve-cleanup.sh
  • docker/unsloth/no_override_requirements.txt
  • pyproject.toml
✅ Files skipped from review due to trivial changes (1)
  • docker/unsloth/no_override_requirements.txt
🚧 Files skipped from review as they are similar to previous changes (2)
  • docker/Dockerfile.safe-synthesizer-tasks
  • docker/scripts/cve-cleanup.sh

Comment thread pyproject.toml
Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
@mckornfield
mckornfield enabled auto-merge June 25, 2026 20:09
@mckornfield
mckornfield added this pull request to the merge queue Jun 25, 2026
Merged via the queue into main with commit 6bced94 Jun 25, 2026
53 checks passed
@mckornfield
mckornfield deleted the cve-fixes-nss/mck branch June 25, 2026 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants