Upgrade terminate function #715
Conversation
for more information, see https://pre-commit.ci
|
Warning Rate limit exceeded@jan-janssen has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
""" WalkthroughA new boolean parameter, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Executor (Flux/Slurm)
participant InputCheck
participant FileExecutor
participant QueueSpawner
User->>Executor: Instantiate with terminate_tasks_on_shutdown
Executor->>InputCheck: Validate terminate_tasks_on_shutdown
Executor->>FileExecutor: create_file_executor with terminate_tasks_on_shutdown
FileExecutor->>QueueSpawner: Select terminate_function internally
User->>Executor: Shutdown
Executor->>FileExecutor: Shutdown, calls terminate_function if set
FileExecutor->>QueueSpawner: terminate_with_pysqa (if applicable)
Possibly related PRs
Poem
✨ Finishing Touches
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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #715 +/- ##
==========================================
- Coverage 96.94% 96.91% -0.03%
==========================================
Files 30 30
Lines 1343 1363 +20
==========================================
+ Hits 1302 1321 +19
- Misses 41 42 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
for more information, see https://pre-commit.ci
![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 (2)
executorlib/standalone/inputcheck.py (2)
210-214: Fix missing space in error message.The error message has missing spaces between sentences.
- "log_obj_size is not supported for the executorlib.SlurmClusterExecutor and executorlib.FluxClusterExecutor." - "Please use log_obj_size=False instead of log_obj_size=True." + "log_obj_size is not supported for the executorlib.SlurmClusterExecutor and executorlib.FluxClusterExecutor. " + "Please use log_obj_size=False instead of log_obj_size=True."
221-226: Fix missing space in error message.The error message has missing spaces between sentences.
- "terminate_tasks_on_shutdown is not supported for the executorlib.SingleNodeExecutor, " - "executorlib.SlurmJobExecutor and executorlib.FluxJobExecutor." - "Please use terminate_tasks_on_shutdown=True instead of terminate_tasks_on_shutdown=False." + "terminate_tasks_on_shutdown is not supported for the executorlib.SingleNodeExecutor, " + "executorlib.SlurmJobExecutor and executorlib.FluxJobExecutor. " + "Please use terminate_tasks_on_shutdown=True instead of terminate_tasks_on_shutdown=False."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
executorlib/executor/flux.py(10 hunks)executorlib/executor/slurm.py(10 hunks)executorlib/standalone/inputcheck.py(1 hunks)executorlib/task_scheduler/file/queue_spawner.py(1 hunks)executorlib/task_scheduler/file/shared.py(2 hunks)tests/test_standalone_inputcheck.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
tests/test_standalone_inputcheck.py (1)
executorlib/standalone/inputcheck.py (2)
check_terminate_tasks_on_shutdown(217-226)check_log_obj_size(206-214)
executorlib/task_scheduler/file/shared.py (1)
executorlib/task_scheduler/file/subprocess_spawner.py (1)
terminate_subprocess(55-64)
executorlib/executor/slurm.py (3)
executorlib/standalone/inputcheck.py (2)
check_log_obj_size(206-214)check_terminate_tasks_on_shutdown(217-226)executorlib/task_scheduler/file/task_scheduler.py (1)
create_file_executor(77-118)executorlib/task_scheduler/file/queue_spawner.py (1)
terminate_with_pysqa(84-102)
executorlib/executor/flux.py (3)
executorlib/standalone/inputcheck.py (2)
check_log_obj_size(206-214)check_terminate_tasks_on_shutdown(217-226)executorlib/task_scheduler/file/task_scheduler.py (1)
create_file_executor(77-118)executorlib/task_scheduler/file/queue_spawner.py (1)
terminate_with_pysqa(84-102)
⏰ 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). (10)
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-openmpi.yml)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.11)
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-mpich.yml)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.13)
- GitHub Check: notebooks
- GitHub Check: unittest_openmpi (macos-latest, 3.13)
- GitHub Check: unittest_mpich (macos-latest, 3.13)
- GitHub Check: unittest_flux_openmpi
- GitHub Check: unittest_win
- GitHub Check: notebooks_integration
🔇 Additional comments (16)
tests/test_standalone_inputcheck.py (2)
21-22: LGTM!The imports for the new validation functions are correctly added.
125-131: LGTM!The test correctly validates that
check_log_obj_sizeraises aValueErrorwhenlog_obj_size=True, which matches the expected behavior of the validation function.executorlib/task_scheduler/file/queue_spawner.py (1)
84-103: LGTM!The
terminate_with_pysqafunction correctly implements job termination for queuing systems. The function signature and implementation align well with the shutdown handling logic inshared.pyand the conditional assignment in the executor classes.executorlib/task_scheduler/file/shared.py (1)
90-102: LGTM!The conditional shutdown handling logic correctly distinguishes between different termination function signatures. The identity check for
terminate_subprocessand the different parameter passing patterns align well with the respective function signatures.executorlib/executor/slurm.py (5)
6-9: LGTM!The import additions for the new validation functions are correctly placed.
101-102: LGTM!The parameter additions to the constructor signature are correctly implemented with appropriate defaults.
161-161: LGTM!The validation call for
log_obj_sizeis correctly placed and follows the established pattern.
169-177: LGTM!The conditional import and assignment of the termination function is well-implemented. The logic correctly imports
terminate_with_pysqaonly when needed and assigns the appropriate termination function based on the parameter value.
360-362: LGTM!The validation call for
terminate_tasks_on_shutdownis correctly implemented and follows the established validation pattern.executorlib/executor/flux.py (7)
7-7: New imports look good.The imports for
check_log_obj_sizeandcheck_terminate_tasks_on_shutdownare properly added and align with the new functionality being introduced.Also applies to: 12-12
67-67: Parameter addition is consistent.The
terminate_tasks_on_shutdownparameter is properly added to both the class docstring and constructor signature with the correct default value ofTrue.Also applies to: 108-108
170-172: Input validation is properly implemented.The validation call for
terminate_tasks_on_shutdownis correctly placed and follows the established pattern for other parameter validations in the constructor.
154-154: Documentation updates are consistent.The parameter documentation is properly updated in both class docstrings and constructor docstrings to reflect the new
terminate_tasks_on_shutdownandlog_obj_sizeparameters.Also applies to: 257-258, 295-296, 338-339
355-355: Input validation for log_obj_size is properly placed.The validation call ensures that
log_obj_sizeis False for FluxClusterExecutor, which aligns with the validation function's purpose.
363-370: Conditional termination function assignment logic is correct.The logic properly imports and assigns the
terminate_with_pysqafunction whenterminate_tasks_on_shutdownis True, and sets it to None otherwise. The import is correctly placed within the conditional block to avoid unnecessary imports.
388-388: Termination function parameter is correctly passed.The
terminate_functionparameter is properly passed tocreate_file_executor, enabling the task scheduler to handle termination as intended.
| plot_dependency_graph: bool = False, | ||
| plot_dependency_graph_filename: Optional[str] = None, | ||
| log_obj_size: bool = False, | ||
| terminate_tasks_on_shutdown: bool = True, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure parameter consistency between executor classes.
The terminate_tasks_on_shutdown parameter is added to SlurmJobExecutor constructor but unlike SlurmClusterExecutor, it's not actually used in the implementation logic. The parameter is validated but not propagated to the underlying executor creation.
Consider either:
- Removing the parameter from
SlurmJobExecutorif termination control is not supported - Implementing the termination logic similar to
SlurmClusterExecutor - Documenting why the parameter exists but isn't used
Let me check if this parameter should be used in the create_slurm_executor function:
🏁 Script executed:
#!/bin/bash
# Description: Check if create_slurm_executor supports termination function parameter
# Search for terminate_function parameter in create_slurm_executor
ast-grep --pattern 'def create_slurm_executor($$$) {
$$$
}'Length of output: 63
I’m diving deeper into the Slurm executor definitions to verify how (or if) the terminate_tasks_on_shutdown flag is handled. Running targeted searches now:
🏁 Script executed:
#!/bin/bash
# 1. Show SlurmJobExecutor definition and its constructor
rg -n "class SlurmJobExecutor" -n executorlib/executor/slurm.py -C10
# 2. Show SlurmClusterExecutor definition and its constructor
rg -n "class SlurmClusterExecutor" -n executorlib/executor/slurm.py -C10
# 3. Inspect create_slurm_executor signature
rg -n "^def create_slurm_executor" -n executorlib/executor/slurm.py -C5
# 4. Find all calls to create_slurm_executor in this file
rg -n "create_slurm_executor" -n executorlib/executor/slurm.py -C5Length of output: 5096
Gathering all references to the shutdown flag and both constructor signatures:
🏁 Script executed:
#!/bin/bash
# 1. Find all occurrences of the shutdown flag
rg -n "terminate_tasks_on_shutdown" -n executorlib/executor/slurm.py -C3
# 2. Show both __init__ methods with surrounding context
rg -n "def __init__" -n executorlib/executor/slurm.py -C5Length of output: 4418
Parameter terminate_tasks_on_shutdown isn’t hooked up in SlurmJobExecutor
In executorlib/executor/slurm.py, SlurmJobExecutor.__init__ declares and validates
terminate_tasks_on_shutdown (line 298 and 360–362) but never applies it—unlike
SlurmClusterExecutor, which conditionally imports terminate_with_pysqa when that flag is true.
• File: executorlib/executor/slurm.py
– SlurmJobExecutor signature (≈line 298):
python … log_obj_size: bool = False, terminate_tasks_on_shutdown: bool = True, ):
– Only check_terminate_tasks_on_shutdown(...) is called; there’s no
if terminate_tasks_on_shutdown: import terminate_with_pysqa or equivalent.
• In contrast, SlurmClusterExecutor (≈lines 99–104 & 166–170) does:
python if terminate_tasks_on_shutdown: from executorlib.task_scheduler.file.queue_spawner import terminate_with_pysqa
Suggested corrective actions:
- Remove
terminate_tasks_on_shutdownfrom SlurmJobExecutor if shutdown control isn’t supported. - Mirror cluster behavior by importing/attaching
terminate_with_pysqa(or other shutdown hooks) when the flag is true. - Document that the flag is intentionally a no-op in SlurmJobExecutor.
🤖 Prompt for AI Agents
In executorlib/executor/slurm.py at line 298, the parameter
terminate_tasks_on_shutdown is declared in SlurmJobExecutor but not used. To fix
this, either remove terminate_tasks_on_shutdown from SlurmJobExecutor's __init__
if shutdown task termination is not supported, or implement similar logic as in
SlurmClusterExecutor by conditionally importing terminate_with_pysqa and
applying it when terminate_tasks_on_shutdown is true. Additionally, update the
documentation to clarify whether this flag is supported or intentionally a no-op
in SlurmJobExecutor.
# Conflicts: # executorlib/executor/flux.py # executorlib/executor/slurm.py # executorlib/standalone/inputcheck.py # tests/test_standalone_inputcheck.py
jan-janssen
changed the title
# Conflicts: # executorlib/task_scheduler/file/shared.py
…to terminate_function
for more information, see https://pre-commit.ci
…to terminate_function
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_fluxclusterexecutor.py (1)
40-40: Parameter addition looks good, but consider enhancing test coverage.The addition of
terminate_tasks_on_shutdown=Falseis correctly implemented. However, this test doesn't verify that tasks are actually preserved during shutdown when this parameter isFalse.Consider adding assertions to verify the actual termination behavior:
# After executor context exit, verify tasks weren't terminated # (implementation would depend on how termination status can be checked)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
executorlib/executor/flux.py(10 hunks)executorlib/executor/slurm.py(10 hunks)executorlib/task_scheduler/file/queue_spawner.py(2 hunks)executorlib/task_scheduler/file/shared.py(2 hunks)tests/test_fluxclusterexecutor.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- executorlib/task_scheduler/file/shared.py
- executorlib/executor/slurm.py
- executorlib/task_scheduler/file/queue_spawner.py
- executorlib/executor/flux.py
⏰ 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). (16)
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-openmpi.yml)
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-mpich.yml)
- GitHub Check: unittest_flux_openmpi
- GitHub Check: unittest_win
- GitHub Check: unittest_flux_mpich
- GitHub Check: unittest_mpich (ubuntu-latest, 3.11)
- GitHub Check: notebooks_integration
- GitHub Check: unittest_openmpi (macos-latest, 3.13)
- GitHub Check: unittest_mpich (ubuntu-latest, 3.12)
- GitHub Check: unittest_old
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.11)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.13)
- GitHub Check: notebooks
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.12)
- GitHub Check: unittest_mpich (ubuntu-latest, 3.13)
- GitHub Check: unittest_mpich (macos-latest, 3.13)
🔇 Additional comments (2)
tests/test_fluxclusterexecutor.py (2)
54-54: Parameter addition is correct.The addition of
terminate_tasks_on_shutdown=Trueproperly tests the alternate value for this parameter.
64-68: Ignore inconsistent parameter usage comment.No tests in tests/test_fluxclusterexecutor.py use the
terminate_tasks_on_shutdownargument—none of the threewith FluxClusterExecutor(...)blocks include it. The original observation about “other two test methods” was incorrect. You can disregard this review comment.Likely an incorrect or invalid review comment.
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
executorlib/executor/flux.py(9 hunks)executorlib/executor/slurm.py(9 hunks)executorlib/task_scheduler/file/queue_spawner.py(1 hunks)executorlib/task_scheduler/file/task_scheduler.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- executorlib/task_scheduler/file/queue_spawner.py
- executorlib/executor/slurm.py
- executorlib/executor/flux.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
executorlib/task_scheduler/file/task_scheduler.py (2)
executorlib/task_scheduler/file/subprocess_spawner.py (1)
terminate_subprocess(55-64)executorlib/task_scheduler/file/queue_spawner.py (2)
execute_with_pysqa(11-81)terminate_with_pysqa(84-104)
⏰ 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). (17)
- GitHub Check: notebooks_integration
- GitHub Check: notebooks
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-openmpi.yml)
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-mpich.yml)
- GitHub Check: minimal
- GitHub Check: unittest_mpich (ubuntu-latest, 3.12)
- GitHub Check: unittest_openmpi (macos-latest, 3.13)
- GitHub Check: unittest_old
- GitHub Check: unittest_mpich (ubuntu-latest, 3.13)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.11)
- GitHub Check: unittest_mpich (ubuntu-latest, 3.11)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.12)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.13)
- GitHub Check: unittest_mpich (macos-latest, 3.13)
- GitHub Check: unittest_win
- GitHub Check: unittest_flux_mpich
- GitHub Check: unittest_flux_openmpi
🔇 Additional comments (4)
executorlib/task_scheduler/file/task_scheduler.py (4)
16-16: LGTM: Import addition is consistent with the new functionality.The import of
terminate_subprocessaligns with the conditional logic that uses this function for subprocess termination.
20-23: LGTM: Updated import structure supports the new termination functionality.The import changes properly include
terminate_with_pysqaalongside the existingexecute_with_pysqaimport, maintaining consistency in the module structure.
27-27: LGTM: Appropriate fallback handling for missing dependency.Setting
terminate_with_pysqa = Nonewhen pysqa is not available is consistent with the existing pattern and properly handled in the conditional logic below.
116-121: No changes needed: pysqa fallback already handled safelyThe import block in
task_scheduler.pyfalls back by settingexecute_with_pysqa = execute_in_subprocess terminate_with_pysqa = Nonewhen pysqa isn’t available. As a result, the default
execute_functionbecomesexecute_in_subprocess, and the second branchelif terminate_tasks_on_shutdown and execute_function == execute_in_subprocess: terminate_function = terminate_subprocessalways applies—there is no case where
terminate_with_pysqa(and thusterminate_function) is unexpectedlyNonewhileexecute_function != execute_in_subprocess. No additionalNonecheck is required.
| disable_dependencies: bool = False, | ||
| execute_function: Callable = execute_with_pysqa, | ||
| terminate_function: Optional[Callable] = None, | ||
| terminate_tasks_on_shutdown: bool = True, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add parameter validation to ensure type safety.
The new terminate_tasks_on_shutdown parameter should be validated to ensure it's a boolean type, similar to other parameter validations in this function.
According to the AI summary, there should be a check_terminate_tasks_on_shutdown validation function. Add the validation call:
check_flux_log_files(flux_log_files=flux_log_files)
+check_terminate_tasks_on_shutdown(terminate_tasks_on_shutdown=terminate_tasks_on_shutdown)Let me verify if the validation function exists:
🏁 Script executed:
#!/bin/bash
# Search for the validation function mentioned in the AI summary
rg -A 5 "check_terminate_tasks_on_shutdown"Length of output: 2496
🏁 Script executed:
#!/bin/bash
# List all validation calls in the FileTaskScheduler initializer
rg -n "executorlib/task_scheduler/file/task_scheduler.py" -A 5 "check_"Length of output: 116
🏁 Script executed:
#!/bin/bash
# Verify the FileTaskScheduler initializer and existing validation calls
echo "---- FileTaskScheduler class and __init__ ----"
rg -n "class FileTaskScheduler" -A 20 executorlib/task_scheduler/file/task_scheduler.py
echo
echo "---- Existing check_ calls in the file ----"
rg -n "check_" executorlib/task_scheduler/file/task_scheduler.py || echo "No check_ calls found in task_scheduler.py"Length of output: 2219
🏁 Script executed:
#!/bin/bash
# Show import statements for validation functions in FileTaskScheduler
sed -n '1,15p' executorlib/task_scheduler/file/task_scheduler.pyLength of output: 579
Ensure terminate_tasks_on_shutdown is validated
The new terminate_tasks_on_shutdown: bool = True parameter isn’t currently being checked in FileTaskScheduler. You should:
• Import the validator
from executorlib.standalone.inputcheck import (
check_executor,
check_flux_executor_pmi_mode,
check_flux_log_files,
check_hostname_localhost,
check_max_workers_and_cores,
check_nested_flux_executor,
+ check_terminate_tasks_on_shutdown,
)• Add the validation call in __init__, e.g. after the existing checks:
check_flux_log_files(flux_log_files=flux_log_files)
+ check_terminate_tasks_on_shutdown(
+ terminate_tasks_on_shutdown=terminate_tasks_on_shutdown
+ )This matches the pattern of other parameter validations and ensures type safety.
📝 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.
| terminate_tasks_on_shutdown: bool = True, | |
| # In executorlib/task_scheduler/file/task_scheduler.py | |
| from executorlib.standalone.inputcheck import ( | |
| check_executor, | |
| check_flux_executor_pmi_mode, | |
| check_flux_log_files, | |
| check_hostname_localhost, | |
| check_max_workers_and_cores, | |
| check_nested_flux_executor, | |
| check_terminate_tasks_on_shutdown, | |
| ) | |
| # … other code … | |
| class FileTaskScheduler: | |
| def __init__( | |
| self, | |
| executor, | |
| flux_executor_pmi_mode, | |
| flux_log_files, | |
| hostname_localhost, | |
| max_workers, | |
| nested_flux_executor, | |
| terminate_tasks_on_shutdown: bool = True, | |
| ): | |
| check_executor(executor=executor) | |
| check_flux_executor_pmi_mode(flux_executor_pmi_mode=flux_executor_pmi_mode) | |
| check_flux_log_files(flux_log_files=flux_log_files) | |
| check_terminate_tasks_on_shutdown( | |
| terminate_tasks_on_shutdown=terminate_tasks_on_shutdown | |
| ) | |
| # … rest of initialization … |
🤖 Prompt for AI Agents
In executorlib/task_scheduler/file/task_scheduler.py at line 98, the new
parameter terminate_tasks_on_shutdown is added but not validated. To fix this,
import the validator function used for parameter checks and add a validation
call for terminate_tasks_on_shutdown in the __init__ method of
FileTaskScheduler, following the pattern of existing parameter validations to
ensure type safety.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
executorlib/task_scheduler/file/task_scheduler.py (1)
98-98: Add parameter validation to ensure type safety.The new
terminate_tasks_on_shutdownparameter should be validated to ensure it's a boolean type, similar to other parameter validations in this function.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
executorlib/task_scheduler/file/task_scheduler.py(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
executorlib/task_scheduler/file/task_scheduler.py (2)
executorlib/task_scheduler/file/subprocess_spawner.py (1)
terminate_subprocess(55-64)executorlib/task_scheduler/file/queue_spawner.py (2)
execute_with_pysqa(11-81)terminate_with_pysqa(84-104)
⏰ 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). (17)
- GitHub Check: notebooks_integration
- GitHub Check: notebooks
- GitHub Check: minimal
- GitHub Check: unittest_old
- GitHub Check: unittest_win
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.11)
- GitHub Check: unittest_flux_openmpi
- GitHub Check: unittest_mpich (ubuntu-latest, 3.12)
- GitHub Check: unittest_flux_mpich
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-openmpi.yml)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.13)
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-mpich.yml)
- GitHub Check: unittest_openmpi (macos-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.11)
- GitHub Check: unittest_mpich (macos-latest, 3.13)
🔇 Additional comments (3)
executorlib/task_scheduler/file/task_scheduler.py (3)
16-16: LGTM: Import addition looks correct.Adding
terminate_subprocessto the import aligns with the new termination logic requirements.
20-23: LGTM: Import expansion is well-structured.The import is properly expanded to include
terminate_with_pysqaalongside the existingexecute_with_pysqa.
27-27: LGTM: Proper fallback handling for missing dependency.Setting
terminate_with_pysqa = Nonein the ImportError block is appropriate when pysqa is not available.
| if terminate_tasks_on_shutdown and execute_function != execute_in_subprocess: | ||
| terminate_function = terminate_with_pysqa | ||
| elif terminate_tasks_on_shutdown and execute_function == execute_in_subprocess: | ||
| terminate_function = terminate_subprocess | ||
| else: | ||
| terminate_function = None |
There was a problem hiding this comment.
Fix potential None assignment and improve logic robustness.
The conditional logic has a critical issue: when terminate_tasks_on_shutdown is True and execute_function != execute_in_subprocess, it assigns terminate_with_pysqa to terminate_function, but terminate_with_pysqa could be None if pysqa is not available (line 27).
Apply this diff to handle the None case safely:
- if terminate_tasks_on_shutdown and execute_function != execute_in_subprocess:
- terminate_function = terminate_with_pysqa
+ if terminate_tasks_on_shutdown and execute_function != execute_in_subprocess:
+ if terminate_with_pysqa is not None:
+ terminate_function = terminate_with_pysqa
+ else:
+ terminate_function = None
elif terminate_tasks_on_shutdown and execute_function == execute_in_subprocess:
terminate_function = terminate_subprocess
else:
terminate_function = NoneAlternatively, consider raising an informative error when termination is requested but not available:
- if terminate_tasks_on_shutdown and execute_function != execute_in_subprocess:
- terminate_function = terminate_with_pysqa
+ if terminate_tasks_on_shutdown and execute_function != execute_in_subprocess:
+ if terminate_with_pysqa is None:
+ raise ImportError("pysqa is required for task termination but is not available")
+ terminate_function = terminate_with_pysqa📝 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.
| if terminate_tasks_on_shutdown and execute_function != execute_in_subprocess: | |
| terminate_function = terminate_with_pysqa | |
| elif terminate_tasks_on_shutdown and execute_function == execute_in_subprocess: | |
| terminate_function = terminate_subprocess | |
| else: | |
| terminate_function = None | |
| if terminate_tasks_on_shutdown and execute_function != execute_in_subprocess: | |
| if terminate_with_pysqa is not None: | |
| terminate_function = terminate_with_pysqa | |
| else: | |
| terminate_function = None | |
| elif terminate_tasks_on_shutdown and execute_function == execute_in_subprocess: | |
| terminate_function = terminate_subprocess | |
| else: | |
| terminate_function = None |
🤖 Prompt for AI Agents
In executorlib/task_scheduler/file/task_scheduler.py around lines 116 to 121,
the current logic assigns terminate_with_pysqa to terminate_function without
checking if terminate_with_pysqa is None, which can cause issues if pysqa is
unavailable. To fix this, add a check to ensure terminate_with_pysqa is not None
before assignment; if it is None, either set terminate_function to None safely
or raise an informative error indicating termination is requested but not
supported. This will make the logic more robust and prevent runtime errors.
Summary by CodeRabbit