[AIDynamo] Runtime discovery - #978
Conversation
📝 WalkthroughWalkthroughAI Dynamo now supports optional per-node startup commands, including containerized execution, runtime configuration localization, and integration with Slurm workload and single-sbatch command generation. ChangesAI Dynamo per-node startup
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
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 `@src/cloudai/workloads/ai_dynamo/ai_dynamo.sh`:
- Around line 635-656: Ensure the manifest-localization Python command fails the
surrounding shell flow when it cannot generate the environment file, before
executing source "$environment_file". Update the block around the embedded
Python script and its caller to use the existing failure-handling path
(including the Line 1410 handler), preventing an empty or partial generated file
from being sourced.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: cc5c2aca-e92f-414e-93d0-e55da2432380
📒 Files selected for processing (5)
doc/workloads/ai_dynamo.rstsrc/cloudai/workloads/ai_dynamo/ai_dynamo.pysrc/cloudai/workloads/ai_dynamo/ai_dynamo.shsrc/cloudai/workloads/ai_dynamo/slurm_command_gen_strategy.pytests/workloads/ai_dynamo/test_command_gen_strategy_slurm.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/cloudai/workloads/ai_dynamo/ai_dynamo.sh (3)
646-656: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winValidate manifest keys before generating shell code.
config_varis inserted directly into a sourced shell script. A malformed manifest key can inject arbitrary commands into the workload shell. Require^[A-Za-z_][A-Za-z0-9_]*$(and reject invalid keys) before emitting theexportstatement.🤖 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 `@src/cloudai/workloads/ai_dynamo/ai_dynamo.sh` around lines 646 - 656, Validate each config_var in the manifest loop before using it to generate the export statement, requiring the shell-variable pattern ^[A-Za-z_][A-Za-z0-9_]*$ and rejecting invalid keys. Keep valid-key processing unchanged, and ensure validation occurs before emitting any sourced shell code.
648-653: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPreserve restrictive permissions on localized configs.
write_text()creates the copy using the process umask, which can broaden access from a source mode such as0600to commonly0644. Preserve the source mode or explicitly create localized files with restrictive permissions, since these paths may reference credential-bearing configuration.🤖 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 `@src/cloudai/workloads/ai_dynamo/ai_dynamo.sh` around lines 648 - 653, Update the localized-config write flow around target.write_text in the replacement loop to preserve the source file’s restrictive permissions on the newly created target. Apply the source mode explicitly after creating the target, or create it with equivalent restrictive permissions, ensuring credential-bearing configs are not broadened by the process umask.
646-653: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPrevent localized-file name collisions.
The target name uses only the source basename components, so distinct files such as
/a/config.yamland/b/config.yamloverwrite each other when referenced by the same manifest. Detect duplicate targets and fail, or include a stable source-path discriminator in the filename before writing.🤖 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 `@src/cloudai/workloads/ai_dynamo/ai_dynamo.sh` around lines 646 - 653, Update the manifest-processing loop to prevent collisions in the target path derived from source.stem and node_name. Track generated targets and fail clearly when distinct source files map to the same target, or add a stable discriminator based on the source path; preserve the existing content replacement and write behavior for unique targets.
🤖 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 `@src/cloudai/workloads/ai_dynamo/ai_dynamo.sh`:
- Around line 646-656: Validate each config_var in the manifest loop before
using it to generate the export statement, requiring the shell-variable pattern
^[A-Za-z_][A-Za-z0-9_]*$ and rejecting invalid keys. Keep valid-key processing
unchanged, and ensure validation occurs before emitting any sourced shell code.
- Around line 648-653: Update the localized-config write flow around
target.write_text in the replacement loop to preserve the source file’s
restrictive permissions on the newly created target. Apply the source mode
explicitly after creating the target, or create it with equivalent restrictive
permissions, ensuring credential-bearing configs are not broadened by the
process umask.
- Around line 646-653: Update the manifest-processing loop to prevent collisions
in the target path derived from source.stem and node_name. Track generated
targets and fail clearly when distinct source files map to the same target, or
add a stable discriminator based on the source path; preserve the existing
content replacement and write behavior for unique targets.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: f578d842-7b3b-4d37-bf29-ae08ab3f626a
📒 Files selected for processing (1)
src/cloudai/workloads/ai_dynamo/ai_dynamo.sh
Summary
Enable runtime environment discovery for AIDynamo for file-based config files
Test Plan
Additional Notes