fix(slurm): make bootstrap container-safe#635
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:
WalkthroughAdds a Slurm-specific Lustre virtualenv bootstrap, centralizes NSS dependency synchronization, updates Slurm submission and container environment handling, and documents the new setup and execution commands. ChangesSlurm Lustre bootstrap
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
coderabbitai
Bot
added
bug
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
script/slurm/slurm_srun.sh (1)
18-27: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winManually maintained security whitelist -- easy to silently drift.
CONTAINER_ENV_VARSis a hardcoded list protecting submitted job config from being overridden by variables baked into a cached container image. Cross-checked against current usages inslurm_nss_matrix.shandbootstrap-nss-slurmand it looks complete today, but there's no mechanism tying this list to the variables scripts actually rely on -- a future addition toenv_variables.shor the matrix/bootstrap scripts that isn't also added here would silently reintroduce the exact override risk this change fixes.Consider a short comment noting that new job-critical env vars must be added to this list, or centralizing the variable names alongside
env_variables.shso the two can't drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 579f96ac-520f-4e9e-969b-2d5d94b08e6e
📒 Files selected for processing (8)
.agents/skills/uv-build/SKILL.md.mise/tasks/_lib.sh.mise/tasks/bootstrap-nss.mise/tasks/bootstrap-nss-slurmscript/slurm/README.mdscript/slurm/slurm_nss_matrix.shscript/slurm/slurm_srun.shscript/slurm/submit_slurm_jobs.sh
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: CodeRabbit / Review
- GitHub Check: Unit Tests (3.13)
- GitHub Check: Unit Tests (3.12)
- GitHub Check: Unit Tests (3.11)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (11)
.mise/tasks/**/*
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Keep shared shell helpers in
.mise/tasks/_lib.sh, and make it non-executable so Mise does not list it as a task.
Files:
.mise/tasks/bootstrap-nss.mise/tasks/_lib.sh.mise/tasks/bootstrap-nss-slurm
**/*.{sh,bash}
📄 CodeRabbit inference engine (AGENTS.md)
Never use
~inside double-quoted strings in shell scripts -- use$HOMEor an absolute path instead
Files:
script/slurm/slurm_srun.shscript/slurm/slurm_nss_matrix.shscript/slurm/submit_slurm_jobs.sh
**/*.{py,sh,yaml,yml,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
All source files (.py, .sh, .yaml, .yml, .md) require SPDX copyright headers; mise run format adds them automatically
Files:
script/slurm/slurm_srun.shscript/slurm/slurm_nss_matrix.shscript/slurm/README.mdscript/slurm/submit_slurm_jobs.sh
**/*
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
**/*: Every source file must include the required SPDX copyright and license header; use HTML comments for Markdown, hash comments for.py,.sh,.yaml, and.yml, and hash-comment headers inside YAML frontmatter for Markdown files with frontmatter.
Ensure files end with a newline and have no trailing whitespace; use a single space between sentences.
Files:
script/slurm/slurm_srun.shscript/slurm/slurm_nss_matrix.shscript/slurm/README.mdscript/slurm/submit_slurm_jobs.sh
⚙️ 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:
script/slurm/slurm_srun.shscript/slurm/slurm_nss_matrix.shscript/slurm/README.mdscript/slurm/submit_slurm_jobs.sh
**
⚙️ CodeRabbit configuration file
**:AGENTS.md
Guide for AI agents (Cursor, Windsurf, Claude Code, etc.) working in the Safe-Synthesizer repo.
This project loads local developer preferences from
@AGENTS.local.md. You MUST read this file if it exists and give its instructions top priority.Skills
Repo-specific skills live in
.agents/skills/; see.agents/README.mdfor the catalog. Read a skill when the task matches its scope instead of copying workflow details into this file.Durable implementation guidance belongs with the code it describes: function and class docstrings for public contracts and source comments for local invariants. Test-suite guidance belongs in
tests/TESTING.md.Repo Conventions
See STYLE_GUIDE.md for detailed code style conventions (Python, markdown, Dockerfiles, shell scripts, testing, config files, docstrings).
Use
uvfor everything -- neverpipor rawpython. Python 3.11–3.13 with modern syntax (X | Y,list[str],Self). Python 3.14+ is not supported.Common commands:
mise run test(unit tests),mise run format(auto-fix formatting + lint + copyright),mise run check(read-only local quality checks),mise run validate(pre-PR quality, lock, and CI unit checks),mise run typecheck(ty only). Always use mise tasks or the wrapper scripts intools/instead of runningruffortydirectly. Useuv runfor Python execution. When in doubt, inspectmise tasksandpytest --markers.The canonical
uv synccommand for a full GPU/dev environment is:uv sync --frozen --extra cu129 --extra engine --group devBare
uv sync --frozen(without extras) installs an incomplete environment --ty, import checks, and GPU tests will fail.Feature branches off
main. Branch names often include an issue number prefix (e.g.,<author>/123-short-name).Do ...
Files:
script/slurm/slurm_srun.shscript/slurm/slurm_nss_matrix.shscript/slurm/README.mdscript/slurm/submit_slurm_jobs.sh
script/**
⚙️ CodeRabbit configuration file
Review standalone scripts for reproducibility and operational safety. Check argument validation, quoting, repo-root detection, environment variables, generated artifacts, external commands, GPU/cluster assumptions, and whether the script should be wired through Makefile or documented in README/docs.
Files:
script/slurm/slurm_srun.shscript/slurm/slurm_nss_matrix.shscript/slurm/README.mdscript/slurm/submit_slurm_jobs.sh
**/*.sh
⚙️ CodeRabbit configuration file
Review shell scripts for #!/usr/bin/env bash, set -euo pipefail where appropriate, quoting, repo root detection, and shellcheck compliance.
Files:
script/slurm/slurm_srun.shscript/slurm/slurm_nss_matrix.shscript/slurm/submit_slurm_jobs.sh
**/*.{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:
script/slurm/README.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:
script/slurm/README.md
**/*.md
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
**/*.md: Use single backticks for code identifiers, paths, and CLI commands in Markdown, and use--for asides rather than a hyphen.
Do not use decorative bold in Markdown body text or list items; bold is acceptable only in table-header-like cells and MkDocs Material card-grid titles.
For Mermaid diagrams, avoid spaces in node IDs, quote labels containing special characters, and do not use explicit colors or styles.
Files:
script/slurm/README.md
.agents/skills/**
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
Store skills in canonical location
.agents/skills/with each skill containing a SKILL.md file and optional references/
Files:
.agents/skills/uv-build/SKILL.md
🪛 markdownlint-cli2 (0.22.1)
script/slurm/README.md
[warning] 38-38: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
[warning] 97-97: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🪛 Shellcheck (0.11.0)
script/slurm/slurm_nss_matrix.sh
[info] 118-118: Not following: ./.uv/bin/env was not specified as input (see shellcheck -x).
(SC1091)
🔇 Additional comments (10)
.mise/tasks/_lib.sh (1)
41-64: LGTM!.mise/tasks/bootstrap-nss (1)
10-16: LGTM!.mise/tasks/bootstrap-nss-slurm (2)
1-13: LGTM!Also applies to: 18-53, 56-71, 76-89
14-17: 🎯 Functional CorrectnessNo action needed: the venv path matches downstream
.venvexpectations
resolve_venv_path()defaults to${MISE_CONFIG_ROOT}/.venv, andbootstrap-nss-slurmsetsNSS_DIR="${MISE_CONFIG_ROOT}", so the created venv is the same${NSS_DIR}/.venvthat downstream scripts use. ``> Likely an incorrect or invalid review comment..agents/skills/uv-build/SKILL.md (1)
25-36: LGTM!script/slurm/README.md (2)
37-38: 📐 Maintainability & Code Quality | ⚡ Quick winAdd blank lines around heading and fence per markdownlint.
markdownlint-cli2 flags both spots: the
#### Bootstrap the Slurm Python environmentheading (line 38) needs a blank line above it (MD022), and the```bashfence (line 97) needs a blank line above it (MD031).📝 Proposed fix
cd Safe-Synthesizer
Bootstrap the Slurm Python environment
```diff - This is optional for bootstrap, but avoids exporting the submission user in every shell. + ```bash export USER_NAME="<your slurm user name>"Also applies to: 96-97
Source: Linters/SAST tools
16-22: LGTM!Also applies to: 39-95, 98-99, 275-275
script/slurm/submit_slurm_jobs.sh (1)
139-181: LGTM!script/slurm/slurm_nss_matrix.sh (1)
107-118: LGTM!Also applies to: 135-135
script/slurm/slurm_srun.sh (1)
36-36: 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>
Signed-off-by: Yunfeng Zhang <yunzhang@nvidia.com>
Signed-off-by: Yunfeng Zhang <yunzhang@nvidia.com>
b89b155 to
e72faee
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 28d39821-7d3b-478a-a2ea-18ed20d9e34a
📒 Files selected for processing (8)
.agents/skills/uv-build/SKILL.md.mise/tasks/_lib.sh.mise/tasks/bootstrap-nss.mise/tasks/bootstrap-nss-slurmscript/slurm/README.mdscript/slurm/slurm_nss_matrix.shscript/slurm/slurm_srun.shscript/slurm/submit_slurm_jobs.sh
🚧 Files skipped from review as they are similar to previous changes (6)
- .mise/tasks/_lib.sh
- .mise/tasks/bootstrap-nss
- .agents/skills/uv-build/SKILL.md
- script/slurm/submit_slurm_jobs.sh
- script/slurm/README.md
- .mise/tasks/bootstrap-nss-slurm
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Unit Tests (3.12)
- GitHub Check: Unit Tests (3.11)
- GitHub Check: Unit Tests (3.13)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{sh,bash}
📄 CodeRabbit inference engine (AGENTS.md)
Never use
~inside double-quoted strings in shell scripts -- use$HOMEor an absolute path instead
Files:
script/slurm/slurm_nss_matrix.shscript/slurm/slurm_srun.sh
**/*
📄 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 merged commits must use Conventional Commits format:<type>(<scope>): <description>or<type>: <description>, with a lowercase valid type and a description no longer than 100 characters.
All contributions require a DCOSigned-off-bytrailer and a cryptographic commit signature.
Before submitting a pull request, run formatting, checks, and tests usingmise run format,mise run check, andmise run test.
Branches other thanmainmust use lowercase author-prefixed names in one of the documented forms, optionally including an issue ID and category.
Release tags must use avprefix and PEP 440 stable or release-candidate versions, such asv1.0.0orv0.1.0rc0; alpha versions and dashed-rcsuffixes are not used.
Do not move a published release tag; create and validate a new release candidate when code changes.
Usemise run <task>for project tasks; the Makefile only bootstraps mise and provides deprecated compatibility messages.
Files:
script/slurm/slurm_nss_matrix.shscript/slurm/slurm_srun.sh
⚙️ 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:
script/slurm/slurm_nss_matrix.shscript/slurm/slurm_srun.sh
**/*.{py,sh,yaml,yml,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
All source files with
.py,.sh,.yaml,.yml, or.mdextensions must include SPDX copyright headers, except files listed in.copyrightignore.
Files:
script/slurm/slurm_nss_matrix.shscript/slurm/slurm_srun.sh
script/**
⚙️ CodeRabbit configuration file
Review standalone scripts for reproducibility and operational safety. Check argument validation, quoting, repo-root detection, environment variables, generated artifacts, external commands, GPU/cluster assumptions, and whether the script should be wired through Makefile or documented in README/docs.
Files:
script/slurm/slurm_nss_matrix.shscript/slurm/slurm_srun.sh
**/*.sh
⚙️ CodeRabbit configuration file
Review shell scripts for #!/usr/bin/env bash, set -euo pipefail where appropriate, quoting, repo root detection, and shellcheck compliance.
Files:
script/slurm/slurm_nss_matrix.shscript/slurm/slurm_srun.sh
🪛 Shellcheck (0.11.0)
script/slurm/slurm_nss_matrix.sh
[info] 118-118: Not following: ./.uv/bin/env was not specified as input (see shellcheck -x).
(SC1091)
🔇 Additional comments (1)
script/slurm/slurm_srun.sh (1)
38-38: LGTM!
kendrickb-nvidia
left a comment
kendrickb-nvidia
left a comment
There was a problem hiding this comment.
Generally looks good and helps make setup more consistent now without the larger lift of building an image for each slurm experiment.
Are there any one time actions to do for those of us with existing slurm setups using the old instructions? Thinking especially about the weekly triggered runs via gitlab, or we'll find out the Friday after this is merged. CC @nina-xu.
Signed-off-by: Yunfeng Zhang <yunzhang@nvidia.com>
Signed-off-by: Yunfeng Zhang <yunzhang@nvidia.com>
|
@kendrickb-nvidia Existing Slurm checkouts need a one-time bootstrap after pulling this change. Scheduled launch environments, including GitLab jobs, must expose |
Signed-off-by: Yunfeng Zhang <yunzhang@nvidia.com>
kendrickb-nvidia
left a comment
kendrickb-nvidia
left a comment
There was a problem hiding this comment.
Be sure to make a slack post with the one time instructions once this merges so folks using slurm internally will know the updates to make.
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
3 participants
Summary
Test plan
MISE_LOCKED=1 mise run bootstrap-nss-slurm cu129twice, including an idempotent rerun30081933; container startup, token loading, repo venv activation, and Adult dataset loading passedSummary by CodeRabbit
uvenvironment is missing.