Repository navigation
Conversation
|
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:
📝 WalkthroughWalkthroughAdds support for the Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant Runner as CI Runner (H200 single)
participant Py as pytest (e2e tests)
participant Spec as MODEL_SPECS
participant Worker as Model Worker
GH->>Runner: start nightly-benchmark (matrix selects zai-org/GLM-4.6)
Runner->>Py: run generated test class TestNightlyGlm46Single
Py->>Spec: resolve spec for "zai-org/GLM-4.6"
Spec-->>Py: return model path, tp=4, args, features
Py->>Runner: request worker launch with args (--trust-remote-code, tp=4)
Runner->>Worker: launch model worker
Worker-->>Py: ready (http/grpc endpoints)
Py->>Runner: execute benchmarks (http + grpc)
Runner-->>GH: upload test results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
|
Hi @nishanthp, the DCO sign-off check has failed. All commits must include a To fix existing commits: # Sign off the last N commits (replace N with the number of unsigned commits)
git rebase HEAD~N --signoff
git push --force-with-leaseTo sign off future commits automatically:
|
![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 adds the GLM-4.6 model to the nightly performance benchmarks and defines its infrastructure specifications. Feedback was provided regarding the need for the --trust-remote-code argument to ensure the model loads correctly and an adjustment to the worker count in the benchmark configuration to avoid redundant test execution.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/nightly-benchmark.yml:
- Line 350: The inline matrix item using braces (the entry containing id:
zai-org/GLM-4.6, slug: zai-org-GLM-4.6, test_class: TestNightlyGlm46Single)
violates YAMLlint brace-spacing; replace the inline mapping with a standard
block mapping instead of braces — expand the list item so it uses separate keys
(id, slug, test_class) on their own indented lines under the dash rather than an
inline "{ ... }" form to satisfy YAMLlint.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: cd3b5ba2-dbba-4924-910b-0c4f8f0faae4
📒 Files selected for processing (3)
.github/workflows/nightly-benchmark.ymle2e_test/benchmarks/test_nightly_perf.pye2e_test/infra/model_specs.py
6555d27 to
e2561c4
Compare
![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: e2561c49ee
ℹ️ 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".
| if _multi_workers > 1: | ||
| variants.append(("Multi", _multi_workers)) |
There was a problem hiding this comment.
This new guard stops generating TestNightly*Multi classes when _multi_workers == 1, which removes TestNightlyGptOss20bMulti from this module even though the nightly workflow still schedules that class in the H100 multi-worker matrix (.github/workflows/nightly-benchmark.yml model entry for gpt-oss and run step pytest ... -k "$K_FILTER"). Because -k only runs matching tests, that job now selects nothing and pytest exits non-zero (no tests collected), so the multi-worker gpt-oss nightly jobs fail.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f93db73bb9
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e_test/benchmarks/test_nightly_perf.py`:
- Around line 146-150: The test variant generation conditionally omits the
"Multi" variant when _multi_workers == 1, which removes the generated
TestNightlyGptOss20bMulti class and breaks the workflow selector; update the
variants creation so "Multi" is always added (e.g., variants = [("Single", 1),
("Multi", max(1, _multi_workers))] or always append ("Multi", _multi_workers))
so the TestNightly*Multi class is emitted even if count is 1, leaving any
runtime skip/behavior decisions to the test logic that consumes variants.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 8e35e324-38a0-466d-9a88-97c827cc0305
📒 Files selected for processing (3)
.github/workflows/nightly-benchmark.ymle2e_test/benchmarks/test_nightly_perf.pye2e_test/infra/model_specs.py
|
Hi @nishanthp, the DCO sign-off check has failed. All commits must include a To fix existing commits: # Sign off the last N commits (replace N with the number of unsigned commits)
git rebase HEAD~N --signoff
git push --force-with-leaseTo sign off future commits automatically:
|
488b4d6 to
d19bc04
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e_test/benchmarks/test_nightly_perf.py`:
- Around line 146-150: Update the module-level docstring to accurately describe
that the test generates "Single" classes for all models and only generates
"Multi" classes when the _multi_workers variable is greater than 1; mention the
conditional behavior tied to the variants list construction and the for loop
iterating over (_suffix, _count) so readers know Multi classes may be skipped
when _multi_workers == 1 (also update any similar wording near the later
reference to the variants/for loop around the second occurrence).
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 85346372-3e1e-4fd5-98e5-1dcf2f9e4d46
📒 Files selected for processing (3)
.github/workflows/nightly-benchmark.ymle2e_test/benchmarks/test_nightly_perf.pye2e_test/infra/model_specs.py
Signed-off-by: Nishanth Prakash <nishanth.prakash@gmail.com>
Signed-off-by: Nishanth Prakash <nishanth.prakash@gmail.com>
Signed-off-by: Nishanth Prakash <nishanth.prakash@gmail.com>
|
@CatherineSue @key4ng Please take a look. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Signed-off-by: Nishanth Prakash <nishanth.prakash@gmail.com>
CatherineSue
left a comment
CatherineSue
left a comment
There was a problem hiding this comment.
Thanks for the PR. It seems the PR title check is failing. And I don't think it is gonna fit in H100.
| "tp": 4, | ||
| "features": ["chat", "streaming", "function_calling", "reasoning"], | ||
| "worker_args": ["--trust-remote-code"], | ||
| "vllm_args": ["--trust-remote-code"], |
There was a problem hiding this comment.
We don't need this parameter for this one. No .py in the model files.
| # GLM-4.6 - nightly benchmarks | ||
| "zai-org/GLM-4.6": { | ||
| "model": _resolve_model_path("zai-org/GLM-4.6"), | ||
| "tp": 4, |
There was a problem hiding this comment.
This model is 665GB in BF16. It won't fit in 8xH100.
There was a problem hiding this comment.
Confirmed with @slin1237 and it looks like it needs multi node support, which is not supported by the nightly benchmarking pipeline.
Closing this PR
4 participants
Description
Problem
The nightly benchmark workflow does not include coverage for
zai-org/GLM-4.6, so regressions for that model are not captured in the nightly run.Solution
Add
zai-org/GLM-4.6to the nightly benchmark configuration and model specs so it runs as part of the nightly benchmark suite.Changes
zai-org/GLM-4.6to the nightly benchmark workflow.tp: 8and the expected feature set.Test Plan
TestNightlyGlm46Single.zai-org/GLM-4.6withtp=8.Summary by CodeRabbit
New Features
Tests
Chores