[Feature] Allow cache_directory in resource_dict#945
Conversation
|
Warning Rate limit exceeded
⌛ 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)
📝 WalkthroughWalkthroughAdds a new optional Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
…into cache_dir_validation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/executorlib/executor/flux.py (1)
578-580:⚠️ Potential issue | 🟠 MajorDon’t clobber
executor_kwargs["cache_directory"]withNone.Line 579 always assigns from the function argument, which drops a value supplied through
resource_dictwhen the top-levelcache_directoryis unset.💡 Proposed fix
- executor_kwargs["cache_directory"] = cache_directory + if cache_directory is not None or "cache_directory" not in executor_kwargs: + executor_kwargs["cache_directory"] = cache_directory🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/executorlib/executor/flux.py` around lines 578 - 580, The code always overwrites executor_kwargs["cache_directory"] with the local argument cache_directory (possibly None), which drops a value provided via resource_dict; change the assignment so you only set executor_kwargs["cache_directory"] when cache_directory is not None (or when the key is absent), e.g. guard the assignment with if cache_directory is not None to avoid clobbering an existing executor_kwargs["cache_directory"] value while leaving cores_per_worker and hostname_localhost behavior unchanged.src/executorlib/executor/slurm.py (1)
549-551:⚠️ Potential issue | 🟠 MajorPreserve
resource_dict["cache_directory"]when top-level arg is omitted.Line 550 unconditionally overwrites the value from
executor_kwargswithNonewhencache_directoryisn’t passed explicitly, soresource_dict={"cache_directory": ...}is ignored.💡 Proposed fix
- executor_kwargs["cache_directory"] = cache_directory + if cache_directory is not None or "cache_directory" not in executor_kwargs: + executor_kwargs["cache_directory"] = cache_directory🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/executorlib/executor/slurm.py` around lines 549 - 551, The current assignment executor_kwargs["cache_directory"] = cache_directory unconditionally overwrites any cache_directory present in resource_dict when the top-level cache_directory arg is omitted; change this to preserve resource_dict["cache_directory"] by only setting executor_kwargs["cache_directory"] when the local variable cache_directory is not None (or use executor_kwargs.setdefault("cache_directory", cache_directory) / executor_kwargs["cache_directory"] = executor_kwargs.get("cache_directory", cache_directory)), leaving executor_kwargs' existing value intact; locate this near cores_per_worker and hostname_localhost in the slurm executor function to apply the conditional/setdefault update.src/executorlib/executor/single.py (1)
508-510:⚠️ Potential issue | 🟠 MajorRespect
cache_directoryalready present inexecutor_kwargs.Line 509 currently overwrites any
resource_dict["cache_directory"]withNonewhen the top-level parameter isn’t provided, which breaks the newly documented input path.💡 Proposed fix
- executor_kwargs["cache_directory"] = cache_directory + if cache_directory is not None or "cache_directory" not in executor_kwargs: + executor_kwargs["cache_directory"] = cache_directory🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/executorlib/executor/single.py` around lines 508 - 510, The code unconditionally overwrites executor_kwargs["cache_directory"], stripping any caller-provided value; change the assignment so it only sets executor_kwargs["cache_directory"] when no value is already present (i.e., if "cache_directory" not in executor_kwargs), using the existing cache_directory variable, so callers that pass a cache_directory inside executor_kwargs are respected; keep the hostname_localhost assignment as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/executorlib/executor/flux.py`:
- Around line 578-580: The code always overwrites
executor_kwargs["cache_directory"] with the local argument cache_directory
(possibly None), which drops a value provided via resource_dict; change the
assignment so you only set executor_kwargs["cache_directory"] when
cache_directory is not None (or when the key is absent), e.g. guard the
assignment with if cache_directory is not None to avoid clobbering an existing
executor_kwargs["cache_directory"] value while leaving cores_per_worker and
hostname_localhost behavior unchanged.
In `@src/executorlib/executor/single.py`:
- Around line 508-510: The code unconditionally overwrites
executor_kwargs["cache_directory"], stripping any caller-provided value; change
the assignment so it only sets executor_kwargs["cache_directory"] when no value
is already present (i.e., if "cache_directory" not in executor_kwargs), using
the existing cache_directory variable, so callers that pass a cache_directory
inside executor_kwargs are respected; keep the hostname_localhost assignment
as-is.
In `@src/executorlib/executor/slurm.py`:
- Around line 549-551: The current assignment executor_kwargs["cache_directory"]
= cache_directory unconditionally overwrites any cache_directory present in
resource_dict when the top-level cache_directory arg is omitted; change this to
preserve resource_dict["cache_directory"] by only setting
executor_kwargs["cache_directory"] when the local variable cache_directory is
not None (or use executor_kwargs.setdefault("cache_directory", cache_directory)
/ executor_kwargs["cache_directory"] = executor_kwargs.get("cache_directory",
cache_directory)), leaving executor_kwargs' existing value intact; locate this
near cores_per_worker and hostname_localhost in the slurm executor function to
apply the conditional/setdefault update.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/executorlib/executor/flux.pysrc/executorlib/executor/single.pysrc/executorlib/executor/slurm.pysrc/executorlib/standalone/validate.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #945 +/- ##
==========================================
- Coverage 94.10% 94.06% -0.05%
==========================================
Files 39 39
Lines 2053 2054 +1
==========================================
Hits 1932 1932
- Misses 121 122 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Summary by CodeRabbit
New Features
Documentation
Tests