Update: simplify NEXT_LEVEL scheduling with explicit targets#1436
Merged
ChaoWao merged 1 commit intoJul 23, 2026
Merged
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
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:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughNEXT_LEVEL submissions now require explicit stable worker IDs. The orchestrator validates and stores targets, directed queues route READY work, the scheduler dispatches exact workers, and tests/examples/documentation adopt the new contract. ChangesDirected NEXT_LEVEL scheduling
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant ParentOrchestrator
participant NextLevelReadyQueues
participant Scheduler
participant WorkerManager
participant TargetWorker
ParentOrchestrator->>NextLevelReadyQueues: enqueue READY task for target worker
Scheduler->>NextLevelReadyQueues: pop target FIFO
Scheduler->>WorkerManager: resolve stable worker ID
WorkerManager-->>Scheduler: return TargetWorker
Scheduler->>TargetWorker: dispatch task
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/ut/py/test_worker/test_child_addr_guard.py (1)
341-351: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake this test reach the owner-resolution path.
workers=[0, 0]is rejected by the public duplicate-worker check before_child_dispatch_candidatesor_child_prov_check_dispatchruns, so this does not test the “same owner” behavior its name describes. Rename it to cover duplicate explicit worker IDs, or construct a case with distinct requested IDs that reaches the owner-resolution check. The supplied orchestrator flow validates duplicates first, andtests/ut/py/test_callable_identity.pyalready covers that validation.🤖 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/py/test_worker/test_child_addr_guard.py` around lines 341 - 351, Update test_group_two_child_members_same_owner_rejected so it does not use workers=[0, 0], which is rejected by the public duplicate-worker validation before owner resolution. Either rename the test to reflect duplicate explicit worker IDs and align its assertions, or use distinct requested worker IDs that reach _child_dispatch_candidates/_child_prov_check_dispatch and genuinely verify same-owner rejection.
🤖 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 `@docs/callable-identity-registration.md`:
- Line 345: Update the sentence describing orch.submit_next_level_group(...,
workers=...) to use correct subject–verb agreement by changing “require” to
“requires,” or rewrite it as “The API requires exact submit-time placement.”
In `@docs/orchestrator.md`:
- Around line 37-43: Update the submit_next_level_group usage example to pass
the required workers vector alongside callable, args_list, and config, and show
that the vector contains one worker ID for each group member. Ensure the example
propagates the worker assignments consistently from group construction through
the submission call.
In `@python/simpler/orchestrator.py`:
- Around line 194-195: Validate worker IDs as non-boolean integers before
coercion in both submission paths: update the single-worker handling around the
existing cpp_worker_id check at python/simpler/orchestrator.py lines 194-195,
and validate every workers entry before length, sign, and duplicate checks at
lines 261-264. Reject floats, numeric strings, booleans, and other non-integer
values while preserving non-negative ID validation.
---
Nitpick comments:
In `@tests/ut/py/test_worker/test_child_addr_guard.py`:
- Around line 341-351: Update test_group_two_child_members_same_owner_rejected
so it does not use workers=[0, 0], which is rejected by the public
duplicate-worker validation before owner resolution. Either rename the test to
reflect duplicate explicit worker IDs and align its assertions, or use distinct
requested worker IDs that reach
_child_dispatch_candidates/_child_prov_check_dispatch and genuinely verify
same-owner rejection.
🪄 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: 1cca16a5-d170-40ef-b8a7-4b7b8ccaa739
📒 Files selected for processing (25)
docs/callable-identity-registration.mddocs/directed-next-level-scheduling/pr1-explicit-placement-contract.mddocs/hierarchical_level_runtime.mddocs/orchestrator.mddocs/remote-l3-worker-design.mddocs/scheduler.mddocs/task-flow.mdexamples/workers/l3/README.mdexamples/workers/l3/per_task_runtime_env/README.mdexamples/workers/l3/per_task_runtime_env/main.pypython/bindings/worker_bind.hpython/simpler/orchestrator.pypython/simpler/worker.pysrc/common/hierarchical/orchestrator.cppsrc/common/hierarchical/orchestrator.htests/st/a2a3/tensormap_and_ringbuffer/dynamic_register/test_dynamic_register.pytests/st/a2a3/tensormap_and_ringbuffer/test_l3_dependency.pytests/st/a2a3/tensormap_and_ringbuffer/test_l3_group.pytests/ut/cpp/hierarchical/test_orchestrator.cpptests/ut/cpp/hierarchical/test_scheduler.cpptests/ut/py/test_callable_identity.pytests/ut/py/test_worker/test_child_addr_guard.pytests/ut/py/test_worker/test_error_propagation.pytests/ut/py/test_worker/test_l4_recursive.pytests/ut/py/test_worker/test_startup_readiness.py
puddingfjz
force-pushed
the
codex/directed-scheduling-pr1
branch
2 times, most recently
from
July 22, 2026 07:04
636672f to
a8c9b19
Compare
puddingfjz
changed the title
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/st/a2a3/tensormap_and_ringbuffer/dynamic_register/test_dynamic_register.py (1)
235-245: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExercise both workers in the parallel-broadcast test.
device_idscontains two workers, but both submissions are pinned to worker0. The assertions therefore cannot detect a registration or execution failure on worker1. Use separate per-worker argument bundles withsubmit_next_level_group(..., workers=[0, 1])(or submit one task to each worker).🤖 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/st/a2a3/tensormap_and_ringbuffer/dynamic_register/test_dynamic_register.py` around lines 235 - 245, Update the parallel-broadcast flow in orch_post to exercise both workers instead of pinning submission to worker 0. Use separate per-worker argument bundles and submit them through submit_next_level_group with workers [0, 1], preserving the existing post_handle and broadcast behavior so assertions cover worker 1 as well.
🧹 Nitpick comments (1)
src/common/hierarchical/scheduler.cpp (1)
411-419: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the group-state reset into a helper.
This reset block is duplicated in
dispatch_sub_ready(Lines 357-364), differing only by the initialgroup_member_statesvalue (RUNNINGvsNOT_DISPATCHED). Extracting a helper such asreset_group_state(s, group_size, initial_member_state)removes the risk of the two sites diverging when a group-state field is added or renamed.🤖 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 `@src/common/hierarchical/scheduler.cpp` around lines 411 - 419, Extract the duplicated group-state initialization into a helper such as reset_group_state, parameterized by s, group_size, and the initial GroupMemberState. Replace the reset blocks in dispatch_sub_ready and the current group setup with this helper, preserving RUNNING at this site and NOT_DISPATCHED at the other while resetting all shared state fields consistently.
🤖 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 `@docs/callable-identity-registration.md`:
- Around line 345-349: Update the placement API sentence in the callable
identity registration documentation to name both submit-time APIs: retain
orch.submit_next_level_group(..., workers=...) and add
orch.submit_next_level(..., worker=...). Ensure the following “they” reference
clearly applies to both APIs without changing the placement or worker-ID
guidance.
In `@docs/hierarchical_level_runtime.md`:
- Around line 145-150: Update the runtime diagram around the READY routing steps
and the associated channel table to show READY work being enqueued directly into
the directed/shared ready queues, followed by waking the Scheduler. Remove
references to routing READY work through the obsolete wiring_queue path, while
preserving the completion_queue, fanout/fanin, and target-resolution flow.
In `@docs/task-flow.md`:
- Around line 345-348: Update the task-flow documentation around Scheduler
dispatch and WorkerThread mailbox encoding to scope the shared-memory mailbox
description to local worker endpoints only. Clarify that remote NEXT_LEVEL
dispatch through RemoteL3Endpoint sends a framed TASK instead, while preserving
the local SUB-worker mailbox flow.
In `@examples/workers/l3/per_task_runtime_env/README.md`:
- Line 25: Update the example’s orch.submit_next_level call to construct the
per-task configuration from the existing _cfg CallConfig, preserving
harness-injected diagnostics while applying task-specific settings; do not
create a fresh standalone CallConfig.
In `@tests/ut/cpp/hierarchical/test_orchestrator.cpp`:
- Around line 355-362: Update the test around submit_next_level and
try_pop_single to assert that worker 3’s queue pop succeeds and that the
retrieved TaskSlot corresponds to producer before submitting the consumer. Use
the existing producer and ready symbols to validate both queue routing and slot
identity.
---
Outside diff comments:
In
`@tests/st/a2a3/tensormap_and_ringbuffer/dynamic_register/test_dynamic_register.py`:
- Around line 235-245: Update the parallel-broadcast flow in orch_post to
exercise both workers instead of pinning submission to worker 0. Use separate
per-worker argument bundles and submit them through submit_next_level_group with
workers [0, 1], preserving the existing post_handle and broadcast behavior so
assertions cover worker 1 as well.
---
Nitpick comments:
In `@src/common/hierarchical/scheduler.cpp`:
- Around line 411-419: Extract the duplicated group-state initialization into a
helper such as reset_group_state, parameterized by s, group_size, and the
initial GroupMemberState. Replace the reset blocks in dispatch_sub_ready and the
current group setup with this helper, preserving RUNNING at this site and
NOT_DISPATCHED at the other while resetting all shared state fields
consistently.
🪄 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: 69dbf5a4-b037-4e7e-b199-c8d9c9b5645c
📒 Files selected for processing (37)
docs/callable-identity-registration.mddocs/directed-next-level-scheduling/implementation-plan.mddocs/hierarchical_level_runtime.mddocs/orchestrator.mddocs/remote-l3-worker-design.mddocs/remote-l3-worker-design/implementation-plan.mddocs/remote-l3-worker-design/pr-split-and-audit-artifacts.mddocs/remote-l3-worker-design/pr-split-and-audit-plan.mddocs/scheduler.mddocs/task-flow.mddocs/worker-manager.mdexamples/workers/l3/README.mdexamples/workers/l3/per_task_runtime_env/README.mdexamples/workers/l3/per_task_runtime_env/main.pypython/bindings/worker_bind.hpython/simpler/orchestrator.pypython/simpler/worker.pysrc/common/hierarchical/orchestrator.cppsrc/common/hierarchical/orchestrator.hsrc/common/hierarchical/scheduler.cppsrc/common/hierarchical/scheduler.hsrc/common/hierarchical/types.cppsrc/common/hierarchical/types.hsrc/common/hierarchical/worker.cppsrc/common/hierarchical/worker.hsrc/common/hierarchical/worker_manager.cppsrc/common/hierarchical/worker_manager.htests/st/a2a3/tensormap_and_ringbuffer/dynamic_register/test_dynamic_register.pytests/st/a2a3/tensormap_and_ringbuffer/test_l3_dependency.pytests/st/a2a3/tensormap_and_ringbuffer/test_l3_group.pytests/ut/cpp/hierarchical/test_orchestrator.cpptests/ut/cpp/hierarchical/test_scheduler.cpptests/ut/py/test_callable_identity.pytests/ut/py/test_worker/test_child_addr_guard.pytests/ut/py/test_worker/test_error_propagation.pytests/ut/py/test_worker/test_l4_recursive.pytests/ut/py/test_worker/test_startup_readiness.py
puddingfjz
force-pushed
the
codex/directed-scheduling-pr1
branch
from
July 22, 2026 08:02
a8c9b19 to
ed7d4ba
Compare
Contributor
Author
|
Addressed the remaining full-review items in ed7d4ba:
|
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 17 minutes. |
puddingfjz
force-pushed
the
codex/directed-scheduling-pr1
branch
3 times, most recently
from
July 22, 2026 09:03
03d2358 to
6ba0b38
Compare
ChaoWao
force-pushed
the
codex/directed-scheduling-pr1
branch
from
July 23, 2026 01:11
6ba0b38 to
a18f08e
Compare
ChaoWao
previously approved these changes
Jul 23, 2026
ChaoWao
force-pushed
the
codex/directed-scheduling-pr1
branch
from
July 23, 2026 01:17
a18f08e to
6581e04
Compare
ChaoWao
force-pushed
the
codex/directed-scheduling-pr1
branch
from
July 23, 2026 02:39
6581e04 to
9f0f22d
Compare
Replace unconstrained NEXT_LEVEL scheduling with explicit placement: every NEXT_LEVEL task names its exact worker at submit time and the Scheduler dispatches only to that worker. The runtime no longer selects a NEXT_LEVEL worker on the caller's behalf. - submit_next_level requires a stable worker id; submit_next_level_group requires one distinct id per member. Booleans, floats, numeric strings, and other coercible non-integers are rejected. - READY singles route through a per-worker FIFO; groups use one shared FIFO whose head launches only when every target worker is idle (all-or-nothing, no partial reservation). SUB placement is unchanged and takes no worker id. - A local (LOCAL_PYTHON / LOCAL_CHIP) callable is rejected when pinned to a remote worker id: a remote endpoint installs only its own dispatcher callables, so a local digest would fail asynchronously as an unknown hashid. - Remove the shared NEXT_LEVEL queue, idle-pool fallback selection, and the queue wait/shutdown/condition-variable state; retain idle-worker selection only for SUB. Docs: rewrite scheduler.md for the directed model, add directed-next-level-scheduling.md as the placement-contract reference, and refresh the maintained call sites and API summaries for the required worker argument. Tests: cover required/malformed target rejection, per-worker FIFO routing, exact member-to-worker mapping (including reversed target order), group-first dispatch, blocked-group non-reservation, and the local-callable remote-target guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ChaoWao
approved these changes
Jul 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Scheduling contract
submit_next_level(..., worker=id)targets exactly one stable NEXT_LEVEL worker.submit_next_level_group(..., workers=[...])requires one unique worker ID per group member.Validation
Out of scope