Conversation
📝 WalkthroughWalkthroughAdds a decorator/descriptor framework and per-call override settings, introduces PromisingFunction.run/protected_run, named sentinels (RECURSIVELY) and trace/child-inspection APIs, reorganizes error hierarchy, enforces event-loop identity on awaiting Promises, changes default child-awaiting to recursive, and adds many tests and examples. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as CallSite
participant PF as PromisingFunction.run
participant AR as asyncio.run
participant PR as protected_run()
participant Ctx as PromisingContext
participant Promise as Promise
Caller->>PF: call .run(*args, await_children=RECURSIVELY)
PF->>AR: asyncio.run(self.protected_run(...))
AR->>PR: start protected_run coroutine
PR->>Ctx: create/enter PromisingContext (select running loop)
PR->>Promise: invoke wrapped function -> returns Promise (children may start)
Promise-->>PR: Promise running
PR->>Promise: await Promise result
PR->>Ctx: finally -> collect_remaining_children(recursively=True)
Ctx->>Promise: collect/await descendant promises
Ctx-->>PR: children awaited, exit context
PR-->>AR: coroutine complete
AR-->>PF: asyncio.run returns result
PF-->>Caller: return result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 review |
✅ Actions performedReview triggered.
|
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)
promising/promising_context.py (1)
753-761: 🧹 Nitpick | 🔵 TrivialTODO comment notes incorrect error class/message.
The comment at lines 755-757 correctly identifies that
SyncUsageErrorand its message are no longer accurate for all cases where this exception is raised. Consider creating a follow-up issue to address this technical debt.Do you want me to open a GitHub issue to track updating the error class and message to accurately reflect all scenarios where this exception can be raised?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@promising/promising_context.py` around lines 753 - 761, The TODO notes that _call_soon_threadsafe currently raises SyncUsageError with a message mentioning synchronous operations, which is inaccurate; change the raised exception in _call_soon_threadsafe from SyncUsageError to a more general/appropriate error (e.g., EventLoopNotRunningError or ContextUsageError) and update the message to not reference "synchronous operation" (suggest: "The event loop associated with this {self.__class__.__name__} is not running"); add or update the custom exception class (if needed) and keep the TODO comment or open a follow-up issue to track any broader refactor of error taxonomy so all callers reflect the new semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@promising/promising_function.py`:
- Around line 155-160: Replace the private asyncio hook usage by using
inspect.markcoroutinefunction() on Python 3.12+ and fall back to the existing
asyncio.coroutines._is_coroutine for older Pythons: update the assignment of
_is_coroutine in promising_function.py so that if sys.version_info >= (3,12) you
call inspect.markcoroutinefunction() to mark the Promising function type,
otherwise keep the current asyncio.coroutines._is_coroutine fallback (keep
reference to the existing symbol _is_coroutine and the current logic that relies
on asyncio.coroutines._is_coroutine for 3.11).
---
Outside diff comments:
In `@promising/promising_context.py`:
- Around line 753-761: The TODO notes that _call_soon_threadsafe currently
raises SyncUsageError with a message mentioning synchronous operations, which is
inaccurate; change the raised exception in _call_soon_threadsafe from
SyncUsageError to a more general/appropriate error (e.g.,
EventLoopNotRunningError or ContextUsageError) and update the message to not
reference "synchronous operation" (suggest: "The event loop associated with this
{self.__class__.__name__} is not running"); add or update the custom exception
class (if needed) and keep the TODO comment or open a follow-up issue to track
any broader refactor of error taxonomy so all callers reflect the new semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d384e834-a393-4a99-8e82-14cb25c1dc9e
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
CONTRIBUTORS.mdREADME.mdpromising/__init__.pypromising/promise.pypromising/promising_context.pypromising/promising_function.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@promising/__init__.py`:
- Around line 44-49: Add a disableable defensive check where synchronous promise
call depth is tracked (the code path that schedules work into the
ThreadPoolExecutor and the Promise/PromiseExecutor logic) to detect when current
synchronous call depth >= executor.max_workers and raise a clear, specific
exception (e.g., SynchronousCallDepthExceededError or ThreadPoolExhaustionError)
instead of deadlocking; make this behavior configurable via a toggle (e.g., a
module-level flag or env var like PROMISING_DISABLE_DEPTH_CHECK or an optional
parameter on Promise/PromiseExecutor with default enabled) so callers can opt
out, and update/add unit tests to assert the exception is raised when depth
reaches max_workers and that the toggle disables the check.
In `@promising/promise.py`:
- Around line 109-113: Create two tracked GitHub issues for the P1 TODOs
referenced in promising/promise.py: one for "support async generator interface"
and one for "memory leak prevention for long sequences/streams". Each issue
should include a clear problem statement, proposed approaches (e.g., async
iterator protocol support, backpressure/eviction strategies or weak buffering,
explicit APIs for one-pass streams), acceptance criteria, and labels (e.g., P1,
architecture). Then update the TODO comments in promising/promise.py to include
the created issue numbers (e.g., TODO #[issue-number]) and a short note pointing
to the issue for design decisions and progress tracking.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 743bf1a5-8a95-4a5c-a34f-f83ce40533d6
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
CONTRIBUTORS.mdREADME.mdpromising/__init__.pypromising/errors.pypromising/promise.pypromising/promising_context.pypromising/promising_function.pypromising/utils.py
Summary by CodeRabbit
New Features
Behavior Changes
Improvements
Tests & Docs