Skip to content

FutureSelector: More generic interface to concurrent.futures.Future #824

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jan-janssen merged 2 commits into from
Sep 14, 2025
Merged

FutureSelector: More generic interface to concurrent.futures.Future #824

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
03c3dd2
FutureSelector: More generic interface to concurrent.futures.Future
jan-janssen Sep 14, 2025
10c7f68
remove another line
jan-janssen Sep 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 11 additions & 26 deletions executorlib/standalone/select.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,24 +4,21 @@

class FutureSelector(Future):
def __init__(self, future: Future, selector: int | str):
super().__init__()
self._future = future
self._selector = selector
super().__init__()
Comment on lines 5 to +9
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Sep 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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.


def cancel(self) -> bool:
return self._future.cancel()

def cancelled(self) -> bool:
return self._future.cancelled()

def running(self) -> bool:
return self._future.running()

def done(self) -> bool:
return self._future.done()
def __getattr__(self, attr: str):
if attr in ["_future", "_selector"]:
return super().__getattribute__(attr)
else:
return getattr(self._future, attr)
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Sep 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

⚠️ Potential issue

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.

Suggested change
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 add_done_callback(self, fn) -> None:
return self._future.add_done_callback(fn=fn)
def __setattr__(self, name: str, value: Any):
if name in ["_future", "_selector"]:
super().__setattr__(name, value)
else:
setattr(self._future, name, value)
Comment on lines +14 to +18
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Sep 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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.


def result(self, timeout: Optional[float] = None) -> Any:
result = self._future.result(timeout=timeout)
Expand All @@ -30,18 +27,6 @@ def result(self, timeout: Optional[float] = None) -> Any:
else:
return None

def exception(self, timeout: Optional[float] = None) -> Optional[BaseException]:
return self._future.exception(timeout=timeout)

def set_running_or_notify_cancel(self) -> bool:
return self._future.set_running_or_notify_cancel()

def set_result(self, result: Any) -> None:
return self._future.set_result(result=result)

def set_exception(self, exception: Optional[BaseException]) -> None:
return self._future.set_exception(exception=exception)


def split_future(future: Future, n: int) -> list[FutureSelector]:
"""
Expand Down
Loading