[maintenance] Always use "executorlib_cache" instead of "cache" #682
Conversation
|
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 3 minutes and 55 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)
WalkthroughThe changes update the default cache directory from "cache" to "executorlib_cache" across documentation strings, class constructors, function parameters, and test cases. This affects both docstrings and the actual directory paths used in tests and one scheduler class, but does not alter any core logic or control flow. Changes
Sequence Diagram(s)Not applicable: No new features or control flow changes introduced. 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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #682 +/- ##
=======================================
Coverage 96.78% 96.78%
=======================================
Files 29 29
Lines 1307 1307
=======================================
Hits 1265 1265
Misses 42 42 ☔ 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: 3
♻️ Duplicate comments (4)
executorlib/executor/slurm.py (4)
110-111: Duplicate of the previous comment – applies here as well.
211-212: Duplicate of the previous comment – applies here as well.
291-292: Duplicate of the previous comment – applies here as well.
393-394: Duplicate of the previous comment – applies here as well.
🧹 Nitpick comments (9)
tests/test_standalone_hdf.py (1)
29-29: Test cache setup updated to use "executorlib_cache".All test methods now create and reference the
"executorlib_cache"directory, matching the new default cache location.Consider extracting the repeated
cache_directorysetup into a sharedsetUpmethod or helper to reduce duplication.Also applies to: 49-49, 66-66, 92-92, 108-108
tests/test_cache_backend_execute.py (1)
31-31: Cache directory usage updated in tests.Test methods now initialize
cache_directorywith"executorlib_cache", ensuring consistency across test setups.The repeated cache setup could be refactored into a common
setUpto adhere to DRY principles.Also applies to: 59-59, 87-87, 115-115
executorlib/task_scheduler/file/task_scheduler.py (1)
30-33: Default value changed correctly – consider promoting a shared constant.
"executorlib_cache"is now hard-coded here while several other modules still default toNoneand rely on helper functions to apply the same string later. For long-term maintainability (and to reduce copy-paste churn when this path changes again) expose a singleDEFAULT_CACHE_DIR = "executorlib_cache"constant in a well-known module (e.g.executorlib.constants) and import it everywhere.No action strictly required for this PR, but worth tracking.
tests/test_fluxclusterexecutor.py (1)
35-40: Relativecwdcould be fragile on HPC back-ends.Using a bare relative path (
"executorlib_cache") forcwdrelies on the launch directory being stable. Consider passing an absolute path to avoid surprises when jobs start in a different working directory.Example:
cwd = os.path.abspath("executorlib_cache")executorlib/executor/slurm.py (1)
33-34: Docstring says default is"executorlib_cache"but signature still defaults toNone.For clarity either:
- Change the parameter default to
"executorlib_cache"- Or keep
Noneand phrase the docstring as “IfNone, defaults to"executorlib_cache".”Option 1 reduces indirection and mirrors
FileTaskScheduler.- cache_directory: Optional[str] = None, + cache_directory: Optional[str] = "executorlib_cache",tests/test_cache_fileexecutor_serial.py (2)
96-98: Usetempfile.TemporaryDirectory()for isolation instead of a hard-coded folderHard-coding
executorlib_cacheand creating it viaos.makedirsrisks:
- leaving artefacts if the test aborts,
- race conditions when tests run in parallel,
- collisions with real user data when the repo root is a shared folder.
A minimal change:
-import os +import os, tempfile @@ - cache_dir = os.path.abspath("executorlib_cache") - os.makedirs(cache_dir, exist_ok=True) + with tempfile.TemporaryDirectory(prefix="executorlib_cache_") as cache_dir:and drop the explicit
tearDowncleanup for this directory.Also applies to: 137-139, 178-180
206-208: tearDown no longer needed iftempfileis adoptedOnce a temporary directory is used (see previous comment), this manual cleanup block becomes dead code.
tests/test_singlenodeexecutor_cache.py (2)
25-26: Same hard-coded cache directory pattern as aboveConsider switching to
tempfile.TemporaryDirectory()here as well for consistency and safer test execution.
47-48: Potential miss if CWD changes during test
tearDownremoves"executorlib_cache"relative to the current working directory, which might differ from the directory where the abs-path cache was created should another test mutateos.chdir.
Using the exact path stored earlier (e.g.,self._cache_dir) or relying onTemporaryDirectoryavoids this edge case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
executorlib/executor/flux.py(5 hunks)executorlib/executor/single.py(3 hunks)executorlib/executor/slurm.py(5 hunks)executorlib/task_scheduler/file/task_scheduler.py(2 hunks)executorlib/task_scheduler/interactive/shared.py(1 hunks)tests/test_cache_backend_execute.py(5 hunks)tests/test_cache_fileexecutor_mpi.py(1 hunks)tests/test_cache_fileexecutor_serial.py(4 hunks)tests/test_fluxclusterexecutor.py(2 hunks)tests/test_mpiexecspawner.py(3 hunks)tests/test_singlenodeexecutor_cache.py(3 hunks)tests/test_singlenodeexecutor_mpi.py(2 hunks)tests/test_standalone_hdf.py(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- 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: notebooks_integration
- 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_mpich (macos-latest, 3.13)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.13)
- GitHub Check: notebooks
- GitHub Check: unittest_win
- GitHub Check: unittest_flux_openmpi
🔇 Additional comments (10)
executorlib/task_scheduler/interactive/shared.py (1)
44-44: Docstring default updated for cache directory naming.The default cache directory in the docstring was correctly changed from
"cache"to"executorlib_cache"to align with project conventions.tests/test_cache_fileexecutor_mpi.py (1)
43-44: Cleanup path updated to new cache directory.The
tearDownmethod now removes"executorlib_cache"instead of"cache", keeping the test cleanup consistent with the standardized directory name.tests/test_standalone_hdf.py (1)
123-124: Test cleanup updated to remove "executorlib_cache".The
tearDownmethod now correctly removes the standardized cache directory after tests.tests/test_cache_backend_execute.py (1)
145-146: Cleanup directory update in tearDown.Removal logic updated to delete
"executorlib_cache"instead of"cache"in the test cleanup phase.executorlib/executor/single.py (1)
32-32: Docstring defaults updated for cache_directory.The default cache directory description was standardized to
"executorlib_cache"across the class constructor and factory function docstrings.Also applies to: 107-107, 205-205
executorlib/task_scheduler/file/task_scheduler.py (1)
41-44: Docstring kept in sync – looks good.tests/test_singlenodeexecutor_mpi.py (1)
92-99: Cache path updated consistently – LGTM.tests/test_fluxclusterexecutor.py (1)
46-49: Good safety check beforermtree.tests/test_mpiexecspawner.py (2)
520-524: Cache argument updated – looks correct.
539-543: Cache argument updated – looks correct.
| @@ -29,7 +29,7 @@ class FluxJobExecutor(BaseExecutor): | |||
| max_workers (int): for backwards compatibility with the standard library, max_workers also defines the number of | |||
| cores which can be used in parallel - just like the max_cores parameter. Using max_cores is | |||
| recommended, as computers have a limited number of compute cores. | |||
| cache_directory (str, optional): The directory to store cache files. Defaults to "cache". | |||
| cache_directory (str, optional): The directory to store cache files. Defaults to "executorlib_cache". | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Docstring says "executorlib_cache" is the default, but the code still defaults to None
All affected docstrings advertise cache_directory defaults to "executorlib_cache", yet every public signature (FluxJobExecutor.__init__, FluxClusterExecutor.__init__, create_flux_executor) still uses cache_directory: Optional[str] = None.
This discrepancy will confuse users and downstream tooling and can silently break workloads that rely on the advertised default.
Proposed quick fix – either update the signature or introduce a fallback inside create_flux_executor (preferred to keep one authoritative place):
@@
- cache_directory: Optional[str] = None,
+ cache_directory: Optional[str] = "executorlib_cache",or
@@
if resource_dict is None:
resource_dict = {}
+
+# honour advertised default
+if cache_directory is None:
+ cache_directory = "executorlib_cache"Please align implementation and documentation across the whole codebase before releasing.
Also applies to: 117-117, 221-221, 297-297, 412-412
🏁 Script executed:
#!/bin/bash
# Locate remaining occurrences where the parameter still defaults to None
rg --line-number $'cache_directory: Optional\\[str\\] = None'Length of output: 1078
Unify cache_directory default to "executorlib_cache" across the codebase
Docstrings currently state that cache_directory defaults to "executorlib_cache", but all public constructors and factory functions still use cache_directory: Optional[str] = None. This mismatch can confuse users and tooling, and may silently break downstream workloads.
Please update every occurrence of
cache_directory: Optional[str] = Noneto either:
- Change the signature default
- cache_directory: Optional[str] = None + cache_directory: Optional[str] = "executorlib_cache"
- Or add a single fallback in each factory function (preferred if you’d rather keep one authoritative place):
if cache_directory is None: cache_directory = "executorlib_cache"
Apply this change at all of the following locations:
- executorlib/task_scheduler/interactive/shared.py:24
- executorlib/task_scheduler/file/queue_spawner.py:18
- executorlib/task_scheduler/file/subprocess_spawner.py:15
- executorlib/executor/slurm.py:86, 267, 377
- executorlib/executor/single.py:84, 190
- executorlib/executor/flux.py:90, 274, 393
- executorlib/task_scheduler/file/task_scheduler.py:88
After updating, verify that every docstring and default value are in sync before releasing.
🤖 Prompt for AI Agents
In executorlib/executor/flux.py at lines 32, 90, 274, and 393, the parameter
cache_directory is documented to default to "executorlib_cache" but the function
signatures still default it to None. To fix this, add a fallback inside each
relevant factory function or constructor so that if cache_directory is None, it
is set to "executorlib_cache". This keeps one authoritative default and aligns
the implementation with the docstrings, preventing confusion and potential
silent failures.
Summary by CodeRabbit
Documentation
Bug Fixes
Refactor