[Feature] Check which future objects are skipped and keep track of their IDs#1014
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughbatched_futures now collects exceptions and may return an exception payload; the dependency scheduler interprets return shapes to distinguish waiting/success/exception, and tests were updated with cases for duplicated results and mixed success/failure propagation. ChangesEnhanced exception handling in batched futures
Sequence DiagramsequenceDiagram
participant TaskScheduler
participant batched_futures
participant FutureCollection
TaskScheduler->>batched_futures: call batched_futures(lst, nested_skip_lst)
batched_futures->>FutureCollection: inspect futures (.done(), .exception(), .result())
alt non-empty batch produced
batched_futures-->>TaskScheduler: return list (done_lst)
TaskScheduler->>TaskScheduler: set_result(done_lst)
else all non-skipped failed
batched_futures-->>TaskScheduler: return BaseException
TaskScheduler->>TaskScheduler: set_exception(BaseException)
else no ready results
batched_futures-->>TaskScheduler: return []
TaskScheduler->>TaskScheduler: keep task waiting
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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 `@tests/unit/standalone/test_batched.py`:
- Around line 22-36: Add a final assertion to test_batched_futures_duplicated
that calls batched_futures with nested_skip_lst=batched_lst and asserts the
fully-consumed completion result (an empty list) is returned; specifically call
batched_futures(lst=lst, nested_skip_lst=batched_lst, n=3) and assertEqual(...,
[]). This ensures the identity-based skip_set / n_expected logic in
batched_futures and the downstream expectation of list-like done_lst are
validated.
🪄 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: dd4b1a52-4e66-49e0-93c1-c9c17f79c182
📒 Files selected for processing (1)
tests/unit/standalone/test_batched.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1014 +/- ##
==========================================
+ Coverage 94.19% 94.22% +0.03%
==========================================
Files 39 39
Lines 2103 2114 +11
==========================================
+ Hits 1981 1992 +11
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
🤖 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 378-392: The current logic computes n_expected using len(skip_set)
which loses multiplicity and then only returns when either n_expected successes
or n_expected failures occur, causing deadlocks for tail batches; fix by
computing already_consumed as the total number of skipped items (preserving
multiplicity) e.g. sum(len(f.result()) for f in nested_skip_lst) and set
n_expected = min(n, len(lst) - already_consumed), and change the termination
condition to return once len(done_lst) + len(failed_lst) == n_expected (return
done_lst if any successes else failed_lst) instead of only when one side reaches
n_expected; update references to skip_set, nested_skip_lst, n_expected,
done_lst, failed_lst and check_exception_was_raised accordingly.
🪄 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: b42001e4-82de-43c5-8f9c-39e94111cbe2
📒 Files selected for processing (4)
src/executorlib/standalone/batched.pysrc/executorlib/task_scheduler/interactive/dependency.pytests/unit/standalone/test_batched.pytests/unit/task_scheduler/interactive/test_dependency.py
💤 Files with no reviewable changes (2)
- src/executorlib/standalone/batched.py
- tests/unit/standalone/test_batched.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
tests/unit/standalone/test_batched.py (1)
23-36: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winMissing terminal test case for fully-consumed state.
As flagged in a previous review, the test should include a final assertion when all batches have been consumed (
nested_skip_lst=batched_lst). This validates the completion behavior and would expose the multiplicity-loss deadlock bug inbatched.py.Current behavior with the multiplicity bug:
batched_futures(lst=lst, nested_skip_lst=batched_lst, n=3) # skip_set = {id(1), id(2), id(3)} - length 3, should be 9 # n_expected = min(3, 9-3) = 3 (wrong, should be 0) # All futures skipped, done_lst = [] # Returns [] (keep waiting) - DEADLOCK! ✗Expected behavior after fix:
# Should return [] correctly recognizing all items consumed🧪 Suggested test addition
self.assertEqual(batched_futures(lst=lst, nested_skip_lst=batched_lst[:2], n=3), [3, 3, 3]) + self.assertEqual(batched_futures(lst=lst, nested_skip_lst=batched_lst, n=3), [])🤖 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 `@tests/unit/standalone/test_batched.py` around lines 23 - 36, Add a terminal assertion to the test_batched_futures_duplicated test to verify the fully-consumed state: call batched_futures with lst and nested_skip_lst equal to the entire batched_lst and assert it returns an empty list ([]). This ensures batched_futures correctly recognizes when all items are consumed (references: test_batched_futures_duplicated, batched_futures, batched_lst, lst).src/executorlib/standalone/batched.py (1)
20-37:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftMultiplicity loss with duplicate values causes deadlock.
This issue was flagged in a previous review and remains unresolved. When
nested_skip_lstcontains duplicate scalar values (e.g.,[1, 1, 1]), the identity-basedskip_setloses multiplicity:
- Line 20:
skip_set = {id(1), id(1), id(1)}becomes{id(1)}(one element, not three) because Python interns small integers- Line 24:
n_expected = min(n, 9 - 1)when it should bemin(n, 9 - 3)- Line 30:
id(v.result()) not in skip_setskips all futures with result1, not just the already-consumed countConcrete deadlock scenario:
# 6 futures all returning value 1, batch size 3 lst = [f1(→1), f2(→1), f3(→1), f4(→1), f5(→1), f6(→1)] # First batch succeeds batched_futures(lst, [], 3) → [1, 1, 1] ✓ # Second batch deadlocks batched_futures(lst, [Future(→[1,1,1])], 3) skip_set = {id(1)} # length 1, should track 3 consumed # All 6 futures skipped because id(1) in skip_set done_lst = [] → returns [] (keep waiting forever) ✗🔧 Recommended fix (from previous review)
+from collections import Counter + def batched_futures( lst: list[Future], nested_skip_lst: list[Future[list]], n: int -) -> list[list]: +) -> list | Exception: - skip_set = {id(item) for f in nested_skip_lst for item in f.result()} - + skip_counter = Counter(id(item) for f in nested_skip_lst for item in f.result()) done_lst = [] failed_lst = [] - n_expected = min(n, len(lst) - len(skip_set)) + n_expected = min(n, len(lst) - sum(skip_counter.values())) for v in lst: if v.done(): excp = v.exception() if excp is not None: failed_lst.append(excp) - elif id(v.result()) not in skip_set: + else: + result_id = id(v.result()) + if skip_counter[result_id] > 0: + skip_counter[result_id] -= 1 + continue done_lst.append(v.result()) if len(done_lst) == n_expected: return done_lst if len(failed_lst) == len(lst) and len(failed_lst) > 0: return failed_lst[0] else: return []🤖 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/batched.py` around lines 20 - 37, The code loses multiplicity by building skip_set = {id(...)}, so replace that with a counts map: build skip_counts (e.g., Counter of id(r) for r in nested_skip_lst results) and compute n_expected = min(n, len(lst) - sum(skip_counts.values())); then when iterating v in lst check r_id = id(v.result()) and if skip_counts.get(r_id, 0) > 0 decrement skip_counts[r_id] and continue (consume one occurrence) otherwise treat the result as non-skipped and append to done_lst; keep the existing failed_lst logic unchanged. Ensure you update references to skip_set, n_expected, and the id(v.result()) check to use skip_counts so duplicate scalar results are only skipped the correct number of times.
🧹 Nitpick comments (1)
tests/unit/standalone/test_batched.py (1)
4-4: 💤 Low valueConsider importing from the definition site.
batched_futuresis defined inexecutorlib.standalone.batchedbut imported here from the re-exporting moduledependency. While functional, direct imports improve clarity and reduce indirection.♻️ Suggested change
-from executorlib.task_scheduler.interactive.dependency import batched_futures +from executorlib.standalone.batched import batched_futures🤖 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 `@tests/unit/standalone/test_batched.py` at line 4, Replace the indirect import of batched_futures from executorlib.task_scheduler.interactive.dependency with a direct import from its definition module executorlib.standalone.batched: update the import statement in tests/unit/standalone/test_batched.py to import the symbol batched_futures from executorlib.standalone.batched so the test references the definition site (locate the current import of batched_futures and change its module path accordingly).
🤖 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/batched.py`:
- Line 35: The comment currently says "raise the exception" but the code returns
failed_lst[0]; update the comment (near the return failed_lst[0] statement in
the function that aggregates failed futures) to accurately state that the
exception object is returned to the caller for the scheduler to propagate (e.g.,
"return the exception object to be propagated by the scheduler") rather than
saying it is raised; do not change behavior.
- Around line 4-6: The return annotation for batched_futures is wrong: the
function actually returns either a flat list (done_lst), a single Exception
(failed_lst[0]), or an empty list, not list[list]; update the signature of
batched_futures to a correct union type (e.g. -> Union[List[Any], Exception])
and import the necessary typing names (from typing import Any, List, Union) and
adjust any callers if needed; reference: function batched_futures and variables
done_lst and failed_lst in the implementation.
In `@src/executorlib/task_scheduler/interactive/dependency.py`:
- Around line 351-356: The branch handling for batched results in the scheduler
is inverted: when batched_futures returns a non-empty list (success) you must
call task_wait_dict["future"].set_result(done_lst), and when it returns a
non-list exception object you must call
task_wait_dict["future"].set_exception(done_lst); keep the existing empty-list
(len==0) behavior that appends to wait_tmp_lst. Update the code around the
done_lst checks in the block that manipulates task_wait_dict (the three-way
if/elif/else using isinstance(done_lst, list)) to swap the set_result and
set_exception calls accordingly, referencing task_wait_dict and done_lst so
successful batches deliver results and exception objects are raised.
In `@tests/unit/standalone/test_batched.py`:
- Line 18: The tests call batched_futures(lst=..., n=3, nested_skip_lst=set()),
but batched_futures expects nested_skip_lst: list[Future[list]]; replace the
incorrect set() arguments with an empty list [] in all occurrences (the failing
calls at the current test lines and the other two instances noted) so the tests
match the declared parameter type for batched_futures.
---
Duplicate comments:
In `@src/executorlib/standalone/batched.py`:
- Around line 20-37: The code loses multiplicity by building skip_set =
{id(...)}, so replace that with a counts map: build skip_counts (e.g., Counter
of id(r) for r in nested_skip_lst results) and compute n_expected = min(n,
len(lst) - sum(skip_counts.values())); then when iterating v in lst check r_id =
id(v.result()) and if skip_counts.get(r_id, 0) > 0 decrement skip_counts[r_id]
and continue (consume one occurrence) otherwise treat the result as non-skipped
and append to done_lst; keep the existing failed_lst logic unchanged. Ensure you
update references to skip_set, n_expected, and the id(v.result()) check to use
skip_counts so duplicate scalar results are only skipped the correct number of
times.
In `@tests/unit/standalone/test_batched.py`:
- Around line 23-36: Add a terminal assertion to the
test_batched_futures_duplicated test to verify the fully-consumed state: call
batched_futures with lst and nested_skip_lst equal to the entire batched_lst and
assert it returns an empty list ([]). This ensures batched_futures correctly
recognizes when all items are consumed (references:
test_batched_futures_duplicated, batched_futures, batched_lst, lst).
---
Nitpick comments:
In `@tests/unit/standalone/test_batched.py`:
- Line 4: Replace the indirect import of batched_futures from
executorlib.task_scheduler.interactive.dependency with a direct import from its
definition module executorlib.standalone.batched: update the import statement in
tests/unit/standalone/test_batched.py to import the symbol batched_futures from
executorlib.standalone.batched so the test references the definition site
(locate the current import of batched_futures and change its module path
accordingly).
🪄 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: 7da6f45c-803b-4a1d-988a-b8f45f0e82af
📒 Files selected for processing (3)
src/executorlib/standalone/batched.pysrc/executorlib/task_scheduler/interactive/dependency.pytests/unit/standalone/test_batched.py
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
♻️ Duplicate comments (2)
tests/unit/standalone/test_batched.py (1)
23-36: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winMissing terminal test case for fully-consumed state.
As flagged in a previous review, the test should include a final assertion when all batches have been consumed (
nested_skip_lst=batched_lst). This validates the completion behavior and would expose the multiplicity-loss deadlock bug inbatched.py.Current behavior with the multiplicity bug:
batched_futures(lst=lst, nested_skip_lst=batched_lst, n=3) # skip_set = {id(1), id(2), id(3)} - length 3, should be 9 # n_expected = min(3, 9-3) = 3 (wrong, should be 0) # All futures skipped, done_lst = [] # Returns [] (keep waiting) - DEADLOCK! ✗Expected behavior after fix:
# Should return [] correctly recognizing all items consumed🧪 Suggested test addition
self.assertEqual(batched_futures(lst=lst, nested_skip_lst=batched_lst[:2], n=3), [3, 3, 3]) + self.assertEqual(batched_futures(lst=lst, nested_skip_lst=batched_lst, n=3), [])🤖 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 `@tests/unit/standalone/test_batched.py` around lines 23 - 36, Add a terminal assertion to the test_batched_futures_duplicated test to verify the fully-consumed state: call batched_futures with lst and nested_skip_lst equal to the entire batched_lst and assert it returns an empty list ([]). This ensures batched_futures correctly recognizes when all items are consumed (references: test_batched_futures_duplicated, batched_futures, batched_lst, lst).src/executorlib/standalone/batched.py (1)
20-37:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftMultiplicity loss with duplicate values causes deadlock.
This issue was flagged in a previous review and remains unresolved. When
nested_skip_lstcontains duplicate scalar values (e.g.,[1, 1, 1]), the identity-basedskip_setloses multiplicity:
- Line 20:
skip_set = {id(1), id(1), id(1)}becomes{id(1)}(one element, not three) because Python interns small integers- Line 24:
n_expected = min(n, 9 - 1)when it should bemin(n, 9 - 3)- Line 30:
id(v.result()) not in skip_setskips all futures with result1, not just the already-consumed countConcrete deadlock scenario:
# 6 futures all returning value 1, batch size 3 lst = [f1(→1), f2(→1), f3(→1), f4(→1), f5(→1), f6(→1)] # First batch succeeds batched_futures(lst, [], 3) → [1, 1, 1] ✓ # Second batch deadlocks batched_futures(lst, [Future(→[1,1,1])], 3) skip_set = {id(1)} # length 1, should track 3 consumed # All 6 futures skipped because id(1) in skip_set done_lst = [] → returns [] (keep waiting forever) ✗🔧 Recommended fix (from previous review)
+from collections import Counter + def batched_futures( lst: list[Future], nested_skip_lst: list[Future[list]], n: int -) -> list[list]: +) -> list | Exception: - skip_set = {id(item) for f in nested_skip_lst for item in f.result()} - + skip_counter = Counter(id(item) for f in nested_skip_lst for item in f.result()) done_lst = [] failed_lst = [] - n_expected = min(n, len(lst) - len(skip_set)) + n_expected = min(n, len(lst) - sum(skip_counter.values())) for v in lst: if v.done(): excp = v.exception() if excp is not None: failed_lst.append(excp) - elif id(v.result()) not in skip_set: + else: + result_id = id(v.result()) + if skip_counter[result_id] > 0: + skip_counter[result_id] -= 1 + continue done_lst.append(v.result()) if len(done_lst) == n_expected: return done_lst if len(failed_lst) == len(lst) and len(failed_lst) > 0: return failed_lst[0] else: return []🤖 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/batched.py` around lines 20 - 37, The code loses multiplicity by building skip_set = {id(...)}, so replace that with a counts map: build skip_counts (e.g., Counter of id(r) for r in nested_skip_lst results) and compute n_expected = min(n, len(lst) - sum(skip_counts.values())); then when iterating v in lst check r_id = id(v.result()) and if skip_counts.get(r_id, 0) > 0 decrement skip_counts[r_id] and continue (consume one occurrence) otherwise treat the result as non-skipped and append to done_lst; keep the existing failed_lst logic unchanged. Ensure you update references to skip_set, n_expected, and the id(v.result()) check to use skip_counts so duplicate scalar results are only skipped the correct number of times.
🧹 Nitpick comments (1)
tests/unit/standalone/test_batched.py (1)
4-4: 💤 Low valueConsider importing from the definition site.
batched_futuresis defined inexecutorlib.standalone.batchedbut imported here from the re-exporting moduledependency. While functional, direct imports improve clarity and reduce indirection.♻️ Suggested change
-from executorlib.task_scheduler.interactive.dependency import batched_futures +from executorlib.standalone.batched import batched_futures🤖 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 `@tests/unit/standalone/test_batched.py` at line 4, Replace the indirect import of batched_futures from executorlib.task_scheduler.interactive.dependency with a direct import from its definition module executorlib.standalone.batched: update the import statement in tests/unit/standalone/test_batched.py to import the symbol batched_futures from executorlib.standalone.batched so the test references the definition site (locate the current import of batched_futures and change its module path accordingly).
🤖 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/batched.py`:
- Line 35: The comment currently says "raise the exception" but the code returns
failed_lst[0]; update the comment (near the return failed_lst[0] statement in
the function that aggregates failed futures) to accurately state that the
exception object is returned to the caller for the scheduler to propagate (e.g.,
"return the exception object to be propagated by the scheduler") rather than
saying it is raised; do not change behavior.
- Around line 4-6: The return annotation for batched_futures is wrong: the
function actually returns either a flat list (done_lst), a single Exception
(failed_lst[0]), or an empty list, not list[list]; update the signature of
batched_futures to a correct union type (e.g. -> Union[List[Any], Exception])
and import the necessary typing names (from typing import Any, List, Union) and
adjust any callers if needed; reference: function batched_futures and variables
done_lst and failed_lst in the implementation.
In `@src/executorlib/task_scheduler/interactive/dependency.py`:
- Around line 351-356: The branch handling for batched results in the scheduler
is inverted: when batched_futures returns a non-empty list (success) you must
call task_wait_dict["future"].set_result(done_lst), and when it returns a
non-list exception object you must call
task_wait_dict["future"].set_exception(done_lst); keep the existing empty-list
(len==0) behavior that appends to wait_tmp_lst. Update the code around the
done_lst checks in the block that manipulates task_wait_dict (the three-way
if/elif/else using isinstance(done_lst, list)) to swap the set_result and
set_exception calls accordingly, referencing task_wait_dict and done_lst so
successful batches deliver results and exception objects are raised.
In `@tests/unit/standalone/test_batched.py`:
- Line 18: The tests call batched_futures(lst=..., n=3, nested_skip_lst=set()),
but batched_futures expects nested_skip_lst: list[Future[list]]; replace the
incorrect set() arguments with an empty list [] in all occurrences (the failing
calls at the current test lines and the other two instances noted) so the tests
match the declared parameter type for batched_futures.
---
Duplicate comments:
In `@src/executorlib/standalone/batched.py`:
- Around line 20-37: The code loses multiplicity by building skip_set =
{id(...)}, so replace that with a counts map: build skip_counts (e.g., Counter
of id(r) for r in nested_skip_lst results) and compute n_expected = min(n,
len(lst) - sum(skip_counts.values())); then when iterating v in lst check r_id =
id(v.result()) and if skip_counts.get(r_id, 0) > 0 decrement skip_counts[r_id]
and continue (consume one occurrence) otherwise treat the result as non-skipped
and append to done_lst; keep the existing failed_lst logic unchanged. Ensure you
update references to skip_set, n_expected, and the id(v.result()) check to use
skip_counts so duplicate scalar results are only skipped the correct number of
times.
In `@tests/unit/standalone/test_batched.py`:
- Around line 23-36: Add a terminal assertion to the
test_batched_futures_duplicated test to verify the fully-consumed state: call
batched_futures with lst and nested_skip_lst equal to the entire batched_lst and
assert it returns an empty list ([]). This ensures batched_futures correctly
recognizes when all items are consumed (references:
test_batched_futures_duplicated, batched_futures, batched_lst, lst).
---
Nitpick comments:
In `@tests/unit/standalone/test_batched.py`:
- Line 4: Replace the indirect import of batched_futures from
executorlib.task_scheduler.interactive.dependency with a direct import from its
definition module executorlib.standalone.batched: update the import statement in
tests/unit/standalone/test_batched.py to import the symbol batched_futures from
executorlib.standalone.batched so the test references the definition site
(locate the current import of batched_futures and change its module path
accordingly).
🪄 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: 7da6f45c-803b-4a1d-988a-b8f45f0e82af
📒 Files selected for processing (3)
src/executorlib/standalone/batched.pysrc/executorlib/task_scheduler/interactive/dependency.pytests/unit/standalone/test_batched.py
🛑 Comments failed to post (4)
src/executorlib/standalone/batched.py (2)
4-6:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winIncorrect return type annotation.
The function signature declares
-> list[list], but the implementation returns different types:
- Line 33: returns
done_lst(a flat list like[1, 2, 3], not nested)- Line 35: returns
failed_lst[0](a singleExceptionobject)- Line 37: returns
[](empty list)📝 Suggested fix
def batched_futures( lst: list[Future], nested_skip_lst: list[Future[list]], n: int -) -> list[list]: +) -> list | Exception:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.def batched_futures( lst: list[Future], nested_skip_lst: list[Future[list]], n: int ) -> list | Exception:🤖 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/batched.py` around lines 4 - 6, The return annotation for batched_futures is wrong: the function actually returns either a flat list (done_lst), a single Exception (failed_lst[0]), or an empty list, not list[list]; update the signature of batched_futures to a correct union type (e.g. -> Union[List[Any], Exception]) and import the necessary typing names (from typing import Any, List, Union) and adjust any callers if needed; reference: function batched_futures and variables done_lst and failed_lst in the implementation.
35-35:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMisleading comment: code returns, does not raise.
The comment states "raise the exception" but the code returns the exception object to the caller. The scheduler is responsible for propagating it via
set_exception.📝 Suggested fix
- return failed_lst[0] # raise the exception only after all futures have failed + return failed_lst[0] # return the exception to propagate after all futures have failed📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.return failed_lst[0] # return the exception to propagate after all futures have failed🤖 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/batched.py` at line 35, The comment currently says "raise the exception" but the code returns failed_lst[0]; update the comment (near the return failed_lst[0] statement in the function that aggregates failed futures) to accurately state that the exception object is returned to the caller for the scheduler to propagate (e.g., "return the exception object to be propagated by the scheduler") rather than saying it is raised; do not change behavior.src/executorlib/task_scheduler/interactive/dependency.py (1)
351-356:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical:
set_resultandset_exceptioncalls are swapped.The type-based dispatch interprets
batched_futuresreturn values incorrectly. Based on the implementation inbatched.py:
- Line 33 returns a list of results (e.g.,
[1, 2, 3]) on success- Line 35 returns a single exception object when all futures fail
- Line 37 returns
[]when incompleteCurrent scheduler logic (INCORRECT):
- Line 351: Empty list
[]→ keep waiting ✓- Line 353-354: Non-list (exception) →
set_result(done_lst)✗ Sets exception as result!- Line 355-356: Non-empty list (results) →
set_exception(done_lst)✗ Raises results list as exception!Concrete failure examples:
Example 1: Successful batch raises as exception
# 3 futures complete successfully batched_futures(...) → [1, 2, 3] # Scheduler line 351: isinstance([1,2,3], list) and len == 0 → False # Scheduler line 353: not isinstance([1,2,3], list) → False # Scheduler line 355: else → set_exception([1,2,3]) # Result: future.result() raises [1,2,3] as an exception! ✗Example 2: Exception returned as result
# All futures fail batched_futures(...) → RuntimeError("failed") # Scheduler line 351: isinstance(exc, list) → False # Scheduler line 353: not isinstance(exc, list) → True → set_result(exc) # Result: future.result() returns the exception object, doesn't raise it! ✗🐛 Proposed fix
done_lst = batched_futures( lst=task_wait_dict["kwargs"]["lst"], n=task_wait_dict["kwargs"]["n"], nested_skip_lst=task_wait_dict["kwargs"]["skip_lst"], ) if isinstance(done_lst, list) and len(done_lst) == 0: wait_tmp_lst.append(task_wait_dict) - elif not isinstance(done_lst, list): - task_wait_dict["future"].set_result(done_lst) - else: + elif isinstance(done_lst, list): + task_wait_dict["future"].set_result(done_lst) + else: task_wait_dict["future"].set_exception(done_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/task_scheduler/interactive/dependency.py` around lines 351 - 356, The branch handling for batched results in the scheduler is inverted: when batched_futures returns a non-empty list (success) you must call task_wait_dict["future"].set_result(done_lst), and when it returns a non-list exception object you must call task_wait_dict["future"].set_exception(done_lst); keep the existing empty-list (len==0) behavior that appends to wait_tmp_lst. Update the code around the done_lst checks in the block that manipulates task_wait_dict (the three-way if/elif/else using isinstance(done_lst, list)) to swap the set_result and set_exception calls accordingly, referencing task_wait_dict and done_lst so successful batches deliver results and exception objects are raised.tests/unit/standalone/test_batched.py (1)
18-18:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winType mismatch: passing
set()instead of empty list.The
batched_futuressignature expectsnested_skip_lst: list[Future[list]], but these test calls passset(). While this works by accident (iterating an empty set produces no items), it violates the type contract.🔧 Proposed fix
- self.assertEqual(batched_futures(lst=lst, n=3, nested_skip_lst=set()), [0, 1, 2]) + self.assertEqual(batched_futures(lst=lst, n=3, nested_skip_lst=[]), [0, 1, 2])Apply the same change to lines 34 and 43.
Also applies to: 34-34, 43-43
🤖 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 `@tests/unit/standalone/test_batched.py` at line 18, The tests call batched_futures(lst=..., n=3, nested_skip_lst=set()), but batched_futures expects nested_skip_lst: list[Future[list]]; replace the incorrect set() arguments with an empty list [] in all occurrences (the failing calls at the current test lines and the other two instances noted) so the tests match the declared parameter type for batched_futures.
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/batched.py (1)
24-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't return
[]after the batch has reached a terminal state.
len(failed_lst) == len(lst)only covers the "everything failed and nothing was skipped" case. After earlier batches have populatednested_skip_lst, or when some inputs succeed and the rest fail, this can still return[]even though every unskipped future is already done and no further results can arrive._update_waiting_task()treats[]as "keep waiting", so the batched task can hang forever. Please base the terminal check on the remaining unskipped futures, not the originallen(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/batched.py` around lines 24 - 37, The terminal-state check is wrong: don't compare failed_lst against len(lst) because some items may be skipped; instead compute the number of unskipped futures (remaining = len(lst) - len(skip_set) or use n_expected) and treat the batch as terminal when done_lst.size + failed_lst.size == remaining; return done_lst when done_lst.size == remaining, or return the first exception (or appropriate aggregate) when failed_lst.size == remaining; update the condition that currently reads "if len(failed_lst) == len(lst) and len(failed_lst) > 0" to use remaining/unskipped counts (n_expected or len(lst)-len(skip_set)) so _update_waiting_task() doesn't interpret a terminal state as "keep waiting".
🤖 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/batched.py`:
- Line 6: The return-shape contract in src/executorlib/standalone/batched.py was
inverted vs _update_waiting_task: change the function (signature currently ")->
list[list] | BaseException") so that success produces a non-list result (e.g.,
the expected success value or None) and failures produce a non-empty list of
exception(s); update the annotation and every return site in this function
(including the returns around the block corresponding to lines 32-35 in the
diff) to return a list[list] for failure and a non-list for success so it aligns
with _update_waiting_task's handling, or if you prefer the other approach, apply
the inverse change to _update_waiting_task so it treats BaseException returns as
failures—prefer changing batched.py to match existing _update_waiting_task
behavior.
---
Outside diff comments:
In `@src/executorlib/standalone/batched.py`:
- Around line 24-37: The terminal-state check is wrong: don't compare failed_lst
against len(lst) because some items may be skipped; instead compute the number
of unskipped futures (remaining = len(lst) - len(skip_set) or use n_expected)
and treat the batch as terminal when done_lst.size + failed_lst.size ==
remaining; return done_lst when done_lst.size == remaining, or return the first
exception (or appropriate aggregate) when failed_lst.size == remaining; update
the condition that currently reads "if len(failed_lst) == len(lst) and
len(failed_lst) > 0" to use remaining/unskipped counts (n_expected or
len(lst)-len(skip_set)) so _update_waiting_task() doesn't interpret a terminal
state as "keep waiting".
🪄 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: ff106972-5ca8-48ca-92a7-7c45a5895d71
📒 Files selected for processing (1)
src/executorlib/standalone/batched.py
jan-janssen
force-pushed
the
batched_duplicate
branch
from
aee97d9 to
87d38ca
Compare
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 `@tests/unit/standalone/test_batched.py`:
- Line 38: The test function test_batched_futures is defined twice, causing the
later definition to override the earlier one; rename the duplicate (the second
occurrence) to a unique name (e.g., test_batched_futures_with_<scenario>) or
merge its assertions into the original so both behaviors are tested; update any
references or fixtures used by the renamed function to match the new name.
🪄 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: 1684885a-df3b-4bb8-a47c-bc9e9ec3622f
📒 Files selected for processing (3)
src/executorlib/standalone/batched.pysrc/executorlib/task_scheduler/interactive/dependency.pytests/unit/standalone/test_batched.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/executorlib/standalone/batched.py
jan-janssen
changed the title
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
Bug Fixes
Tests