restart: resolve entries under --restartdir#486
Conversation
📝 WalkthroughWalkthrough
ChangesRestart directory handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bin/mana_restart`:
- Around line 33-34: Update the dmtcp_flags handling around the existing default
--restartdir append so ./ is added only when the user has not already supplied
--restartdir. Preserve the existing missing-value validation for a user-provided
flag, ensuring a bare --restartdir is not satisfied by the default value and
reports the intended missing-value error.
In `@ci/unit-test.sh`:
- Around line 25-26: Update the regression test invocation in unit-test.sh to
reference test-mana-restart.py through the existing SCRIPT_DIR variable,
ensuring it resolves correctly from the mpi-proxy-split/unit-test working
directory while preserving the current verbose execution and failure handling.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fbf4fd85-a761-4f3a-806e-b5d20f8b24ff
📒 Files selected for processing (3)
bin/mana_restartci/test-mana-restart.pyci/unit-test.sh
| dmtcp_flags.extend(["--restartdir", "./"]) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Conditionally append --restartdir to avoid duplicating the argument.
Unconditionally extending dmtcp_flags introduces a bug when the user explicitly provides --restartdir. If the user passes --restartdir /path, this logic adds a second --restartdir ./ to the end of the argument list, which will be erroneously forwarded to the lower half.
Furthermore, if the user provides --restartdir without a value, the added flag (--restartdir) is treated as the missing value, bypassing the missing-value check entirely and failing with an incorrect "Restart directory doesn't exist" error instead.
🐛 Proposed fix to apply the default conditionally
- dmtcp_flags.extend(["--restartdir", "./"])
+if "--restartdir" not in dmtcp_flags:
+ dmtcp_flags.extend(["--restartdir", "./"])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| dmtcp_flags.extend(["--restartdir", "./"]) | |
| if "--restartdir" not in dmtcp_flags: | |
| dmtcp_flags.extend(["--restartdir", "./"]) |
🤖 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 `@bin/mana_restart` around lines 33 - 34, Update the dmtcp_flags handling
around the existing default --restartdir append so ./ is added only when the
user has not already supplied --restartdir. Preserve the existing missing-value
validation for a user-provided flag, ensuring a bare --restartdir is not
satisfied by the default value and reports the intended missing-value error.
| # Additional wrapper regression test. | ||
| python3 ci/test-mana-restart.py -v || exit 1 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Fix the path to the test script.
At this point in the script, the working directory is mpi-proxy-split/unit-test, so the relative path ci/test-mana-restart.py does not exist. This will cause the pipeline to fail with a "No such file or directory" error, which likely prevented CI from catching the bug in bin/mana_restart.
Use the $SCRIPT_DIR variable to correctly reference the script regardless of the current working directory.
🐛 Proposed fix
# Additional wrapper regression test.
-python3 ci/test-mana-restart.py -v || exit 1
+python3 "$SCRIPT_DIR/test-mana-restart.py" -v || exit 1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Additional wrapper regression test. | |
| python3 ci/test-mana-restart.py -v || exit 1 | |
| # Additional wrapper regression test. | |
| python3 "$SCRIPT_DIR/test-mana-restart.py" -v || exit 1 |
🤖 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 `@ci/unit-test.sh` around lines 25 - 26, Update the regression test invocation
in unit-test.sh to reference test-mana-restart.py through the existing
SCRIPT_DIR variable, ensuring it resolves correctly from the
mpi-proxy-split/unit-test working directory while preserving the current verbose
execution and failure handling.
1 participant
Refs #485
Background
While validating distributed restart from two MPICH/Hydra nodes, I found that
mana_restart --restartdirdepends on the caller's current working directory. The wrapper lists rank entries from the supplied restart directory, but later opens the returned names as relative paths. My test environment therefore had to launch each restart process from inside the checkpoint root.This PR removes that working-directory dependency and adds focused regression coverage.
What changes
--restartdiras a normal two-argument option.--restartdirhas no value.~and normalize the supplied path to an absolute directory..tmpfiles through the full rank path.--restartdir DIRpair after the other DMTCP options. The lower half replaces this pair with the rank-specific checkpoint image, so this keeps the resulting image in a deterministic final position.Why this is useful
A restart directory should identify the checkpoint tree completely. Users should not also have to change into that directory before invoking
mana_restart, especially when each MPI rank is started remotely by Hydra.Validation
The new tests cover:
.tmpdetection;--restartdirvalue;Scope
This PR intentionally does not change coordinator lifecycle, loader debug-symbol discovery, DMTCP source, MPI configuration, or cluster provisioning. Those topics are discussed separately in the umbrella issue.
Summary by CodeRabbit
Bug Fixes
Tests