style: fix Ruff 0.8.0 UP031 errors #510
Conversation
Generated by the task: njzjz-bot/njzjz-bot#12.
for more information, see https://pre-commit.ci
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request includes updates across various files, primarily focusing on modernizing string formatting from older methods to f-strings for improved readability. Changes are made to error message formatting in multiple classes and methods, including Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (14)
dpdispatcher/machines/shell.py (2)
Line range hint
99-116: Consider removing commented-out codeThere are large blocks of commented-out code that appear to be old implementations. These should be removed as they can be retrieved from version control if needed.
45-48: Consider extracting common error handling logicThe error handling pattern is duplicated in both
do_submitandcheck_statusmethods. Consider extracting this into a helper method to improve maintainability.+ def _handle_command_error(self, cmd: str, ret: int, stderr) -> None: + err_str = stderr.read().decode("utf-8") + raise RuntimeError( + f"status command {cmd} fails to execute\nerror message:{err_str}\nreturn code {ret}\n" + ) def do_submit(self, job): # ... if ret != 0: - err_str = stderr.read().decode("utf-8") - raise RuntimeError( - f"status command {cmd} fails to execute\nerror message:{err_str}\nreturn code {ret}\n" - ) + self._handle_command_error(cmd, ret, stderr) # ... def check_status(self, job): # ... if ret != 0: - err_str = stderr.read().decode("utf-8") - raise RuntimeError( - f"status command {cmd} fails to execute\nerror message:{err_str}\nreturn code {ret}\n" - ) + self._handle_command_error(cmd, ret, stderr) # ...Also applies to: 81-84
dpdispatcher/base_context.py (1)
Line range hint
105-111: Convert to f-string for better readabilityWhile the current change moves away from %-formatting, it doesn't fully address the Ruff UP031 rule which recommends using f-strings. Let's modernize this further.
- "Get error code {} in calling {} with job: {} . message: {}".format( - exit_status, - cmd, - self.submission.submission_hash, - stderr.read().decode("utf-8"), - ) + f"Get error code {exit_status} in calling {cmd} with job: {self.submission.submission_hash} . message: {stderr.read().decode('utf-8')}"dpdispatcher/utils/utils.py (1)
194-194: LGTM! Consider adding retry count to the error message.The conversion to f-string is correct and aligns with modern Python practices. To make debugging easier, consider including the max retry limit in the error message.
- f"Failed to run {func.__name__} for {current_retry} times" + f"Failed to run {func.__name__} after {current_retry}/{max_retry} attempts"dpdispatcher/machines/JH_UniScheduler.py (2)
Line range hint
40-44: Consider modernizing additional string formattingThere are more opportunities to modernize string formatting in this file. For example, the script header generation could be simplified using f-strings.
- script_header_dict = { - "JH_UniScheduler_nodes_line": f"#JSUB -n {resources.number_node * resources.cpu_per_node}", - "JH_UniScheduler_ptile_line": f"#JSUB -R 'span[ptile={resources.cpu_per_node}]'", - "JH_UniScheduler_partition_line": f"#JSUB -q {resources.queue_name}", - } + JH_UniScheduler_script_header = f"""\ + #!/bin/bash -l + #JSUB -e %J.err + #JSUB -o %J.out + #JSUB -n {resources.number_node * resources.cpu_per_node} + #JSUB -R 'span[ptile={resources.cpu_per_node}]' + #JSUB -q {resources.queue_name} + {custom_gpu_line if custom_gpu_line else f'#JSUB -gpgpu {resources.gpu_per_node}'} + """
Line range hint
86-90: Consider modernizing string concatenationThe command construction in
do_submitcould be simplified using f-strings.- stdin, stdout, stderr = self.context.block_checkcall( - "cd {} && {} {}".format( - shlex.quote(self.context.remote_root), - "jsub < ", - shlex.quote(script_file_name), - ) - ) + stdin, stdout, stderr = self.context.block_checkcall( + f"cd {shlex.quote(self.context.remote_root)} && jsub < {shlex.quote(script_file_name)}" + )dpdispatcher/machines/distributed_shell.py (1)
Line range hint
184-202: Consider standardizing string formatting across the fileWhile the current changes correctly address the immediate UP031 violations, there are other instances in the file (e.g., submit_command construction) that still use the .format() method. Consider creating a follow-up PR to standardize all string formatting to f-strings for consistency.
dpdispatcher/machines/pbs.py (1)
140-142: LGTM: Improved string formatting with refactor opportunityThe conversion from %-formatting to f-string improves readability. However, there's an opportunity to reduce code duplication between PBS and Torque classes.
Consider extracting the common error handling logic to a shared method in the PBS base class, since both PBS and Torque classes use identical error messages and handling:
class PBS(Machine): + def _handle_qstat_error(self, command: str, ret: int, err_str: str) -> None: + if "qstat: Unknown Job Id" in err_str or "Job has finished" in err_str: + if self.check_finish_tag(job=job): + return JobStatus.finished + else: + return JobStatus.terminated + else: + raise RuntimeError( + f"status command {command} fails to execute. erro info: {err_str} return code {ret}" + ) def check_status(self, job): # ... existing code ... if ret != 0: - if "qstat: Unknown Job Id" in err_str or "Job has finished" in err_str: - if self.check_finish_tag(job=job): - return JobStatus.finished - else: - return JobStatus.terminated - else: - raise RuntimeError( - f"status command {command} fails to execute. erro info: {err_str} return code {ret}" - ) + return self._handle_qstat_error(command, ret, err_str)This would eliminate the duplicated error handling code in the Torque class.
dpdispatcher/machines/slurm.py (2)
146-150: Consider improving multi-line f-string formattingWhile the conversion to f-strings is correct, the multi-line error message could be more readable.
Consider this format:
- f"status command {command} fails to execute." - f"job_id:{job_id} \n error message:{err_str}\n return code {ret}\n" + f"""status command {command} fails to execute. + job_id: {job_id} + error message: {err_str} + return code: {ret} + """
254-256: Maintain consistent string formatting styleThe code uses % formatting while the rest of the file is being modernized to use f-strings.
Consider using f-string for consistency:
- return super().gen_script_header(job) + "\n#SBATCH --array=0-%s" % ( - math.ceil(len(job.job_task_list) / slurm_job_size) - 1 - ) + return super().gen_script_header(job) + f"\n#SBATCH --array=0-{math.ceil(len(job.job_task_list) / slurm_job_size) - 1}"dpdispatcher/contexts/ssh_context.py (4)
Line range hint
75-82: Remove commented-out code blocksThese commented code blocks should be removed as they add unnecessary noise to the codebase. The old implementations are preserved in version control if needed for reference.
Also applies to: 117-125, 402-411
Line range hint
271-279: Consider enforcing key-based authenticationWhile password authentication is marked as deprecated in the documentation, it's still fully implemented. Consider:
- Adding a deprecation warning when password authentication is used
- Setting a timeline for removing password authentication
- Enforcing key-based authentication as the only method
Line range hint
853-868: Enhance security of temporary file handlingThe temporary file operations for tar commands could be more secure:
- Use
mkstempfor creating temporary files with proper permissions- Ensure cleanup in case of errors using try-finally blocks
- file_list_file = pathlib.PurePath( - os.path.join(self.remote_root, f".tmp_tar_{uuid.uuid4()}") - ).as_posix() - self.write_file(file_list_file, "\n".join(files)) + import tempfile + temp_fd, file_list_file = tempfile.mkstemp(prefix='.tmp_tar_', dir=self.remote_root) + try: + with os.fdopen(temp_fd, 'w') as temp_file: + temp_file.write("\n".join(files)) + # ... rest of the code ... + finally: + if self.check_file_exists(file_list_file): + self.sftp.remove(file_list_file)
Line range hint
873-879: Improve error handling specificityThe error handling for file not found could be more specific and informative:
- if "No such file or directory" in str(e): - raise FileNotFoundError( - "Backward files do not exist in the remote directory." - ) from e + if "No such file or directory" in str(e): + missing_files = [f for f in files if not self.check_file_exists(f)] + raise FileNotFoundError( + f"The following files do not exist in the remote directory: {', '.join(missing_files)}" + ) from e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
doc/conf.py(1 hunks)dpdispatcher/base_context.py(1 hunks)dpdispatcher/contexts/ssh_context.py(2 hunks)dpdispatcher/machines/JH_UniScheduler.py(1 hunks)dpdispatcher/machines/distributed_shell.py(2 hunks)dpdispatcher/machines/lsf.py(1 hunks)dpdispatcher/machines/pbs.py(2 hunks)dpdispatcher/machines/shell.py(2 hunks)dpdispatcher/machines/slurm.py(5 hunks)dpdispatcher/utils/utils.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- doc/conf.py
🔇 Additional comments (11)
dpdispatcher/machines/shell.py (2)
46-46: LGTM! String formatting modernized correctly
The change from %-formatting to f-string improves readability while maintaining the same error information.
82-82: LGTM! String formatting modernized correctly
The change from %-formatting to f-string improves readability while maintaining the same error information.
dpdispatcher/machines/JH_UniScheduler.py (1)
108-108: LGTM! String formatting modernized correctly.
The error message has been properly converted to use f-string formatting, which is more readable and efficient than older string formatting methods.
dpdispatcher/machines/distributed_shell.py (2)
184-184: LGTM: Appropriate f-string conversion
The error message formatting has been correctly modernized to use an f-string, which aligns with the Ruff UP031 rule requirements.
202-202: LGTM: Appropriate f-string conversion
The error message formatting has been correctly modernized to use an f-string, which aligns with the Ruff UP031 rule requirements.
dpdispatcher/machines/lsf.py (2)
132-132: LGTM! Clean conversion to f-string format.
The conversion from %-formatting to f-string improves readability while maintaining the same error information. The error message properly includes all necessary debugging details (return code, job hash, and error message).
Line range hint 1-232: Verify remaining string formatting patterns.
Let's check if there are any remaining instances of old-style string formatting that should be updated for consistency.
✅ Verification successful
No string formatting inconsistencies found
The search results show that:
- The
%Joccurrences are part of LSF directives in the script header template and are required by the LSF system - The only
.format()calls use modern formatting with named parameters (**script_header_dict) or positional parameters ({}) - No old-style
%string formatting is used in the actual code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for remaining %-formatting patterns in the file
# Excluding the lsf_script_header_template which uses % for LSF directives
rg '%(?!\{)[^%]*%[^{]' dpdispatcher/machines/lsf.py
Length of output: 275
Script:
#!/bin/bash
# Search for string formatting patterns in the file
# Using simpler patterns to catch different formatting styles
rg -U '%.+[^{]' dpdispatcher/machines/lsf.py
# Also search for format() method calls for completeness
rg '\.format\(' dpdispatcher/machines/lsf.py
Length of output: 254
dpdispatcher/machines/pbs.py (1)
90-92: LGTM: Improved string formatting
The conversion from %-formatting to f-string improves readability while maintaining the same error information.
dpdispatcher/machines/slurm.py (2)
Line range hint 100-111: LGTM: Improved error message formatting
The conversion to f-strings enhances readability while maintaining the same error information.
332-336: Similar multi-line f-string formatting can be improved
This section has the same formatting opportunity as discussed in the base class's check_status method.
dpdispatcher/contexts/ssh_context.py (1)
93-94: LGTM: Improved string formatting
The conversion to f-string improves readability and follows modern Python best practices.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #510 +/- ##
===========================================
- Coverage 60.19% 47.78% -12.42%
===========================================
Files 39 39
Lines 3859 3859
===========================================
- Hits 2323 1844 -479
- Misses 1536 2015 +479 ☔ View full report in Codecov by Sentry. |
Generated by the task: njzjz-bot/njzjz-bot#12.
Summary by CodeRabbit
Release Notes
Bug Fixes
Chores
Documentation