FutureSelector: More generic interface to concurrent.futures.Future #824
Conversation
WalkthroughRefactors FutureSelector in executorlib/standalone/select.py to proxy an inner future via dynamic attribute forwarding, removes explicit delegation methods, adjusts constructor order, and changes result() to return a selected item from the inner future’s result. split_future and get_item_from_future remain unchanged in interface and behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant FS as FutureSelector
participant IF as InnerFuture
Client->>FS: result(timeout)
FS->>IF: result(timeout)
IF-->>FS: inner_result
alt inner_result is not None
FS-->>Client: inner_result[selector]
else
FS-->>Client: None
end
Note over Client,FS: Attribute access/assignment forwarding
Client->>FS: get/set non-private attribute
FS->>IF: forward via __getattr__/__setattr__
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #824 +/- ##
==========================================
- Coverage 98.17% 98.16% -0.02%
==========================================
Files 34 34
Lines 1698 1686 -12
==========================================
- Hits 1667 1655 -12
Misses 31 31 ☔ 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
executorlib/standalone/select.py (1)
23-28: Result should rely on wrapper state, not call inner.result() directlyDirectly calling the inner future bypasses the wrapper’s timeout/callback semantics. With the chaining above, this should delegate to the parent implementation.
Apply this diff:
- def result(self, timeout: Optional[float] = None) -> Any: - result = self._future.result(timeout=timeout) - if result is not None: - return result[self._selector] - else: - return None + def result(self, timeout: Optional[float] = None) -> Any: + return super().result(timeout=timeout)
🧹 Nitpick comments (1)
executorlib/standalone/select.py (1)
9-9: Initialize and chain correctly in initCall super().init first and immediately attach the propagation callback; this avoids any transient inconsistent state. Covered by the diff in my other comment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
executorlib/standalone/select.py(1 hunks)
⏰ 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). (15)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.13)
- GitHub Check: unittest_old
- GitHub Check: unittest_openmpi (ubuntu-22.04-arm, 3.13)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.11)
- GitHub Check: unittest_openmpi (macos-latest, 3.13)
- GitHub Check: unittest_openmpi (ubuntu-24.04-arm, 3.13)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.12)
- GitHub Check: unittest_mpich (macos-latest, 3.13)
- GitHub Check: unittest_flux_openmpi
- GitHub Check: unittest_win
- GitHub Check: unittest_flux_mpich
- GitHub Check: unittest_slurm_mpich
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-openmpi.yml)
- GitHub Check: notebooks_integration
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-mpich.yml)
executorlib/standalone/select.py
Outdated
| def __getattr__(self, attr: str): | ||
| if attr in ["_future", "_selector"]: | ||
| return super().__getattribute__(attr) | ||
| else: | ||
| return getattr(self._future, attr) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
getattr doesn’t delegate Future methods; cancel/done/exception act on the wrapper, not the inner future
Because these methods exist on the superclass, getattr is never invoked. The wrapper never transitions state; add_done_callback/done/cancel semantics are broken.
Apply this diff to chain inner→wrapper completion and rely on the wrapper’s own Future state (no generic getattr/setattr needed):
-from concurrent.futures import Future
+from concurrent.futures import Future, CancelledError
@@
-class FutureSelector(Future):
- def __init__(self, future: Future, selector: int | str):
- self._future = future
- self._selector = selector
- super().__init__()
-
- def __getattr__(self, attr: str):
- if attr in ["_future", "_selector"]:
- return super().__getattribute__(attr)
- else:
- return getattr(self._future, attr)
+class FutureSelector(Future):
+ def __init__(self, future: Future, selector: int | str):
+ super().__init__()
+ self._future = future
+ self._selector = selector
+ # Propagate completion/cancel/exception from inner -> wrapper
+ def _propagate(src: Future):
+ if src.cancelled():
+ Future.cancel(self) # mark wrapper as cancelled
+ return
+ try:
+ res = src.result()
+ val = res[self._selector] if res is not None else None
+ Future.set_result(self, val)
+ except BaseException as e:
+ Future.set_exception(self, e)
+ self._future.add_done_callback(_propagate)
+
+ # Propagate cancel from wrapper -> inner
+ def cancel(self) -> bool:
+ ok = self._future.cancel()
+ if ok:
+ Future.cancel(self)
+ return ok📝 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 __getattr__(self, attr: str): | |
| if attr in ["_future", "_selector"]: | |
| return super().__getattribute__(attr) | |
| else: | |
| return getattr(self._future, attr) | |
| from concurrent.futures import Future, CancelledError | |
| class FutureSelector(Future): | |
| def __init__(self, future: Future, selector: int | str): | |
| super().__init__() | |
| self._future = future | |
| self._selector = selector | |
| # Propagate completion/cancel/exception from inner -> wrapper | |
| def _propagate(src: Future): | |
| if src.cancelled(): | |
| Future.cancel(self) # mark wrapper as cancelled | |
| return | |
| try: | |
| res = src.result() | |
| val = res[self._selector] if res is not None else None | |
| Future.set_result(self, val) | |
| except BaseException as e: | |
| Future.set_exception(self, e) | |
| self._future.add_done_callback(_propagate) | |
| # Propagate cancel from wrapper -> inner | |
| def cancel(self) -> bool: | |
| ok = self._future.cancel() | |
| if ok: | |
| Future.cancel(self) | |
| return ok |
🤖 Prompt for AI Agents
In executorlib/standalone/select.py around lines 11-15, the current __getattr__
delegates all attributes to the inner future which fails for methods that exist
on the Future superclass (cancel/done/exception/add_done_callback), so implement
explicit chaining: remove the generic __getattr__ delegation for Future methods
and instead register a done callback on the inner future that transfers
result/exception/cancellation to the wrapper via the wrapper's own
set_result/set_exception/cancel methods; override cancel to cancel the inner
future and set the wrapper cancelled state, and for
result/done/exception/add_done_callback rely on the wrapper's Future
implementation (no generic getattr/setattr) so wrapper state reflects inner
future completion.
| def __setattr__(self, name: str, value: Any): | ||
| if name in ["_future", "_selector"]: | ||
| super().__setattr__(name, value) | ||
| else: | ||
| setattr(self._future, name, value) |
There was a problem hiding this comment.
Remove setattr: it corrupts Future internals
This forwards assignments like _state/_condition to the inner future, breaking the wrapper’s own Future state and callback machinery.
Apply this diff to drop it:
- def __setattr__(self, name: str, value: Any):
- if name in ["_future", "_selector"]:
- super().__setattr__(name, value)
- else:
- setattr(self._future, name, value)📝 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 __setattr__(self, name: str, value: Any): | |
| if name in ["_future", "_selector"]: | |
| super().__setattr__(name, value) | |
| else: | |
| setattr(self._future, name, value) |
🤖 Prompt for AI Agents
In executorlib/standalone/select.py around lines 17 to 21, the custom
__setattr__ forwards all attribute assignments (including _state/_condition) to
the wrapped Future which corrupts the inner Future's internals and breaks
callback/state machinery; remove this __setattr__ method entirely so attribute
assignments operate normally on the wrapper object and let explicit attribute
forwarding be handled only where safe and necessary.
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)
executorlib/standalone/select.py (1)
20-25: Drop the custom result(); rely on the wrapper’s Future.resultOnce propagation sets the wrapper’s result, overriding result() is unnecessary and bypasses the wrapper’s callback chain.
Apply this diff to remove it:
- def result(self, timeout: Optional[float] = None) -> Any: - result = self._future.result(timeout=timeout) - if result is not None: - return result[self._selector] - else: - return None
♻️ Duplicate comments (1)
executorlib/standalone/select.py (1)
9-18: Fix wrapper state corruption: drop getattr/setattr, propagate completion via callback, and override cancelBlanket delegation breaks Future semantics and corrupts the wrapper’s internal state; this also mirrors prior feedback.
Apply this diff to make the wrapper a proper Future that mirrors the inner one’s completion/cancel/exception and still returns the selected item:
-from concurrent.futures import Future -from typing import Any, Optional +from concurrent.futures import Future @@ class FutureSelector(Future): def __init__(self, future: Future, selector: int | str): - self._future = future - self._selector = selector - super().__init__() + super().__init__() + self._future = future + self._selector = selector + # Propagate inner completion -> wrapper state + def _propagate(src: Future): + if src.cancelled(): + Future.cancel(self) + return + try: + res = src.result() + val = res[self._selector] if res is not None else None + Future.set_result(self, val) + except BaseException as e: + Future.set_exception(self, e) + future.add_done_callback(_propagate) - def __getattr__(self, attr: str) -> Any: - return getattr(self._future, attr) - - def __setattr__(self, name: str, value: Any): - if name in ["_future", "_selector"]: - super().__setattr__(name, value) - else: - setattr(self._future, name, value) + def cancel(self) -> bool: + ok = self._future.cancel() + if ok: + Future.cancel(self) + return ok
🧹 Nitpick comments (2)
executorlib/standalone/select.py (2)
2-2: Clean up unused typing importsAfter removing getattr/setattr/result, the typing imports are unused.
-from typing import Any, Optional
20-25: Optional: friendlier error for bad selectorIf the inner result isn’t indexable or the key is missing, callers get TypeError/IndexError/KeyError. Consider re-raising with context.
If you want, I can draft a small helper to map these to a consistent ValueError with details.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
executorlib/standalone/select.py(1 hunks)
⏰ 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.11)
- GitHub Check: unittest_openmpi (ubuntu-24.04-arm, 3.13)
- GitHub Check: unittest_openmpi (macos-latest, 3.13)
- GitHub Check: unittest_mpich (macos-latest, 3.13)
- GitHub Check: unittest_openmpi (ubuntu-22.04-arm, 3.13)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.11)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.12)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.13)
- GitHub Check: unittest_win
- GitHub Check: notebooks_integration
- GitHub Check: minimal
- GitHub Check: unittest_slurm_mpich
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-mpich.yml)
- GitHub Check: unittest_flux_openmpi
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-openmpi.yml)
- GitHub Check: notebooks
- GitHub Check: unittest_flux_mpich
| class FutureSelector(Future): | ||
| def __init__(self, future: Future, selector: int | str): | ||
| super().__init__() | ||
| self._future = future | ||
| self._selector = selector | ||
| super().__init__() |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Sanity checks before merge
- Ensure callbacks added to the wrapper receive the wrapper (not the inner) after the propagation change.
- Verify cancel() on the wrapper cancels the inner and marks the wrapper cancelled.
Run a quick repo scan for potential coupling to the previous delegation behavior:
Also applies to: 28-39, 42-55
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Call sites creating FutureSelector or using helpers
rg -nC2 -P '\b(FutureSelector|split_future|get_item_from_future)\s*\('
# Places adding callbacks to these futures
rg -nC2 -P '\.add_done_callback\s*\('
# Potential direct access to inner future fields (should be avoided)
rg -nC2 -P '\._future\b'Length of output: 12653
Sanity checks before merge — fix FutureSelector delegation
- Callbacks: add_done_callback must register callbacks on the wrapper so callbacks receive the wrapper. Current FutureSelector (init/getattr/setattr) forwards Future internals to the inner future, causing wrapper.add_done_callback to attach to the inner and callbacks to be invoked with the inner. See executorlib/standalone/select.py (FutureSelector init/getattr/setattr).
- Cancel: wrapper.cancel() must cancel the inner and mark the wrapper cancelled (tests expect this). See tests/test_standalone_select.py (fs2.cancel / fs2.cancelled assertions around lines 68–71).
- Coupling: executorlib/task_scheduler/interactive/dependency_plot.py directly reads arg._future and arg._selector (≈lines 39 and 122); update those call sites to use a public API or preserve the wrapper internals if you change delegation.
🤖 Prompt for AI Agents
In executorlib/standalone/select.py around lines 5–9, FutureSelector currently
forwards Future internals to the inner future which causes add_done_callback to
attach to the inner, prevents the wrapper from reflecting cancellation, and
breaks coupling with code that reads _future/_selector; modify FutureSelector so
add_done_callback registers callbacks on the wrapper (so callbacks receive the
wrapper), implement cancel() to call cancel() on the inner future and mark the
wrapper as cancelled (and cancelled() should reflect that), stop blindly
delegating attribute access of future internals (remove or limit
__getattr__/__setattr__ passthrough) or if you must keep internal fields,
preserve _future and _selector as attributes and update
executorlib/task_scheduler/interactive/dependency_plot.py to use public
accessors (e.g., .future and .selector) or read the preserved _future/_selector
directly; ensure wrapper forwards result/exception/state queries to the inner
but keeps control of callback registration and cancellation state.
1 participant
Summary by CodeRabbit
New Features
Breaking Changes
Chores