[Feature] Faster batching#1015
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughDependencyTaskScheduler replaces ChangesDependency Resolution Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/executorlib/task_scheduler/interactive/dependency.py`:
- Around line 346-349: The loop iterating future_dependency_lst is accessing
task_wait_dict["future_lst"] unconditionally which raises KeyError for batched
tasks created by DependencyTaskScheduler.batched(); modify the loop to first
check task_wait_dict["fn"] != "batched" (or use task_wait_dict.get("future_lst")
and skip if missing) before calling get_exception_lst(future_lst=...), and only
then call task_wait_dict["future"].set_exception(exception_lst[0]) when
exceptions exist; reference the loop over future_dependency_lst, the
task_wait_dict keys ("fn", "future_lst", "future"), get_exception_lst, and
DependencyTaskScheduler.batched() when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c8206b07-8076-408a-8f58-1db5240877fd
📒 Files selected for processing (1)
src/executorlib/task_scheduler/interactive/dependency.py
jan-janssen
force-pushed
the
faster_batching
branch
from
44fb96b to
cb96b26
Compare
for more information, see https://pre-commit.ci
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1015 +/- ##
==========================================
+ Coverage 94.22% 94.24% +0.01%
==========================================
Files 39 39
Lines 2114 2119 +5
==========================================
+ Hits 1992 1997 +5
Misses 122 122 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/executorlib/standalone/interactive/arguments.py (1)
14-15:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the docstring to match the new return type.
The docstring still describes the old return type
(list, boolean), but the function now returns only alistof futures. The readiness check has been moved tocheck_list_of_futures_is_done.📝 Proposed docstring fix
Returns: - list, boolean: list of future objects and boolean flag if all future objects are already done + list: list of future objects found in the input arguments and keyword arguments🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/executorlib/standalone/interactive/arguments.py` around lines 14 - 15, Update the function docstring's Returns section to reflect that it now returns only a list of future objects (remove the boolean flag mention); specifically, replace "list, boolean: list of future objects and boolean flag if all future objects are already done" with a concise description like "list: list of future objects" and note (if needed) that readiness is handled by check_list_of_futures_is_done. Ensure this change is applied to the Returns block in the docstring for the function in arguments.py so it matches the new return type and references check_list_of_futures_is_done for readiness checks.
🧹 Nitpick comments (1)
src/executorlib/standalone/interactive/arguments.py (1)
35-35: ⚡ Quick winConsider using
all()for better readability and performance.The current implementation builds a list and compares lengths, which is less idiomatic than using the built-in
all()function. Theall()approach is more readable and short-circuits on the first incomplete future.♻️ Proposed refactor
- return len([future for future in future_lst if future.done()]) == len(future_lst) + return all(future.done() for future in future_lst)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/executorlib/standalone/interactive/arguments.py` at line 35, Replace the list-construction length comparison with Python's all() using a generator to improve readability and short-circuiting: instead of building [future for future in future_lst if future.done()] and comparing lengths, call all(future.done() for future in future_lst) so the check over future_lst uses future.done() directly and stops early on the first incomplete future.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/executorlib/standalone/interactive/arguments.py`:
- Around line 34-35: Add a concise docstring to the public helper function
check_list_of_futures_is_done explaining its purpose (returns True when all
futures in the given list are complete), documenting the parameter (future_lst:
list[Future]) and return value (bool), and explicitly noting the edge case that
an empty list returns True; place this docstring immediately below the def
check_list_of_futures_is_done(...) line so tools and readers can discover the
behavior.
---
Outside diff comments:
In `@src/executorlib/standalone/interactive/arguments.py`:
- Around line 14-15: Update the function docstring's Returns section to reflect
that it now returns only a list of future objects (remove the boolean flag
mention); specifically, replace "list, boolean: list of future objects and
boolean flag if all future objects are already done" with a concise description
like "list: list of future objects" and note (if needed) that readiness is
handled by check_list_of_futures_is_done. Ensure this change is applied to the
Returns block in the docstring for the function in arguments.py so it matches
the new return type and references check_list_of_futures_is_done for readiness
checks.
---
Nitpick comments:
In `@src/executorlib/standalone/interactive/arguments.py`:
- Line 35: Replace the list-construction length comparison with Python's all()
using a generator to improve readability and short-circuiting: instead of
building [future for future in future_lst if future.done()] and comparing
lengths, call all(future.done() for future in future_lst) so the check over
future_lst uses future.done() directly and stops early on the first incomplete
future.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e9ea483e-d360-4730-a76c-703759560aef
📒 Files selected for processing (3)
src/executorlib/standalone/interactive/arguments.pysrc/executorlib/task_scheduler/interactive/dependency.pytests/unit/standalone/interactive/test_arguments.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/executorlib/task_scheduler/interactive/dependency.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit