Repository navigation
feat(bfcl): nightly BFCL A/B — SMG frontend vs pure vLLM - #1724
Conversation
Track B of the parser-verification proposal: run the official Berkeley Function Calling Leaderboard (bfcl-eval, FC mode) against two arms that differ ONLY in the serving frontend, so any score delta is attributable to the tokenization+parsing layer — the data that argues for engines adopting SMG's frontend. - scripts/bfcl/launch_arm.sh: bring up arm A (pure vLLM) or arm B (vLLM gRPC + SMG), parameterised via env; prints base_url; 'stop' tears down via pidfiles. - scripts/bfcl/run_ab.py: point official bfcl generate+evaluate (FC mode, the -FC handler, LOCAL_SERVER_ENDPOINT/PORT) at both arms, parse per-category accuracy, emit a markdown+JSON comparison table, regression gate on overall. - .github/workflows/nightly-bfcl.yml: cron nightly modeled on nightly-benchmark (build-wheel + 2-gpu job, one GPU per arm), uploads the comparison, summary. - README documents the FC-mode requirement, per-model parser flags, and the empirically-found gotchas (soundfile dep, max-model-len cap, HF_HUB_OFFLINE, vLLM EngineDeadError under load). Validation: brought up on the dev H100 box vs Qwen3-4B — arm A served native tool_calls and bfcl FC mode drove /v1/chat/completions (parser on the scored path) confirmed; a full end-to-end score was blocked by vLLM EngineDeadError under load on the shared GPU. Pipeline is complete; run on a dedicated GPU for the table. Signed-off-by: key4ng <rukeyang@gmail.com>
Ran the full pipeline on a dev H100 box (Qwen3-4B-Instruct-2507, BFCL simple_python, FC mode): pure vLLM 95.50% vs SMG->vLLM-gRPC 95.25% (Δ -0.25pp, 1 case — parity). run_ab.py drove evaluate+parse+diff and emitted the table. Fixes from the live run: - run_ab.py: match BFCL's real score filename (BFCL_v4_<category>_score.json is nested under score/<model>/<section>/) via a trailing-wildcard glob. - launch_arm.sh: BFCL_VLLM_EXTRA passthrough (e.g. --enforce-eager) on both arms; refactor the arm-B worker launch to an arg array. --enforce-eager + HF_HUB_OFFLINE were needed for a stable, fast run on the shared GPU. - README: record the validated result and the two shared-GPU gotchas. Signed-off-by: key4ng <rukeyang@gmail.com>
…fline band-aids Ran the A/B on Qwen/Qwen3.6-27B at TP=2 (one arm per GPU pair), BFCL simple_python FC mode: pure vLLM (qwen3_xml) 94.75% vs SMG->vLLM-gRPC (qwen_xml) 94.25% (Δ -0.50pp, 2 cases — parity). Both ran WITHOUT --enforce-eager and WITHOUT HF_HUB_OFFLINE. Root-caused the two first-cut band-aids: - --enforce-eager was masking a missing 'ninja' (vLLM torch.compile/CUDA-graph kernel build, needed for the qwen3_5 arch). Real fix: pip install ninja + PATH. - HF_HUB_OFFLINE was unnecessary; cached + online runs ~7 req/s (earlier crawl was a transient HF hiccup). Removed the forced offline from run_ab.py. - launch_arm.sh: add BFCL_TP knob (multi-GPU TP) + BFCL_GPU accepts '0,1'. - register_bfcl_model.py: NEW — register models bfcl-eval doesn't ship a handler for yet (e.g. Qwen/Qwen3.6-27B), by cloning an existing FC entry. Idempotent. - nightly-bfcl.yml: install ninja+soundfile, register the model, TP=2 on a 4-gpu runner, qwen3_xml/qwen_xml + qwen3 reasoning defaults. - README: real root causes, Qwen3.6-27B result, register/TP docs, note that SMG's auto model->parser map lacks Qwen3.6 (pass qwen_xml explicitly). Signed-off-by: key4ng <rukeyang@gmail.com>
…tegory A/B Ran the full BFCL non_live set (7 AST categories, 1390 cases) on Qwen3.6-27B at TP=2, FC mode. SMG->vLLM-gRPC vs pure vLLM, overall (unweighted) 84.05 vs 83.87 (Δ +0.18pp): SMG's frontend is at parity — marginally ahead, never worse than -0.25pp on any category. Low java/js scores are the model's non-Python ability (identical on both arms), confirming the A/B isolates the frontend. - nightly-bfcl.yml: default categories = full non_live set (simple_python/java/ javascript, multiple, parallel, parallel_multiple, irrelevance). Non-live only (reproducible, no live/internet data); PR-time stays on the CPU Track-A gate. - README: full per-category result table + non-live scope note. Signed-off-by: key4ng <rukeyang@gmail.com>
github-actions
Bot
added
documentation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a complete BFCL (Berkeley Function Calling Leaderboard) nightly A/B testing infrastructure: comprehensive documentation, a Python A/B driver ( ChangesBFCL Nightly A/B Testing Infrastructure
Sequence Diagram(s)sequenceDiagram
participant trigger as Cron / Dispatch
participant build_wheel as build-wheel job
participant bfcl_ab as bfcl-ab GPU job
participant launch_arm as launch_arm.sh
participant run_ab as run_ab.py
participant bfcl_cli as bfcl CLI
trigger->>build_wheel: schedule or workflow_dispatch
build_wheel->>build_wheel: build SMG wheel (sccache + Rust cache)
build_wheel-->>bfcl_ab: upload wheel artifact
bfcl_ab->>bfcl_ab: install wheel + bfcl-eval, register model
bfcl_ab->>launch_arm: mode=a → vLLM serve (GPUs 0-3, port 8000)
launch_arm-->>bfcl_ab: Arm A base URL
bfcl_ab->>launch_arm: mode=b → vLLM gRPC + SMG gateway (GPUs 4-7, port 8001)
launch_arm-->>bfcl_ab: Arm B base URL
bfcl_ab->>run_ab: baseline=Arm A, candidate=Arm B
run_ab->>bfcl_cli: bfcl generate + evaluate (Arm A)
run_ab->>bfcl_cli: bfcl generate + evaluate (Arm B)
run_ab->>run_ab: build_report → .md + .json
run_ab-->>bfcl_ab: exit 0/1 (BFCL_AB_REGRESSION=1 on failure)
bfcl_ab->>bfcl_ab: render summary + upload artifacts
bfcl_ab->>launch_arm: mode=stop (kill pidfiles)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
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 docstrings
🧪 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 introduces a nightly A/B testing framework for the Berkeley Function Calling Leaderboard (BFCL) under scripts/bfcl/, including scripts to launch baseline and candidate model serving arms, run the benchmark, and register new models. The review feedback highlights several critical reliability and robustness improvements: ensuring process termination in launch_arm.sh kills the entire process group to prevent orphaned GPU processes, replacing bash-specific /dev/tcp checks with a portable Python socket check, enabling check=True on subprocess runs in run_ab.py to avoid silent failures, filtering score files by model name to prevent cross-contamination, and explicitly validating that scores are successfully parsed for all requested categories before generating reports.
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.
| for pf in "$RUN_DIR"/*.pid; do | ||
| [ -e "$pf" ] || continue | ||
| pid="$(cat "$pf")" | ||
| kill "$pid" 2>/dev/null && echo "[launch_arm] killed $(basename "$pf" .pid) (pid $pid)" >&2 || true |
There was a problem hiding this comment.
The background processes are started using setsid, which runs them in a new session (and thus a new process group). Calling kill "$pid" only terminates the parent process (the session leader), leaving any child processes (such as vLLM's tensor-parallel worker processes) running as orphans. On GPU systems, these orphaned processes will continue to occupy GPU memory, leading to Out-Of-Memory (OOM) errors on subsequent runs.
To ensure all child processes are cleaned up, send the signal to the entire process group by passing the negative PID (e.g., kill -- "-$pid").
| kill "$pid" 2>/dev/null && echo "[launch_arm] killed $(basename "$pf" .pid) (pid $pid)" >&2 || true | |
| kill -- "-$pid" 2>/dev/null && echo "[launch_arm] killed $(basename "$pf" .pid) (pid $pid)" >&2 || true |
| proc = subprocess.run(cmd, env=env, check=False) | ||
| if proc.returncode != 0: | ||
| print(f"WARNING: {label} exited {proc.returncode}", file=sys.stderr) |
There was a problem hiding this comment.
The _run helper runs critical benchmark steps (bfcl generate and bfcl evaluate) with check=False. If either of these commands fails, the script merely prints a warning and continues. This can lead to silent failures where the benchmark fails to run or evaluate, but the script still exits with a success code (0) because no regression was detected on the non-existent scores.
Setting check=True ensures that any failure in the underlying benchmark commands will raise a CalledProcessError and correctly fail the script/CI run.
| proc = subprocess.run(cmd, env=env, check=False) | |
| if proc.returncode != 0: | |
| print(f"WARNING: {label} exited {proc.returncode}", file=sys.stderr) | |
| subprocess.run(cmd, env=env, check=True) |
| print(f"WARNING: no score dir at {score_root}", file=sys.stderr) | ||
| return out | ||
| for cat in categories: | ||
| acc = _find_category_accuracy(score_root, cat) |
There was a problem hiding this comment.
Pass the model parameter to _find_category_accuracy to allow filtering score files by the current model being evaluated.
| acc = _find_category_accuracy(score_root, cat) | |
| acc = _find_category_accuracy(score_root, cat, model) |
| def _find_category_accuracy(score_root: Path, category: str) -> float | None: | ||
| # BFCL nests scores as <model>/<section>/BFCL_v4_<category>_score.json, so | ||
| # match a trailing-wildcard pattern (the BFCL_v4_ prefix varies by version). | ||
| for path in score_root.rglob(f"*{category}_score.json"): |
There was a problem hiding this comment.
The _find_category_accuracy function searches for score files using score_root.rglob(f"*{category}_score.json") without filtering by the model name. If the project root (which defaults to /tmp/bfcl_ab) is reused across multiple runs with different models, this glob pattern can match and return score files from previous runs of other models.
To prevent cross-contamination of scores, filter the paths to ensure they contain the sanitized model name.
def _find_category_accuracy(score_root: Path, category: str, model: str) -> float | None:
# BFCL nests scores as <model>/<section>/BFCL_v4_<category>_score.json, so
# match a trailing-wildcard pattern (the BFCL_v4_ prefix varies by version).
sanitized_model = model.replace("/", "_")
for path in score_root.rglob(f"*{category}_score.json"):
if sanitized_model not in str(path):
continue| skip_generate=args.skip_generate, | ||
| ) | ||
|
|
||
| report_md, payload = build_report(baseline, candidate, categories) |
There was a problem hiding this comment.
If any of the requested categories fail to produce scores (e.g., due to an evaluation error or incorrect category name), the script currently proceeds to build the report with missing values and exits with 0.
To prevent silent failures in CI, explicitly verify that scores were successfully parsed for all requested categories on both arms, and exit with a non-zero code if any are missing.
missing_b = [c for c in categories if c not in baseline.scores]
missing_c = [c for c in categories if c not in candidate.scores]
if missing_b or missing_c:
if missing_b:
print(f"ERROR: Baseline missing scores for: {', '.join(missing_b)}", file=sys.stderr)
if missing_c:
print(f"ERROR: Candidate missing scores for: {', '.join(missing_c)}", file=sys.stderr)
return 1
report_md, payload = build_report(baseline, candidate, categories)| wait_grpc() { # crude TCP-listen check for the gRPC port | ||
| local port="$1" timeout="${2:-300}" waited=0 | ||
| until (exec 3<>"/dev/tcp/127.0.0.1/$port") 2>/dev/null; do | ||
| sleep 5; waited=$((waited + 5)) | ||
| if [ "$waited" -ge "$timeout" ]; then echo "[launch_arm] TIMEOUT waiting for grpc :$port" >&2; return 1; fi | ||
| done | ||
| exec 3>&- 2>/dev/null || true | ||
| } |
There was a problem hiding this comment.
The current implementation of wait_grpc uses bash's /dev/tcp redirection, which is not supported by all shells or bash compilations (e.g., some Debian/Ubuntu environments disable it). Additionally, opening the file descriptor inside a subshell (...) means it is immediately closed, making the subsequent exec 3>&- cleanup in the parent shell redundant and incorrect.
Using the already-defined VLLM_PYTHON executable to perform a quick socket connection check is highly portable, robust, and avoids any shell-specific limitations or file descriptor management.
| wait_grpc() { # crude TCP-listen check for the gRPC port | |
| local port="$1" timeout="${2:-300}" waited=0 | |
| until (exec 3<>"/dev/tcp/127.0.0.1/$port") 2>/dev/null; do | |
| sleep 5; waited=$((waited + 5)) | |
| if [ "$waited" -ge "$timeout" ]; then echo "[launch_arm] TIMEOUT waiting for grpc :$port" >&2; return 1; fi | |
| done | |
| exec 3>&- 2>/dev/null || true | |
| } | |
| wait_grpc() { # crude TCP-listen check for the gRPC port | |
| local port="$1" timeout="${2:-300}" waited=0 | |
| until "$VLLM_PYTHON" -c "import socket; s = socket.socket(); s.settimeout(1); s.connect(('127.0.0.1', $port))" 2>/dev/null; do | |
| sleep 5; waited=$((waited + 5)) | |
| if [ "$waited" -ge "$timeout" ]; then echo "[launch_arm] TIMEOUT waiting for grpc :$port" >&2; return 1; fi | |
| done | |
| } |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| def _find_category_accuracy(score_root: Path, category: str) -> float | None: | ||
| # BFCL nests scores as <model>/<section>/BFCL_v4_<category>_score.json, so | ||
| # match a trailing-wildcard pattern (the BFCL_v4_ prefix varies by version). |
There was a problem hiding this comment.
🔴 Important: The glob pattern *{category}_score.json is ambiguous — for category="multiple", the rglob("*multiple_score.json") pattern matches both …_multiple_score.json and …_parallel_multiple_score.json (the * prefix matches any characters). Since rglob order is non-deterministic, this can return the wrong category's accuracy.
This affects the multiple category in the default category set.
A simple fix is to require a non-alphanumeric character (or start-of-name) before the category by matching on _ prefix:
| # match a trailing-wildcard pattern (the BFCL_v4_ prefix varies by version). | |
| for path in score_root.rglob(f"*_{category}_score.json"): |
This works because BFCL names all score files BFCL_v4_{category}_score.json (or similar versioned prefixes), so the underscore before the category name is always present.
|
|
||
| def _run(cmd: list[str], env: dict[str, str], label: str) -> None: | ||
| print(f"\n=== {label}: {' '.join(cmd)}", flush=True) | ||
| proc = subprocess.run(cmd, env=env, check=False) | ||
| if proc.returncode != 0: |
There was a problem hiding this comment.
🟡 Nit: _run() swallows non-zero exit codes — if bfcl generate fails for both arms (e.g. unknown model, missing handler, server down), parse_scores returns empty dicts, overall["delta"] is None, the regression check is skipped, and main() returns 0 (success). A completely broken run silently reports success with all "—" scores.
Consider at least tracking whether any _run call failed and returning non-zero from main() if all scores are missing:
if not baseline.scores and not candidate.scores:
print("ERROR: no scores produced for either arm", file=sys.stderr)
return 1
| --enable-auto-tool-choice --tool-call-parser "$VLLM_TOOL_PARSER" | ||
| --host 0.0.0.0 --port "$ARM_A_PORT" | ||
| --tensor-parallel-size "$TP" --max-model-len "$MAX_MODEL_LEN" | ||
| --gpu-memory-utilization "$GPU_MEM_UTIL" |
There was a problem hiding this comment.
🟡 Nit: Processes are started with setsid (new session), but stop kills only the leader PID. vLLM with --tensor-parallel-size 2 spawns worker child processes that won't receive the signal, leaving them holding GPU memory. In CI this can cause the next run to OOM.
Consider killing the process group instead:
| --gpu-memory-utilization "$GPU_MEM_UTIL" | |
| kill -TERM -- -"$pid" 2>/dev/null && echo "[launch_arm] killed $(basename "$pf" .pid) (pgid $pid)" >&2 || true |
(kill -- -$pid sends the signal to the entire process group whose PGID equals $pid, which is the case for session leaders created by setsid.)
| run: cat "$RUNNER_TEMP/bfcl_ab.md" >> "$GITHUB_STEP_SUMMARY" || echo "no report produced" >> "$GITHUB_STEP_SUMMARY" | ||
| - name: Upload comparison | ||
| if: always() | ||
| uses: actions/upload-artifact@v7 |
There was a problem hiding this comment.
🟡 Nit: BFCL_AB_REGRESSION is written to $GITHUB_ENV but no subsequent step reads it — the regression gate is effectively a no-op (the workflow always succeeds regardless of delta). If this is intentional (informational-only), the env-var write is dead code and can be removed. If you want it to fail the job, add a final step:
- name: Check regression
if: always()
run: |
if [ "${BFCL_AB_REGRESSION:-}" = "1" ]; then
echo "::error::BFCL regression detected"; exit 1
fi![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: db053c183c
ℹ️ 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".
| def _find_category_accuracy(score_root: Path, category: str) -> float | None: | ||
| # BFCL nests scores as <model>/<section>/BFCL_v4_<category>_score.json, so | ||
| # match a trailing-wildcard pattern (the BFCL_v4_ prefix varies by version). | ||
| for path in score_root.rglob(f"*{category}_score.json"): |
There was a problem hiding this comment.
When the default category set includes both multiple and parallel_multiple, this glob for category == "multiple" also matches BFCL_v4_parallel_multiple_score.json because both paths end in multiple_score.json. If rglob yields that path first, or the real multiple score is missing, the report and regression gate use the parallel_multiple accuracy as the multiple score, corrupting the A/B deltas. Match the filename/suffix for the requested category exactly instead of using a broad leading wildcard.
Useful? React with 👍 / 👎.
| proc = subprocess.run(cmd, env=env, check=False) | ||
| if proc.returncode != 0: | ||
| print(f"WARNING: {label} exited {proc.returncode}", file=sys.stderr) |
There was a problem hiding this comment.
In any run where bfcl generate or bfcl evaluate exits non-zero, such as an unknown -FC handler, server crash, or evaluation error, _run only prints a warning and returns. run_ab.py then parses whatever files happen to exist and can exit 0 with empty scores because overall['delta'] is None, making the nightly look successful without a valid benchmark. Propagate the non-zero exit, or fail when expected score files are missing.
Useful? React with 👍 / 👎.
| - name: Register model in bfcl (no-op if bfcl already ships a handler) | ||
| run: python scripts/bfcl/register_bfcl_model.py --model-id "$MODEL" || true | ||
| - name: Stage model weights | ||
| run: bash scripts/ci_download_model.sh --gpu-tier 2 |
There was a problem hiding this comment.
The workflow default MODEL is Qwen/Qwen3.6-27B, but this step stages --gpu-tier 2, which resolves only entries in e2e_test/infra/model_specs.py; repo-wide search shows that model is only in the new BFCL files, not in MODEL_SPECS. On default or manual runs for an unlisted model, the requested weights are not prefetched and server startup must download them during the 420s launch window, so a cold runner can time out before any benchmark runs. Pass $MODEL to the downloader or add it to the resolved set.
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: 7
🤖 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:
- Line 112: Remove the `|| true` fallback from the end of the python command
that executes `scripts/bfcl/register_bfcl_model.py` with the `--model-id
"$MODEL"` parameter. Since this script is already idempotent for "already
registered" scenarios, the `|| true` only masks genuine setup errors that should
cause the workflow to fail. Removing it will allow real registration failures to
be properly surfaced instead of silently suppressed.
- Line 59: Replace all floating version tags for third-party GitHub Actions with
immutable commit SHAs to reduce supply-chain risk. For actions/checkout,
actions/cache, actions/upload-artifact, and actions/download-artifact throughout
the workflow, change the uses statements from floating tags (like `@v6`, `@v5`, `@v7`,
`@v8`) to their corresponding full commit SHAs. Look up the current commit SHA for
each action version and replace the floating reference with the complete SHA
format to ensure reproducibility and security.
In `@scripts/bfcl/launch_arm.sh`:
- Line 121: In the wait_http call on line 121 of the launch_arm.sh script,
replace the /health endpoint with /readiness. The /health endpoint only
indicates liveness and can return a successful status before tokenizer
registration is complete, causing subsequent BFCL requests to fail. The
/readiness endpoint properly waits until the gateway is fully initialized,
ensuring that Arm B startup is complete before proceeding.
- Around line 59-61: The start function launches processes using setsid which
creates a new session and process group, but the stop function kills only the
session leader PID without terminating descendant processes. Modify the kill
command in the stop function (around line 129) to terminate the entire process
group instead of just the individual PID. Use a process group kill approach such
as prepending a minus sign to the PID variable in the kill command (kill -TERM
-- -$pid) or using pkill with the parent PID flag to ensure all child processes
spawned under that session are properly terminated along with the session
leader, preventing orphaned processes from holding resources across runs.
In `@scripts/bfcl/run_ab.py`:
- Around line 173-179: The issue is that b_vals and c_vals are computed
independently by filtering rows separately, which means they may contain values
from different sets of categories if some categories are missing in one arm.
This makes the overall_delta calculation invalid. Fix this by filtering rows to
only include those where both baseline and candidate are not None before
extracting the values into b_vals and c_vals, ensuring that both lists contain
values from the same matched categories only.
- Around line 130-146: The parse_scores function receives a model parameter but
does not use it when searching for score files, which allows
_find_category_accuracy to potentially find score files from other models.
Modify parse_scores to scope the score search to the requested model's
directory. Specifically, after obtaining score_root, use the model parameter to
locate the sanitized model directory within score_root (using glob or similar
matching since sanitization differs across versions), then pass this
model-scoped directory to _find_category_accuracy instead of passing score_root
directly. This ensures each category score is found only within the correct
model's subdirectory.
- Around line 123-127: The _run function currently allows the script to continue
even when commands fail, which can lead to incomplete A/B reports. Change the
subprocess.run call parameter from check=False to check=True so that the
function raises CalledProcessError when a command exits with a non-zero return
code, causing the script to fail fast. This eliminates the need for the manual
return code check and warning log since subprocess will now automatically
propagate the error up the call stack.
🪄 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: 7a3d8a62-5ed7-4ddf-8cdb-9bb922fc5e4d
📒 Files selected for processing (5)
.github/workflows/nightly-bfcl.ymlscripts/bfcl/README.mdscripts/bfcl/launch_arm.shscripts/bfcl/register_bfcl_model.pyscripts/bfcl/run_ab.py
| permissions: | ||
| contents: read | ||
| steps: | ||
| - uses: actions/checkout@v6 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify unpinned uses entries in this workflow.
rg -n '^\s*-\s*uses:\s*[^@]+@v[0-9]+' .github/workflows/nightly-bfcl.ymlRepository: lightseekorg/smg
Length of output: 181
🏁 Script executed:
cat -n .github/workflows/nightly-bfcl.yml | sed -n '50,110p'Repository: lightseekorg/smg
Length of output: 2848
🏁 Script executed:
cat -n .github/workflows/nightly-bfcl.yml | sed -n '140,160p'Repository: lightseekorg/smg
Length of output: 988
🏁 Script executed:
rg -n '^\s*uses:' .github/workflows/nightly-bfcl.ymlRepository: lightseekorg/smg
Length of output: 276
Pin third-party actions to immutable commit SHAs.
The following uses: entries use floating version tags instead of commit SHAs, creating a supply-chain risk:
- Line 59:
actions/checkout@v6 - Line 62:
actions/cache@v5 - Line 79:
actions/upload-artifact@v7 - Line 99:
actions/checkout@v6 - Line 100:
actions/download-artifact@v8 - Line 149:
actions/upload-artifact@v7
Replace floating tags with pinned commit SHAs for reproducibility and security.
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 59-59: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 59-59: 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 59, Replace all floating version
tags for third-party GitHub Actions with immutable commit SHAs to reduce
supply-chain risk. For actions/checkout, actions/cache, actions/upload-artifact,
and actions/download-artifact throughout the workflow, change the uses
statements from floating tags (like `@v6`, `@v5`, `@v7`, `@v8`) to their corresponding
full commit SHAs. Look up the current commit SHA for each action version and
replace the floating reference with the complete SHA format to ensure
reproducibility and security.
Source: Linters/SAST tools
| pip install wheel/*.whl | ||
| pip install "bfcl-eval" soundfile ninja | ||
| - name: Register model in bfcl (no-op if bfcl already ships a handler) | ||
| run: python scripts/bfcl/register_bfcl_model.py --model-id "$MODEL" || true |
There was a problem hiding this comment.
Remove || true from BFCL model registration.
Line 112 masks real registration failures. The script is already idempotent for “already registered”, so this fallback mainly hides actionable setup errors.
🤖 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 112, Remove the `|| true`
fallback from the end of the python command that executes
`scripts/bfcl/register_bfcl_model.py` with the `--model-id "$MODEL"` parameter.
Since this script is already idempotent for "already registered" scenarios, the
`|| true` only masks genuine setup errors that should cause the workflow to
fail. Removing it will allow real registration failures to be properly surfaced
instead of silently suppressed.
| setsid env "$@" >"$log" 2>&1 </dev/null & | ||
| echo $! >"$RUN_DIR/$name.pid" | ||
| echo "[launch_arm] started $name (pid $(cat "$RUN_DIR/$name.pid")) -> $log" >&2 |
There was a problem hiding this comment.
Teardown should terminate the full process group, not only the session leader PID.
start launches via setsid (Line 59), but stop uses kill "$pid" (Line 129). That may leave descendant processes alive and still holding GPU/ports across runs.
Suggested fix
- kill "$pid" 2>/dev/null && echo "[launch_arm] killed $(basename "$pf" .pid) (pid $pid)" >&2 || true
+ kill -- "-$pid" 2>/dev/null && echo "[launch_arm] killed $(basename "$pf" .pid) (pgid $pid)" >&2 || trueAlso applies to: 125-131
🤖 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 59 - 61, The start function launches
processes using setsid which creates a new session and process group, but the
stop function kills only the session leader PID without terminating descendant
processes. Modify the kill command in the stop function (around line 129) to
terminate the entire process group instead of just the individual PID. Use a
process group kill approach such as prepending a minus sign to the PID variable
in the kill command (kill -TERM -- -$pid) or using pkill with the parent PID
flag to ensure all child processes spawned under that session are properly
terminated along with the session leader, preventing orphaned processes from
holding resources across runs.
| ) | ||
| [ -n "$SMG_REASONING_PARSER" ] && smg_cmd+=(--reasoning-parser "$SMG_REASONING_PARSER") | ||
| start arm_b_gateway "$RUN_DIR/arm_b_gateway.log" "${smg_cmd[@]}" | ||
| wait_http "http://127.0.0.1:$ARM_B_GW_PORT/health" "${BFCL_STARTUP_TIMEOUT:-420}" |
There was a problem hiding this comment.
Use gateway /readiness instead of /health for Arm B startup.
Line 121 waits on /health, but that endpoint is liveness-only. It can return 200 before tokenizer registration completes, which makes early BFCL requests fail. Arm B should wait on /readiness.
🤖 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` at line 121, In the wait_http call on line 121 of
the launch_arm.sh script, replace the /health endpoint with /readiness. The
/health endpoint only indicates liveness and can return a successful status
before tokenizer registration is complete, causing subsequent BFCL requests to
fail. The /readiness endpoint properly waits until the gateway is fully
initialized, ensuring that Arm B startup is complete before proceeding.
| def _run(cmd: list[str], env: dict[str, str], label: str) -> None: | ||
| print(f"\n=== {label}: {' '.join(cmd)}", flush=True) | ||
| proc = subprocess.run(cmd, env=env, check=False) | ||
| if proc.returncode != 0: | ||
| print(f"WARNING: {label} exited {proc.returncode}", file=sys.stderr) |
There was a problem hiding this comment.
Fail fast when bfcl commands exit non-zero.
Line 125 runs with check=False, and Line 127 only logs a warning. That lets the script continue and potentially emit a “successful” A/B report from partial or stale data.
Suggested fix
def _run(cmd: list[str], env: dict[str, str], label: str) -> None:
print(f"\n=== {label}: {' '.join(cmd)}", flush=True)
- proc = subprocess.run(cmd, env=env, check=False)
- if proc.returncode != 0:
- print(f"WARNING: {label} exited {proc.returncode}", file=sys.stderr)
+ subprocess.run(cmd, env=env, check=True)🧰 Tools
🪛 ast-grep (0.43.0)
[error] 124-124: Command coming from incoming request
Context: subprocess.run(cmd, env=env, check=False)
Note: [CWE-20].
(subprocess-from-request)
[error] 124-124: Use of unsanitized data to create processes
Context: subprocess.run(cmd, env=env, check=False)
Note: [CWE-78].
(os-system-unsanitized-data)
🤖 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/run_ab.py` around lines 123 - 127, The _run function currently
allows the script to continue even when commands fail, which can lead to
incomplete A/B reports. Change the subprocess.run call parameter from
check=False to check=True so that the function raises CalledProcessError when a
command exits with a non-zero return code, causing the script to fail fast. This
eliminates the need for the manual return code check and warning log since
subprocess will now automatically propagate the error up the call stack.
| def parse_scores(project_root: Path, model: str, categories: list[str]) -> dict[str, float]: | ||
| """Extract per-category accuracy from BFCL's score output. | ||
|
|
||
| BFCL writes ``<root>/score/<sanitized-model>/<category>_score.json`` whose | ||
| FIRST line is a summary dict containing ``accuracy``. We glob for the model | ||
| dir (sanitization differs across versions) and read each category's summary. | ||
| """ | ||
| score_root = project_root / "score" | ||
| out: dict[str, float] = {} | ||
| if not score_root.is_dir(): | ||
| print(f"WARNING: no score dir at {score_root}", file=sys.stderr) | ||
| return out | ||
| for cat in categories: | ||
| acc = _find_category_accuracy(score_root, cat) | ||
| if acc is not None: | ||
| out[cat] = acc | ||
| return out |
There was a problem hiding this comment.
Scope score discovery to the requested model.
parse_scores receives model but ignores it. The current rglob("*{category}_score.json") can read a score file from another model under the same score/ tree, corrupting A/B comparisons.
Also applies to: 149-152
🤖 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/run_ab.py` around lines 130 - 146, The parse_scores function
receives a model parameter but does not use it when searching for score files,
which allows _find_category_accuracy to potentially find score files from other
models. Modify parse_scores to scope the score search to the requested model's
directory. Specifically, after obtaining score_root, use the model parameter to
locate the sanitized model directory within score_root (using glob or similar
matching since sanitization differs across versions), then pass this
model-scoped directory to _find_category_accuracy instead of passing score_root
directly. This ensures each category score is found only within the correct
model's subdirectory.
| b_vals = [r["baseline"] for r in rows if r["baseline"] is not None] | ||
| c_vals = [r["candidate"] for r in rows if r["candidate"] is not None] | ||
| b_overall = sum(b_vals) / len(b_vals) if b_vals else None | ||
| c_overall = sum(c_vals) / len(c_vals) if c_vals else None | ||
| overall_delta = ( | ||
| (c_overall - b_overall) if (b_overall is not None and c_overall is not None) else None | ||
| ) |
There was a problem hiding this comment.
Compute overall delta from matched categories only.
Line 173–179 averages baseline and candidate independently. If one arm is missing categories, you compare different sets and the overall delta/regression gate becomes misleading. Use only rows where both sides are present (or fail the run on missing categories).
🤖 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/run_ab.py` around lines 173 - 179, The issue is that b_vals and
c_vals are computed independently by filtering rows separately, which means they
may contain values from different sets of categories if some categories are
missing in one arm. This makes the overall_delta calculation invalid. Fix this
by filtering rows to only include those where both baseline and candidate are
not None before extracting the values into b_vals and c_vals, ensuring that both
lists contain values from the same matched categories only.
…ng hook) 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: 469e19c62e
ℹ️ 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".
| import argparse | ||
| import importlib.util | ||
| import sys | ||
| from pathlib import Path |
There was a problem hiding this comment.
When the default nightly runs for a model BFCL does not yet ship, this helper is the only registration path, but the hard-coded Qwen/Qwen3-32B-FC anchor is brittle: current upstream bfcl_eval/constants/model_config.py no longer has this exact entry in the location this script expects, so args.anchor not in src aborts and the workflow then continues with || true, leaving Qwen/Qwen3.6-27B-FC unregistered. In that environment bfcl generate --model "$BFCL_MODEL_FC" fails as an unknown model instead of producing the A/B benchmark; anchor on a discovered QwenFCHandler entry or append to the relevant map rather than requiring this exact line.
Useful? React with 👍 / 👎.
Adds a path-filtered pull_request trigger to nightly-bfcl.yml so changes to the BFCL A/B pipeline exercise it end-to-end (engine + FC mode + parser + scoring) before merge — confidence the pipeline works, not a statistical run. - PR event -> quick non-live subset (simple_python,irrelevance); schedule/ dispatch -> full non-live set. PR runs cancel-in-progress. - paths filter limits it to scripts/bfcl/** + this workflow, so unrelated PRs are unaffected. Doubles as the live-CI-runner shakeout for the workflow. Signed-off-by: key4ng <rukeyang@gmail.com>
| pull_request: | ||
| paths: | ||
| - "scripts/bfcl/**" | ||
| - ".github/workflows/nightly-bfcl.yml" |
There was a problem hiding this comment.
🟡 Nit: This adds a PR trigger for a 4-GPU H100 job (up to 120 min), but cancel-pr-workflows.yml (added in this PR) doesn't include a matching entry. If a PR is closed while the BFCL job is running, it won't be cancelled — the expensive GPU job will run to completion.
The comment in cancel-pr-workflows.yml says "Keep these in sync with the concurrency groups used by PR workflows." Add:
- workflow: Nightly BFCL
group: nightly-bfcl-pull_request-refs/pull/{0}/merge
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a123ac325
ℹ️ 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".
| p.add_argument("--model-id", required=True, help="HF id, e.g. Qwen/Qwen3.6-27B") | ||
| p.add_argument( | ||
| "--handler", | ||
| default="QwenFCHandler", |
There was a problem hiding this comment.
When the default new Qwen model needs auto-registration, this default wires it to bfcl-eval's local QwenFCHandler; checked current bfcl_eval.model_handler.local_inference.qwen_fc.QwenFCHandler, and despite the name it formats tools into the Qwen XML prompt and decodes <tool_call> text from completions instead of sending native tools and reading message.tool_calls. That bypasses the vLLM/SMG tool-call parsers that this nightly is supposed to compare, so a successful report can show parity while neither parser was exercised; register with a native OpenAI-compatible FC handler, or verify the selected handler uses chat tools, for this A/B.
Useful? React with 👍 / 👎.
…ghtly ci_download_model.sh stages models from MODEL_SPECS by tp<=tier, so the nightly BFCL A/B (and its PR sanity run) can pre-stage Qwen3.6-27B instead of relying on a slow first-launch HF download. 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: f464d3720e
ℹ️ 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".
| "Qwen/Qwen3.6-27B": { | ||
| "model": _resolve_model_path("Qwen/Qwen3.6-27B"), | ||
| "tp": 2, |
There was a problem hiding this comment.
Adding this entry to MODEL_SPECS makes it part of the global model downloader: scripts/ci_download_model.sh prints every spec with tp <= tier, and .github/workflows/e2e-gpu-job.yml runs that resolver for reusable PR jobs such as pr-test-rust.yml's e2e-2gpu-pd. On any 2+-GPU PR job, setup now pre-downloads/verifies Qwen/Qwen3.6-27B even though those tests do not use the BFCL nightly model, which can consume the 30-minute setup budget and shared cache; prefer passing the BFCL model explicitly in nightly-bfcl or excluding it from generic tier resolution.
Useful? React with 👍 / 👎.
| --project-root "$RUNNER_TEMP/bfcl_ab" \ | ||
| --out "$RUNNER_TEMP/bfcl_ab.md" \ | ||
| --json-out "$RUNNER_TEMP/bfcl_ab.json" \ | ||
| --tolerance 0.02 || echo "BFCL_AB_REGRESSION=1" >> "$GITHUB_ENV" |
There was a problem hiding this comment.
This || echo turns any non-zero run_ab.py result, including the tolerance regression path, into a successful step, but repo-wide search shows BFCL_AB_REGRESSION is never read later in nightly-bfcl.yml or elsewhere. In the scheduled workflow I checked, a real candidate-below-baseline regression will therefore leave the run green unless someone opens the artifact manually, defeating the regression gate; add a follow-up failure/notice/summary step or let the command fail for scheduled runs.
Useful? React with 👍 / 👎.
2 participants
Description
Problem
We have no credible, reproducible way to show that SMG's parsing frontend (chat template + tokenization + tool/reasoning parsing) is as good as an inference engine's own parsers — the data we'd need to argue for engines adopting SMG's frontend as a shared layer. A raw benchmark score also conflates model quality with parser quality.
Solution
A nightly A/B built on the official Berkeley Function Calling Leaderboard (
bfcl-eval, FC mode). Two arms expose an identical OpenAI/v1endpoint and differ only in the frontend:--tool-call-parser/--reasoning-parser)Model, engine, checkpoint and sampling are held fixed, so any score Δ is attributable to the tokenization+parsing layer. FC mode is required so the server's parsed
tool_callsare what gets scored (prompt mode would bypass the parser). This is Track B of the parser-verification effort; the per-PR, CPU-only Track-A parser-conformance gate is separate.Changes
scripts/bfcl/launch_arm.sh— bring up arm A (pure vLLM) or arm B (vLLM gRPC + SMG); env-parameterised (model,BFCL_TP, GPUs, ports, parsers);stoptears down via pidfiles.scripts/bfcl/run_ab.py— point officialbfcl generate+evaluate(FC mode,LOCAL_SERVER_ENDPOINT/--skip-server-setup) at both arms, parse per-category accuracy, emit a markdown + JSON comparison table, and a regression gate on the overall delta.scripts/bfcl/register_bfcl_model.py— register a model bfcl-eval doesn't ship a handler for yet (e.g. brand-new SKUs likeQwen/Qwen3.6-27B) by cloning an existing FC entry. Idempotent..github/workflows/nightly-bfcl.yml— cron nightly (build-wheel + GPU job, TP=2 per arm), runs the non-live category set, uploads the comparison artifact + step summary.scripts/bfcl/README.md— methodology, per-model parser flags, gotchas, and the validated result.Test Plan
Ran end-to-end on a dev H100 box:
Qwen/Qwen3.6-27Bat TP=2 (one arm per GPU pair), full BFCLnon_liveset (7 AST categories, 1390 cases), FC mode, temp 0.001 — clean config (no--enforce-eager, online HF, afterpip install ninja):qwen3_xml)qwen_xml)SMG's Rust frontend is at parity with vLLM's native parser — marginally ahead overall, never worse than −0.25pp on any category. Both arms emit native
tool_callsand parse<think>intoreasoning_content(FC + reasoning confirmed end to end). The low java/js scores are the model's non-Python function-calling ability (identical on both arms), which confirms the A/B isolates the frontend, not model quality.Tooling checks:
ruff check+ruff format --checkpass on the Python;bash -non the shell scripts. The workflow itself still needs one live CI-runner shakeout (noted in the file header).Checklist
cargo +nightly fmtpasses (n/a — no Rust changes)cargo clippy --all-targets --all-features -- -D warningspasses (n/a — no Rust changes)🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Tests
Documentation
Chores