Include recipe name in output directory naming#197
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughgenerate_minimal_sbatch_script now returns (rendered_script, recipe_name). recipe_name is extracted from the config path and threaded into SBATCH template rendering, output directory naming (now "{job_id}-{recipe_name}"), and per-job metadata (adds agg_nodes). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
4aa0bbf to
d840fa1
Compare
Unpack (script, recipe_name) tuple in 3 tests that broke after the function signature change. Made-with: Cursor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/srtctl/cli/submit.py`:
- Line 159: generate_minimal_sbatch_script was changed to return tuple[str, str]
but callers still expect a single string; update every call site (e.g., the
preview code in src/srtctl/cli/interactive.py and affected tests) to unpack the
returned tuple into two variables (e.g., script, meta) and use the first element
(script) where a string was expected, and adjust assertions/expectations in
tests to validate either the script string or the new second value as
appropriate; ensure any other usages of generate_minimal_sbatch_script are
located and updated to avoid type errors.
- Around line 209-211: The derived recipe_name using config_path.stem is
unstable for sweep jobs because config_path points to a temp file (e.g.,
srtctl_sweep_*); update the logic in submit.py where recipe_name is set so it
detects temp sweep filenames (e.g., startswith "srtctl_sweep_" or matches a
regex) and instead derives a stable name from the original recipe metadata or
input (for example use a provided original_config variable, a "name" field
inside the loaded config object, or an explicit CLI/args value) and only fall
back to config_path.stem when no metadata/original name is available; change the
assignment around recipe_name and config_path handling to implement this
fallback behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1a82ac6b-e942-49f5-b822-ed7b6d014344
📒 Files selected for processing (2)
src/srtctl/cli/submit.pysrc/srtctl/templates/job_script_minimal.j2
- Unpack (script, recipe_name) tuple in interactive.py preview_sbatch - Use config_path.stem for recipe_name normally; fall back to config.name when path is a sweep temp file (srtctl_sweep_*) to avoid random names Made-with: Cursor
0f11451 to
6896fe6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/srtctl/cli/interactive.py (1)
322-322:⚠️ Potential issue | 🟠 MajorAdd type narrowing before calling functions that require non-optional
Path.Inside the while loop,
config_pathcan be reassigned toPath | Nonein the "reselect" action (line 313), but subsequent branches use it without checking. These functions requirePath, not optional:
preview_sbatch()at line 322submit_sweep()at lines 341, 354confirm_submission()at line 346Add
if config_path is None: continuebefore each of these calls to narrow the type.Proposed fix
elif action == "preview": + if config_path is None: + continue preview_sbatch(config_path, config) elif action == "dry-run": from srtctl.cli.submit import submit_single, submit_sweep console.print() + if config_path is None: + continue if is_sweep: submit_sweep(config_path, dry_run=True) else: submit_single(config_path=config_path, dry_run=True) elif action == "submit": + if config_path is None: + continue if not confirm_submission(config_path, config, is_sweep): console.print("[yellow]Submission cancelled.[/]") continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/srtctl/cli/interactive.py` at line 322, config_path can be set to None in the "reselect" branch, but preview_sbatch, submit_sweep, and confirm_submission expect a non-optional Path; add an explicit runtime type-narrowing check before each call (e.g., before preview_sbatch(config_path, config) and before submit_sweep(...) and confirm_submission(...)) such as "if config_path is None: continue" so the functions only run when config_path is a Path; update calls in the while loop around preview_sbatch, submit_sweep, and confirm_submission to perform this guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/srtctl/cli/submit.py`:
- Around line 209-217: The recipe_name taken from path_stem or config.name must
be sanitized before use in paths; update the assignment logic around
path_stem/recipe_name so that after computing recipe_name you replace or remove
unsafe characters (e.g., strip path separators, replace whitespace with
underscores, and allow only alphanumerics, hyphen, and underscore) and collapse
consecutive invalid chars into a single separator. Apply the same sanitization
helper or inline logic to the other identical block referenced around lines
320-330 so both places use the same safe recipe_name for directory and sbatch
output path construction.
In `@tests/test_configs.py`:
- Around line 459-463: The test currently discards the second return value from
generate_minimal_sbatch_script and thus fails to verify the new recipe-based
naming; update existing tests (around the calls that currently do "script, _ =
generate_minimal_sbatch_script(...)") to capture the second return value
(recipe_name) and add assertions verifying recipe_name derivation for both
normal config paths and the srtctl_sweep_* fallback (e.g., assert recipe_name ==
config.name and that "%j-{recipe_name}" appears in the returned script). Locate
uses of generate_minimal_sbatch_script in tests/test_configs.py (and the other
mentioned locations) and replace the underscore with a variable name, then add
the two assertions per case.
---
Outside diff comments:
In `@src/srtctl/cli/interactive.py`:
- Line 322: config_path can be set to None in the "reselect" branch, but
preview_sbatch, submit_sweep, and confirm_submission expect a non-optional Path;
add an explicit runtime type-narrowing check before each call (e.g., before
preview_sbatch(config_path, config) and before submit_sweep(...) and
confirm_submission(...)) such as "if config_path is None: continue" so the
functions only run when config_path is a Path; update calls in the while loop
around preview_sbatch, submit_sweep, and confirm_submission to perform this
guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e47e577d-8220-4670-b203-2675a49fdedd
📒 Files selected for processing (3)
src/srtctl/cli/interactive.pysrc/srtctl/cli/submit.pytests/test_configs.py
| # Extract recipe name for output directory naming ({job_id}-{recipe_name}). | ||
| # Use filename stem normally; fall back to config.name when the path is a | ||
| # temp file (e.g. sweep flow generates "srtctl_sweep_*" temp configs). | ||
| path_stem = config_path.stem if config_path else "" | ||
| if path_stem.startswith("srtctl_sweep_") or not path_stem: | ||
| recipe_name = config.name or "unknown" | ||
| else: | ||
| recipe_name = path_stem | ||
|
|
There was a problem hiding this comment.
Sanitize recipe_name before using it in directory/script paths.
On Line [214], recipe_name may come from config.name. If that value includes /, spaces, or shell-sensitive characters, path layout and sbatch output path construction can break.
💡 Proposed fix
- path_stem = config_path.stem if config_path else ""
- if path_stem.startswith("srtctl_sweep_") or not path_stem:
- recipe_name = config.name or "unknown"
- else:
- recipe_name = path_stem
+ path_stem = config_path.stem
+ raw_recipe_name = (config.name or "unknown") if path_stem.startswith("srtctl_sweep_") or not path_stem else path_stem
+ recipe_name = re.sub(r"[^A-Za-z0-9._-]+", "-", raw_recipe_name).strip("-") or "unknown"Also applies to: 320-330
🧰 Tools
🪛 GitHub Actions: CI
[error] 213-216: Ruff SIM108: Use ternary operator instead of if-else block in submit.py. Replace the block starting at line 213 with a single line: recipe_name = config.name or "unknown" if path_stem.startswith("srtctl_sweep_") or not path_stem else path_stem. No fixes available.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/srtctl/cli/submit.py` around lines 209 - 217, The recipe_name taken from
path_stem or config.name must be sanitized before use in paths; update the
assignment logic around path_stem/recipe_name so that after computing
recipe_name you replace or remove unsafe characters (e.g., strip path
separators, replace whitespace with underscores, and allow only alphanumerics,
hyphen, and underscore) and collapse consecutive invalid chars into a single
separator. Apply the same sanitization helper or inline logic to the other
identical block referenced around lines 320-330 so both places use the same safe
recipe_name for directory and sbatch output path construction.
| script, _ = generate_minimal_sbatch_script( | ||
| config=config, | ||
| config_path=Path("/tmp/test.yaml"), | ||
| setup_script=None, | ||
| ) |
There was a problem hiding this comment.
Add assertions for recipe_name; current test updates don’t validate the new feature.
These changes only discard the second return value. The PR’s main behavior is recipe-based naming, so please add tests asserting recipe_name derivation (normal config path and srtctl_sweep_* fallback).
💡 Proposed test direction
- script, _ = generate_minimal_sbatch_script(
+ script, recipe_name = generate_minimal_sbatch_script(
config=config,
config_path=Path("/tmp/test.yaml"),
setup_script=None,
)
+ assert recipe_name == "test"
- script, _ = generate_minimal_sbatch_script(config, Path("/tmp/test.yaml"))
+ script, recipe_name = generate_minimal_sbatch_script(config, Path("/tmp/test.yaml"))
+ assert recipe_name == "test"def test_recipe_name_fallback_for_sweep_temp_config():
config = SrtConfig(
name="1p1d-dep-acc",
model=ModelConfig(path="/model", container="/container.sqsh", precision="fp8"),
resources=ResourceConfig(gpu_type="h100", gpus_per_node=8, agg_nodes=1),
)
script, recipe_name = generate_minimal_sbatch_script(config, Path("/tmp/srtctl_sweep_abcd.yaml"))
assert recipe_name == "1p1d-dep-acc"
assert "%j-1p1d-dep-acc" in scriptAs per coding guidelines: tests/**/*.py: Add a new test for every significant feature change.
Also applies to: 467-471, 854-854, 881-881
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_configs.py` around lines 459 - 463, The test currently discards
the second return value from generate_minimal_sbatch_script and thus fails to
verify the new recipe-based naming; update existing tests (around the calls that
currently do "script, _ = generate_minimal_sbatch_script(...)") to capture the
second return value (recipe_name) and add assertions verifying recipe_name
derivation for both normal config paths and the srtctl_sweep_* fallback (e.g.,
assert recipe_name == config.name and that "%j-{recipe_name}" appears in the
returned script). Locate uses of generate_minimal_sbatch_script in
tests/test_configs.py (and the other mentioned locations) and replace the
underscore with a variable name, then add the two assertions per case.
1 participant
Summary
Output directories now use
{job_id}-{recipe_name}format (e.g.,1049234-1p1d-dep-acc) instead of just{job_id}. This makes it much easier to identify which recipe produced which logs when browsing the outputs directory.generate_minimal_sbatch_scriptnow returnsrecipe_nameextracted from config path{job_id}-{recipe_name}patternjob_script_minimal.j2template updated to use recipe name in SLURM output pathagg_nodescountTest plan
{job_id}-{recipe_name}Summary by CodeRabbit
New Features
Tests