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 all commits
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
34 changes: 8 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,18 @@

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
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) -> Any:
return getattr(self._future, attr)

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
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 +24,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