Repository navigation
Revalidate NFS lookup before reporting a Slurm log missing (OR-631) - #799
devin-ai-integration[bot] wants to merge 3 commits into
Conversation
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
[Medium impact] The PR appears safe to merge. SummaryThe helper lists the run directory and retries before reporting a missing log.
Reviews (2) · Last reviewed commit: "Assert the retry lists the run directory..." · Reviewed by Greptile |
| try: | ||
| os.listdir(os.path.dirname(path)) | ||
| file = open(path, "rb", buffering=0) | ||
| except FileNotFoundError: | ||
| return {"missing": True} |
There was a problem hiding this comment.
The new retry-success path has no automated test. The existing missing-log test deletes the file, so both opens fail; it would still pass if the retry were removed. That leaves this fix unprotected against a later change.
Add a test that makes the first open raise FileNotFoundError, checks that os.listdir receives the run directory, and lets the second open read a real log. Assert that the returned bytes match the log.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/jobs/ssh/helper.py
Line: 99-103
Comment:
**Successful retry has no test**
The new retry-success path has no automated test. The existing missing-log test deletes the file, so both opens fail; it would still pass if the retry were removed. That leaves this fix unprotected against a later change.
Add a test that makes the first `open` raise `FileNotFoundError`, checks that `os.listdir` receives the run directory, and lets the second `open` read a real log. Assert that the returned bytes match the log.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
@greptileai review |
|
superseded by #862. |
|
Noted. One caveat: #862's description lists "OR-631: NFS log lookup unchanged". If |
Fixes OR-631.
Summary
log_chunk(src/jobs/ssh/helper.py) runs on the login node. Its first probe of<run>/log, made before the compute node creates it, leaves an NFS negative-lookup cache entry. The cache can last for the directory's attribute-cache window (up to 60 s by default). A job that fails quickly creates and writesloginside that window, so everycollectstill gets{"missing": true}. Thenrun_slurm's 20 s terminal drain insupervise.rstimes out and the run ends with an empty local log, even though the remote log has content.openraisesFileNotFoundError, list the run directory once and retry the open. On Linux NFS, opening a directory revalidates its attributes, which drops the stale negative entries. If the log really is absent, the helper still returnsmissing.Test plan
main(Linux, local Slurm 21.08 with/scratchNFS-mounted from the same host, OpenResearch 0.2.21). I ranorx exp run … --backend slurmon atrain.pythat fails an assert right away. The run ended Failed and the remote log held 330 bytes with the traceback.orx logssaid no log was captured and the dashboard log pane was empty. The supervisor log showedRemote log is not available yetuntil the drain gave up.orx logsand the dashboard log pane both show the full 330-byte traceback.openstill failed, whileos.listdir(dir)followed byopensucceeded.scripts/test_ssh_helper.pygainstest_stale_missing_log_is_revalidated_by_listing_the_run_directory: the firstopenhits a cached miss,listdirgets the run directory, and the retry returns the log bytes. It fails on main and passes here.cargo build --locked,cargo fmt --checkandcargo clippy --locked -- -D warningspass. Incargo test --locked, 1276 tests passed and 3 failed inlocal::projects::tests. They fail because this VM has a global git identity that differs from the one those tests expect; the failures are unrelated to this change.Automation improvements
CODEX_AUTH_JSONis stale (refresh token already used, 401), so Codex live preflight could not run. Refresh the secret from a new ChatGPT Codex login so both agents can be verified./srv/export→/scratch, withHWLOC_SYNTHETICforslurmd) to the VM blueprint. That would turn about an hour of Slurm-bug setup into minutes.Link to Devin session: https://app.devin.ai/sessions/37aa99ba59484e86bec3eb67a115cd4e
Open in Devin Desktop: https://app.devin.ai/desktop/session/37aa99ba59484e86bec3eb67a115cd4e?variant=devin