Create cache directory only during dump() #706
Conversation
WalkthroughThis change removes the automatic creation of the cache directory from two locations: the Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
💤 Files with no reviewable changes (2)
⏰ 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). (18)
✨ 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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #706 +/- ##
==========================================
- Coverage 96.89% 96.89% -0.01%
==========================================
Files 29 29
Lines 1321 1319 -2
==========================================
- Hits 1280 1278 -2
Misses 41 41 ☔ 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
🔭 Outside diff range comments (1)
tests/test_cache_fileexecutor_serial.py (1)
104-134: Tests must be updated to match the newexecute_tasks_h5signatureThe
execute_tasks_h5function no longer accepts a top-levelcache_directoryparameter (its signature is nowdef execute_tasks_h5( future_queue: queue.Queue, execute_function: Callable, resource_dict: dict, terminate_function: Optional[Callable] = None, pysqa_config_directory: Optional[str] = None, backend: Optional[str] = None, disable_dependencies: bool = False, ) -> None:). All occurrences in your serial tests that pass
"cache_directory": cache_dirwill fail at runtime. Please remove or relocate this argument into the appropriate place (e.g. insideresource_dictor via the new high-level executor API).Locations to fix in tests/test_cache_fileexecutor_serial.py:
- Lines 118–122
- Lines 159–163
- Lines 200–204
Each block’s
kwargsshould drop"cache_directory": cache_dir. For example:kwargs={ - "future_queue": q, - "cache_directory": cache_dir, + "future_queue": q, "execute_function": execute_in_subprocess, "resource_dict": {"cores": 1, "cwd": None}, "terminate_function": terminate_subprocess, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
executorlib/task_scheduler/file/task_scheduler.py(3 hunks)tests/test_cache_fileexecutor_mpi.py(1 hunks)tests/test_cache_fileexecutor_serial.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/test_cache_fileexecutor_mpi.py (1)
executorlib/task_scheduler/file/subprocess_spawner.py (1)
execute_in_subprocess(8-52)
tests/test_cache_fileexecutor_serial.py (3)
executorlib/task_scheduler/file/task_scheduler.py (1)
FileTaskScheduler(27-76)executorlib/task_scheduler/file/subprocess_spawner.py (1)
execute_in_subprocess(8-52)tests/test_cache_backend_execute.py (1)
my_funct(18-19)
⏰ 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: unittest_mpich (ubuntu-latest, 3.13)
- GitHub Check: unittest_mpich (ubuntu-latest, 3.11)
- GitHub Check: unittest_mpich (macos-latest, 3.13)
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-mpich.yml)
- GitHub Check: unittest_mpich (ubuntu-latest, 3.12)
- GitHub Check: unittest_flux_openmpi
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-openmpi.yml)
- GitHub Check: unittest_flux_mpich
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.11)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.12)
- GitHub Check: unittest_openmpi (macos-latest, 3.13)
- GitHub Check: unittest_old
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.13)
- GitHub Check: notebooks_integration
- GitHub Check: notebooks
- GitHub Check: unittest_win
- GitHub Check: minimal
🔇 Additional comments (4)
tests/test_cache_fileexecutor_mpi.py (1)
35-36: LGTM! Correctly adapts test to new cache directory interface.The change properly moves the cache directory specification from a separate parameter to within the
resource_dict, which aligns with the refactoring described in the PR. The explicit addition ofexecute_function=execute_in_subprocessalso makes the test more explicit about its dependencies.tests/test_cache_fileexecutor_serial.py (1)
39-42: LGTM! Consistently adapts high-level tests to new cache directory interface.The changes correctly move the cache directory specification from separate parameters to within the
resource_dict, making the tests compatible with the refactoredFileTaskSchedulerinterface.Also applies to: 49-52, 59-62, 78-81, 88-90, 97-99
executorlib/task_scheduler/file/task_scheduler.py (2)
63-63: LGTM! Correctly removes cache_directory from process kwargs.The removal of
"cache_directory": cache_directoryfromself._process_kwargsis consistent with the refactoring to move cache directory handling intoresource_dict.
96-99: LGTM! Proper cache directory handling in resource_dict.The refactoring correctly moves cache directory specification into
resource_dictand maintains proper path normalization withos.path.abspath. This approach centralizes cache directory management and aligns with the PR objective.
|
|
||
|
|
||
| def create_file_executor( | ||
| resource_dict: dict, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for required resource_dict parameter.
The function signature now requires resource_dict (no longer Optional), but there's no validation to ensure it's not None. This could cause runtime errors if someone passes None.
def create_file_executor(
resource_dict: dict,
# ... other parameters
):
+ if resource_dict is None:
+ raise ValueError("resource_dict is required and cannot be None")
if cache_directory is None:
resource_dict["cache_directory"] = os.path.abspath("executorlib_cache")Also applies to: 96-99
🤖 Prompt for AI Agents
In executorlib/task_scheduler/file/task_scheduler.py around lines 80 and 96 to
99, the resource_dict parameter is required but lacks validation to ensure it is
not None. Add explicit checks at the start of the functions to raise a
ValueError or similar if resource_dict is None, preventing runtime errors and
enforcing the parameter's required status.
jan-janssen
force-pushed
the
remove_makedirs
branch
from
452f3f5 to
6d8e451
Compare
Summary by CodeRabbit