feat(runtime): add bounded distributed dispatch handles - #2270
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe runtime adds bounded asynchronous distributed dispatch through ChangesAsynchronous distributed execution
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15fbbe0536
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
docs/en/user/distributed/03-execution.md (1)
76-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider documenting the handle API surface.
The section names
DistributedRunHandlebut does not describe its methods.result(timeout=...),wait(timeout=...), anddoneare public. A short list helps callers poll or bound their wait.🤖 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 `@docs/en/user/distributed/03-execution.md` around lines 76 - 81, Add a concise API summary in the distributed execution section for DistributedRunHandle, documenting the public result(timeout=...), wait(timeout=...), and done members, including that result returns the run outcome, wait supports bounded waiting, and done indicates completion.python/pypto/runtime/distributed_runner.py (1)
98-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
generationfield.
_DispatchFrame.generationis only incremented when acquiring a dispatch frame. If stale frame references are not detected withgeneration, drop the field.🤖 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 `@python/pypto/runtime/distributed_runner.py` around lines 98 - 108, Remove the unused generation field from _DispatchFrame and delete any related initialization or increment logic when dispatch frames are acquired, while preserving the remaining frame metadata and reuse behavior.tests/ut/runtime/test_distributed_worker.py (1)
2503-2511: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the two unused events and align the test double with the asserted failure.
request_finalizer_startedandallow_request_finalizer_to_finishare created and set, but no code waits on them. They add no synchronization to this test.
worker_submitalso discardsfn, sofailing_entrynever runs. The asserted error comes only fromnative.complete(RuntimeError("persistent dispatch failed before cleanup")). The test therefore does not exercise a request-side entry failure, which its name implies. Either invokefninsideworker_submitsofailing_entryraises, or rename the test to describe native-handle failure propagation.♻️ Proposed cleanup of the unused events
m = patched_setup m["worker"]._live_domains = {} - request_finalizer_started = threading.Event() - allow_request_finalizer_to_finish = threading.Event() native = _ControlledNativeHandle()assert native.result_started.wait(timeout=2) - request_finalizer_started.set() # A failing request may already have submitted device work. Its caller # must not observe completion while the native handle is still finalizing. assert not caller_done.is_set() - allow_request_finalizer_to_finish.set() native.complete(RuntimeError("persistent dispatch failed before cleanup"))Also applies to: 2544-2551
🤖 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/ut/runtime/test_distributed_worker.py` around lines 2503 - 2511, Remove the unused request_finalizer_started and allow_request_finalizer_to_finish events and their associated set/wait logic. Update worker_submit to invoke the submitted fn so failing_entry actually raises, preserving the test’s request-side failure scenario and existing assertions.
🤖 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 `@python/pypto/runtime/distributed_runner.py`:
- Around line 1634-1639: Update the backpressure drain in the dispatch
submission path around _active_dispatch_handles so draining oldest does not
propagate that handle’s cached failure to the new submitter. Consume the oldest
handle’s result while suppressing any exception, preserving the handle’s cached
outcome for its owner and allowing frame acquisition to continue.
- Around line 2381-2391: Make close() idempotent for concurrent callers by
adding and initializing a _closing flag alongside _accepting_dispatches, then
check and set _closing inside the existing _dispatch_submit_mu block before
entering teardown. Have subsequent close() calls return immediately, while
preserving the existing _closed guard and normal drain and release behavior.
---
Nitpick comments:
In `@docs/en/user/distributed/03-execution.md`:
- Around line 76-81: Add a concise API summary in the distributed execution
section for DistributedRunHandle, documenting the public result(timeout=...),
wait(timeout=...), and done members, including that result returns the run
outcome, wait supports bounded waiting, and done indicates completion.
In `@python/pypto/runtime/distributed_runner.py`:
- Around line 98-108: Remove the unused generation field from _DispatchFrame and
delete any related initialization or increment logic when dispatch frames are
acquired, while preserving the remaining frame metadata and reuse behavior.
In `@tests/ut/runtime/test_distributed_worker.py`:
- Around line 2503-2511: Remove the unused request_finalizer_started and
allow_request_finalizer_to_finish events and their associated set/wait logic.
Update worker_submit to invoke the submitted fn so failing_entry actually
raises, preserving the test’s request-side failure scenario and existing
assertions.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cfe77f61-a75c-4618-9fb1-ea0e7924b0f3
📒 Files selected for processing (7)
docs/en/user/distributed/03-execution.mddocs/zh/user/distributed/03-execution.mdpython/pypto/runtime/__init__.pypython/pypto/runtime/distributed_runner.pyruntimetests/st/distributed/test_l3_device_tensor.pytests/ut/runtime/test_distributed_worker.py
|
please fix ai comment |
15fbbe0 to
03aec67
Compare
|
Proposal: remove the persistent-only Python dispatcher queue/thread and submit the persistent orchestration directly from The queue was necessary in the original persistent implementation ( At this point the queue is only a synchronous RPC hop: It provides no buffering or additional serialization: public I suggest replacing the queue path with: Keep This still permits the intended overlap: construction of request N+1 happens synchronously in its Removing the queue also eliminates several cross-thread cancellation/error-transfer states implicated by the review findings. It does not by itself fix the accepted-native-run publication window: acceptance through installation in |
03aec67 to
13d8d1d
Compare
|
@YunjiQin Addressed in
The branch is also rebased onto current |
13d8d1d to
1370137
Compare
Summary
DistributedWorker.submit()and publicDistributedRunHandlewhile keepingrun()and__call__()blockingCallConfig, generated task metadata, and native handles through terminal completionclose()safe for concurrent callersWorker.submit(), retaining CommDomains without a Python dispatcher queue or background threadDependency
This is Q1 in the worker asynchronous pipeline stack. It is rebased on current PyPTO
main(df3ac122) and uses the existingruntimegitlink at3165cc89; this PR does not change the runtime submodule. Serving integration, end-to-end queue semantics, and cross-layer performance acceptance remain outside this PR.Validation
tests/ut/runtime/test_distributed_worker.py: 144 passedtests/ut/runtime: 587 passedtests/ut: 9389 passed, 3 skippedpre-commit run --all-files: passed, including Ruff, Pyright, Markdown, C/C++, headers, English-only, docs en/zh parity/navigation/symbol coverage, broad-exception, and operator-name checks