Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
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 Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughPersistent prepared workers gain an opt-in ChangesPersistent L3 execution
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant DistributedWorker
participant HostOrchestrator
participant CommDomain
Caller->>DistributedWorker: run(compiled, ...)
DistributedWorker->>HostOrchestrator: dispatch through persistent queue
HostOrchestrator->>CommDomain: retain or allocate via _domain_provider
HostOrchestrator-->>DistributedWorker: complete after drain
DistributedWorker-->>Caller: return result or propagate error
Caller->>DistributedWorker: close()
DistributedWorker->>CommDomain: release retained domains
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 |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/codegen/distributed/distributed_codegen.cpp`:
- Around line 290-294: Update the generated function body for functions whose
level is at least 3 to forward _domain_provider in direct
orchestrator-to-orchestrator calls. Reuse the existing _domain_provider
parameter emitted by the signature construction near func->name_, ensuring
nested host orchestrators preserve the caller’s communication-domain scope
instead of allocating a transient domain.
🪄 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
Run ID: 1a8d3056-b25b-435d-bc94-39ea22bdf577
📒 Files selected for processing (7)
docs/en/dev/06-persistent-l3.mddocs/zh-cn/dev/06-persistent-l3.mdpython/pypto/ir/distributed_compiled_program.pypython/pypto/runtime/distributed_runner.pysrc/codegen/distributed/distributed_codegen.cpptests/ut/codegen/distributed/test_host_orch_distributed.pytests/ut/runtime/test_distributed_worker.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2972d5a029
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
d09446c to
c1f40a8
Compare
Keep one L3 Worker.run active across prepared dispatches and retain CommDomains per compiled program while preserving fresh-window semantics. Add generated domain-provider support, synchronous per-request cleanup, regression coverage, and bilingual documentation.
YunjiQin
left a comment
There was a problem hiding this comment.
Approve.
设计合理、CI 全绿、测试覆盖到位(prepare flag 透传、缺 hook 拒绝、复用+清零、TaskArgs 生命周期、多程序隔离、错误传播、错误等待 outer cleanup)。codegen 门控用 level_.value_or(AIV) >= 3 双重判断,默认路径行为不变,向后兼容。
对持久模式下'主线程内存原语 vs 后台常驻 Worker.run'的跨线程担忧,已核对 runtime 实现后消解:malloc/free/copy_to 走 control mailbox 控制面(mailbox_mu_ 全程持锁),与 scope/drain/ring 任务面互不相干,不存在记账污染;且请求间后台线程阻塞于 queue.get、子 worker 静止,close() 中的 owned-tensor 回收命中空闲态。
低危收尾建议(非阻塞,可后续跟进):
- close() 里将 _stop_persistent_dispatcher() 移到 _close_owned_tensors() 之前,使正确性不依赖'请求间后台空闲'这条隐式不变量;或在文档/注释中显式声明该不变量。
- 依赖 Simpler 私有 drain / Worker cleanup API 属临时方案,PR 描述已声明,待未来公开 request-boundary API 封装。
Summary
DistributedCompiledProgram.prepare(persistent=True)execution backed by one long-running L3Worker.run()Motivation
DeepSeek decode repeatedly enters
Worker.run()and allocates/releases the same communication domains. For small decode DAGs, that host-side lifecycle can cost more than the effective device work. Persistent mode moves the L3 orchestration and physical CommDomain allocation to the prepared-worker lifetime while keeping normal dispatch as the default.Compatibility and limitations
prepare()behavior is unchanged_domain_providerhookValidation
7397 passed, 2 skipped122 passedruff checkandruff format --check: passedgit diff --check: passeddistributed_codegen.cppRelate
#2069