Repository navigation
feat(e2e): Add BFCL v3 function calling test infrastructure - #523
vschandramourya wants to merge 24 commits into
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Summary of ChangesHello @vschandramourya, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request establishes a robust, automated testing framework for assessing the accuracy of function calling within the SMG gateway. By integrating the Berkeley Function Calling Leaderboard (BFCL) v3, it provides a standardized method for validating tool call behavior across various complexities. The new infrastructure handles data acquisition, format conversion, test execution, and detailed result logging, significantly enhancing the ability to perform both rapid PR-level validation and extensive nightly runs for function calling capabilities. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
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 a BFCL end-to-end test suite: downloader, data loader + BFCL→OpenAI tool converter, evaluator with per-test logging and run summaries, session-scoped result aggregation, and pytest integration that writes a session summary. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as "pytest test_case"
participant Runner as "_run_bfcl_case"
participant Client as "OpenAI Client"
participant Gateway as "Gateway/Backend"
participant Evaluator as "evaluate_tool_calls"
participant Logger as "save_test_log / session_state"
Test->>Runner: start case
Runner->>Client: send chat completion (messages + tools)
Client->>Gateway: forward request
Gateway-->>Client: completion with tool_call(s)
Runner->>Runner: extract tool_call(s)
Runner->>Evaluator: evaluate actual vs ground truth
Evaluator-->>Runner: pass/fail + errors
Runner->>Logger: persist per-test log
Runner->>Logger: append_result for session
Note right of Logger: session summary written at end
sequenceDiagram
participant Script as "download_data.py"
participant FS as "e2e_test/bfcl/data/*.json"
participant Loader as "load_bfcl_category"
participant Converter as "bfcl_to_openai_tools"
Script->>FS: ensure data dir exists
Script->>FS: download HF files -> data/*.json
Loader->>FS: read questions and answer files
FS-->>Loader: return content
Loader->>Loader: merge questions + answers, normalize types
Loader-->>Converter: provide BFCL functions
Converter-->>Script: return OpenAI-compatible tools
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
vschandramourya
changed the title
![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
The pull request introduces comprehensive test infrastructure for BFCL v3 function calling, including data loading, evaluation, and logging. The changes are well-structured and provide good coverage for different function calling scenarios. The use of pytest fixtures for setup and teardown, along with detailed logging, will greatly aid in debugging and tracking performance over time. The download_data.py script is a good addition for managing test data. The evaluator.py and loader.py modules are well-designed for their respective tasks. The test_bfcl.py integrates these components effectively. Overall, this is a solid addition to the testing suite.
| failures.append({ | ||
| "test_id": r.get("test_id", "?"), | ||
| "category": cat, | ||
| "errors": r.get("errors", []), | ||
| "latency_ms": round(lat, 1), | ||
| "finish_reason": r.get("finish_reason"), | ||
| "completion_tokens": r.get("completion_tokens"), | ||
| "had_reasoning": r.get("had_reasoning", False), | ||
| "log_file": r.get("log_file", ""), | ||
| }) |
There was a problem hiding this comment.
The failures list appends a dictionary with several keys. It would be more robust and readable to define a dataclass or a TypedDict for FailureEntry to ensure consistency and type safety, especially as the structure grows.
| if float(actual) == float(expected): | ||
| return True | ||
| except (TypeError, ValueError): |
There was a problem hiding this comment.
The float(actual) == float(expected) comparison can lead to precision issues with floating-point numbers. It's generally recommended to compare floats within a small epsilon to account for potential inaccuracies. For example, abs(float(actual) - float(expected)) < epsilon.
![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 the current code and only fix it if needed.
Inline comments:
In `@e2e_test/bfcl/download_data.py`:
- Around line 39-56: In download(), replace urllib.request.urlretrieve() with
urllib.request.urlopen(url, timeout=timeout_s) and stream the response into a
temporary file (e.g., dest.with_suffix('.tmp')) using a with-open context to
ensure the handle is closed, then atomically move the temp into place with
dest.replace() to avoid partial files; do this for both the FILES loop and
ANSWER_FILES loop, and replace the line-counting expression sum(1 for _ in
open(dest)) with a with dest.open('r') as f: sum(1 for _ in f) (or equivalent)
so file handles are properly closed and downloads time out on stalls.
In `@e2e_test/bfcl/evaluator.py`:
- Around line 64-101: The current index-based comparison between
actual_tool_calls and ground_truth causes false failures for parallel calls;
change to order-insensitive (greedy) matching when ground_truth represents
parallel/parallel_multiple cases by iterating expected entries and for each
expected entry (from ground_truth) searching any unmatched actual call in
actual_tool_calls with the same name and args using the existing _values_match
predicate, mark matches as consumed and only report errors for expected entries
that could not be matched (and optionally for leftover unmatched actual calls),
replacing the index-based loop that compares actual["name"] to expected_name and
the positional arg-checking logic with this greedy matching approach while still
using expected_args and _values_match for argument validation.
In `@e2e_test/bfcl/loader.py`:
- Around line 74-85: The loader currently silently returns empty ground truth
when an expected answer file is missing; update the block that looks up
answer_filename from _ANSWER_FILE_MAP in e2e_test/bfcl/loader.py so that if
answer_filename is truthy but (DATA_DIR / answer_filename).exists() is false you
raise a FileNotFoundError (including the missing filename and category in the
message) instead of continuing; keep the existing behavior of reading lines into
answers_by_id when the file exists and do not change the data structure names
(answer_filename, answer_path, answers_by_id, DATA_DIR) so callers can detect
and skip tests cleanly.
In `@e2e_test/chat_completions/test_bfcl.py`:
- Around line 218-245: The failure branch that logs API errors builds a
_all_results entry with "log_file" as f"{category}/{test_id.replace('/',
'_')}_FAIL.json" but the evaluation-failure branch (the other
save_test_log/_all_results block) uses only the filename; make both branches
consistent by setting "log_file" in the evaluation-failure branch to the
category-aware relative path (same pattern used in the API-error branch) so both
save_test_log calls and their corresponding _all_results entries store the path
relative to run_dir; update the evaluation-failure block (the other _all_results
creation around the evaluation failure handling) to use
f"{category}/{test_id.replace('/', '_')}_FAIL.json" as well.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
e2e_test/bfcl/__init__.pye2e_test/bfcl/data/.gitignoree2e_test/bfcl/download_data.pye2e_test/bfcl/evaluator.pye2e_test/bfcl/loader.pye2e_test/chat_completions/test_bfcl.py
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
e2e_test/chat_completions/test_bfcl.py (1)
283-294:log_filepath is inconsistent between error and success branches.Line 242 (API error):
f"{category}/{test_id.replace('/', '_')}_FAIL.json"— includes category prefix.
Line 292 (eval result):log_path.name— just the filename, no category prefix.This inconsistency makes it harder to locate log files from the summary.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/chat_completions/test_bfcl.py` around lines 283 - 294, The summary entries created in the _all_results dict use inconsistent log_file values between the API error branch (which sets log file to f"{category}/{test_id.replace('/', '_')}_FAIL.json") and the success/eval branch (which uses log_path.name only); update the success branch where the dict is built (the "log_file" key near the block that sets "test_id", "category", "passed", etc.) to use the same category-prefixed path format (e.g., construct and assign a category + filename like the error branch does from log_path or test_id) so both branches store a consistent, category-prefixed log file path.e2e_test/bfcl/download_data.py (2)
39-46:urllib.request.urlretrieve()still lacks timeout and atomic write guarantees.This was flagged in a prior review.
urlretrievecan hang indefinitely on network stalls and writes directly to the destination, leaving partial/corrupt files on interruption. Consider switching tourllib.request.urlopen(..., timeout=...)with a temp-file + rename pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/bfcl/download_data.py` around lines 39 - 46, The _download_one function currently uses urllib.request.urlretrieve without a timeout and writes directly to the final path; change it to open the URL with urllib.request.urlopen(url, timeout=...) to enforce a timeout, stream the response into a temporary file (e.g., dest.with_suffix(".tmp") or use tempfile.NamedTemporaryFile in the same directory), flush and fsync the temp file, then atomically rename/move the temp file to dest to avoid partial/corrupt outputs; preserve the existing line-count logic by opening the final dest with encoding="utf-8" after the atomic move.
49-58: 🧹 Nitpick | 🔵 TrivialConsider skipping already-downloaded files for faster re-runs.
download()unconditionally re-downloads all files. For iterative development, checking if the file already exists (and optionally verifying a checksum) would save time and bandwidth.♻️ Example: skip existing files
def _download_one(remote_path: str, local_name: str) -> None: url = f"{HF_BASE}/{remote_path}" dest = DATA_DIR / local_name + if dest.exists(): + print(f" ✓ {local_name} already present, skipping.") + return print(f"Downloading {local_name}...") urllib.request.urlretrieve(url, dest)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/bfcl/download_data.py` around lines 49 - 58, The download() function always re-downloads everything; update it to skip files that already exist in DATA_DIR to speed re-runs by checking destination paths before calling _download_one (use DATA_DIR, FILES, ANSWER_FILES and the _download_one helper); implement a simple existence check for each target file and only call _download_one when the file is missing (optionally add a --force or force_redownload flag to download() to override the check), and consider adding an optional checksum verification step after download if you want integrity checks.e2e_test/bfcl/loader.py (1)
74-84: Answer file silently ignored when missing — still unfixed from prior review.When
answer_filenameis truthy (i.e., the category has expected answers) but the file doesn't exist on disk, the loader silently returns emptyground_truthfor every case. This causes misleading test results: the evaluator will fail any test where the model correctly produces tool calls with "No ground truth available but model produced tool calls."Raise
FileNotFoundErrorto surface the real problem (missing download) instead of producing confusing failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/bfcl/loader.py` around lines 74 - 84, When an expected answer file is configured (answer_filename from _ANSWER_FILE_MAP) but missing, do not silently continue; check (DATA_DIR / answer_filename).exists() and if it does not, raise a FileNotFoundError with a clear message including the category and expected filename so the missing-download error surfaces. Keep the existing logic that opens and parses the file unchanged when the path exists and continue building answers_by_id as before.
🤖 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/chat_completions/test_bfcl.py`:
- Line 115: The module-level list _all_results is mutated from _run_bfcl_case
and can be accessed concurrently under pytest-parallel; make this explicit by
protecting mutations with a threading.Lock (e.g., create a module-level lock
like _all_results_lock) or replace _all_results with a thread-safe collection
(e.g., queue.Queue or collections.deque with a lock). Update every append/remove
access in _run_bfcl_case (and any other places that touch _all_results) to
acquire the lock (or use the thread-safe API) so concurrent tests cannot corrupt
the list.
- Around line 193-213: The test duplicates request parameters: a request_payload
dict is built but then the same args are passed again to
client.chat.completions.create; change the call to use request_payload as the
single source of truth by passing that dict to client.chat.completions.create
(e.g., unpack or pass as kwargs) so the logged request_payload always matches
the actual request, leaving identifiers request_payload and
client.chat.completions.create as the touchpoints to update.
---
Duplicate comments:
In `@e2e_test/bfcl/download_data.py`:
- Around line 39-46: The _download_one function currently uses
urllib.request.urlretrieve without a timeout and writes directly to the final
path; change it to open the URL with urllib.request.urlopen(url, timeout=...) to
enforce a timeout, stream the response into a temporary file (e.g.,
dest.with_suffix(".tmp") or use tempfile.NamedTemporaryFile in the same
directory), flush and fsync the temp file, then atomically rename/move the temp
file to dest to avoid partial/corrupt outputs; preserve the existing line-count
logic by opening the final dest with encoding="utf-8" after the atomic move.
- Around line 49-58: The download() function always re-downloads everything;
update it to skip files that already exist in DATA_DIR to speed re-runs by
checking destination paths before calling _download_one (use DATA_DIR, FILES,
ANSWER_FILES and the _download_one helper); implement a simple existence check
for each target file and only call _download_one when the file is missing
(optionally add a --force or force_redownload flag to download() to override the
check), and consider adding an optional checksum verification step after
download if you want integrity checks.
In `@e2e_test/bfcl/loader.py`:
- Around line 74-84: When an expected answer file is configured (answer_filename
from _ANSWER_FILE_MAP) but missing, do not silently continue; check (DATA_DIR /
answer_filename).exists() and if it does not, raise a FileNotFoundError with a
clear message including the category and expected filename so the
missing-download error surfaces. Keep the existing logic that opens and parses
the file unchanged when the path exists and continue building answers_by_id as
before.
In `@e2e_test/chat_completions/test_bfcl.py`:
- Around line 283-294: The summary entries created in the _all_results dict use
inconsistent log_file values between the API error branch (which sets log file
to f"{category}/{test_id.replace('/', '_')}_FAIL.json") and the success/eval
branch (which uses log_path.name only); update the success branch where the dict
is built (the "log_file" key near the block that sets "test_id", "category",
"passed", etc.) to use the same category-prefixed path format (e.g., construct
and assign a category + filename like the error branch does from log_path or
test_id) so both branches store a consistent, category-prefixed log file path.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
e2e_test/bfcl/__init__.pye2e_test/bfcl/download_data.pye2e_test/bfcl/loader.pye2e_test/chat_completions/test_bfcl.py
slin1237
left a comment
slin1237
left a comment
There was a problem hiding this comment.
Hey, took a look at this against the existing e2e infra. The core BFCL logic (loader, evaluator, schema conversion) looks solid — nice work on the type coercion and the BFCL→OpenAI fixups. A few things need fixing before this can merge though:
Must fix
Thread safety — _all_results is a module-level mutable list that multiple threads will append to concurrently under --tests-per-worker N. The existing infra has extensive comments warning about exactly this pattern (see setup_backend.py:148-179). The session-scoped autouse fixture _write_summary_on_exit defined directly in the test file will also interact poorly with pytest-parallel's thread model — we route all session lifecycle through conftest.py → fixtures/hooks.py for a reason.
Module-level data loading — _all_cases loads and parses ~1,240 JSON entries from 5 files at import time during test collection. Every pytest invocation in e2e_test/ pays this cost, even pytest e2e_test/router/. The existing tests parametrize with small static lists, not data loaded from disk. Consider lazy loading or gating this behind a check.
Missing @pytest.mark.e2e — All existing GPU-dependent test classes use this marker. Without it, CI filtering like -m e2e won't pick these up, and hooks.py:pytest_collection_modifyitems won't properly detect worker requirements.
Phantom baselines.compare import — This module doesn't exist in the repo. The try/except makes it soft, but it's dead code adding complexity. If baselines are planned, bring them in a follow-up PR.
e2e_test/bfcl_logs/ not gitignored — There's a .gitignore for bfcl/data/*.json but none for the logs directory. Running tests locally will leave untracked files everywhere.
Should fix
_extract_tool_calls silently swallows malformed arguments — A tool call with garbage JSON gets recorded as {}, then you get a confusing "expected X, got None" error downstream instead of "malformed arguments". At least record the parse failure.
download_data.py uses urllib.request.urlretrieve with no timeout — can hang indefinitely on slow networks.
Nits
loader.pytrailing blank line at endsave_summaryp95 calcs[int(len(s) * 0.95)]givess[0]whenlen(s) == 1(min, not p95)- Only one model (Qwen2.5-7B) and only gRPC — fine as a starting point, just noting it
The standalone mode with BFCL_BASE_URL and the per-test JSON logging are both great ideas. Looking forward to the next iteration.
|
Hi @vschandramourya, 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:
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e_test/fixtures/hooks.py (1)
450-462: 🧹 Nitpick | 🔵 TrivialConsider guarding
write_summary_if_needed()against unexpected errors.If
write_summary_if_needed()raises (e.g., disk full, permission denied), it would propagate throughpytest_sessionfinishand potentially obscure the real test exit status. A try/except with a warning log would make this more resilient.Proposed fix
from bfcl.session_state import write_summary_if_needed - write_summary_if_needed() + try: + write_summary_if_needed() + except Exception: + logging.getLogger(__name__).warning( + "Failed to write BFCL summary", exc_info=True + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/fixtures/hooks.py` around lines 450 - 462, Wrap the final call to write_summary_if_needed() inside a try/except in pytest_sessionfinish so any exceptions (e.g., IO errors) are caught and logged as a warning instead of propagating and affecting pytest's exit; keep calling cleanup_all_cached_backends() as-is, import the standard logging module (or use an existing logger) and log the caught exception with context including the function name write_summary_if_needed to aid debugging.
♻️ Duplicate comments (6)
e2e_test/chat_completions/test_bfcl.py (2)
192-192:log_filepaths are inconsistent between error and success branches.The API-error branch (line 192) stores
f"{category}/{test_id...}_FAIL.json"(includes category prefix), while the success/eval-failure branch (line 242) stores onlylog_path.name(filename without category). This makes log lookup inconsistent in the summary.Also applies to: 242-242
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/chat_completions/test_bfcl.py` at line 192, The log file path construction is inconsistent between the API-error branch (using "log_file": f"{category}/{test_id.replace('/', '_')}_FAIL.json") and the success/eval-failure branch (using only log_path.name); pick one canonical format and apply it to both places—e.g., change the success/eval-failure branch to use the category-prefixed path by setting log_file to f"{category}/{log_path.name}" (or alternatively update the error branch to use log_path.name) so that both branches use the same combination of category and filename; update references to log_file, log_path, test_id, and category accordingly to keep naming consistent.
143-163: Request parameters duplicated betweenrequest_payloadand the API call.
request_payload(lines 143–149) is built for logging, but theclient.chat.completions.create(...)call (lines 156–163) repeats the same parameters independently. If one is updated without the other, the logged payload diverges from what was actually sent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/chat_completions/test_bfcl.py` around lines 143 - 163, The test duplicates request parameters between the request_payload dict and the client.chat.completions.create(...) call; add the missing "model": model key to request_payload and then call the API with the single source of truth by unpacking it (client.chat.completions.create(**request_payload)) so logging and the actual call always match (referencing request_payload and the client.chat.completions.create call).e2e_test/bfcl/loader.py (1)
74-84: Silently empty ground truth when answer file is missing will produce misleading test failures.For categories with expected answers (simple, multiple, parallel, parallel_multiple), a missing answer file silently yields empty
ground_truth. The evaluator then reports "No ground truth available but model produced tool calls" instead of a clear data-not-found error. RaiseFileNotFoundErrorwhenanswer_filenameis set but the file doesn't exist.Proposed fix
answer_filename = _ANSWER_FILE_MAP.get(category) answers_by_id: dict[str, list] = {} - if answer_filename and (DATA_DIR / answer_filename).exists(): - answer_path = DATA_DIR / answer_filename + if answer_filename: + answer_path = DATA_DIR / answer_filename + if not answer_path.exists(): + raise FileNotFoundError( + f"BFCL answer file not found: {answer_path}. " + "Run: python e2e_test/bfcl/download_data.py" + ) with open(answer_path, encoding="utf-8") as f:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/bfcl/loader.py` around lines 74 - 84, The code currently sets answer_filename via _ANSWER_FILE_MAP and silently leaves ground truth empty if the corresponding file under DATA_DIR is missing; change the logic in the loader where answer_filename, DATA_DIR and answers_by_id are handled so that when answer_filename is truthy but (DATA_DIR / answer_filename).exists() is False you raise a FileNotFoundError (including the missing filename in the exception message) instead of proceeding with an empty answers_by_id; keep the existing file-reading behavior (open and json.loads into answers_by_id) when the file exists and do nothing only when answer_filename is falsy.e2e_test/bfcl/evaluator.py (3)
64-101: Order-sensitive matching will mis-score parallel tool calls.The index-based comparison at lines 71–73 requires tool calls to appear in the same order as the ground truth, which is incorrect for
parallelandparallel_multiplecategories where call order is semantically irrelevant. This can produce false failures for correct outputs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/bfcl/evaluator.py` around lines 64 - 101, The current index-based comparison using actual_tool_calls and ground_truth causes false negatives for parallel/parallel_multiple cases; change the matching to be order-insensitive for those categories by replacing the positional loop (for i in range(check_count) / comparing actual_tool_calls[i] to ground_truth[i]) with a matching algorithm that, for each ground-truth entry (gt_entry), searches among remaining unmatched actual_tool_calls for an entry where actual["name"] == expected_name and all expected args match via _values_match; mark matched actuals as consumed and only error if no unmatched actual satisfies a gt_entry, keeping the same error messages and using actual_tool_calls, ground_truth, expected_name, expected_args and _values_match to locate and compare entries.
223-241:⚠️ Potential issue | 🟡 Minor
_values_matchequates booleans with integers due tofloat()coercion.
float(True) == float(1)→True, so_values_match(True, 1)and_values_match(False, 0)will match even when the types carry different semantics. For BFCL argument validation this could mask type mismatches. Consider guarding againstboolinputs before thefloat()coercion.Proposed fix
try: + if isinstance(actual, bool) or isinstance(expected, bool): + raise TypeError("skip float coercion for bools") if float(actual) == float(expected): return True except (TypeError, ValueError): pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/bfcl/evaluator.py` around lines 223 - 241, In _values_match, avoid coercing booleans to floats (which makes True==1 and False==0) by adding a guard before the numeric coercion: if either actual or expected is a bool, skip the float() comparison branch (the equality check at top already handles identical booleans); this prevents matching True/False with ints while leaving other numeric/coercion logic intact.
84-85:⚠️ Potential issue | 🟡 Minor
actual["arguments"]willKeyErrorif an entry lacks the"arguments"key.
evaluate_tool_callsis a public API. While the current caller (_extract_tool_calls) always provides"arguments", external usage or malformed data could trigger aKeyError. Use.get("arguments", {})for defensive access.Proposed fix
- actual_val = actual["arguments"].get(param_name) + actual_val = actual.get("arguments", {}).get(param_name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/bfcl/evaluator.py` around lines 84 - 85, The loop in evaluate_tool_calls uses actual["arguments"] which can raise KeyError for malformed input; change accesses to use actual.get("arguments", {}) so missing "arguments" yields an empty dict and prevents exceptions—update the code in evaluate_tool_calls where actual["arguments"] is read (and any other spots reading actual["arguments"]) to use .get("arguments", {}) and adjust logic accordingly while keeping compatibility with calls from _extract_tool_calls.
🤖 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/chat_completions/test_bfcl.py`:
- Around line 58-74: The _extract_tool_calls function assumes
response.choices[0] exists and will raise IndexError for empty choices; update
_extract_tool_calls to first check that response.choices is a non-empty sequence
(e.g., if not response.choices: return [] or handle as empty) before accessing
response.choices[0], then proceed to read .message.tool_calls safely (use
attribute checks or defaults) so malformed responses with no choices return an
empty list instead of raising.
- Around line 90-105: The pytest.skip() call inside pytest_generate_tests causes
the whole module to be skipped during collection when _cases_cache is empty;
instead, modify pytest_generate_tests (the function named pytest_generate_tests)
to avoid calling pytest.skip and either return early or call
metafunc.parametrize("case", [], ids=[]) so the test module is collected with
zero parametrized cases; update the branch that currently does pytest.skip("No
BFCL data found — run: python e2e_test/bfcl/download_data.py") to perform a
graceful no-case parametrization using metafunc.parametrize or simply return
after ensuring "case" was handled.
---
Outside diff comments:
In `@e2e_test/fixtures/hooks.py`:
- Around line 450-462: Wrap the final call to write_summary_if_needed() inside a
try/except in pytest_sessionfinish so any exceptions (e.g., IO errors) are
caught and logged as a warning instead of propagating and affecting pytest's
exit; keep calling cleanup_all_cached_backends() as-is, import the standard
logging module (or use an existing logger) and log the caught exception with
context including the function name write_summary_if_needed to aid debugging.
---
Duplicate comments:
In `@e2e_test/bfcl/evaluator.py`:
- Around line 64-101: The current index-based comparison using actual_tool_calls
and ground_truth causes false negatives for parallel/parallel_multiple cases;
change the matching to be order-insensitive for those categories by replacing
the positional loop (for i in range(check_count) / comparing
actual_tool_calls[i] to ground_truth[i]) with a matching algorithm that, for
each ground-truth entry (gt_entry), searches among remaining unmatched
actual_tool_calls for an entry where actual["name"] == expected_name and all
expected args match via _values_match; mark matched actuals as consumed and only
error if no unmatched actual satisfies a gt_entry, keeping the same error
messages and using actual_tool_calls, ground_truth, expected_name, expected_args
and _values_match to locate and compare entries.
- Around line 223-241: In _values_match, avoid coercing booleans to floats
(which makes True==1 and False==0) by adding a guard before the numeric
coercion: if either actual or expected is a bool, skip the float() comparison
branch (the equality check at top already handles identical booleans); this
prevents matching True/False with ints while leaving other numeric/coercion
logic intact.
- Around line 84-85: The loop in evaluate_tool_calls uses actual["arguments"]
which can raise KeyError for malformed input; change accesses to use
actual.get("arguments", {}) so missing "arguments" yields an empty dict and
prevents exceptions—update the code in evaluate_tool_calls where
actual["arguments"] is read (and any other spots reading actual["arguments"]) to
use .get("arguments", {}) and adjust logic accordingly while keeping
compatibility with calls from _extract_tool_calls.
In `@e2e_test/bfcl/loader.py`:
- Around line 74-84: The code currently sets answer_filename via
_ANSWER_FILE_MAP and silently leaves ground truth empty if the corresponding
file under DATA_DIR is missing; change the logic in the loader where
answer_filename, DATA_DIR and answers_by_id are handled so that when
answer_filename is truthy but (DATA_DIR / answer_filename).exists() is False you
raise a FileNotFoundError (including the missing filename in the exception
message) instead of proceeding with an empty answers_by_id; keep the existing
file-reading behavior (open and json.loads into answers_by_id) when the file
exists and do nothing only when answer_filename is falsy.
In `@e2e_test/chat_completions/test_bfcl.py`:
- Line 192: The log file path construction is inconsistent between the API-error
branch (using "log_file": f"{category}/{test_id.replace('/', '_')}_FAIL.json")
and the success/eval-failure branch (using only log_path.name); pick one
canonical format and apply it to both places—e.g., change the
success/eval-failure branch to use the category-prefixed path by setting
log_file to f"{category}/{log_path.name}" (or alternatively update the error
branch to use log_path.name) so that both branches use the same combination of
category and filename; update references to log_file, log_path, test_id, and
category accordingly to keep naming consistent.
- Around line 143-163: The test duplicates request parameters between the
request_payload dict and the client.chat.completions.create(...) call; add the
missing "model": model key to request_payload and then call the API with the
single source of truth by unpacking it
(client.chat.completions.create(**request_payload)) so logging and the actual
call always match (referencing request_payload and the
client.chat.completions.create call).
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
e2e_test/bfcl/download_data.pye2e_test/bfcl/evaluator.pye2e_test/bfcl/loader.pye2e_test/bfcl/session_state.pye2e_test/bfcl_logs/.gitignoree2e_test/chat_completions/test_bfcl.pye2e_test/conftest.pye2e_test/fixtures/hooks.py
|
Hey @slin1237, thanks for the review. I’ve fixed the changes; please let me know if anything else needs tweaking. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
e2e_test/chat_completions/test_bfcl.py (4)
57-60:⚠️ Potential issue | 🟡 MinorGuard empty
choicesbefore indexing.
_extract_tool_callsassumesresponse.choices[0]exists. Empty/malformed responses will raise before tool-call evaluation.Proposed fix
def _extract_tool_calls(response: Any) -> list[dict[str, Any]]: """Pull structured tool calls out of an OpenAI ChatCompletion response.""" - tool_calls = response.choices[0].message.tool_calls or [] + choices = getattr(response, "choices", None) or [] + if not choices: + return [] + tool_calls = choices[0].message.tool_calls or []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/chat_completions/test_bfcl.py` around lines 57 - 60, The helper _extract_tool_calls currently indexes response.choices[0] without guarding for empty or malformed responses; update _extract_tool_calls to first verify that response has a non-empty choices list and that choices[0].message exists, and if not return an empty list immediately, otherwise safely read choices[0].message.tool_calls (falling back to [] if that attribute is missing) so no IndexError/AttributeError is raised when responses are empty or malformed.
167-181:⚠️ Potential issue | 🟡 MinorPersist
log_fileconsistently as a run-relative path.API-error and evaluation paths currently write different
log_fileformats, which complicates summary consumers.Proposed fix
- save_test_log( + log_path = save_test_log( run_dir, test_id=test_id, category=category, @@ append_result({ @@ - "log_file": f"{category}/{test_id.replace('/', '_')}_FAIL.json", + "log_file": str(log_path.relative_to(run_dir)), "model": model, "backend": backend, }) @@ - "log_file": log_path.name, + "log_file": str(log_path.relative_to(run_dir)), "model": model, "backend": backend, })Also applies to: 191-192, 241-241
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/chat_completions/test_bfcl.py` around lines 167 - 181, The API-error branch calls save_test_log with response_payload=None and writes errors=[f"API error: {exc}"] but uses a different log_file format than the evaluation path; canonicalize log_file to a run-relative path before any save_test_log calls by computing a single run_relative_log_file value (e.g., relative to run_dir) and pass that same variable into save_test_log in the API-error branch and the other places (the evaluation-success/failure branches), or wrap the path conversion in a helper used by save_test_log callers so all calls (including the call in the API-exception handler and the other save_test_log invocations) persist log_file consistently as run-relative.
142-148: 🧹 Nitpick | 🔵 TrivialUse
request_payloadas the single source of truth.The logged payload can drift from the actual request because parameters are duplicated.
Proposed fix
request_payload = { + "model": model, "messages": messages, "tools": tools, "tool_choice": "auto", "temperature": 0.01, "max_tokens": 1024, @@ - response = client.chat.completions.create( - model=model, - messages=messages, - tools=tools, - tool_choice="auto", - temperature=0.01, - max_tokens=1024, - ) + response = client.chat.completions.create(**request_payload)Also applies to: 155-162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/chat_completions/test_bfcl.py` around lines 142 - 148, The test duplicates request parameters instead of using request_payload as the single source of truth; update the test to build the dict once into request_payload and then pass that same variable to both the request send call and any logged/verified payloads (replace any separate dicts or inline kwargs between lines around the request send and the log/assert sections). Locate the usages of request_payload and the subsequent call that sends the request (e.g., the HTTP client post/send and any logger or assertion that currently reconstructs the payload) and refactor them to reference request_payload only, removing the duplicated parameter dictionaries.
102-104:⚠️ Potential issue | 🟡 MinorAvoid
pytest.skip()insidepytest_generate_tests.This skips module collection rather than yielding zero parametrized BFCL cases.
Proposed fix
if not _cases_cache: - pytest.skip("No BFCL data found — run: python e2e_test/bfcl/download_data.py") + metafunc.parametrize("case", [], ids=[]) + return metafunc.parametrize("case", _cases_cache, ids=[c["id"] for c in _cases_cache])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/chat_completions/test_bfcl.py` around lines 102 - 104, Inside pytest_generate_tests, do not call pytest.skip(); instead handle the empty _cases_cache by calling metafunc.parametrize with an empty iterable so the test is collected with zero cases. Replace the pytest.skip branch so that when _cases_cache is falsy you call metafunc.parametrize("case", [], ids=[]), leaving the existing parametrization call (metafunc.parametrize("case", _cases_cache, ids=[c["id"] for c in _cases_cache])) to only run when _cases_cache is truthy; update the logic around the pytest_generate_tests function and the _cases_cache variable accordingly.
🤖 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/chat_completions/test_bfcl.py`:
- Around line 287-290: The current class-level pytest.mark.skipif using
BFCL_BASE_URL can hide misconfiguration; replace the decorator with an explicit
runtime check that fails the suite when BFCL_BASE_URL is not set: remove the
pytest.mark.skipif(...) decorator and add a setup hook (e.g., a module-level
setup_module or class method setup_class) that checks
os.environ.get("BFCL_BASE_URL") and calls pytest.fail("BFCL_BASE_URL not set")
(or raises RuntimeError) to make the test fail instead of being skipped; keep
the BFCL_BASE_URL env var name and the original intent of gating tests but
change the mechanism from skipif to an explicit failure.
---
Duplicate comments:
In `@e2e_test/chat_completions/test_bfcl.py`:
- Around line 57-60: The helper _extract_tool_calls currently indexes
response.choices[0] without guarding for empty or malformed responses; update
_extract_tool_calls to first verify that response has a non-empty choices list
and that choices[0].message exists, and if not return an empty list immediately,
otherwise safely read choices[0].message.tool_calls (falling back to [] if that
attribute is missing) so no IndexError/AttributeError is raised when responses
are empty or malformed.
- Around line 167-181: The API-error branch calls save_test_log with
response_payload=None and writes errors=[f"API error: {exc}"] but uses a
different log_file format than the evaluation path; canonicalize log_file to a
run-relative path before any save_test_log calls by computing a single
run_relative_log_file value (e.g., relative to run_dir) and pass that same
variable into save_test_log in the API-error branch and the other places (the
evaluation-success/failure branches), or wrap the path conversion in a helper
used by save_test_log callers so all calls (including the call in the
API-exception handler and the other save_test_log invocations) persist log_file
consistently as run-relative.
- Around line 142-148: The test duplicates request parameters instead of using
request_payload as the single source of truth; update the test to build the dict
once into request_payload and then pass that same variable to both the request
send call and any logged/verified payloads (replace any separate dicts or inline
kwargs between lines around the request send and the log/assert sections).
Locate the usages of request_payload and the subsequent call that sends the
request (e.g., the HTTP client post/send and any logger or assertion that
currently reconstructs the payload) and refactor them to reference
request_payload only, removing the duplicated parameter dictionaries.
- Around line 102-104: Inside pytest_generate_tests, do not call pytest.skip();
instead handle the empty _cases_cache by calling metafunc.parametrize with an
empty iterable so the test is collected with zero cases. Replace the pytest.skip
branch so that when _cases_cache is falsy you call metafunc.parametrize("case",
[], ids=[]), leaving the existing parametrization call
(metafunc.parametrize("case", _cases_cache, ids=[c["id"] for c in
_cases_cache])) to only run when _cases_cache is truthy; update the logic around
the pytest_generate_tests function and the _cases_cache variable accordingly.
76624c3 to
a512f85
Compare
|
any update on this PR? |
|
Hi @vschandramourya, this PR has been inactive for 14 days. Please update it or close it if it's no longer needed. |
|
Hi @vschandramourya, this PR has merge conflicts that must be resolved before it can be merged. Please rebase your branch: git fetch origin main
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-lease |
mergify
Bot
added
needs-rebase
|
This pull request has been automatically marked as stale because it has not had any activity within 14 days. It will be automatically closed if no further activity occurs within 16 days. Leave a comment if you feel this pull request should remain open. Thank you! |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/bfcl/evaluator.py`:
- Around line 147-152: The per-test filename generation (in cat_dir / safe_id /
filename) can collide across TestBFCLQwen and TestBFCLStandalone because
bfcl_run_dir is session-scoped; change the filename to include the run/execution
mode to make it unique (e.g., append a run_mode or bfcl_run_dir.name/session
id). Update the logic that builds filename (symbols: safe_id, status, filename
in e2e_test/bfcl/evaluator.py) to incorporate an execution-mode identifier
passed from the caller (or derived from bfcl_run_dir) so each mode writes
distinct files and avoids overwrites.
- Around line 41-47: The scoring loop assumes actual["arguments"] is a dict and
ignores extra keys; modify the code in evaluator.py (around expected_args, the
for loop and use of _values_match) to first fetch arguments =
actual.get("arguments") and return False unless isinstance(arguments, dict) and
set(arguments.keys()) == set(expected_args.keys()); then iterate expected_args
using arguments.get(param_name) (not actual.get(...)) and keep using
_values_match for comparisons, returning False on any mismatch so hallucinated
or missing keys fail scoring.
In `@e2e_test/bfcl/loader.py`:
- Around line 102-109: The code currently falls back to an empty list when a
test_id is missing from answers_by_id which masks truncated/out-of-sync answer
files; update the block that sets ground_truth so that if an answers mapping
file was provided (reference the variable answer_filename or similar) and
test_id is not present in answers_by_id, you raise an exception
(ValueError/KeyError) including test_id and answer_filename instead of using
[]—otherwise retrieve the real value from answers_by_id and keep the normal
flow; modify the place where ground_truth is assigned and where results are
appended (see variables ground_truth, answers_by_id, answer_filename and the
dict construction for "id"/"ground_truth") to implement this check and error.
In `@e2e_test/chat_completions/test_bfcl.py`:
- Around line 99-106: The current `_cases_cache` is populated by eagerly calling
`_load("simple")`, `_load("multiple")`, `_load("parallel")`,
`_load("parallel_multiple")`, and `_load("irrelevance")` which forces all
categories to load for every test run; change the logic so `_cases_cache` is a
mapping keyed by category (e.g., dict) and call `_load(category)` lazily only
for the category being requested when parametrizing tests (use
`_cases_cache.get(category)` / populate on first access). Update any code that
iterates `_cases_cache` to iterate requested categories and ensure `_load` is
only invoked for those categories actually used by the parametrization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 102877b1-192b-4fba-99be-ae5dc493727b
📒 Files selected for processing (6)
e2e_test/bfcl/__init__.pye2e_test/bfcl/download_data.pye2e_test/bfcl/evaluator.pye2e_test/bfcl/loader.pye2e_test/chat_completions/test_bfcl.pye2e_test/fixtures/hooks.py
| expected_args = gt_entry.get(expected_name, {}) | ||
| for param_name, possible_values in expected_args.items(): | ||
| actual_val = actual.get("arguments", {}).get(param_name) | ||
| if not isinstance(possible_values, list): | ||
| possible_values = [possible_values] | ||
| if not any(_values_match(actual_val, pv) for pv in possible_values): | ||
| return False |
There was a problem hiding this comment.
Validate actual["arguments"] before scoring.
json.loads() can produce null, arrays, or scalars, so actual.get("arguments", {}).get(...) can raise here. The matcher also ignores unexpected keys, which lets hallucinated arguments count as correct. Return False unless arguments is a dict with the same key set as expected_args.
🛠️ Proposed fix
def _call_matches(actual: dict[str, Any], gt_entry: dict[str, Any]) -> bool:
"""Check whether a single actual tool call satisfies a ground truth entry."""
expected_name = next(iter(gt_entry.keys()), None)
if actual.get("name") != expected_name:
return False
expected_args = gt_entry.get(expected_name, {})
+ if not isinstance(expected_args, dict):
+ return False
+ actual_args = actual.get("arguments")
+ if not isinstance(actual_args, dict):
+ return False
+ if set(actual_args) != set(expected_args):
+ return False
for param_name, possible_values in expected_args.items():
- actual_val = actual.get("arguments", {}).get(param_name)
+ actual_val = actual_args.get(param_name)
if not isinstance(possible_values, list):
possible_values = [possible_values]
if not any(_values_match(actual_val, pv) for pv in possible_values):
return False
return True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e_test/bfcl/evaluator.py` around lines 41 - 47, The scoring loop assumes
actual["arguments"] is a dict and ignores extra keys; modify the code in
evaluator.py (around expected_args, the for loop and use of _values_match) to
first fetch arguments = actual.get("arguments") and return False unless
isinstance(arguments, dict) and set(arguments.keys()) ==
set(expected_args.keys()); then iterate expected_args using
arguments.get(param_name) (not actual.get(...)) and keep using _values_match for
comparisons, returning False on any mismatch so hallucinated or missing keys
fail scoring.
| cat_dir = run_dir / category | ||
| cat_dir.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| safe_id = test_id.replace("/", "_").replace(" ", "_") | ||
| status = "PASS" if passed else "FAIL" | ||
| filename = f"{safe_id}_{status}.json" |
There was a problem hiding this comment.
Make per-test log filenames unique across execution modes.
bfcl_run_dir is session-scoped in e2e_test/chat_completions/test_bfcl.py, and both TestBFCLQwen and TestBFCLStandalone can write the same test_id into the same category directory. With filename = f"{safe_id}_{status}.json", the later run overwrites the earlier one whenever the status matches, so one mode’s diagnostics disappear.
🛠️ Proposed fix
cat_dir = run_dir / category
cat_dir.mkdir(parents=True, exist_ok=True)
safe_id = test_id.replace("/", "_").replace(" ", "_")
+ safe_model = model.replace("/", "_").replace(" ", "_")
+ safe_parser = parser.replace("/", "_").replace(" ", "_")
+ safe_backend = backend.replace("/", "_").replace(" ", "_")
status = "PASS" if passed else "FAIL"
- filename = f"{safe_id}_{status}.json"
+ filename = f"{safe_id}_{safe_model}_{safe_backend}_{safe_parser}_{status}.json"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e_test/bfcl/evaluator.py` around lines 147 - 152, The per-test filename
generation (in cat_dir / safe_id / filename) can collide across TestBFCLQwen and
TestBFCLStandalone because bfcl_run_dir is session-scoped; change the filename
to include the run/execution mode to make it unique (e.g., append a run_mode or
bfcl_run_dir.name/session id). Update the logic that builds filename (symbols:
safe_id, status, filename in e2e_test/bfcl/evaluator.py) to incorporate an
execution-mode identifier passed from the caller (or derived from bfcl_run_dir)
so each mode writes distinct files and avoids overwrites.
| ground_truth = answers_by_id.get(test_id, []) | ||
|
|
||
| results.append( | ||
| { | ||
| "id": test_id, | ||
| "question": messages, | ||
| "function": entry.get("function", []), | ||
| "ground_truth": ground_truth, |
There was a problem hiding this comment.
Fail when a mapped answer file omits a test ID.
Falling back to [] here turns a truncated or out-of-sync answer file into “no tool calls expected”. e2e_test/bfcl/evaluator.py::evaluate_tool_calls() treats empty ground truth that way, so these cases can pass incorrectly and inflate BFCL accuracy. Raise as soon as answer_filename is set but test_id is absent from answers_by_id.
🛠️ Proposed fix
for test_id, entry in questions_by_id.items():
+ if answer_filename and test_id not in answers_by_id:
+ raise MissingBFCLAnswerFileError(
+ f"BFCL ground truth missing for category={category!r}, "
+ f"id={test_id!r} in {answer_filename}"
+ )
raw_question = entry.get("question", [])
messages = (
raw_question[0] if raw_question and isinstance(raw_question[0], list) else raw_question
)
ground_truth = answers_by_id.get(test_id, [])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e_test/bfcl/loader.py` around lines 102 - 109, The code currently falls
back to an empty list when a test_id is missing from answers_by_id which masks
truncated/out-of-sync answer files; update the block that sets ground_truth so
that if an answers mapping file was provided (reference the variable
answer_filename or similar) and test_id is not present in answers_by_id, you
raise an exception (ValueError/KeyError) including test_id and answer_filename
instead of using []—otherwise retrieve the real value from answers_by_id and
keep the normal flow; modify the place where ground_truth is assigned and where
results are appended (see variables ground_truth, answers_by_id, answer_filename
and the dict construction for "id"/"ground_truth") to implement this check and
error.
| if _cases_cache is None: | ||
| _cases_cache = ( | ||
| _load("simple") | ||
| + _load("multiple") | ||
| + _load("parallel") | ||
| + _load("parallel_multiple") | ||
| + _load("irrelevance") | ||
| ) |
There was a problem hiding this comment.
Don't eagerly load every BFCL category into _cases_cache.
The module docstring advertises pytest ... -k "simple_", but this block still calls _load() for all five categories as soon as any BFCL test is collected. A missing answer file in an unneeded category can still abort that run, and isolated category runs still pay the full data-loading cost. Cache per category and only load the categories actually being parametrized.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e_test/chat_completions/test_bfcl.py` around lines 99 - 106, The current
`_cases_cache` is populated by eagerly calling `_load("simple")`,
`_load("multiple")`, `_load("parallel")`, `_load("parallel_multiple")`, and
`_load("irrelevance")` which forces all categories to load for every test run;
change the logic so `_cases_cache` is a mapping keyed by category (e.g., dict)
and call `_load(category)` lazily only for the category being requested when
parametrizing tests (use `_cases_cache.get(category)` / populate on first
access). Update any code that iterates `_cases_cache` to iterate requested
categories and ensure `_load` is only invoked for those categories actually used
by the parametrization.
3be7d98 to
b6cd2a1
Compare
mergify
Bot
removed
the
needs-rebase
8ed0199 to
b6b75f7
Compare
Signed-off-by: VS Chandra Mourya <msrinivasa@together.ai>
7d0fe53 to
17f3b98
Compare
Signed-off-by: VS Chandra Mourya <msrinivasa@together.ai>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| logger = logging.getLogger(__name__) | ||
|
|
||
| BFCL_LIMIT = int(os.environ.get("BFCL_LIMIT", "0")) or None | ||
| BFCL_CATEGORIES = ("simple", "multiple", "parallel", "parallel_multiple", "irrelevance") |
There was a problem hiding this comment.
🟡 Nit: BFCL_CATEGORIES is redefined here as a tuple, duplicating the canonical list in bfcl/loader.py. If a category is added to or removed from one but not the other, they'll silently diverge. Consider importing the canonical list instead:
| BFCL_CATEGORIES = ("simple", "multiple", "parallel", "parallel_multiple", "irrelevance") | |
| BFCL_CATEGORIES = ("simple", "multiple", "parallel", "parallel_multiple", "irrelevance") |
→
from bfcl import BFCL_CATEGORIES(It's already exported from bfcl/__init__.py but not imported here.)
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
|
Hi @vschandramourya, 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:
|
| Handles the BFCL-specific quirks: | ||
| - parameters.type "dict" → "object" | ||
| - parameters.type "float" → "number" | ||
| - Wraps in {"type": "function", "function": ...} |
There was a problem hiding this comment.
🟡 Nit: The docstring still only mentions dict → object and float → number, but the implementation now also handles int → integer and list/tuple → array. Worth updating the docstring to reflect the complete set of conversions.
| - Wraps in {"type": "function", "function": ...} | |
| """Convert BFCL function definitions to OpenAI tools format. | |
| Handles the BFCL-specific quirks: | |
| - parameters.type "dict" → "object" | |
| - parameters.type "float" → "number" | |
| - parameters.type "int" → "integer" | |
| - parameters.type "list"/"tuple" → "array" | |
| - Wraps in {"type": "function", "function": ...} | |
| """ |
|
Hi @vschandramourya, this PR has merge conflicts that must be resolved before it can be merged. Please rebase your branch: git fetch origin main
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-lease |
mergify
Bot
added
needs-rebase
|
This pull request has been automatically marked as stale because it has not had any activity within 14 days. It will be automatically closed if no further activity occurs within 16 days. Leave a comment if you feel this pull request should remain open. Thank you! |
|
This pull request has been automatically closed due to inactivity. Please feel free to reopen if you intend to continue working on it. Thank you! |
|
Hi @vschandramourya, 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:
|
There was a problem hiding this comment.
Thorough review complete. All major issues from prior reviews have been addressed: atomic downloads with timeouts, order-insensitive bipartite matching for parallel tool calls, thread-safe result collection, lazy category loading, proper error handling for missing answer files, and complete BFCL type mapping. Two minor nits from a previous review remain open (incomplete docstring in converter.py and duplicated BFCL_CATEGORIES constant) but are non-blocking. Code is well-structured and ready to merge.
|
This pull request has been automatically marked as stale because it has not had any activity within 14 days. It will be automatically closed if no further activity occurs within 16 days. Leave a comment if you feel this pull request should remain open. Thank you! |
|
This pull request has been automatically closed due to inactivity. Please feel free to reopen if you intend to continue working on it. Thank you! |
3 participants
Description
Problem
SMG has no automated function calling accuracy tests. SMG requires a "Lighter BFCL" suite for PR-level validation and full BFCL for nightly runs, but the test infrastructure doesn't exist yet.
Solution
Add BFCL v3 (Berkeley Function Calling Leaderboard) test infrastructure that downloads test data from HuggingFace at runtime and validates tool call accuracy across 5 categories: simple, multiple, parallel, parallel_multiple, and irrelevance (1,240 total cases).
Changes
e2e_test/bfcl/— data loader, evaluator with per-test JSON logging, and HuggingFace data downloadere2e_test/bfcl/data/.gitignore— keeps downloaded JSON data out of the repoe2e_test/chat_completions/test_bfcl.py— pytest test classes for all 5 categories (fixture-based + standalone mode); supportsBFCL_LIMITenv var to restrict cases per categoryTest Plan
Validated locally on H100 with Qwen2.5-7B-Instruct via SGLang gRPC:
-kfilter)Checklist
cargo +nightly fmtpasses (no Rust changes)cargo clippy --all-targets --all-features -- -D warningspasses (no Rust changes)Summary by CodeRabbit
New Features
Tests
Chores