Move scheduler to standalone #758
Conversation
for more information, see https://pre-commit.ci
WalkthroughA new module, Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Scheduler (standalone)
participant pysqa.QueueAdapter
participant Subprocess
Caller->>Scheduler (standalone): terminate_with_pysqa(queue_id, config_dir, backend)
Scheduler (standalone)->>pysqa.QueueAdapter: Initialize with config_dir, backend, execute_command
Scheduler (standalone)->>pysqa.QueueAdapter: get_status(queue_id)
pysqa.QueueAdapter-->>Scheduler (standalone): status
alt Job is active
Scheduler (standalone)->>pysqa.QueueAdapter: delete_job(queue_id)
pysqa.QueueAdapter->>Subprocess: pysqa_execute_command(...)
Subprocess-->>pysqa.QueueAdapter: output/error
pysqa.QueueAdapter-->>Scheduler (standalone): result
end
Scheduler (standalone)-->>Caller: (done)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #758 +/- ##
=======================================
Coverage ? 97.43%
=======================================
Files ? 32
Lines ? 1403
Branches ? 0
=======================================
Hits ? 1367
Misses ? 36
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
🧹 Nitpick comments (1)
executorlib/standalone/scheduler.py (1)
53-61: Simplify shell parameter logic for better clarity.The current logic for handling the
shellparameter is complex and potentially confusing. The automatic detectionshell=not isinstance(commands, list)at line 60 may not align with the explicitshellparameter passed by the caller.Consider simplifying the logic:
def pysqa_execute_command( commands: Union[str, list[str]], working_directory: Optional[str] = None, split_output: bool = True, shell: bool = False, ) -> Union[str, list[str]]: """...""" - if shell and isinstance(commands, list): - commands = " ".join(commands) + # Convert list to string if shell mode is requested + if shell and isinstance(commands, list): + commands = " ".join(commands) + out = subprocess.check_output( commands, cwd=working_directory, stderr=subprocess.STDOUT, universal_newlines=True, - shell=not isinstance(commands, list), + shell=shell, )This makes the shell parameter behavior more predictable and explicit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
executorlib/standalone/scheduler.py(1 hunks)executorlib/task_scheduler/file/queue_spawner.py(2 hunks)executorlib/task_scheduler/file/task_scheduler.py(1 hunks)tests/test_fluxclusterexecutor.py(1 hunks)tests/test_interactive_slurmspawner.py(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: unittest_mpich (ubuntu-latest, 3.11)
- GitHub Check: unittest_openmpi (macos-latest, 3.13)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.11)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.13)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.12)
- GitHub Check: unittest_mpich (ubuntu-latest, 3.13)
- GitHub Check: unittest_mpich (ubuntu-latest, 3.12)
- GitHub Check: unittest_mpich (macos-latest, 3.13)
- GitHub Check: notebooks_integration
- GitHub Check: unittest_flux_openmpi
- GitHub Check: unittest_slurm_mpich
- GitHub Check: unittest_win
- GitHub Check: unittest_flux_mpich
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-mpich.yml)
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-openmpi.yml)
🔇 Additional comments (8)
tests/test_fluxclusterexecutor.py (1)
14-15: Import changes look correct for the refactoring.The updated import correctly reflects the extraction of
terminate_with_pysqato the new standalone scheduler module while keeping the other functions in their original location.tests/test_interactive_slurmspawner.py (2)
5-5: Import update correctly reflects the refactoring.The import has been properly updated to use
pysqa_execute_commandfrom the new standalone scheduler module.
17-17: Function calls consistently updated.All test method calls have been properly updated to use the new function name
pysqa_execute_commandinstead of the previous_pysqa_execute_command.Also applies to: 28-28, 40-40
executorlib/task_scheduler/file/task_scheduler.py (1)
20-21: Import restructuring aligns with the refactoring objective.The split import correctly reflects the extraction of
terminate_with_pysqato the standalone scheduler module while keepingexecute_with_pysqain its current location. This maintains the logical separation of concerns.executorlib/task_scheduler/file/queue_spawner.py (3)
2-2: Import cleanup is appropriate.Correctly removed unused imports (
contextlib,subprocess,Union) that are no longer needed after moving the functions to the standalone module.
7-7: Import from new standalone module is correct.Properly imports the extracted functions from the new
executorlib.standalone.schedulermodule.
45-45: Function call updated correctly.The call to
pysqa_execute_command(without underscore prefix) correctly uses the imported function from the standalone module.executorlib/standalone/scheduler.py (1)
8-30: Function implementation looks solid.The
terminate_with_pysqafunction correctly handles job termination with proper status checking and error suppression. The logic is defensive and appropriate for job queue management.
| def pysqa_execute_command( | ||
| commands: str, | ||
| working_directory: Optional[str] = None, | ||
| split_output: bool = True, | ||
| shell: bool = False, | ||
| error_filename: str = "pysqa.err", | ||
| ) -> Union[str, list[str]]: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix type annotation and parameter usage inconsistency.
The function signature declares commands: str but the implementation handles both strings and lists. Additionally, the error_filename parameter is documented but never used in the implementation.
def pysqa_execute_command(
- commands: str,
+ commands: Union[str, list[str]],
working_directory: Optional[str] = None,
split_output: bool = True,
shell: bool = False,
- error_filename: str = "pysqa.err",
) -> Union[str, list[str]]:If error_filename is intended for future use, consider adding a comment explaining its purpose or implement its usage.
📝 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.
| def pysqa_execute_command( | |
| commands: str, | |
| working_directory: Optional[str] = None, | |
| split_output: bool = True, | |
| shell: bool = False, | |
| error_filename: str = "pysqa.err", | |
| ) -> Union[str, list[str]]: | |
| def pysqa_execute_command( | |
| commands: Union[str, list[str]], | |
| working_directory: Optional[str] = None, | |
| split_output: bool = True, | |
| shell: bool = False, | |
| ) -> Union[str, list[str]]: | |
| # ... |
🤖 Prompt for AI Agents
In executorlib/standalone/scheduler.py around lines 32 to 38, the function
pysqa_execute_command has a type annotation for commands as str, but the
implementation supports both str and list types, causing inconsistency. Also,
the error_filename parameter is declared but unused. Fix this by updating the
commands parameter type annotation to accept both str and list[str], and either
implement the usage of error_filename in the function or add a comment
explaining its intended future use.
Summary by CodeRabbit
New Features
Refactor
Tests