handle more job status on SGE system #491
Conversation
- add option: "strategy": {"customized_script_header_template_file": ""},
- add option: `sge_pe_name`
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Update pbs.py
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
put `sge_pe_name` in `kwargs`
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
WalkthroughWalkthroughThe changes to the Changes
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Additional context usedGitHub Check: codecov/patch
Additional comments not posted (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #491 +/- ##
===========================================
- Coverage 60.25% 47.78% -12.47%
===========================================
Files 39 39
Lines 3857 3859 +2
===========================================
- Hits 2324 1844 -480
- Misses 1533 2015 +482 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
dpdispatcher/machines/pbs.py (1)
264-264: Improved job status handling with additional states.The added comment and new status words enhance the method's ability to handle different job states. This is a good improvement that makes the code more robust.
Consider adding a brief explanation for each new status word in a comment, to improve code maintainability. For example:
# Status word meanings: # qw: queued and waiting # hqw: hold, queued, and waiting # t: transferring # r: running # Rr: running (restart) # Eqw: error, queued, and waiting # dr: deleted and running # dt: deleted and transferringAlso applies to: 298-303
| if status_word in ["qw", "hqw", "t"]: | ||
| return JobStatus.waiting | ||
| elif status_word in ["r"]: | ||
| elif status_word in ["r", "Rr"]: | ||
| return JobStatus.running | ||
| elif status_word in ["Eqw", "dr", "dt"]: | ||
| return JobStatus.terminated |
There was a problem hiding this comment.
Consider refactoring status checks for better maintainability.
The current structure of multiple if-elif statements might become hard to maintain if more status words are added in the future. Consider refactoring this part of the code to use a dictionary for status mapping, which would make it more maintainable and easier to extend.
Here's a suggested refactoring:
STATUS_MAPPING = {
'qw': JobStatus.waiting,
'hqw': JobStatus.waiting,
't': JobStatus.waiting,
'r': JobStatus.running,
'Rr': JobStatus.running,
'Eqw': JobStatus.terminated,
'dr': JobStatus.terminated,
'dt': JobStatus.terminated,
}
def check_status(self, job):
# ... (previous code remains the same)
status_word = status_line.split()[4]
return STATUS_MAPPING.get(status_word, JobStatus.unknown)This approach centralizes the status mapping, making it easier to add or modify statuses in the future without changing the method's logic.
Tools
GitHub Check: codecov/patch
[warning] 298-298: dpdispatcher/machines/pbs.py#L298
Added line #L298 was not covered by tests
[warning] 300-300: dpdispatcher/machines/pbs.py#L300
Added line #L300 was not covered by tests
[warning] 302-303: dpdispatcher/machines/pbs.py#L302-L303
Added lines #L302 - L303 were not covered by tests
Summary by CodeRabbit