docs(examples): demonstrate per-ring runtime_env sizing#1122
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 Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughBoth the L2 and L3 per-task runtime-env examples are extended to support per-ring array sizing alongside existing scalar fields. A ChangesPer-ring array runtime_env sizing in L2/L3 examples
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for per-ring sizing (independent sizing of the four scope-depth rings) alongside the existing scalar form (broadcasting one value to all rings) for L2 and L3 workers. It updates the examples, configurations, and documentation to demonstrate and support both forms. The reviewer suggests improving the _l2_config helper in the L3 example to preserve pre-existing runtime_env fields from the base configuration that are not overridden by the task specification, and recommends using getattr for safer attribute access.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
examples/workers/l2/per_task_runtime_env/README.md (1)
53-53: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the stale run count in the layout comment.
Line 53 says
3 runs, but this example now runs four configs (scalar_small,scalar_large,per_ring,env_or_default), so the docs are inconsistent.Suggested patch
- main.py # 3 runs, one CallConfig.runtime_env each + main.py # 4 runs, one CallConfig.runtime_env each🤖 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 `@examples/workers/l2/per_task_runtime_env/README.md` at line 53, The layout comment in the README incorrectly states that main.py performs 3 runs, but the example actually contains 4 CallConfig.runtime_env configurations (scalar_small, scalar_large, per_ring, and env_or_default). Update the comment that says "3 runs" to say "4 runs" to accurately reflect the current number of configurations in the example.
🤖 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 `@examples/workers/l3/per_task_runtime_env/README.md`:
- Around line 18-26: The code snippet unconditionally accesses both scalar form
keys (ring_task_window, ring_heap, ring_dep_pool) and per-ring form keys
(ring_task_windows, ring_heaps, ring_dep_pools) when assigning to
cfg.runtime_env. However, according to the actual task specs in main.py, each
spec contains only one form, not both, so this code will raise KeyError. Make
the assignments conditional by checking which form exists in the spec and only
accessing the appropriate keys for that particular spec, either the scalar form
or the per-ring form, but not both.
---
Outside diff comments:
In `@examples/workers/l2/per_task_runtime_env/README.md`:
- Line 53: The layout comment in the README incorrectly states that main.py
performs 3 runs, but the example actually contains 4 CallConfig.runtime_env
configurations (scalar_small, scalar_large, per_ring, and env_or_default).
Update the comment that says "3 runs" to say "4 runs" to accurately reflect the
current number of configurations in the example.
🪄 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: 0baf788b-adb0-449f-a1a8-b9d6bb98bc50
📒 Files selected for processing (4)
examples/workers/l2/per_task_runtime_env/README.mdexamples/workers/l2/per_task_runtime_env/main.pyexamples/workers/l3/per_task_runtime_env/README.mdexamples/workers/l3/per_task_runtime_env/main.py
Address review feedback on hw-native-sys#1122: - L2 README layout comment said 3 runs; now 4 after adding per_ring. - L3 README orch_fn snippet indexed both scalar and per-ring keys unconditionally, which would KeyError since each spec carries only one form. Match the real main.py: iterate RING_FIELDS and set the keys the spec actually has.
hw-native-sys#1099 added per-ring array fields (ring_task_windows / ring_heaps / ring_dep_pools) alongside the scalar runtime_env knobs, but neither per_task_runtime_env example exercised them. Extend both the L2 and L3 examples to also cover the per-ring form: each scope-depth ring (0..3) sized independently. The config helpers now iterate a RING_FIELDS tuple so a spec dict can carry either the scalar or the array keys, and the READMEs document the full precedence chain and the --enable-scope-stats verification path.
ChaoZheng109
force-pushed
the
docs/per-ring-runtime-env-examples
branch
from
a13231c to
39af1a4
Compare
1 participant
What
#1099 added per-ring array fields (
ring_task_windows/ring_heaps/ring_dep_pools) toCallConfig.runtime_env, alongside the existing scalar knobs. The twoper_task_runtime_envexamples, however, only demonstrated the scalar form — there was no worked example of the new per-ring sizing.This extends both examples to also cover the per-ring form, where each scope-depth ring (
0..3) is sized independently.Changes
examples/workers/l2/per_task_runtime_env/— newper_ringconfig entry sizing rings 0..3 with the array fields;_make_confignow iterates aRING_FIELDStuple so a spec dict can carry either the scalar or array keys.examples/workers/l3/per_task_runtime_env/— newl2_per_ringtask showing heterogeneous per-ring footprints across L2s in one launch;_l2_configsimilarly generalized.per-ring field > scalar field > per-ring env > scalar env > default), and the--enable-scope-statsverification path.Per-ring values are valid per
RuntimeEnv::validate()(task_windows are powers of 2 in[4, INT32_MAX], heaps>= 1024, dep_pools in[4, INT32_MAX]), and total footprint stays below the already-workingscalar_largebroadcast.Test
Both examples pass under
a2a3sim, including the new per-ring configs (golden max diff = 0). The STs callrun()directly, so existingtest_per_task_runtime_env.pyautomatically covers the added configs.