Skip to content

feat: re-enable safe-synthesizer container runs - #599

Merged
mckornfield merged 1 commit into
mainfrom
nss-container-runtime/mck
Jul 9, 2026
Merged

feat: re-enable safe-synthesizer container runs#599
mckornfield merged 1 commit into
mainfrom
nss-container-runtime/mck

Conversation

@mckornfield

@mckornfield mckornfield commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • New Features

    • Safe Synthesizer jobs now support a fully specified container image reference, with updated defaults for task image selection.
    • Added a reusable Docker Buildx setup action for CI image builds.
  • Bug Fixes

    • Improved job image selection so platform runs use the configured image consistently.
    • Added dependency pinning and build constraints for more reliable runtime setup.
  • Documentation

    • Updated Safe Synthesizer guides, setup, and troubleshooting steps to reflect platform job usage, container image configuration, and local GPU workflows.

@mckornfield
mckornfield requested review from a team as code owners July 7, 2026 21:02
@mckornfield
mckornfield requested a review from tylersbray July 7, 2026 21:02
@github-actions github-actions Bot added the feat label Jul 7, 2026
@mckornfield
mckornfield force-pushed the nss-container-runtime/mck branch from 73f3d2c to 85e1116 Compare July 7, 2026 21:02
@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

@coderabbitai

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds a container_image_ref override to SafeSynthesizerConfig and job endpoint image selection, updates the Safe Synthesizer task Dockerfile (build-essential, NEMO_DEPLOYMENT_TYPE, constraints file applied to uv pip install steps), introduces a reusable Buildx setup GitHub Action wired into CI image jobs, updates docs/workflows, pins kagglehub, and adds container-mode unit tests.

Changes

Safe Synthesizer container image flow

Layer / File(s) Summary
Config and image selection
plugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/config.py, plugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/api/v2/jobs/endpoints.py
Default container_image changes to safe-synthesizer-tasks; new container_image_ref field bypasses qualification via a new _container_image() helper used by the GPU executor.
Runtime image and Buildx setup
docker/Dockerfile.safe-synthesizer-tasks, plugins/nemo-safe-synthesizer/constraints.txt, .github/actions/setup-docker-buildx/action.yaml, .github/actions/changes/action.yaml, .github/workflows/ci.yaml
Dockerfile adds build-essential, NEMO_DEPLOYMENT_TYPE=nmp, and applies constraints.txt (with new gliner<0.2.27 pin) across uv pip install steps; new composite action relocates Docker's data-root and manages Buildx builder lifecycle, wired into three CI image jobs and added to the k8s-smoke filter.
Container mode tests
plugins/nemo-safe-synthesizer/tests/unit/test_jobs.py
New async tests verify container-mode job compilation with qualified image fallback and with container_image_ref override, without calling qualification.
Docs and workflow guidance
docs/set-up/config-reference.mdx, docs/safe-synthesizer/tutorials/safe-synthesizer-101.mdx, plugins/nemo-safe-synthesizer/src/.../SKILL.md, plugins/nemo-safe-synthesizer/src/.../workflows/*.md
Docs updated for new config default/field, pinned kagglehub==0.3.13, and reorganized guidance favoring platform container jobs, Jobs API/SDK usage, container image env vars, and expanded diagnose/run prerequisites.

Sequence Diagram(s)

sequenceDiagram
  participant JobsCompiler
  participant Endpoints
  participant SafeSynthesizerConfig

  JobsCompiler->>Endpoints: compile job spec
  Endpoints->>SafeSynthesizerConfig: read container_image_ref, container_image
  alt container_image_ref set
    Endpoints-->>JobsCompiler: use override image directly
  else not set
    Endpoints->>Endpoints: get_qualified_image(container_image)
    Endpoints-->>JobsCompiler: use qualified image
  end
Loading

Suggested labels: ci

Suggested reviewers: maxdubrinsky, svvarom

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title matches the main change: restoring Safe Synthesizer container-based runs.
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch nss-container-runtime/mck

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

@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

🤖 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 `@docs/safe-synthesizer/tutorials/safe-synthesizer-101.mdx`:
- Around line 39-42: The pip fallback command in the tutorial snippet has an
unterminated quote, which breaks the shell example. Update the install line in
the safe-synthesizer tutorial so it matches the quoting used by the uv pip
install example, with the kagglehub version and matplotlib each properly quoted
as needed. Check the shell snippet around the fallback install command to ensure
the quote is closed and the command is syntactically valid.
🪄 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: bfd604fd-1d44-4d4e-a1e3-40b7a574a0e5

📥 Commits

Reviewing files that changed from the base of the PR and between 0d5ad27 and 85e1116.

📒 Files selected for processing (10)
  • docker/Dockerfile.safe-synthesizer-tasks
  • docs/safe-synthesizer/tutorials/safe-synthesizer-101.mdx
  • docs/set-up/config-reference.mdx
  • plugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/api/v2/jobs/endpoints.py
  • plugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/config.py
  • plugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/skills/safe-synthesizer/SKILL.md
  • plugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/skills/safe-synthesizer/workflows/config-runs.md
  • plugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/skills/safe-synthesizer/workflows/diagnose.md
  • plugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/skills/safe-synthesizer/workflows/run.md
  • plugins/nemo-safe-synthesizer/tests/unit/test_jobs.py

Comment thread docs/safe-synthesizer/tutorials/safe-synthesizer-101.mdx
@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor
Suite Lines Covered Line Rate Branch Rate
Unit Tests 23572/30681 76.8% 61.6%
Integration Tests 13722/29330 46.8% 19.9%

@mckornfield
mckornfield force-pushed the nss-container-runtime/mck branch from 85e1116 to 266afd0 Compare July 8, 2026 16:35
@mckornfield
mckornfield requested a review from a team as a code owner July 8, 2026 16:35
@mckornfield
mckornfield force-pushed the nss-container-runtime/mck branch from e075efd to f045054 Compare July 8, 2026 21:21

@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.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
docker/Dockerfile.safe-synthesizer-tasks (1)

121-129: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Replace --constraints with --constraint in all four uv pip install commands. uv only accepts the singular flag, so this will fail the Docker build.

🤖 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.safe-synthesizer-tasks` around lines 121 - 129, The uv
install commands are using the wrong plural constraints flag, which breaks the
Docker build. Update every affected `uv pip install` invocation in
`Dockerfile.safe-synthesizer-tasks` to use the singular `--constraint` option
instead of `--constraints`, keeping the existing paths and other flags
unchanged.
🤖 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.

Outside diff comments:
In `@docker/Dockerfile.safe-synthesizer-tasks`:
- Around line 121-129: The uv install commands are using the wrong plural
constraints flag, which breaks the Docker build. Update every affected `uv pip
install` invocation in `Dockerfile.safe-synthesizer-tasks` to use the singular
`--constraint` option instead of `--constraints`, keeping the existing paths and
other flags unchanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 398a36f0-5cca-44ef-866d-22879753593f

📥 Commits

Reviewing files that changed from the base of the PR and between 266afd0 and e075efd.

📒 Files selected for processing (5)
  • .github/actions/setup-docker-buildx/action.yaml
  • docker/Dockerfile.safe-synthesizer-tasks
  • packages/nemo_platform_plugin/src/nemo_platform_plugin/jobs/file_manager.py
  • packages/nmp_common/tests/jobs/test_file_manager.py
  • plugins/nemo-safe-synthesizer/constraints.txt
✅ Files skipped from review due to trivial changes (1)
  • plugins/nemo-safe-synthesizer/constraints.txt

@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

🤖 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
`@plugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/skills/safe-synthesizer/workflows/config-runs.md`:
- Around line 135-142: The config-runs markdown uses fenced bash code blocks
that violate the repo’s markdownlint MD046 expectation. Update the affected
examples in this section to indented code blocks instead of fenced blocks,
keeping the same content while preserving the existing guidance around the Jobs
API/SDK and container execution settings.
🪄 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: 493adec0-9047-45dd-98b1-da0e261f3efa

📥 Commits

Reviewing files that changed from the base of the PR and between e075efd and f045054.

📒 Files selected for processing (16)
  • .github/actions/changes/action.yaml
  • .github/actions/setup-docker-buildx/action.yaml
  • .github/workflows/ci.yaml
  • docker/Dockerfile.safe-synthesizer-tasks
  • docs/safe-synthesizer/tutorials/safe-synthesizer-101.mdx
  • docs/set-up/config-reference.mdx
  • packages/nemo_platform_plugin/src/nemo_platform_plugin/jobs/file_manager.py
  • packages/nmp_common/tests/jobs/test_file_manager.py
  • plugins/nemo-safe-synthesizer/constraints.txt
  • plugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/api/v2/jobs/endpoints.py
  • plugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/config.py
  • plugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/skills/safe-synthesizer/SKILL.md
  • plugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/skills/safe-synthesizer/workflows/config-runs.md
  • plugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/skills/safe-synthesizer/workflows/diagnose.md
  • plugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/skills/safe-synthesizer/workflows/run.md
  • plugins/nemo-safe-synthesizer/tests/unit/test_jobs.py
✅ Files skipped from review due to trivial changes (4)
  • plugins/nemo-safe-synthesizer/constraints.txt
  • .github/actions/changes/action.yaml
  • plugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/skills/safe-synthesizer/SKILL.md
  • docs/set-up/config-reference.mdx
🚧 Files skipped from review as they are similar to previous changes (10)
  • plugins/nemo-safe-synthesizer/tests/unit/test_jobs.py
  • plugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/api/v2/jobs/endpoints.py
  • docs/safe-synthesizer/tutorials/safe-synthesizer-101.mdx
  • .github/actions/setup-docker-buildx/action.yaml
  • packages/nemo_platform_plugin/src/nemo_platform_plugin/jobs/file_manager.py
  • .github/workflows/ci.yaml
  • docker/Dockerfile.safe-synthesizer-tasks
  • plugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/skills/safe-synthesizer/workflows/run.md
  • packages/nmp_common/tests/jobs/test_file_manager.py
  • plugins/nemo-safe-synthesizer/src/nemo_safe_synthesizer_plugin/config.py

@tylersbray tylersbray 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.

🤌 (channeling my inner Rob)

Comment thread docs/safe-synthesizer/tutorials/safe-synthesizer-101.mdx
Comment thread packages/nmp_common/tests/jobs/test_file_manager.py Outdated
@mckornfield
mckornfield force-pushed the nss-container-runtime/mck branch 2 times, most recently from 037bc0f to 3b79508 Compare July 9, 2026 20:41
Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
@mckornfield
mckornfield force-pushed the nss-container-runtime/mck branch from 3b79508 to ab2e340 Compare July 9, 2026 21:13
@mckornfield
mckornfield enabled auto-merge July 9, 2026 21:13
@mckornfield
mckornfield added this pull request to the merge queue Jul 9, 2026
Merged via the queue into main with commit a5bbdba Jul 9, 2026
57 checks passed
@mckornfield
mckornfield deleted the nss-container-runtime/mck branch July 9, 2026 23:38
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