Refactor Task scheduler #637
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
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 5 minutes and 3 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 (8)
WalkthroughThis pull request refactors the executorlib codebase by replacing the previous executor interface classes with a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ExecutorBase
participant TaskSchedulerBase
User->>ExecutorBase: submit(fn, args, resource_dict)
ExecutorBase->>TaskSchedulerBase: submit task with resources
TaskSchedulerBase-->>ExecutorBase: Future object
ExecutorBase-->>User: Future object returned
User->>ExecutorBase: shutdown(wait, cancel_futures)
ExecutorBase->>TaskSchedulerBase: shutdown with parameters
TaskSchedulerBase-->>ExecutorBase: cleanup done
ExecutorBase-->>User: shutdown complete
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:
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: 3
🔭 Outside diff range comments (4)
executorlib/task_scheduler/base.py (1)
19-179: 💡 Verification agent🧩 Analysis chain
Architectural change: ExecutorInterface removal.
The removal of the
ExecutorInterfaceclass, which previously wrapped theExecutorBase(nowTaskSchedulerBase), indicates a significant architectural change. According to the summary, a newExecutorBaseclass has been introduced elsewhere that wraps theTaskSchedulerBaseinstance, effectively replacing the removed abstraction layer.Please confirm that all callers previously using
ExecutorInterfacehave been updated to use either the newExecutorBasewrapper orTaskSchedulerBasedirectly, and that this change doesn't break backward compatibility for library users.
🏁 Script executed:
#!/bin/bash # Search for any remaining references to ExecutorInterface in the codebase echo "Searching for remaining references to ExecutorInterface..." rg "ExecutorInterface" -lLength of output: 152
No internal references to ExecutorInterface found; public API removal is breaking change
A full-code search for
ExecutorInterfacereturned no results, confirming all internal callers have been updated to use the newExecutorBasewrapper orTaskSchedulerBase. However, removingExecutorInterfacefrom the public API will break backward compatibility for existing users.• No files reference
ExecutorInterface
• All internal call sites now point to the new classesAction items:
- Introduce a deprecated alias or shim for
ExecutorInterfacein the public API to smooth migration- Update documentation and release notes to highlight this breaking change
- Bump the library’s major version if you intend to remove the alias in a future release
executorlib/executor/slurm.py (1)
414-419: 🛠️ Refactor suggestionAvoid mutating caller-supplied
resource_dictinsidecreate_slurm_executor
resource_dict["cache_directory"] = cache_directorymutates the dictionary received from the caller.
If the same dict is reused elsewhere the unexpected side-effect can produce hard-to-trace bugs.Guard against this by copying:
- if resource_dict is None: - resource_dict = {} + if resource_dict is None: + resource_dict = {} + else: + resource_dict = resource_dict.copy() # prevent side-effectsexecutorlib/executor/flux.py (1)
441-456: 🛠️ Refactor suggestionDo not mutate the caller’s
resource_dictin-place
create_flux_executordeletes and overwrites keys of theresource_dictpassed by the caller. Because dictionaries are mutable, callers that reuse the same dict will observe surprising side-effects.- if resource_dict is None: - resource_dict = {} + if resource_dict is None: + resource_dict = {} + else: + # operate on a shallow copy to keep the caller’s data intact + resource_dict = dict(resource_dict)Apply this before any
update,del, or assignment.executorlib/executor/single.py (1)
238-244: 🛠️ Refactor suggestionAvoid mutating
resource_dictprovided by the callerKeys are deleted directly from the caller-supplied dict, which can lead to unexpected
KeyErrors later. Work on a copy instead.- if resource_dict is None: - resource_dict = {} + if resource_dict is None: + resource_dict = {} + else: + resource_dict = dict(resource_dict)
🧹 Nitpick comments (16)
executorlib/task_scheduler/base.py (1)
17-17: Class renamed to better reflect its purpose.The class has been renamed from
ExecutorBasetoTaskSchedulerBaseto more accurately represent its role in the architecture. This is part of a broader refactoring where executor classes are being renamed to task scheduler classes.However, there's a minor inconsistency in the docstring on line 27, which still refers to "ExecutorBase". Consider updating this reference.
- Initialize the ExecutorBase class. + Initialize the TaskSchedulerBase class.executorlib/task_scheduler/file/task_scheduler.py (2)
39-39: Update docstring to match new class nameThe docstring still references "Initialize the FileExecutor" even though the class has been renamed to "FileTaskScheduler". This should be updated for consistency.
- Initialize the FileExecutor. + Initialize the FileTaskScheduler.
84-99: Consider renaming function to match new class naming conventionWhile all the executor classes are being renamed to task schedulers, the factory function
create_file_executorstill uses the old naming convention. Consider renaming it tocreate_file_task_schedulerfor consistency, while potentially keeping the old function name for backward compatibility.tests/test_fluxjobexecutor.py (1)
11-11: LGTM - Import path correctly updated, but static analysis flags unused importThe import path for
FluxPythonSpawnerhas been correctly updated to reflect the new module organization. However, static analysis indicates this import is unused in the code.Since the import is primarily used for the try/except block to check if Flux is available, consider using
importlib.util.find_specinstead to test for module availability without importing the actual class.- from executorlib.task_scheduler.interactive import FluxPythonSpawner + import importlib.util + flux_spawner_spec = importlib.util.find_spec("executorlib.task_scheduler.interactive.FluxPythonSpawner") + skip_flux_test = "FLUX_URI" not in os.environ or flux_spawner_spec is None - skip_flux_test = "FLUX_URI" not in os.environ🧰 Tools
🪛 Ruff (0.8.2)
11-11:
executorlib.task_scheduler.interactive.FluxPythonSpawnerimported but unused; consider usingimportlib.util.find_specto test for availability(F401)
executorlib/backend/cache_serial.py (1)
3-3: Add CLI argument validation. Referencingsys.argv[1]without checkinglen(sys.argv)may raise anIndexErrorwhen no argument is provided. Consider usingargparseor at minimum:if __name__ == "__main__": - backend_execute_task_in_file(file_name=sys.argv[1]) + if len(sys.argv) != 2: + print(f"Usage: {sys.argv[0]} <file_name>", file=sys.stderr) + sys.exit(1) + backend_execute_task_in_file(file_name=sys.argv[1])tests/test_fluxjobexecutor_plot.py (1)
13-13: Remove unused import ofFluxPythonSpawner. The import is not used anywhere in this test, leading to an unnecessary dependency (Ruff F401).- from executorlib.task_scheduler.interactive import FluxPythonSpawner🧰 Tools
🪛 Ruff (0.8.2)
13-13:
executorlib.task_scheduler.interactive.FluxPythonSpawnerimported but unused; consider usingimportlib.util.find_specto test for availability(F401)
executorlib/task_scheduler/interactive/shared.py (2)
43-44: Doc-string wording is confusing (“file files”)Small nit, but the phrase “The directory to store file files.” is tautological and unclear.
Replacing it with “The directory used to persist task files (HDF5).” improves readability.
134-142: Rename variables & comments consistently to prevent cognitive overheadEven after the rename from “cache” → “file”, a mixed vocabulary remains:
_execute_task_with_cache(function name)- Comment “…using the file in the file directory.”
- Variable
cache_directorystill carries the old term.Refactor to something like
_execute_task_with_store()andstore_directory(or similar) to avoid misleading readers and prepare the ground for future storage back-ends.Also applies to: 151-156
executorlib/executor/base.py (1)
52-86: Forwardingsubmitvia list-concatenation is brittle
self._task_scheduler.submit(fn, *args, …)is clearer thanself._task_scheduler.submit(*([fn] + list(args)), …)and avoids the tiny overhead of creating an intermediate list.
- return self._task_scheduler.submit( - *([fn] + list(args)), resource_dict=resource_dict, **kwargs - ) + return self._task_scheduler.submit( + fn, *args, resource_dict=resource_dict, **kwargs + )executorlib/executor/slurm.py (1)
30-36: Update doc-strings (“file files”) & stale class namesSeveral doc-strings still reference the removed
executorlib.Executorclass and contain the awkward phrase “file files”.
Refreshing these strings reduces confusion for new users skimming the API.Example fix:
- preferable the flux framework for distributing python functions … + preferable the flux framework for distributing python functions … - cache_directory (str, optional): The directory to store file files. Defaults to "file". + cache_directory (str, optional): Directory used to persist task files (HDF5). Defaults to "file".Also applies to: 106-112
executorlib/executor/flux.py (2)
39-50: Fix typos in docstring parameter descriptionsMinor wording issues can confuse users and doc-generation tools:
- “directory to store file files” → “…to store cached files”
- “compute notes” → “compute nodes”
Correcting these keeps the public API documentation professional and machine-readable.
- cache_directory (str, optional): The directory to store file files. Defaults to "file". + cache_directory (str, optional): Directory that stores cached files produced by tasks. Defaults to "file". ... - - exclusive (bool): Whether to exclusively reserve the compute nodes, or allow sharing - compute notes. Defaults to False. + - exclusive (bool): Whether to exclusively reserve the compute nodes, or allow sharing + compute nodes. Defaults to False.
472-484: Validation asymmetry between allocation modes
validate_max_workersensures the block-allocation branch cannot oversubscribe CPU resources, but the one-process branch skips this check. Passing a largemax_workerswith smallmax_corescould therefore oversubscribe the node.Run the same validation (or an equivalent sanity check) for the
OneProcessTaskSchedulerpath.tests/test_mpiexecspawner.py (2)
506-507: Make cleanup idempotent
shutil.rmtree("./file")raisesFileNotFoundErrorwhen the directory was never created (e.g., if tests abort early). Addignore_errors=Trueto prevent spurious failures.- shutil.rmtree("./file") + shutil.rmtree("./file", ignore_errors=True)
522-523: Use a temporary directory for cache in testsHard-coding
./filerisks collisions when tests run in parallel or on shared CI runners. Leveragingtempfile.mkdtemp()or pytest’stmp_pathfixture would isolate artefacts automatically.executorlib/executor/single.py (2)
32-41: Correct minor docstring typosSame “file files” and “compute notes” typos found here—please update for clarity and consistency with the public API documentation.
247-255: Missing core / worker validation in non-block-allocation path
validate_number_of_coresis used only for the block-allocation branch. Applying the same safeguard to theOneProcessTaskSchedulerbranch would prevent oversubscription.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
executorlib/__init__.py(1 hunks)executorlib/backend/cache_parallel.py(1 hunks)executorlib/backend/cache_serial.py(1 hunks)executorlib/executor/base.py(1 hunks)executorlib/executor/flux.py(15 hunks)executorlib/executor/single.py(10 hunks)executorlib/executor/slurm.py(14 hunks)executorlib/standalone/hdf.py(1 hunks)executorlib/task_scheduler/base.py(1 hunks)executorlib/task_scheduler/file/backend.py(1 hunks)executorlib/task_scheduler/file/shared.py(3 hunks)executorlib/task_scheduler/file/task_scheduler.py(3 hunks)executorlib/task_scheduler/interactive/blockallocation.py(3 hunks)executorlib/task_scheduler/interactive/dependency.py(4 hunks)executorlib/task_scheduler/interactive/onetoone.py(2 hunks)executorlib/task_scheduler/interactive/shared.py(3 hunks)notebooks/1-single-node.ipynb(4 hunks)notebooks/2-hpc-cluster.ipynb(1 hunks)notebooks/3-hpc-job.ipynb(1 hunks)notebooks/4-developer.ipynb(1 hunks)tests/test_base_executor_queue.py(1 hunks)tests/test_cache_backend_execute.py(6 hunks)tests/test_cache_fileexecutor_mpi.py(3 hunks)tests/test_cache_fileexecutor_serial.py(7 hunks)tests/test_fluxclusterexecutor.py(2 hunks)tests/test_fluxjobexecutor.py(1 hunks)tests/test_fluxjobexecutor_plot.py(1 hunks)tests/test_fluxpythonspawner.py(6 hunks)tests/test_interactive_dependencies.py(5 hunks)tests/test_interactive_slurmspawner.py(1 hunks)tests/test_mpiexecspawner.py(26 hunks)tests/test_singlenodeexecutor_cache.py(2 hunks)tests/test_singlenodeexecutor_dependencies.py(1 hunks)tests/test_singlenodeexecutor_mpi.py(2 hunks)tests/test_singlenodeexecutor_shell_executor.py(1 hunks)tests/test_singlenodeexecutor_shell_interactive.py(1 hunks)tests/test_standalone_hdf.py(6 hunks)tests/test_standalone_interactive_backend.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (17)
tests/test_singlenodeexecutor_shell_interactive.py (1)
executorlib/task_scheduler/interactive/shared.py (1)
execute_tasks(18-76)
tests/test_base_executor_queue.py (1)
executorlib/standalone/queue.py (1)
cancel_items_in_queue(4-19)
tests/test_standalone_interactive_backend.py (1)
executorlib/task_scheduler/interactive/slurmspawner.py (1)
SrunSpawner(23-81)
executorlib/task_scheduler/file/backend.py (1)
executorlib/task_scheduler/file/shared.py (1)
FutureItem(14-49)
tests/test_fluxjobexecutor.py (1)
executorlib/task_scheduler/interactive/fluxspawner.py (1)
FluxPythonSpawner(23-151)
tests/test_singlenodeexecutor_dependencies.py (2)
executorlib/executor/single.py (1)
create_single_node_executor(184-262)executorlib/task_scheduler/interactive/dependency.py (1)
_execute_tasks_with_dependencies(188-262)
tests/test_interactive_slurmspawner.py (2)
executorlib/task_scheduler/interactive/slurmspawner.py (1)
generate_slurm_command(84-125)executorlib/task_scheduler/file/queue_spawner.py (1)
_pysqa_execute_command(82-115)
tests/test_fluxjobexecutor_plot.py (1)
executorlib/task_scheduler/interactive/fluxspawner.py (1)
FluxPythonSpawner(23-151)
tests/test_interactive_dependencies.py (1)
executorlib/task_scheduler/interactive/blockallocation.py (1)
BlockAllocationTaskScheduler(16-177)
executorlib/backend/cache_serial.py (1)
executorlib/task_scheduler/file/backend.py (1)
backend_execute_task_in_file(60-85)
executorlib/backend/cache_parallel.py (1)
executorlib/task_scheduler/file/backend.py (2)
backend_load_file(9-29)backend_write_file(32-57)
tests/test_singlenodeexecutor_shell_executor.py (1)
executorlib/task_scheduler/interactive/shared.py (1)
execute_tasks(18-76)
tests/test_cache_backend_execute.py (11)
executorlib/task_scheduler/file/backend.py (1)
backend_execute_task_in_file(60-85)executorlib/task_scheduler/file/shared.py (4)
_check_task_output(185-208)FutureItem(14-49)done(41-49)result(25-39)executorlib/standalone/serialize.py (1)
serialize_funct_h5(31-75)tests/test_cache_fileexecutor_serial.py (3)
my_funct(22-23)get_error(30-31)tearDown(205-207)tests/test_standalone_hdf.py (2)
my_funct(20-21)tearDown(122-124)executorlib/standalone/hdf.py (1)
get_runtime(83-97)tests/test_singlenodeexecutor_cache.py (2)
get_error(16-17)tearDown(44-48)tests/test_cache_fileexecutor_mpi.py (1)
tearDown(42-44)tests/test_fluxclusterexecutor.py (1)
tearDown(46-48)tests/test_singlenodeexecutor_mpi.py (1)
tearDown(85-86)tests/test_mpiexecspawner.py (1)
tearDown(505-506)
tests/test_cache_fileexecutor_serial.py (4)
executorlib/task_scheduler/file/subprocess_spawner.py (2)
execute_in_subprocess(8-52)terminate_subprocess(55-64)executorlib/task_scheduler/file/task_scheduler.py (2)
FileTaskScheduler(27-81)create_file_executor(84-122)executorlib/task_scheduler/file/shared.py (2)
execute_tasks_h5(52-155)done(41-49)tests/test_cache_backend_execute.py (1)
my_funct(18-19)
executorlib/executor/base.py (1)
executorlib/task_scheduler/base.py (7)
TaskSchedulerBase(17-181)max_workers(36-37)max_workers(40-41)info(44-60)future_queue(63-70)submit(72-127)shutdown(129-152)
executorlib/task_scheduler/interactive/onetoone.py (2)
executorlib/task_scheduler/base.py (1)
TaskSchedulerBase(17-181)executorlib/task_scheduler/interactive/shared.py (1)
execute_tasks(18-76)
executorlib/task_scheduler/interactive/dependency.py (1)
executorlib/task_scheduler/base.py (1)
TaskSchedulerBase(17-181)
🪛 Ruff (0.8.2)
tests/test_fluxjobexecutor.py
11-11: executorlib.task_scheduler.interactive.FluxPythonSpawner imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
tests/test_fluxjobexecutor_plot.py
13-13: executorlib.task_scheduler.interactive.FluxPythonSpawner imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: unittest_win
- 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 (77)
tests/test_standalone_interactive_backend.py (1)
7-7: Align import with refactored module path
The import forSrunSpawnerhas been correctly updated to reflect the newexecutorlib.task_scheduler.interactivepackage structure.tests/test_interactive_slurmspawner.py (1)
2-2: Update import path forgenerate_slurm_command
The import has been updated to reflect the new module location underexecutorlib.task_scheduler.interactive.slurmspawner. This aligns with the refactor and matches the relevant function definition.executorlib/task_scheduler/file/backend.py (1)
6-6: Import path updated to reflect module reorganization.The import statement for
FutureItemhas been updated fromexecutorlib.cache.sharedtoexecutorlib.task_scheduler.file.shared, which is consistent with the codebase refactoring where file-based task scheduling components were moved from the cache module to a dedicated task scheduler module.executorlib/backend/cache_parallel.py (1)
7-10: Import path updated for backend functions.The import statements for
backend_load_fileandbackend_write_filewere refactored fromexecutorlib.cache.backendtoexecutorlib.task_scheduler.file.backend, consistent with the reorganization of the codebase. The import style was also changed to use multi-line format with parentheses, which improves readability.executorlib/__init__.py (1)
2-10: Import paths updated to reflect module reorganization.The import paths for executor classes have been updated to import from
executorlib.executorsubmodules instead ofexecutorlib.interfaces. This change is consistent with the broader refactoring where executor classes have been relocated.The fact that the
__all__list remains unchanged indicates that these changes maintain backward compatibility for library users who import these classes from the top-level package.executorlib/task_scheduler/file/task_scheduler.py (4)
13-18: LGTM - Import paths correctly updatedThe import paths have been properly updated to reflect the new module organization, aligning with the broader task scheduler refactoring.
27-37: LGTM - Class name and inheritance changed correctlyThe class has been properly renamed from
FileExecutortoFileTaskSchedulerand now inherits fromTaskSchedulerBase, which aligns with the refactoring effort.
30-30: LGTM - Default cache directory updatedThe default value for
cache_directoryparameter has been appropriately updated from "cache" to "file" to match the new naming convention.
116-122: LGTM - Return type updated correctlyThe factory function now correctly returns a
FileTaskSchedulerinstance instead ofFileExecutor.tests/test_singlenodeexecutor_shell_interactive.py (1)
9-9: LGTM - Import path correctly updatedThe import path for
execute_taskshas been correctly updated to reflect the new module organization, aligning with the task scheduler refactoring.tests/test_base_executor_queue.py (1)
5-5: LGTM - Import path correctly updatedThe import path for
cancel_items_in_queuehas been correctly updated to reflect the new module organization, aligning with the broader refactoring effort.tests/test_singlenodeexecutor_cache.py (2)
25-25: Rename cache directory path. Updated the cache directory from"./cache"to"./file"intest_cache_data, aligning with the global refactoring of default cache naming.
45-46: Update teardown cleanup path. ThetearDownmethod now removes the"file"directory instead of"cache", ensuring tests clean up the correct directory.tests/test_singlenodeexecutor_shell_executor.py (1)
8-8: Import path updated correctly.execute_tasksis now imported fromexecutorlib.task_scheduler.interactive.shared, matching the new module layout. All subsequent calls toexecute_tasksremain valid.tests/test_fluxclusterexecutor.py (2)
36-39: Update cache and working directory parameters. The test now passescwd="file"withinresource_dictandcache_directory="file"toFluxClusterExecutor, reflecting the renamed directory convention across executors.
47-48: Update teardown to remove new directory. ThetearDownmethod has been adjusted to clean up the"file"directory, matching the updated cache path.tests/test_standalone_hdf.py (6)
29-29: Consistent cache directory renamingPath renamed from "cache" to "file" as part of the task scheduler refactoring.
49-49: Consistent cache directory renamingPath renamed from "cache" to "file" as part of the task scheduler refactoring.
66-66: Consistent cache directory renamingPath renamed from "cache" to "file" as part of the task scheduler refactoring.
92-92: Consistent cache directory renamingPath renamed from "cache" to "file" as part of the task scheduler refactoring.
108-108: Consistent cache directory renamingPath renamed from "cache" to "file" as part of the task scheduler refactoring.
123-124: Consistent tearDown cleanup for renamed cache directoryUpdated to check and remove the "file" directory instead of "cache" in the tearDown method.
tests/test_singlenodeexecutor_dependencies.py (1)
8-9: Updated import paths for refactored modulesThe import paths have been correctly updated to reflect the module reorganization:
create_single_node_executormoved frominterfaces.singletoexecutor.single_execute_tasks_with_dependenciesmoved frominteractive.dependencytotask_scheduler.interactive.dependencyThese changes are consistent with the broader refactoring of executor classes to task scheduler-based implementations.
tests/test_singlenodeexecutor_mpi.py (2)
86-86: Consistent cache directory renaming in tearDownUpdated to remove the "./file" directory instead of "./cache" in the tearDown method.
96-96: Consistent cache directory renaming in SingleNodeExecutorUpdated the cache_directory parameter from "./cache" to "./file" to maintain consistency with the refactored naming convention.
notebooks/1-single-node.ipynb (4)
533-533: Consistent cache directory renamingUpdated the SingleNodeExecutor initialization to use "./file" as the cache directory path instead of "./cache" to align with the refactored naming convention.
566-566: Consistent cache directory renamingUpdated the SingleNodeExecutor initialization to use "./file" as the cache directory path instead of "./cache" to align with the refactored naming convention.
667-667: Consistent cache directory renaming in get_cache_data callUpdated the cache_directory parameter to "./file" to maintain consistency with the refactored naming convention.
697-697: Consistent cache directory renaming in cleanup variableUpdated the cache directory variable used for listing and cleanup operations to "./file" to maintain consistency with the refactored naming convention.
tests/test_interactive_dependencies.py (5)
8-8: Import path updated to reflect rename from executor to task schedulerThe import has been updated to use the new module path, correctly aligning with the broader refactoring where executor classes have been renamed to task scheduler classes.
21-25: Class name updated from BlockAllocationExecutor to BlockAllocationTaskSchedulerThis change is consistent with the broader refactoring pattern where interactive executors have been renamed to task schedulers.
37-41: Class name updated in multi-core testThe class name has been correctly updated here as well, maintaining consistent naming throughout the test suite.
70-74: Updated class name in independence testThe class instantiation has been properly updated to use the new name while maintaining the same parameters and behavior.
111-115: Updated class name in nested class testThe final instance of the class name has been updated correctly within the Foo class implementation.
executorlib/task_scheduler/file/shared.py (4)
113-114: File naming convention updated from "cache.h5out" to "file.h5out"This change is part of a broader refactoring to update naming conventions from "cache" to "file" throughout the codebase, providing consistency across file references.
117-117: Input file path updated to use "file.h5in"The input file path has been updated to follow the new naming convention.
144-144: Output file path updated to use "file.h5out"Updated consistently with the other file path changes.
200-200: Updated file path in _check_task_output functionThe file path in the task output checking function has been updated to match the new naming convention.
notebooks/2-hpc-cluster.ipynb (2)
100-100: Updated cache_directory parameter to use "./file"The cache directory path has been updated from "./cache" to "./file" to align with the broader refactoring of directory naming conventions.
173-173: Cache directory variable updated to "./file"The cache directory variable reference has been updated to use the new naming convention.
tests/test_cache_backend_execute.py (10)
8-9: Updated import paths to use task_scheduler.file modulesThe imports have been updated to reflect the new module organization, importing from
executorlib.task_scheduler.fileinstead of the previousexecutorlib.cachemodule.
31-31: Cache directory and file paths updated to use "file" naming conventionCache directory and file paths in the first test have been updated to use the new naming convention.
Also applies to: 38-38
49-50: Output file path updated for runtime checking and FutureItemThe file paths for runtime checking and FutureItem instantiation have been updated to use the "file.h5out" naming convention.
Also applies to: 53-54
59-59: Cache directory and file paths updated in test_execute_function_argsThe directory and file paths in the second test have been consistently updated.
Also applies to: 66-66
77-78: Output file paths updated in the second testFile paths for runtime checking and FutureItem have been updated in the second test as well.
Also applies to: 81-82
87-87: Cache directory and file paths updated in test_execute_function_kwargsDirectory and file paths in the third test have been updated to match the new convention.
Also applies to: 94-94
105-106: Output file paths updated in the third testFile paths for runtime checking and FutureItem in the third test have been properly updated.
Also applies to: 109-110
115-115: Cache directory and file paths updated in test_execute_function_errorThe directory and file paths in the error test case have been updated to use the new naming convention.
Also applies to: 122-122
134-135: Output file paths updated in the error test caseRuntime checking and FutureItem file paths have been updated in the error test as well.
Also applies to: 138-139
145-146: Updated tearDown method to clean up "file" directoryThe tearDown method has been updated to remove the "file" directory instead of the "cache" directory, ensuring proper cleanup after tests.
tests/test_cache_fileexecutor_mpi.py (3)
6-7: Changes to import paths aligned with task scheduler refactoring.The imports have been updated to reflect the reorganization from the executor-based architecture to task scheduler-based architecture. This is consistent with the broader refactoring effort.
Also applies to: 10-11
34-36: Class usage updated correctly.
FileExecutorhas been renamed toFileTaskSchedulerwhile maintaining the same parameters and functionality. The constructor parameters remain unchanged, which ensures backward compatibility.
43-44: Cache directory renamed consistently.The cache directory name has been updated from "cache" to "file" in the tearDown method, which aligns with the broader naming convention changes in the codebase.
executorlib/standalone/hdf.py (1)
111-112:✅ Verification successful
H5 cache filename updated consistently.
The filename pattern for output files has been changed from "cache.h5out" to "file.h5out", aligning with the project-wide renaming of cache-related files and directories.
🏁 Script executed:
#!/bin/bash # Verify that no references to "cache.h5out" remain in the codebase to ensure consistency echo "Checking for any remaining references to cache.h5out..." rg "cache\.h5out" -g "!*.md" -g "!*/\.git/*" echo -e "\nChecking for consistent usage of file.h5out..." rg "file\.h5out"Length of output: 1888
H5 cache output filename rename verified
All instances of"cache.h5out"have been removed and replaced with"file.h5out"consistently across the codebase.Key locations updated:
- executorlib/standalone/hdf.py
- executorlib/task_scheduler/interactive/shared.py
- executorlib/task_scheduler/file/shared.py
- tests/test_cache_backend_execute.py
notebooks/3-hpc-job.ipynb (2)
186-189: Cache directory parameter updated consistently.The cache directory parameter in the FluxJobExecutor constructor has been changed from "./cache" to "./file", maintaining the same functionality while aligning with the project-wide cache directory renaming.
209-215: Cleanup code updated for new cache directory name.The cleanup code correctly references the new "./file" directory to ensure proper cleanup after execution. This maintains the same behavior while using the updated directory name.
tests/test_fluxpythonspawner.py (3)
8-10: Imports updated to reflect new task scheduler module structure.The imports have been updated to use the new
task_schedulermodule path, and the class name has been changed fromBlockAllocationExecutortoBlockAllocationTaskSchedulerto reflect the refactoring of the core architecture.
14-16: FluxPythonSpawner import updated consistently.The import for FluxPythonSpawner has been updated to use the new module path in the task_scheduler namespace, maintaining consistency with other import changes.
51-56:✅ Verification successful
Class instantiations updated consistently throughout test file.
All instances of
BlockAllocationExecutorhave been correctly replaced withBlockAllocationTaskSchedulerwhile maintaining the same parameter structure and usage pattern, ensuring the tests continue to work as expected.Also applies to: 64-71, 80-88, 94-102, 141-149
🏁 Script executed:
#!/bin/bash # Verify that no references to BlockAllocationExecutor remain in the test file echo "Checking for any remaining references to BlockAllocationExecutor..." rg "BlockAllocationExecutor" tests/test_fluxpythonspawner.py echo -e "\nVerifying all imports and usage of BlockAllocationTaskScheduler..." rg "BlockAllocationTaskScheduler" tests/test_fluxpythonspawner.pyLength of output: 715
All references to BlockAllocationExecutor removed in tests/test_fluxpythonspawner.py
- Ran
rg "BlockAllocationExecutor"against tests/test_fluxpythonspawner.py and found no occurrences.- Confirmed each import and usage has been updated to
BlockAllocationTaskScheduler.LGTM.
notebooks/4-developer.ipynb (3)
83-83: Documentation updated to reflect refactored class names.The text has been correctly updated to reflect the renaming from
*Executorclasses to*TaskSchedulerclasses.
85-86: Table header correctly aligned with renamed classes.The table header formatting has been appropriately updated to match the renamed task scheduler classes.
87-99: Table contents updated consistently with the refactoring.All table rows have been updated to reflect the new task scheduler class names while maintaining the same functionality mappings between executors and their underlying task schedulers.
executorlib/task_scheduler/interactive/blockallocation.py (4)
11-13: Import paths correctly updated to match refactored module structure.The imports have been properly updated to reflect the new module structure:
cancel_items_in_queuenow fromexecutorlib.standalone.queueTaskSchedulerBasefromexecutorlib.task_scheduler.baseexecute_tasksfromexecutorlib.task_scheduler.interactive.shared
16-16: Class renamed from BlockAllocationExecutor to BlockAllocationTaskScheduler.The class has been appropriately renamed to better reflect its responsibility as a task scheduler rather than an executor.
32-33: Example import path and class reference updated.The documentation example has been updated to show the correct import path and class name.
43-43: Example instance creation updated.The example showing how to use the class has been updated to reflect the new class name.
executorlib/task_scheduler/interactive/onetoone.py (4)
6-7: Import paths correctly updated to match refactored module structure.Imports have been properly updated to reflect the new module structure:
TaskSchedulerBasefromexecutorlib.task_scheduler.baseexecute_tasksfromexecutorlib.task_scheduler.interactive.shared
10-10: Class renamed from OneTaskPerProcessExecutor to OneProcessTaskScheduler.The class has been renamed to better reflect its responsibility as a task scheduler. The naming is also more concise now.
26-26: Example import path updated.The documentation example has been updated to show the correct import path for the renamed class.
34-34: Example instance creation updated.The example showing how to use the class has been updated to reflect the new class name.
executorlib/task_scheduler/interactive/dependency.py (4)
18-18: Import path updated to use TaskSchedulerBase.The import has been updated to use
TaskSchedulerBasefrom the new module path.
21-21: Class renamed from DependencyExecutor to DependencyTaskScheduler.The class has been appropriately renamed to better reflect its responsibility as a task scheduler.
41-41: Parameter type annotation updated.The executor parameter type has been updated from
ExecutorBasetoTaskSchedulerBaseto align with the refactored class hierarchy.
191-191: Parameter type annotation updated in function signature.The executor parameter type in the
_execute_tasks_with_dependenciesfunction has been updated fromExecutorBasetoTaskSchedulerBase.tests/test_cache_fileexecutor_serial.py (1)
39-47: Context manager relies on__enter__which is missing inFileTaskScheduler’s ancestry
with FileTaskScheduler(...) as exe:will succeed only ifFileTaskScheduler(or one of its bases) implements__enter__/__exit__.
TaskSchedulerBaseshown in the snippets does not define them, therefore the context-manager protocol will raiseAttributeError: __enter__at runtime.Please verify that
FileTaskSchedulerreally implements__enter__; if not, either:
- add the methods to
TaskSchedulerBase/FileTaskScheduler; or- drop the
with … as …syntax in the tests.Also applies to: 61-64, 70-72, 77-82
executorlib/executor/flux.py (1)
21-24: Silently swallowingImportErrorcan hide configuration issues
contextlib.suppress(ImportError)means that a missingFluxPythonSpawnerwill only be detected deep at runtime, producing hard-to-debug failures. Consider failing fast unless this module is genuinely optional and guarded later.Would raising a clear exception with installation instructions be acceptable here?
tests/test_mpiexecspawner.py (1)
10-15: Imports updated correctly to the new task-scheduler APIThe test suite now targets the refactored classes without further changes—good job keeping the tests in sync.
| cache_dir = os.path.abspath("file") | ||
| os.makedirs(cache_dir, exist_ok=True) | ||
| process = Thread( |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid re-using the hard-coded “file” directory across tests
All three parametrised tests plus tearDown point to the same on-disk path (os.path.abspath("file")).
When the suite is executed in parallel (e.g. pytest -n auto) or re-run without a clean workspace, races or stale artefacts can break determinism.
Consider isolating test artefacts via tempfile.TemporaryDirectory():
-import os
+import os, tempfile
...
-cache_dir = os.path.abspath("file")
-os.makedirs(cache_dir, exist_ok=True)
+with tempfile.TemporaryDirectory(prefix="executorlib_") as cache_dir:
+ # keep the rest of the logic unchangedtearDown then becomes unnecessary and can be removed to speed up the suite by avoiding needless I/O.
Also applies to: 137-139, 178-180, 205-208
| """ | ||
| self._task_scheduler.shutdown(wait=wait, cancel_futures=cancel_futures) | ||
|
|
||
| def __len__(self) -> int: | ||
| """ | ||
| Get the length of the executor. | ||
|
|
||
| Returns: | ||
| int: The length of the executor. | ||
| """ | ||
| return len(self._task_scheduler) | ||
|
|
||
| def __exit__(self, *args, **kwargs) -> None: | ||
| """ | ||
| Exit method called when exiting the context manager. | ||
| """ | ||
| self._task_scheduler.__exit__(*args, **kwargs) |
There was a problem hiding this comment.
Missing __enter__ breaks context-manager usage
ExecutorBase implements __exit__ but not __enter__.
Any subclass used with a with-statement (see the doctstrings for the Slurm executors) will fail.
Add a trivial pass-through implementation:
+ def __enter__(self):
+ # Delegate to the underlying task-scheduler’s context behaviour if present
+ if hasattr(self._task_scheduler, "__enter__"):
+ self._task_scheduler.__enter__()
+ return self📝 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.
| """ | |
| self._task_scheduler.shutdown(wait=wait, cancel_futures=cancel_futures) | |
| def __len__(self) -> int: | |
| """ | |
| Get the length of the executor. | |
| Returns: | |
| int: The length of the executor. | |
| """ | |
| return len(self._task_scheduler) | |
| def __exit__(self, *args, **kwargs) -> None: | |
| """ | |
| Exit method called when exiting the context manager. | |
| """ | |
| self._task_scheduler.__exit__(*args, **kwargs) | |
| def __len__(self) -> int: | |
| """ | |
| Get the length of the executor. | |
| Returns: | |
| int: The length of the executor. | |
| """ | |
| return len(self._task_scheduler) | |
| def __enter__(self): | |
| # Delegate to the underlying task-scheduler’s context behaviour if present | |
| if hasattr(self._task_scheduler, "__enter__"): | |
| self._task_scheduler.__enter__() | |
| return self | |
| def __exit__(self, *args, **kwargs) -> None: | |
| """ | |
| Exit method called when exiting the context manager. | |
| """ | |
| self._task_scheduler.__exit__(*args, **kwargs) |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #637 +/- ##
==========================================
+ Coverage 96.65% 96.67% +0.01%
==========================================
Files 28 29 +1
Lines 1287 1293 +6
==========================================
+ Hits 1244 1250 +6
Misses 43 43 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tests/test_fluxjobexecutor_plot.py (1)
13-13: Update import forFluxPythonSpawner
The import path has been correctly updated toexecutorlib.task_scheduler.interactive.fluxspawner, aligning with the new module structure. However, sinceFluxPythonSpawneris only used to catch anImportError(to decide whether to skip tests) and isn’t referenced anywhere else in this file, linters will flag it as unused (F401). To suppress that warning, add a# noqa: F401comment:-from executorlib.task_scheduler.interactive.fluxspawner import FluxPythonSpawner +from executorlib.task_scheduler.interactive.fluxspawner import FluxPythonSpawner # noqa: F401🧰 Tools
🪛 Ruff (0.8.2)
13-13:
executorlib.task_scheduler.interactive.fluxspawner.FluxPythonSpawnerimported but unused; consider usingimportlib.util.find_specto test for availability(F401)
tests/test_fluxjobexecutor.py (1)
11-11: Remove or refactor unusedFluxPythonSpawnerimport
TheFluxPythonSpawnerclass is imported in thetryblock but never actually used in this test file, causing an F401 lint warning. Since you’re only checking for module availability, consider one of the following:
- Remove the import entirely and use
importlib.util.find_specto test for the module’s presence.- Keep the import but add a
# noqa: F401comment to suppress the unused‐import warning if you prefer to fail fast on missing modules.🧰 Tools
🪛 Ruff (0.8.2)
11-11:
executorlib.task_scheduler.interactive.fluxspawner.FluxPythonSpawnerimported but unused; consider usingimportlib.util.find_specto test for availability(F401)
executorlib/executor/slurm.py (2)
377-377: Update return type documentation to match implementation.The return type annotation has been correctly updated to
Union[OneProcessTaskScheduler, BlockAllocationTaskScheduler], but the docstring still mentions returning "InteractiveStepExecutor/ InteractiveExecutor" which is outdated.Update the docstring to match the new return types:
Returns: - InteractiveStepExecutor/ InteractiveExecutor + OneProcessTaskScheduler or BlockAllocationTaskSchedulerAlso applies to: 413-415
22-81: Consider updating remaining "Executor" references in docstrings.While the class implementation has been refactored to use task schedulers, the docstrings still reference "executorlib.Executor" and various executor classes that may have been renamed or refactored.
Consider updating these docstrings to reflect the new architecture with task schedulers and the
ExecutorBasewrapper to maintain consistency with the code changes.Also applies to: 198-260
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
executorlib/executor/flux.py(15 hunks)executorlib/executor/slurm.py(14 hunks)tests/test_base_executor_queue.py(1 hunks)tests/test_cache_backend_execute.py(6 hunks)tests/test_cache_fileexecutor_mpi.py(3 hunks)tests/test_cache_fileexecutor_serial.py(7 hunks)tests/test_fluxjobexecutor.py(1 hunks)tests/test_fluxjobexecutor_plot.py(1 hunks)tests/test_fluxpythonspawner.py(6 hunks)tests/test_interactive_dependencies.py(5 hunks)tests/test_interactive_slurmspawner.py(1 hunks)tests/test_mpiexecspawner.py(26 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/test_cache_backend_execute.py
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/test_base_executor_queue.py
- tests/test_interactive_slurmspawner.py
- tests/test_interactive_dependencies.py
- tests/test_cache_fileexecutor_mpi.py
- tests/test_fluxpythonspawner.py
- executorlib/executor/flux.py
- tests/test_cache_fileexecutor_serial.py
- tests/test_mpiexecspawner.py
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/test_fluxjobexecutor.py (1)
executorlib/task_scheduler/interactive/fluxspawner.py (1)
FluxPythonSpawner(23-151)
tests/test_fluxjobexecutor_plot.py (1)
executorlib/task_scheduler/interactive/fluxspawner.py (1)
FluxPythonSpawner(23-151)
🪛 GitHub Check: codecov/patch
executorlib/executor/slurm.py
[warning] 155-155: executorlib/executor/slurm.py#L155
Added line #L155 was not covered by tests
[warning] 435-435: executorlib/executor/slurm.py#L435
Added line #L435 was not covered by tests
🪛 Ruff (0.8.2)
tests/test_fluxjobexecutor.py
11-11: executorlib.task_scheduler.interactive.fluxspawner.FluxPythonSpawner imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
tests/test_fluxjobexecutor_plot.py
13-13: executorlib.task_scheduler.interactive.fluxspawner.FluxPythonSpawner imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: notebooks
- GitHub Check: unittest_mpich (macos-latest, 3.13)
- GitHub Check: notebooks_integration
- 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_openmpi (macos-latest, 3.13)
- GitHub Check: unittest_win
🔇 Additional comments (3)
executorlib/executor/slurm.py (3)
3-3: Import updates aligned with the task scheduler refactoring.The imports have been updated to use the new
ExecutorBaseclass and the task scheduler implementations instead of the previous executor interfaces, which aligns well with the refactoring objectives.Also applies to: 10-18
21-21: Class inheritance updated to use ExecutorBase.Both
SlurmClusterExecutorandSlurmJobExecutornow properly inherit fromExecutorBaseas part of the refactoring from executor interfaces to task schedulers.Also applies to: 197-197
33-33: Documentation updated for directory naming convention.All references to the cache directory have been consistently updated from "cache" to "file" throughout the docstrings, aligning with the standardized naming convention change mentioned in the PR summary.
Also applies to: 110-110, 209-209, 288-288, 386-386
| from executorlib.task_scheduler.file.task_scheduler import ( | ||
| create_file_executor, | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add test coverage for file task scheduler import.
This import path has been updated to use the file task scheduler implementation, but static analysis indicates this code is not covered by tests.
Consider adding test cases that exercise this code path to ensure it works as expected after refactoring.
🏁 Script executed:
#!/bin/bash
# Find tests that might be related to this functionality
fd -t f "test.*slurm" | grep -v "__pycache__"Length of output: 84
Add missing tests for the file-based task scheduler import
The code in executorlib/executor/slurm.py (lines 155–157) now pulls in the file-based task scheduler via:
from executorlib.task_scheduler.file.task_scheduler import (
create_file_executor,
)However, our test suite has no coverage for this path (only tests/test_interactive_slurmspawner.py exists). Please add one or more tests that:
- Import and invoke create_file_executor
- Exercise the behavior of the file task scheduler in the Slurm executor context
- Live under tests/, e.g.
- tests/test_slurm_file_scheduler.py
- tests/test_executor_slurm_file_path.py
This will ensure the refactoring is validated and prevent regressions.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 155-155: executorlib/executor/slurm.py#L155
Added line #L155 was not covered by tests
| @@ -425,13 +432,13 @@ | |||
| cores=cores_per_worker, | |||
| threads_per_core=resource_dict.get("threads_per_core", 1), | |||
| ) | |||
| return BlockAllocationExecutor( | |||
| return BlockAllocationTaskScheduler( | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add test coverage for BlockAllocationTaskScheduler.
The return statement has been updated to use BlockAllocationTaskScheduler, but static analysis indicates this code path is not covered by tests.
Consider adding test cases with block_allocation=True to ensure this code path works as expected after refactoring.
🏁 Script executed:
#!/bin/bash
# Check if any tests use block_allocation=True with SlurmClusterExecutor
rg "SlurmClusterExecutor.*block_allocation\s*=\s*True" -A 5 -B 5Length of output: 66
🏁 Script executed:
#!/bin/bash
# Search for any instantiation of SlurmClusterExecutor in test files
rg "SlurmClusterExecutor" -n tests
# Search for any usage of the block_allocation parameter in tests
rg "block_allocation" -n testsLength of output: 7136
🏁 Script executed:
#!/bin/bash
# Inspect SlurmClusterExecutor usage to see if block_allocation is passed
sed -n '225,245p' tests/test_singlenodeexecutor_plot_dependency.pyLength of output: 1085
Add test coverage for SlurmClusterExecutor’s block_allocation path
Currently no test instantiates SlurmClusterExecutor with block_allocation=True, so the code at executorlib/executor/slurm.py:435 (the return BlockAllocationTaskScheduler(…) path) is never exercised.
Points to address:
- executorlib/executor/slurm.py (line 435): returns
BlockAllocationTaskSchedulerwhenblock_allocation=True - tests/ : no occurrences of
SlurmClusterExecutor(..., block_allocation=True)
Suggested test stub to include (e.g. in tests/test_slurmexecutor.py or appended to TestSlurmSubmissionExecutorWithDependencies):
def test_slurm_block_allocation_scheduler():
with SlurmClusterExecutor(block_allocation=True) as exe:
# The executor should use BlockAllocationTaskScheduler under the hood
from executorlib.executor.slurm import BlockAllocationTaskScheduler
assert isinstance(exe._task_scheduler, BlockAllocationTaskScheduler)
# Optionally submit a trivial task to ensure it still runs
future = exe.submit(lambda x: x + 1, 1)
assert future.result() == 2🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 435-435: executorlib/executor/slurm.py#L435
Added line #L435 was not covered by tests
Summary by CodeRabbit