Move cleanup of previous jobs into execute function #732
Conversation
When `shutdown(wait=True)` is called - the default - then the executor waits until all future objects completed. In contrast when `shutdown(wait=True)` is called the future objects are cancelled on the queuing system.
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
WalkthroughThis update refactors several task execution functions to require explicit Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant SubprocessSpawner
participant HDF5File
Caller->>SubprocessSpawner: execute_in_subprocess(command, file_name, data_dict, ...)
SubprocessSpawner->>HDF5File: Remove existing file (if any)
SubprocessSpawner->>HDF5File: Dump data_dict to file_name
SubprocessSpawner->>SubprocessSpawner: Spawn subprocess with command
sequenceDiagram
participant Caller
participant QueueSpawner
participant HDF5File
Caller->>QueueSpawner: execute_with_pysqa(command, file_name, data_dict, ...)
QueueSpawner->>HDF5File: Get queue_id from file (if exists)
alt queue_id missing or status unknown
QueueSpawner->>HDF5File: Remove and re-dump file with data_dict
else file does not exist
QueueSpawner->>HDF5File: Dump data_dict to file_name
end
QueueSpawner->>QueueSpawner: Submit job to queue
QueueSpawner->>HDF5File: Dump queue_id to file
Possibly related PRs
Suggested reviewers
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 (
|
|
@liamhuber This stacked pull request should address the challenges in #721 . I am going to open a separate pull request for the deleting of jobs with wrong resource specifications. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #732 +/- ##
==========================================
+ Coverage 96.90% 96.92% +0.01%
==========================================
Files 30 30
Lines 1357 1364 +7
==========================================
+ Hits 1315 1322 +7
Misses 42 42 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Yep, all working! |
Co-authored-by: Liam Huber <liam.huber@gmail.com>
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
executorlib/task_scheduler/file/hdf.py(1 hunks)executorlib/task_scheduler/file/queue_spawner.py(3 hunks)executorlib/task_scheduler/file/shared.py(2 hunks)executorlib/task_scheduler/file/subprocess_spawner.py(1 hunks)tests/test_cache_fileexecutor_mpi.py(1 hunks)tests/test_cache_fileexecutor_serial.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
tests/test_cache_fileexecutor_mpi.py (1)
executorlib/task_scheduler/file/subprocess_spawner.py (1)
execute_in_subprocess(10-59)
tests/test_cache_fileexecutor_serial.py (1)
executorlib/task_scheduler/file/subprocess_spawner.py (2)
execute_in_subprocess(10-59)terminate_subprocess(62-71)
executorlib/task_scheduler/file/shared.py (1)
executorlib/task_scheduler/file/hdf.py (1)
get_output(58-74)
executorlib/task_scheduler/file/queue_spawner.py (2)
executorlib/task_scheduler/file/hdf.py (2)
get_queue_id(94-108)dump(11-28)executorlib/standalone/inputcheck.py (1)
check_file_exists(196-203)
executorlib/task_scheduler/file/subprocess_spawner.py (2)
executorlib/standalone/inputcheck.py (1)
check_file_exists(196-203)executorlib/task_scheduler/file/hdf.py (1)
dump(11-28)
🔇 Additional comments (13)
tests/test_cache_fileexecutor_mpi.py (1)
8-8: LGTM! Import consolidation improves error handling.Moving the
execute_in_subprocessimport into the try block ensures it's only imported when the package is available, consistent with the conditional import pattern used throughout the test file.executorlib/task_scheduler/file/hdf.py (1)
104-104: LGTM! Defensive programming improvement.Adding the file existence check prevents h5py from attempting to open non-existent files, making the function more robust and aligning with the improved file lifecycle management across the codebase.
executorlib/task_scheduler/file/shared.py (2)
12-12: LGTM! Refactoring moves file handling responsibility.Removing the
dumpimport is consistent with the refactoring that moves file handling (cleanup of previous jobs) into the individual execute functions, as stated in the PR objectives.
159-159: LGTM! Direct data_dict passing aligns with refactoring.Passing
data_dictdirectly to the execute function is part of the refactoring to move cleanup responsibilities into the execute function, improving the separation of concerns.tests/test_cache_fileexecutor_serial.py (3)
9-12: LGTM! Import consolidation improves error handling.Moving the subprocess spawner imports into the try block ensures they're only imported when the package is available, consistent with the conditional import pattern.
203-206: LGTM! Test setup aligns with new function signature.Creating the test file and directory structure properly sets up the test environment for the updated
execute_in_subprocessfunction that now requires explicit file_name and data_dict parameters.
208-220: LGTM! Function calls updated for new signature.The updated function calls correctly pass the required
file_nameanddata_dictparameters, ensuring the tests work with the refactoredexecute_in_subprocessfunction interface.executorlib/task_scheduler/file/queue_spawner.py (2)
13-14: LGTM! Parameter changes align with refactoring objectives.Making
file_nameanddata_dictmandatory parameters enables the function to handle cleanup of previous jobs internally, which aligns with the PR objectives.
47-55: LGTM! Improved file lifecycle management.The new file handling logic effectively moves cleanup of previous jobs into the execute function. The logic correctly:
- Retrieves the queue_id from existing files
- Conditionally removes and re-dumps files when the queue_id is missing or job status is unknown
- Ensures the file is always present and synchronized before job submission
This addresses the PR objective of moving cleanup responsibilities into the execute function.
executorlib/task_scheduler/file/subprocess_spawner.py (4)
7-7: LGTM: Import addition is appropriate.The import of
dumpfunction is correctly added and necessary for the new file handling logic.
26-28: LGTM: Docstring properly updated.The docstring has been correctly updated to reflect the new parameters and their descriptions.
42-44: Excellent: Cleanup logic successfully moved into execute function.The new file handling logic properly implements the PR objective of moving cleanup into the execute function:
- Removes existing file to ensure clean state
- Dumps new data using the standardized
dumpfunction- Maintains consistency with the existing
check_file_existsvalidationThis approach ensures data consistency and prevents issues with stale or corrupted files from previous executions.
58-58: LGTM: Improved fallback directory handling.The
cwdassignment now properly usescache_directoryas a fallback whenresource_dict.get("cwd")returns None, which is more robust than the previous implementation.
| file_name: str, | ||
| data_dict: dict, | ||
| cache_directory: Optional[str] = None, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify breaking change impact across the codebase.
The function signature has been significantly changed:
file_nameis now required (was optional)data_dictis a new required parametercache_directoryparameter position has changed
This is a breaking change that will require updates to all callers of this function.
🏁 Script executed:
#!/bin/bash
# Description: Find all calls to execute_in_subprocess to verify they match the new signature
# Expected: All calls should pass file_name and data_dict as required parameters
rg -A 5 "execute_in_subprocess\(" --type pyLength of output: 1393
Update execute_in_subprocess callers to match the new signature
The recent signature change in executorlib/task_scheduler/file/subprocess_spawner.py breaks existing calls. Please update all callers—particularly in your tests—to use the new parameters:
- In tests/test_cache_fileexecutor_serial.py:
- Rename
config_directory=→cache_directory= - Remove or refactor the unsupported
backend=argument - Ensure you’re passing the three required parameters:
execute_in_subprocess( command=[…], file_name=file_name, data_dict={…}, cache_directory="test", # renamed from config_directory task_dependent_lst=None # if needed )
- Rename
- Search for any other calls to
execute_in_subprocess(and apply the same updates.
🤖 Prompt for AI Agents
In executorlib/task_scheduler/file/subprocess_spawner.py around lines 12 to 14,
the function execute_in_subprocess has updated parameters, renaming
config_directory to cache_directory and removing the backend argument. Update
all calls to execute_in_subprocess, especially in
tests/test_cache_fileexecutor_serial.py, by renaming config_directory= to
cache_directory=, removing the backend= argument, and ensuring the three
required parameters (command, file_name, data_dict) plus optional
cache_directory and task_dependent_lst are passed correctly. Also, search the
codebase for other calls to execute_in_subprocess and apply these same changes.
Summary by CodeRabbit
New Features
Bug Fixes
Tests