Repository navigation
Conversation
|
Warning Review limit reached
More reviews will be available in 7 minutes and 26 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds a batch-invariance testing framework ( ChangesBatch-Invariant Elementwise & RoPE Audit
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
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. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
dc1434f to
b641e2f
Compare
![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)
.github/workflows/ci.yml (1)
34-66:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit least-privilege
permissionsto the workflow/job.This workflow relies on default token permissions; that’s broader than necessary for test-only execution and weakens CI hardening.
Suggested fix
name: CI +permissions: + contents: read @@ jobs: unit-tests: + permissions: + contents: read needs: linting runs-on: ubuntu-latest🤖 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 @.github/workflows/ci.yml around lines 34 - 66, The unit-tests job currently relies on default token permissions which are overly broad for a test-only CI execution. Add an explicit permissions block to the unit-tests job that grants only the minimal required permissions. For this test-only workflow that checks out code and runs tests, add a permissions configuration that specifies contents: read to allow repository checkout, and explicitly deny or omit any other permissions that are not required for the job's functionality.Source: Linters/SAST tools
🧹 Nitpick comments (1)
docs/design/batch-invariant-elementwise-rope.md (1)
3-3: 💤 Low valueMinor style improvement: rephrase wordy preposition.
The phrase "with respect to" on line 3 can be simplified. Consider "relative to", "across", or "per" instead to improve clarity.
💬 Suggested rephrase
-Issue `#149` audits the forward-path operations that should be pass-through with respect to batch configuration. +Issue `#149` audits the forward-path operations that should be pass-through across batch configuration.🤖 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/design/batch-invariant-elementwise-rope.md` at line 3, The phrase "with respect to" in the document is wordy and impacts clarity. Find and replace this phrase with a more concise alternative such as "relative to", "across", or "per" depending on the context of the sentence to improve readability.Source: Linters/SAST tools
🤖 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 `@rl_engine/testing/forward_invariance.py`:
- Around line 121-139: The build_rope_cache function lacks validation for the
dtype parameter, which can lead to silent corruption of trigonometric values if
non-floating types like torch.int* or torch.bool are passed. Add a dtype
validation check in the function alongside the existing parameter validations
(similar to the checks for max_position, head_dim, and base) to ensure dtype is
a floating-point type. Raise a ValueError with an appropriate message if a
non-float dtype is provided, preventing the function from returning incorrectly
quantized cosine and sine values that would corrupt apply_rope_reference
results.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 34-66: The unit-tests job currently relies on default token
permissions which are overly broad for a test-only CI execution. Add an explicit
permissions block to the unit-tests job that grants only the minimal required
permissions. For this test-only workflow that checks out code and runs tests,
add a permissions configuration that specifies contents: read to allow
repository checkout, and explicitly deny or omit any other permissions that are
not required for the job's functionality.
---
Nitpick comments:
In `@docs/design/batch-invariant-elementwise-rope.md`:
- Line 3: The phrase "with respect to" in the document is wordy and impacts
clarity. Find and replace this phrase with a more concise alternative such as
"relative to", "across", or "per" depending on the context of the sentence to
improve readability.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e31b468-85fc-4d3a-9900-05c2e927ee6b
📒 Files selected for processing (5)
.github/workflows/ci.ymldocs/design/batch-invariant-elementwise-rope.mdrl_engine/testing/__init__.pyrl_engine/testing/forward_invariance.pytests/test_forward_invariance.py
b641e2f to
0466ac1
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
34-66:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope the job token explicitly.
This job does not declare any
permissions:block, so it inherits the workflow default scope. Add a read-onlycontentspermission (or tighter) to keep the test job from running with broader token access than it needs.🤖 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 @.github/workflows/ci.yml around lines 34 - 66, The unit-tests job lacks an explicit permissions block, which causes it to inherit the workflow default scope and run with broader token access than necessary. Add a permissions block to the unit-tests job definition that explicitly grants read-only access to the contents permission scope, ensuring the job has the minimum required token access for its operations.Source: Linters/SAST tools
🤖 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.
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 34-66: The unit-tests job lacks an explicit permissions block,
which causes it to inherit the workflow default scope and run with broader token
access than necessary. Add a permissions block to the unit-tests job definition
that explicitly grants read-only access to the contents permission scope,
ensuring the job has the minimum required token access for its operations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8ac135da-448f-4907-b5a0-215a9e1b95e1
📒 Files selected for processing (5)
.github/workflows/ci.ymldocs/design/batch-invariant-elementwise-rope.mdrl_engine/testing/__init__.pyrl_engine/testing/forward_invariance.pytests/test_forward_invariance.py
🚧 Files skipped from review as they are similar to previous changes (3)
- rl_engine/testing/init.py
- tests/test_forward_invariance.py
- rl_engine/testing/forward_invariance.py
195d23e to
075a37f
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/design/batch-invariant-elementwise-rope.md (1)
3-4: 💤 Low valueMinor wording: Simplify "with respect to".
The phrase "with respect to batch configuration" is correct but can be tightened. Consider alternatives like "for batch configuration", "given batch configuration", or restructuring to "batch-configuration-dependent" for brevity.
🤖 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/design/batch-invariant-elementwise-rope.md` around lines 3 - 4, The phrase "with respect to batch configuration" in the opening paragraph of the document is wordy and can be simplified. Replace "with respect to batch configuration" with a more concise alternative such as "for batch configuration", "given batch configuration", or restructure the sentence to use "batch-configuration-dependent" to improve clarity and brevity while maintaining the intended meaning about operations being pass-through for different batch configurations.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@docs/design/batch-invariant-elementwise-rope.md`:
- Around line 3-4: The phrase "with respect to batch configuration" in the
opening paragraph of the document is wordy and can be simplified. Replace "with
respect to batch configuration" with a more concise alternative such as "for
batch configuration", "given batch configuration", or restructure the sentence
to use "batch-configuration-dependent" to improve clarity and brevity while
maintaining the intended meaning about operations being pass-through for
different batch configurations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8fb1da7a-992c-479e-b6fb-f4a6dcfb777e
📒 Files selected for processing (5)
.github/workflows/ci.ymldocs/design/batch-invariant-elementwise-rope.mdrl_engine/testing/__init__.pyrl_engine/testing/forward_invariance.pytests/test_forward_invariance.py
✅ Files skipped from review due to trivial changes (1)
- rl_engine/testing/init.py
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/ci.yml
- tests/test_forward_invariance.py
- rl_engine/testing/forward_invariance.py
075a37f to
a5d5d5b
Compare
|
cc @a-kaa PTAL |
|
Hi, the WS1 be assigned to me @maxiaosong1124. I am gladed to your contribution but we have some contact for the code. We want the interfaces are aligned. PLS contact with wechat(yye6964434), we haved finished some ops(actually include rope). It is best to avoid doing repetitive work. |
|
Hi @a-kaa Sorry for the mix up. The issue actually still shows as unassigned on my end, so I didn't realize you were already working on WS1. I agree we should align interfaces to avoid duplicate work. However, I keep my WeChat mostly for personal use, so I'd much prefer we discuss things here on GitHub. But if you think it's better to discuses it in person I'd like to do so. |
|
@haoruilee I apologize for causing any unnecessary misunderstanding. Actually, staring with WS issues have already been taken. You ca take on other tasks expect the five workstreams of the P0.3+ roadmap. |
|
Hi @a-kaa, I got it and feel free to close this PR if it's no longer needed. Sorry for my misunderstanding. |
|
@haoruilee We will have some issues(not WS issue) in next few days and welcome to contribute. |
3 participants
Summary
rl_engine.testing.forward_invarianceatol=1e-6, rtol=1e-6) for CPU libm/vector paths while keeping the other pass-through ops and RoPE bitwise by defaulttests/test_forward_invariance.pyinto the normal CI unit-test job as its own narrow stepCloses #149
Notes
No audited op showed reduction-style drift. This PR does not add a production RoPE kernel or change runtime dispatch; it adds the reference contract, audit, CI coverage, and tests required to close the pass-through audit.
Validation
python3 -m pytest rl_engine/tests/test_dispatch.py -v && python3 -m pytest tests/test_forward_invariance.py -v->3 passed;44 passed, 14 skipped/tmp/rl-kernel-cpu-ci-venv/bin/python -m pytest tests/test_forward_invariance.py -qwithtorch-2.12.1+cpu->17 passed, 15 skippedpython3 -m pytest tests/test_forward_invariance.py tests/test_reference_ops.py -q->51 passed, 14 skippedpython3 -m pytest rl_engine/tests/test_dispatch.py tests/test_op_accuracy.py tests/test_grpo_single_gpu_example.py tests/test_vllm_rollout_sampler.py -q->35 passed/tmp/rl-kernel-test-venv/bin/python -m pytest tests/ rl_engine/tests/ -q->205 passed, 15 skippeduvx mypy --ignore-missing-imports rl_engine/-> passeduvx pre-commit run --files .github/workflows/ci.yml docs/design/batch-invariant-elementwise-rope.md rl_engine/testing/__init__.py rl_engine/testing/forward_invariance.py tests/test_forward_invariance.py-> passeduvx ruff check rl_engine/testing/forward_invariance.py rl_engine/testing/__init__.py tests/test_forward_invariance.py-> passedpython3 -m compileall rl_engine/testing tests/test_forward_invariance.py-> passedgit diff --check-> passed/tmp/rl-kernel-docs-venv/bin/mkdocs build --strict -f mkdocs.yaml-> passedSummary by CodeRabbit