Uh oh!
There was an error while loading. Please reload this page.
[Bug] Add error handling for submitting jobs using pysqa - #961
Conversation
for more information, see https://pre-commit.ci
📝 WalkthroughWalkthroughError handling is added to the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR adds explicit error handling around pysqa job submission so that submission failures are captured and reflected in the cache files instead of leaving tasks in an indeterminate state.
Changes:
- Catch
ValueError/CalledProcessErrorfromqa.submit_job(...)during task submission. - On submission failure, persist the error into the task HDF5 file and rename the input file to an output file to signal completion.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## main #961 +/- ##
==========================================
+ Coverage 94.14% 94.15% +0.01%
==========================================
Files 39 39 Lines 2083 2089 +6 ==========================================
+ Hits 1961 1967 +6
Misses 122 122 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/executorlib/task_scheduler/file/spawner_pysqa.py`:
- Around line 96-102: The except block that catches (ValueError,
CalledProcessError) must clear any stale queue_id to avoid returning an old job
id; inside that except (in spawner_pysqa.py where queue_id is used after calling
qa.submit_job()), set queue_id = None (or another explicit sentinel) before
dumping the error and renaming files so the function returns a cleared value on
failure instead of a previous id.
🪄 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
Run ID: db7cdd8b-e2f6-4054-92d6-0e2797b048af
📒 Files selected for processing (2)
src/executorlib/task_scheduler/file/spawner_pysqa.pytests/unit/executor/test_flux_cluster.py
| except (ValueError, CalledProcessError) as error: | ||
| dump(file_name=file_name, data_dict={"error": error}) | ||
| file_name_out = os.path.splitext(file_name)[0][:-2] | ||
| os.rename(file_name_out + "_i.h5", file_name_out + "_o.h5") | ||
| else: | ||
| dump(file_name=file_name, data_dict={"queue_id": queue_id}) | ||
| return queue_id |
There was a problem hiding this comment.
Reset queue_id on submit failure to avoid returning stale IDs.
If file_name already contained a previous queue_id, a failed qa.submit_job() keeps that old value and the function returns it on Line 102. This can propagate an invalid job id to downstream cleanup/termination flows.
💡 Proposed fix
try:
queue_id = qa.submit_job(**submit_kwargs)
except (ValueError, CalledProcessError) as error:
+ queue_id = None
dump(file_name=file_name, data_dict={"error": error})
file_name_out = os.path.splitext(file_name)[0][:-2]
- os.rename(file_name_out + "_i.h5", file_name_out + "_o.h5")+ os.replace(file_name_out + "_i.h5", file_name_out + "_o.h5")
else:
dump(file_name=file_name, data_dict={"queue_id": queue_id})📝 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.
| except (ValueError, CalledProcessError) aserror: | |
| dump(file_name=file_name, data_dict={"error": error}) | |
| file_name_out=os.path.splitext(file_name)[0][:-2] | |
| os.rename(file_name_out+"_i.h5", file_name_out+"_o.h5") | |
| else: | |
| dump(file_name=file_name, data_dict={"queue_id": queue_id}) | |
| returnqueue_id | |
| except (ValueError, CalledProcessError) aserror: | |
| queue_id=None | |
| dump(file_name=file_name, data_dict={"error": error}) | |
| file_name_out=os.path.splitext(file_name)[0][:-2] | |
| os.replace(file_name_out+"_i.h5", file_name_out+"_o.h5") | |
| else: | |
| dump(file_name=file_name, data_dict={"queue_id": queue_id}) | |
| returnqueue_id |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/executorlib/task_scheduler/file/spawner_pysqa.py` around lines 96 - 102,
The except block that catches (ValueError, CalledProcessError) must clear any
stale queue_id to avoid returning an old job id; inside that except (in
spawner_pysqa.py where queue_id is used after calling qa.submit_job()), set
queue_id = None (or another explicit sentinel) before dumping the error and
renaming files so the function returns a cleared value on failure instead of a
previous id.
Uh oh!
There was an error while loading. Please reload this page.
Summary by CodeRabbit
Bug Fixes
Tests