Repository navigation
feat(bfcl): multi-model nightly A/B matrix on per-model runners - #1791
Conversation
…to-detect) Signed-off-by: key4ng <rukeyang@gmail.com>
Signed-off-by: key4ng <rukeyang@gmail.com>
…ntext in if GitHub Actions does not allow the \`matrix\` context in a job-level \`if\` (only github/inputs/needs/vars are permitted there). The \`bfcl-ab\` job was referencing \`matrix.name\` and \`matrix.runner\` in its \`if\`, causing a runtime "Unrecognized named-value: 'matrix'" error. Fix: add a lightweight \`setup\` job that emits the filtered leg list as JSON via a Python heredoc (dispatch \`only\` selector; exclude Blackwell legs on PRs). \`bfcl-ab\` now consumes it with \`fromJSON(needs.setup.outputs.matrix)\` and its \`if\` no longer references \`matrix\`. The static \`strategy.matrix.include\` list in \`bfcl-ab\` is deleted (the leg definitions live in the \`setup\` Python). Signed-off-by: key4ng <rukeyang@gmail.com>
Signed-off-by: key4ng <rukeyang@gmail.com>
Signed-off-by: key4ng <rukeyang@gmail.com>
github-actions
Bot
added
documentation
|
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:
📝 WalkthroughWalkthroughThe nightly BFCL workflow is refactored to drive model legs from a dynamically generated matrix. A new ChangesBFCL Nightly Matrix Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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)
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 updates the README documentation to detail the nightly matrix configurations, runners, and reasoning-parser fallbacks, and modifies launch_arm.sh to conditionally append the --tool-call-parser flag. Feedback identifies a bug where SMG_TOOL_PARSER falls back to a default value when BFCL_SMG_TOOL_PARSER is explicitly set to an empty string, and suggests refactoring the conditional check to a standard if block for better compatibility with set -e.
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.
| ) | ||
| # Empty tool-call-parser => let SMG auto-detect (e.g. gpt-oss routes through | ||
| # the harmony pipeline via HarmonyDetector; passing a parser there is wrong). | ||
| [ -n "$SMG_TOOL_PARSER" ] && smg_cmd+=(--tool-call-parser "$SMG_TOOL_PARSER") |
There was a problem hiding this comment.
There is a bug that prevents BFCL_SMG_TOOL_PARSER="" from actually omitting the --tool-call-parser flag.
On line 36, SMG_TOOL_PARSER is initialized using the :- operator:
SMG_TOOL_PARSER="${BFCL_SMG_TOOL_PARSER:-qwen}"Because of :-, if BFCL_SMG_TOOL_PARSER is set to an empty string (""), it falls back to "qwen". Thus, SMG_TOOL_PARSER will never be empty, and --tool-call-parser qwen will always be appended, breaking the gpt-oss leg.
To fix this, change line 36 to use ${BFCL_SMG_TOOL_PARSER-qwen} (without the colon), which only falls back to "qwen" if the variable is completely unset, preserving the explicit empty string.
Additionally, for better compatibility with set -e (to avoid returning a non-zero exit code when the condition is false), refactor the one-liner && into a standard if block.
| [ -n "$SMG_TOOL_PARSER" ] && smg_cmd+=(--tool-call-parser "$SMG_TOOL_PARSER") | |
| if [ -n "$SMG_TOOL_PARSER" ]; then | |
| smg_cmd+=(--tool-call-parser "$SMG_TOOL_PARSER") | |
| fi |
References
- In shell scripts with set -e enabled, avoid chaining commands with && if you expect the script to exit immediately upon the failure of the first command.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![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: 0cbc70e91d
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) | ||
| # Empty tool-call-parser => let SMG auto-detect (e.g. gpt-oss routes through | ||
| # the harmony pipeline via HarmonyDetector; passing a parser there is wrong). | ||
| [ -n "$SMG_TOOL_PARSER" ] && smg_cmd+=(--tool-call-parser "$SMG_TOOL_PARSER") |
There was a problem hiding this comment.
When the gpt-oss matrix leg sets BFCL_SMG_TOOL_PARSER to the empty matrix.smg_tool, launch_arm.sh still initializes SMG_TOOL_PARSER with ${BFCL_SMG_TOOL_PARSER:-qwen}, so the value becomes qwen; this added condition is then true and the gateway is launched with --tool-call-parser qwen. That bypasses the Harmony auto-detect path the gpt-oss leg depends on, so this leg is tested with the wrong parser or can fail at startup. The default needs to apply only when the variable is unset, not when it is explicitly empty.
Useful? React with 👍 / 👎.
| {"name": "deepseek-v4", "runner": "blackwell", "tp": 4, | ||
| "gpu_a": "0,1,2,3", "gpu_b": "4,5,6,7", | ||
| "model": "deepseek-ai/DeepSeek-V4-Flash", "bfcl_model": "deepseek-ai/DeepSeek-V4-Flash-FC", |
There was a problem hiding this comment.
For these new Blackwell matrix legs, the selected checkpoints are TP=4 models, but the later Stage model weights step is still hard-coded to scripts/ci_download_model.sh --gpu-tier 2; that script's tier resolver only downloads models with spec['tp'] <= tier (scripts/ci_download_model.sh:37-48). On a cold Blackwell runner these checkpoints are not pre-staged, so launch_arm.sh has only its 420s startup wait while vLLM tries to fetch hundreds of GB outside the download lock, which makes the new nightly legs time out before serving.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 @.github/workflows/nightly-bfcl.yml:
- Around line 30-35: The workflow descriptions for `model` and `bfcl_model` warn
users not to override only one parameter, but there is no actual validation
enforcing this constraint. Add a setup-time validation step that implements an
XOR check before matrix execution to ensure either both `model` and `bfcl_model`
are provided together or neither is provided. If the check detects that only one
of these parameters is set (anywhere in the workflow where they appear,
including around lines 185-186), the workflow should fail immediately with a
clear error message about the mismatched pair requirement, preventing any
downstream matrix jobs from executing.
- Line 247: The `actions/upload-artifact@v7` action reference uses a mutable
version tag which poses a supply-chain security risk. Replace the `@v7` tag with
a full-length commit SHA (format: `actions/upload-artifact@{COMMIT_SHA} #
v7.0.1`) to pin the action to an immutable reference. Apply this same fix to all
other mutable action references in the workflow file (also present at lines 72,
75, 92, and 190-191) to ensure consistent security practices across the entire
workflow.
- Line 213: The bash parameter expansion in scripts/bfcl/launch_arm.sh at line
40 uses the `${BFCL_SMG_TOOL_PARSER:-qwen}` syntax which treats empty strings as
unset variables and defaults to qwen, breaking the auto-detect feature that
relies on empty parser values. Change the parameter expansion from `:-`
(colon-dash) to just `-` (dash only) so that empty string values pass through
unchanged and only truly unset variables trigger the qwen default, preserving
the intended auto-detection behavior via HarmonyDetector.
In `@scripts/bfcl/README.md`:
- Line 66: In the gpt-oss note that explains the tool-call-parser behavior
(after line 74), add a clarification statement that gpt-oss also lacks a
reasoning-parser for both the openai arm and the secondary arm. This will ensure
readers understand that the "—" symbols in both reasoning-parser columns for
gpt-oss are intentional and explained, preventing confusion about missing
configuration details.
🪄 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: 3174226b-125a-4f0c-b380-cdd0e455b8fd
📒 Files selected for processing (3)
.github/workflows/nightly-bfcl.ymlscripts/bfcl/README.mdscripts/bfcl/launch_arm.sh
| } >> "$GITHUB_STEP_SUMMARY" || echo "no report produced for ${{ matrix.name }}" >> "$GITHUB_STEP_SUMMARY" | ||
| - name: Upload comparison | ||
| if: always() | ||
| uses: actions/upload-artifact@v7 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only check for mutable action refs in this workflow.
nl -ba .github/workflows/nightly-bfcl.yml | rg -n 'uses:\s*[^#]+@v[0-9]+'Repository: lightseekorg/smg
Length of output: 101
🏁 Script executed:
# First, check if the file exists and read around line 247
if [ -f ".github/workflows/nightly-bfcl.yml" ]; then
echo "=== File exists, reading around line 247 ==="
sed -n '240,255p' .github/workflows/nightly-bfcl.yml | cat -n
else
echo "File not found"
git ls-files | grep -i nightly
fiRepository: lightseekorg/smg
Length of output: 830
🏁 Script executed:
# Count lines and find all 'uses' directives with version tags
echo "=== Total lines and all 'uses' with version tags ==="
wc -l .github/workflows/nightly-bfcl.yml
echo ""
echo "=== All 'uses' directives with `@v` or `@tags` ==="
grep -n "uses:" .github/workflows/nightly-bfcl.yml | head -20Repository: lightseekorg/smg
Length of output: 538
🌐 Web query:
GitHub Actions security best practice pin commit SHA vs version tag
💡 Result:
In GitHub Actions, pinning to a full-length commit SHA is considered the security gold standard, whereas pinning to a version tag is a matter of convenience that carries inherent risks [1][2]. The fundamental difference lies in immutability: 1. Commit SHA (Immutable): A full-length commit SHA is a unique, fixed identifier for a specific state of a repository [3][4]. Once pinned to a SHA, your workflow is guaranteed to execute the exact same code every time, making it immune to "tag retargeting" or "force-push" attacks where a malicious actor alters the code behind a seemingly legitimate tag [1][2][5]. 2. Version Tag (Mutable): Tags (e.g., v4, v4.1.0) are references that can be moved or deleted by repository maintainers [1][5]. Even if you trust an action's author, a compromised account or repository could allow an attacker to update a tag to point to malicious code, which your workflows would then automatically adopt [1][2][4]. Best Practices: - Always use the full-length commit SHA (e.g., actions/checkout@3df4ab1) instead of an abbreviated one [3][4]. - Maintain readability and auditability by including the version tag as a comment in your workflow file (e.g., uses: actions/checkout@3df4ab1 # v4.0.0) [4][6]. - Automate the maintenance of these pins. Tools like Renovate (using presets such as helpers:pinGitHubActionDigests) or Dependabot can automatically open pull requests to update your SHAs when new releases are published, allowing you to review changes before merging [7][4][5]. - If you are an administrator, consider enabling organization or repository-level policies that enforce commit SHA pinning, which will cause any non-pinned actions to fail [1][4][8]. While pinning to a tag is permissible if you strictly trust the creator, it remains a secondary choice compared to the immutability and security provided by commit SHAs [1][2].
Citations:
- 1: https://docs.github.com/en/actions/reference/security/secure-use
- 2: https://www.stepsecurity.io/blog/pinning-github-actions-for-enhanced-security-a-complete-guide
- 3: https://docs.github.com/en/actions/how-tos/create-and-publish-actions/manage-custom-actions
- 4: https://architecture.lullabot.com/adr/20260505-pin-github-actions-to-hash/
- 5: https://www.systemshardening.com/articles/cicd/github-actions-supply-chain-hardening/
- 6: https://nearform.com/insights/why-you-should-pin-your-github-actions-by-commit-hash/
- 7: https://emmer.dev/blog/pin-your-github-actions-to-protect-against-mutability/
- 8: https://github.blog/changelog/2025-08-15-github-actions-policy-now-supports-blocking-and-sha-pinning-actions/
🌐 Web query:
actions/upload-artifact@v7 commit SHA pinning security
💡 Result:
Pinning actions/upload-artifact to a commit SHA is a recommended security best practice for GitHub Actions workflows [1][2][3]. As of June 2026, the latest version is v7.0.1 [4][5]. Security Context Using a tag like @v7 is a mutable reference; if the action's repository is compromised, an attacker could force-push a new commit to that tag, potentially injecting malicious code into your workflows [1][3]. Pinning to an immutable 40-character commit SHA ensures that your workflow always executes the exact same code version, protecting against supply chain attacks [6][1][3]. Implementation To use SHA pinning, replace the tag in your workflow file with the full commit hash. To maintain readability and allow automated tools to track updates, it is standard practice to include the version as a comment [6][7][3]: uses: actions/upload-artifact@bbbca2d # v7.0.1 Automated Maintenance You do not need to manually hunt for SHA updates. Tools like Dependabot and Renovate support SHA-pinned actions and will automatically open pull requests to update the SHA when a new release is published, keeping your workflows both secure and current [6][7][3]. Policy Enforcement Note that many organizations now enforce SHA pinning via GitHub organization-level policies [8][7]. If such a policy is active, workflows using tag-based references (e.g., @v7) will fail [7]. Furthermore, if you maintain actions that reference other actions internally, those sub-actions should also be SHA-pinned to ensure compatibility with these strict security policies [7].
Citations:
- 1: https://docs.github.com/en/actions/reference/security/secure-use
- 2: https://www.stepsecurity.io/blog/pinning-github-actions-for-enhanced-security-a-complete-guide
- 3: https://github.com/kubernetes/community/blob/main/github-management/github-actions-policy.md
- 4: https://github.com/actions/upload-artifact/releases
- 5: https://github.com/actions/upload-artifact/releases/tag/v7.0.1
- 6: 🔒 Security: Pin all GitHub Actions to commit SHAs fastapi/fastapi#15225
- 7: https://www.romainlespinasse.dev/posts/github-actions-commit-sha-pinning/
- 8: https://github.blog/changelog/2025-08-15-github-actions-policy-now-supports-blocking-and-sha-pinning-actions/
Pin actions/upload-artifact@v7 to a commit SHA.
Line 247 uses a mutable version tag (@v7), which is vulnerable to tag retargeting attacks. Pinning to a full-length commit SHA (e.g., actions/upload-artifact@bbbca2ddaa5d8feaa63e36b76fdaad77386f024f # v7.0.1) eliminates this supply-chain risk by making the action reference immutable. This aligns with GitHub's security best practices for Actions.
Note: This same pattern exists elsewhere in the workflow (e.g., lines 72, 75, 92, 190–191). Consider fixing all mutable action references.
🧰 Tools
🪛 zizmor (1.25.2)
[error] 247-247: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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/nightly-bfcl.yml at line 247, The
`actions/upload-artifact@v7` action reference uses a mutable version tag which
poses a supply-chain security risk. Replace the `@v7` tag with a full-length
commit SHA (format: `actions/upload-artifact@{COMMIT_SHA} # v7.0.1`) to pin the
action to an immutable reference. Apply this same fix to all other mutable
action references in the workflow file (also present at lines 72, 75, 92, and
190-191) to ensure consistent security practices across the entire workflow.
Source: Linters/SAST tools
… names Signed-off-by: key4ng <rukeyang@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
scripts/bfcl/README.md (1)
71-74: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winClarify that gpt-oss has no reasoning-parser for either arm.
The table shows gpt-oss with "—" for both reasoning-parser columns, and the explanation (lines 71–74) covers only the tool-call-parser behavior. The absence of reasoning-parser for gpt-oss deserves a brief mention to prevent confusion.
💡 Suggested clarification
Extend the gpt-oss note (after line 74) to clarify reasoning-parser absence:
> **gpt-oss has no SMG tool-call-parser.** SMG handles gpt-oss through its harmony > pipeline (`model_gateway/src/routers/grpc/harmony/`), auto-activated by > `HarmonyDetector` on a `gpt-oss` model id / `GptOssForCausalLM` architecture. So > the SMG arm passes **no** `--tool-call-parser` (`BFCL_SMG_TOOL_PARSER=""`). +> Similarly, gpt-oss reasoning-content is handled natively; no explicit +> reasoning-parser is needed on either arm.🤖 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 `@scripts/bfcl/README.md` around lines 71 - 74, The existing gpt-oss explanation in the README (lines 71-74) only clarifies the absence of a tool-call-parser for the SMG arm, but the table shows gpt-oss with "—" for both reasoning-parser columns as well, which is not explained. Extend the gpt-oss note after line 74 to add a brief clarification that gpt-oss also has no reasoning-parser for either the SMG arm or the direct arm, to align with what the table displays and prevent confusion about why both reasoning-parser columns show dashes.
🤖 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 @.github/workflows/nightly-bfcl.yml:
- Around line 157-161: The setup function in the nightly-bfcl workflow currently
includes Blackwell legs in PR validation runs, which contradicts the intent to
limit PR checks to a quick H100-only sanity subset. Modify the logic in the
setup function where legs are determined or filtered to exclude Blackwell
runners when the workflow is triggered by a pull request event, ensuring that
only H100 legs are included in PR matrix runs while maintaining the full matrix
(H100 and Blackwell) for nightly/main branch runs. The validation error at line
157 checking for unknown leg values should remain unchanged.
In `@scripts/bfcl/launch_arm.sh`:
- Around line 109-111: The script uses stream_log to create background tail
processes and stores their PIDs in variables like log_tail, but if wait_http or
wait_grpc fails due to set -euo pipefail, the script exits immediately without
cleaning up these background processes. Add a trap handler near the top of the
script after variable declarations that will be triggered on EXIT to ensure all
tracked background tail process PIDs are killed. Modify the pattern wherever
stream_log is called to capture and track the returned PID in a variable, then
reference these variables in the trap handler to clean up all background
processes when the script exits, whether normally or due to an error. This
applies to the stream_log calls for log_tail variables at multiple locations
throughout the script.
---
Duplicate comments:
In `@scripts/bfcl/README.md`:
- Around line 71-74: The existing gpt-oss explanation in the README (lines
71-74) only clarifies the absence of a tool-call-parser for the SMG arm, but the
table shows gpt-oss with "—" for both reasoning-parser columns as well, which is
not explained. Extend the gpt-oss note after line 74 to add a brief
clarification that gpt-oss also has no reasoning-parser for either the SMG arm
or the direct arm, to align with what the table displays and prevent confusion
about why both reasoning-parser columns show dashes.
🪄 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: 02855d75-a2c2-49f3-aa9b-ca06cfd63b62
📒 Files selected for processing (3)
.github/workflows/nightly-bfcl.ymlscripts/bfcl/README.mdscripts/bfcl/launch_arm.sh
| log_tail=$(stream_log "$RUN_DIR/arm_a_vllm.log") | ||
| wait_http "http://127.0.0.1:$ARM_A_PORT/health" "${BFCL_STARTUP_TIMEOUT:-420}" | ||
| kill "$log_tail" 2>/dev/null || true |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Tail processes may be orphaned if health checks fail.
If wait_http or wait_grpc returns non-zero (timeout or failure), the script exits immediately due to set -euo pipefail, leaving the background tail process running. In CI, the runner typically cleans up child processes when the job terminates, but on development machines these tail processes might persist.
Consider adding a trap handler to ensure cleanup on early exit.
🧹 Proposed trap-based cleanup
Add near the top of the script (after variable declarations):
+TAIL_PIDS=()
+cleanup_tails() {
+ for pid in "${TAIL_PIDS[@]}"; do
+ kill "$pid" 2>/dev/null || true
+ done
+}
+trap cleanup_tails EXITThen modify the pattern to track PIDs:
- log_tail=$(stream_log "$RUN_DIR/arm_a_vllm.log")
+ log_tail=$(stream_log "$RUN_DIR/arm_a_vllm.log")
+ TAIL_PIDS+=("$log_tail")
wait_http "http://127.0.0.1:$ARM_A_PORT/health" "${BFCL_STARTUP_TIMEOUT:-420}"
- kill "$log_tail" 2>/dev/null || true
+ kill "$log_tail" 2>/dev/null || trueAlso applies to: 128-130, 144-146
🤖 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 `@scripts/bfcl/launch_arm.sh` around lines 109 - 111, The script uses
stream_log to create background tail processes and stores their PIDs in
variables like log_tail, but if wait_http or wait_grpc fails due to set -euo
pipefail, the script exits immediately without cleaning up these background
processes. Add a trap handler near the top of the script after variable
declarations that will be triggered on EXIT to ensure all tracked background
tail process PIDs are killed. Modify the pattern wherever stream_log is called
to capture and track the returned PID in a variable, then reference these
variables in the trap handler to clean up all background processes when the
script exits, whether normally or due to an error. This applies to the
stream_log calls for log_tail variables at multiple locations throughout the
script.
Signed-off-by: key4ng <rukeyang@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c923ee471
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # gpt-oss SMG arm passes NO tool parser (smg_tool empty) -> harmony auto-detect. | ||
| {"name": "gpt-oss", "runner": "4-gpu-h100", "tp": 2, | ||
| "gpu_a": "0,1", "gpu_b": "2,3", | ||
| "model": "openai/gpt-oss-120b", "bfcl_model": "openai/gpt-oss-120b-FC", |
There was a problem hiding this comment.
For new legs such as gpt-oss/DeepSeek/MiniMax/Kimi that bfcl-eval does not already ship, the existing registration step registers $MODEL through scripts/bfcl/register_bfcl_model.py with its default QwenFCHandler. In those scenarios BFCL formats and decodes Qwen-style XML instead of the native tool-call protocol these legs are meant to compare, so the run measures Qwen-prompt compatibility or invalid scores rather than SMG/vLLM parser parity; each non-Qwen leg needs a matching/generic FC handler or should avoid this fallback when unsupported.
Useful? React with 👍 / 👎.
…or big MoEs Signed-off-by: key4ng <rukeyang@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/workflows/nightly-bfcl.yml:
- Around line 139-140: Pin immutable model revisions for the deepseek-v4,
minimax-m2.7, and kimi-k2.6 legs in the workflow matrix to prevent supply-chain
risks when executing remote code with the --trust-remote-code flag in
vllm_extra. Modify the model references to use the format model-name@commit-sha
(e.g., deepseek-ai/DeepSeek-V4-Flash@<commit-sha>) in the matrix configuration,
and ensure that both the weight-staging and serving execution paths reference
the same pinned revision to maintain consistency throughout the nightly
automation workflow.
🪄 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: 46ea045e-cec2-4824-b0aa-5adde161e802
📒 Files selected for processing (1)
.github/workflows/nightly-bfcl.yml
| "vllm_extra": "--tokenizer-mode deepseek_v4 --trust-remote-code --kv-cache-dtype fp8 --block-size 256", | ||
| "gpu_mem": "0.90", "startup_timeout": "2400"}, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify evidence for remote-code execution + token presence in this workflow.
# Expected: Lines showing both `HF_TOKEN` and `--trust-remote-code` in the same job.
nl -ba .github/workflows/nightly-bfcl.yml | sed -n '130,160p;184,194p;218,230p'Repository: lightseekorg/smg
Length of output: 101
🏁 Script executed:
# Read the workflow file to examine the specific lines and context
cat -n .github/workflows/nightly-bfcl.yml | sed -n '130,165p'Repository: lightseekorg/smg
Length of output: 2889
🏁 Script executed:
# Check for HF_TOKEN environment setup
cat -n .github/workflows/nightly-bfcl.yml | sed -n '180,200p'Repository: lightseekorg/smg
Length of output: 1427
🏁 Script executed:
# Check if there are model revision pins anywhere (e.g., using `@revision` or similar)
grep -n "revision\|@\|deepseek\|minimax\|kimi" .github/workflows/nightly-bfcl.yml | head -20Repository: lightseekorg/smg
Length of output: 1522
🏁 Script executed:
# Find scripts that handle model loading/launching
fd -t f "launch" . | head -10Repository: lightseekorg/smg
Length of output: 336
🏁 Script executed:
# Check if any revision pins exist elsewhere (e.g., in Python setup or launch scripts)
grep -r "revision\|@[a-f0-9]\|--revision" . --include="*.sh" --include="*.py" | grep -i "deepseek\|minimax\|kimi" | head -20Repository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Check how vLLM is invoked with the model
grep -n "python.*vllm\|vLLM\|serve\|MODEL" .github/workflows/nightly-bfcl.yml | grep -A2 -B2 "vllm"Repository: lightseekorg/smg
Length of output: 483
🏁 Script executed:
# Check launch_arm.sh for how models are specified/loaded
cat scripts/bfcl/launch_arm.sh | head -100Repository: lightseekorg/smg
Length of output: 4598
🏁 Script executed:
# Also check register_bfcl_model.py to see if it accepts or enforces revision specs
cat bindings/python/src/smg/launch_router.py | grep -A10 -B10 "model\|revision" | head -50Repository: lightseekorg/smg
Length of output: 42
Pin immutable model revisions when enabling --trust-remote-code on the deepseek-v4, minimax-m2.7, and kimi-k2.6 legs.
These legs execute remote model repo Python during startup while HF_TOKEN is present in the job environment. Without pinned revisions, the models pull from untagged references (default branch), creating a supply-chain path to token/runner compromise if an upstream repo is retagged or compromised. Pin immutable revisions in the workflow matrix (e.g., deepseek-ai/DeepSeek-V4-Flash@<commit-sha>) and ensure both the weight-staging and serving paths consume the same pinned revision before relying on nightly automation.
🤖 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/nightly-bfcl.yml around lines 139 - 140, Pin immutable
model revisions for the deepseek-v4, minimax-m2.7, and kimi-k2.6 legs in the
workflow matrix to prevent supply-chain risks when executing remote code with
the --trust-remote-code flag in vllm_extra. Modify the model references to use
the format model-name@commit-sha (e.g.,
deepseek-ai/DeepSeek-V4-Flash@<commit-sha>) in the matrix configuration, and
ensure that both the weight-staging and serving execution paths reference the
same pinned revision to maintain consistency throughout the nightly automation
workflow.
…ckstop Signed-off-by: key4ng <rukeyang@gmail.com>
… paired-override guard + doc Signed-off-by: key4ng <rukeyang@gmail.com>
…ilure only Signed-off-by: key4ng <rukeyang@gmail.com>
Signed-off-by: key4ng <rukeyang@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea0ce36ea5
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Per-leg health-wait budget: big MoEs need minutes for load + compile + quant. | ||
| BFCL_STARTUP_TIMEOUT: ${{ matrix.startup_timeout }} | ||
| # Arm logs + pidfiles under RUNNER_TEMP (per-job, uploadable) not /tmp. | ||
| BFCL_RUN_DIR: ${{ runner.temp }}/bfcl_run |
There was a problem hiding this comment.
When this CI step sets BFCL_RUN_DIR here, the pidfiles are written under ${{ runner.temp }}/bfcl_run, but step-level env does not carry over to the later Teardown arms step, which runs launch_arm.sh stop with the script's default /tmp/bfcl_ab. In any run that reaches arm launch, the intended pidfile/process-group cleanup therefore finds no pids and leaves cleanup to the GPU nuke backstop, including leaking the non-GPU SMG gateway until the job exits.
Useful? React with 👍 / 👎.
| MODEL: ${{ github.event.inputs.model || matrix.model }} | ||
| BFCL_MODEL_FC: ${{ github.event.inputs.bfcl_model || matrix.bfcl_model }} |
There was a problem hiding this comment.
only before applying model overrides
For a workflow_dispatch run where model and bfcl_model are set but only is left at its default empty value, this job-level override is applied to every matrix row while the row-specific parsers, GPU split, runner, and extra vLLM flags still come from qwen/gpt-oss/deepseek/etc. That produces several expensive A/B jobs with mismatched parser configs and weights rather than overriding a single selected leg as the input text promises; fail the setup job unless overrides are paired with only, or apply them only to the intended row.
Useful? React with 👍 / 👎.
…kip download Signed-off-by: key4ng <rukeyang@gmail.com>
…on shared host) Signed-off-by: key4ng <rukeyang@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5dc5b4e844
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # hard-killed launch step — exactly the leak that pinned ~250 GiB/GPU | ||
| # across jobs. Each bfcl leg uses the whole node (TP fills it), so no | ||
| # co-tenant job shares these GPUs. | ||
| run: bash scripts/ci_killall_sglang.sh nuke_gpus || true |
There was a problem hiding this comment.
When scripts/ci_killall_sglang.sh nuke_gpus cannot free the device or cannot query it, that helper emits ::error:: and exits nonzero after the dirty-GPU check, but this || true masks the failure. In the exact cleanup scenario this step is meant to catch—surviving vLLM/worker processes holding GPU memory—the workflow will still report success and return a dirty runner to the pool, which can contaminate subsequent GPU jobs.
Useful? React with 👍 / 👎.
Signed-off-by: key4ng <rukeyang@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bda57e2a98
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # picks a port, frees it, then vLLM binds it ~40s later — another leg | ||
| # grabs it meanwhile) and on GPU memory. A host-wide flock runs them one | ||
| # at a time; the lock releases when this step's shell exits. | ||
| exec 9>/tmp/bfcl-host.lock |
There was a problem hiding this comment.
For the 4-gpu-h100 matrix legs that can be bin-packed onto one H100 node, this lock lives in each runner pod's private /tmp, not on the host. The runner config shows hostNetwork: true and Volcano binpacking, but only mounts /models, /dev/shm, the workspace, /var/run, /var/lib/docker, and /dev/infiniband into the runner (scripts/k8s-runner-resources/runner-values-4-gpu-h100.yaml:23-30,45-55,103-115), so co-located PR/nightly legs still do not see the same flock while sharing the host network; the free-port race this step is trying to serialize can still happen. Put the lock on a hostPath/shared mount or enforce this with workflow-level concurrency before launching arms.
Useful? React with 👍 / 👎.
1 participant
Description
Problem
The nightly BFCL A/B (
scripts/bfcl/) — the parser-parity comparison of pure vLLM vs SMG→vLLM gRPC — only exercised a single model (Qwen/Qwen3.6-27B) on one4-gpu-h100node. We want coverage of the mid-2026 target SKUs (Kimi K2.6, DeepSeek V4, MiniMax M2.7, gpt-oss-120b) and the ability to run the large models on Blackwell (B300) hardware.Solution
Convert the single A/B job into a 5-model matrix, each leg on its own runner with its own parser config. Because the A/B isolates the parser frontend (model size is irrelevant — a smaller same-family checkpoint exercises the identical parse path), the large legs use flash/int4 checkpoints that fit half a B300 node at TP=4, so every leg runs both arms concurrently with no perf tuning. gpt-oss needs no SMG tool-call-parser — SMG's
HarmonyDetectorauto-routes it through the harmony pipeline — solaunch_arm.shnow omits the flag when unset.4-gpu-h100qwen3_xml/qwen3qwen_xml/qwen34-gpu-h100openai/—blackwelldeepseek_v4/deepseek_v4deepseek_v4/deepseek_v31†blackwellminimax_m2/minimax_m2minimax_m2/minimaxblackwellkimi_k2/kimi_k2kimik2/kimi_k25†Changes
scripts/bfcl/launch_arm.sh: make the SMG arm's--tool-call-parserconditional (omit whenBFCL_SMG_TOOL_PARSERis empty → gpt-oss harmony auto-detect)..github/workflows/nightly-bfcl.yml: convertbfcl-abto a 5-leg matrix on per-model runners (4-gpu-h100,blackwell); leg filtering (dispatchonly, PR-runs-H100-only) lives in a newsetupjob that emits the matrix as JSON, since thematrixcontext isn't allowed in a job-levelif.scripts/bfcl/README.md: per-model parser table + matrix/runner docs.run_ab.pyis unchanged — it already loops over both arms.† To confirm on the first nightly: SMG has no exact
deepseek_v4/kimi_k2reasoning parser yet (using closestdeepseek_v31/kimi_k25, markedTODO(confirm)); the new model IDs and the int4 Kimi invocation should be verified against the installed vLLM build.Test Plan
shellcheck scripts/bfcl/launch_arm.shpasses; behavioral check confirms the SMG tool-call-parser is omitted when empty and included when set.actionlint .github/workflows/nightly-bfcl.ymlclean (only pre-existing custom-runner-label warnings); matrix parses to the 5 expected legs;setupfilter logic verified for schedule / PR / dispatch-only/ unknown-leg (guarded) cases.4-gpu-h100legs only as an end-to-end pipeline sanity check; full validation is the scheduled nightly on real runners.Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit