Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ebbabd53f4
ℹ️ 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".
|
|
||
| impl LinearAttentionLayer { | ||
| /// Phase 2b: shard linear attention over TP ranks. The value-head unit | ||
| /// Linear-attention sharding over TP ranks. The value-head unit |
There was a problem hiding this comment.
This commit's stated invariant is a behavior-preserving TP worker module split, but this edit only removes historical phase wording; lib.rs contains the same ride-along cleanup. Neither file is needed for that split, so exclude these changes rather than expanding the refactor's scope contrary to the repository's requirement that every changed file serve the production invariant.
AGENTS.md reference: AGENTS.md:L123-L123
Useful? React with 👍 / 👎.
55e217e to
48ec7cf
Compare
|
@codex review please |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48ec7cf56a
ℹ️ 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".
| const TP_WORKER_SHUTDOWN_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(30); | ||
| const TP_RUNTIME_MEMORY_RESERVE_BYTES: usize = 512 * 1024 * 1024; | ||
|
|
||
| pub(super) fn spawn_nccl_startup_watchdog() -> Result<(mpsc::SyncSender<()>, JoinHandle<()>)> { |
There was a problem hiding this comment.
The current revision still includes a fresh scope violation: while moving this watchdog into worker.rs, it deletes nccl_startup_watchdog_disarms_after_success instead of moving it with the implementation. That test verifies the watchdog observes the completion signal and joins promptly; removing it changes the verification surface even though this commit promises a behavior-preserving module split. Restore the test in the new module or handle its retirement as a separately justified change.
AGENTS.md reference: AGENTS.md:L121-L123
Useful? React with 👍 / 👎.
48ec7cf to
ed9b5be
Compare
Move the per-rank worker runtime out of the tp_executor.rs God module:
TpWorker + spawn/drop lifecycle, TpStartupGate, the NCCL startup watchdog,
TpWorkerState/TpWorkerPrepared and its command loop (run/respond/
execute_*/precapture/drop), the slot bookkeeping helpers, the shared
decode-row view/sampling helpers, recurrent-capacity math, and the worker
CublasThreadGuard binding. The entry keeps command/reply types, the
executor orchestration (including the precapture sweep), plan validators,
and response validation, reaching worker items via the same `use super::*`
style as scheduler/{backend,tp}. Rebased over pegainfer-project#720/pegainfer-project#722. Behavior
unchanged; the watchdog disarm test moves into worker.rs with its
implementation.
Signed-off-by: CAICAIIs <3360776475@qq.com>
ed9b5be to
fbe384b
Compare
The tp_executor entry and the worker module carried two near-identical condvar gates: TpCommandStartGate (Pending/Execute/Cancel, first-write resolve returning whether it won) and TpStartupGate (Pending/Connect/ Cancel, silent set). Both broadcast one go/cancel decision that is resolved exactly once, so they collapse into TpGate + TpGateDecision with the first-write-wins resolve; startup treats Go as connect and asserts on the returned decision instead of a bool. Behavior unchanged. Signed-off-by: CAICAIIs <3360776475@qq.com>
The scheduler and the TP worker carried byte-identical CUDA/cuBLAS thread bindings (guard + set_device + bind_to_thread + cublas_init); the worker split moved one of the two copies, making the duplication visible. Both now call crate::cublas_thread::bind_model_thread with a role tag (the failure messages are unchanged); the scheduler keeps its tune_decode_gemm_algos step at its own call site. Behavior unchanged. Signed-off-by: CAICAIIs <3360776475@qq.com>
|
@codex review thx |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c087a4998
ℹ️ 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".
| match crate::cublas_thread::bind_model_thread(backend.model(), "scheduler") { | ||
| Ok(_guard) => { | ||
| if let Err(err) = backend.model().tune_decode_gemm_algos() { |
There was a problem hiding this comment.
Fresh evidence beyond the earlier phase-wording comment is that this revision now rewrites the single-GPU scheduler to use a new crate-level cuBLAS binder and moves GEMM tuning out of the binder. Neither the scheduler path nor cublas_thread.rs is needed to move the TP worker runtime—worker.rs can retain its existing worker binder—so this broadens a stated behavior-preserving split and changes the tuning-failure cleanup path without separate justification; keep the scheduler unchanged and limit this commit to the worker move.
AGENTS.md reference: AGENTS.md:L121-L123
Useful? React with 👍 / 👎.
2 participants
What
Split the TP worker runtime out of the
tp_executor.rsGod module intotp_executor/worker.rs, the same seam pattern as the landedscheduler/{backend,tp}.rssplits (#967, #968). Pure move; behavior unchanged. Rebased over #1004/#1005/#1033/#1038 — the moved content includes the batched eager decode, decode-graph precapture, and slot-compaction machinery as worker-concern code.Stacked PR 1/3 — the follow-ups (#1036 responses split, #1037 scheduler steps split) sit on top of this branch; merge in order.
Structure
tp_executor.rskeeps command/reply types, theQwen35TpExecutororchestration (including the precapture sweep and retired-slot tracking), plan validators, and response validation.tp_executor/worker.rsnow owns the per-rank runtime:TpWorkerspawn/drop lifecycle,TpStartupGate, the NCCL startup watchdog,TpWorkerState/TpWorkerPreparedand the per-rank command loop (run/respond/execute_*/run_decode_batch{,_graph}/precapture_*), the slot bookkeeping helpers, the shared decode-row view/sampling helpers, recurrent-capacity math, and the workerCublasThreadGuardbinding. Cross-concern items reach each other via the sameuse super::*;style the landed scheduler splits use; moved items gainpub(super)only where the entry or its tests name them.Defense table (all inherited — pure move)
worker.rsspawn_nccl_startup_watchdogworker.rsTpWorker::join_boundedworker.rsspawn closure catch_unwindmax_batch > 0ensure)worker.rsTpWorkerPrepared::newworker.rsbind_worker_thread/CublasThreadGuardworker.rsTpWorkerStatefield-order commentValidation
cargo fmt --checkclean;cargo clippy -p pegainfer-qwen35 --features qwen35 --all-targets -- -D warningsclean.cargo test --libgreen; the seventp2_*GPU tests pass on 2×A100 (-- --ignored --test-threads=1) against Qwen3.5-4B;e2e_schedulergreen.