feat(m3): add capability_4 data and enable testing real follow-up questions - #127
Closed
haroldship wants to merge 12 commits into
Closed
haroldship wants to merge 12 commits into
haroldship wants to merge 12 commits into
Conversation
…'s registry setting config/global.env has set DYNACONF_ADVANCED_FEATURES__REGISTRY=false since this repo's first commit, contradicting every benchmark's own .env (m3, appworld, bpo, oak_health_insurance all set it to true). This was masked for months by config_loader/loader.py's override=True behavior, which let the benchmark-specific file always win. #114 changed that loader to os.environ.setdefault (for an unrelated port-override use case), which now respects whatever the shell already exported - and load_env.sh's own no-override, global-loads-first order has always resolved that value to false. Net effect: every benchmark silently gets zero tools. - Remove the DYNACONF_ADVANCED_FEATURES__REGISTRY=false line and its stale "tools loaded directly from containers" comment (that direct-load mechanism doesn't exist anywhere in this repo's history) - No replacement default is set here on purpose: each benchmark's own .env already declares the value it wants Fixes #124
address.json, hockey.json, olympics.json, and olympics_multiturn.json had no documented source or license. Two matched VAKRA's uuid-prefixed sample schema but were never labeled as such; the other two used different, unexplained schemas. - Several scripts (eval.sh, compare.sh, eval_m3_react.py, eval_m3_multiturn.py, scripts/create_eval_bundle.py) reference these as hardcoded default paths; fixing those now-broken defaults is tracked as follow-up, not done here. Part of #125
New VAKRA-derived --m3-data corpus (300 samples: 150 with a special- instructions policy attached / 150 without, 133 single-turn / 167 multi- turn), licensed and documented the same way as the existing small_train.zip. - .gitignore: exception for the new zip alongside small_train.zip - LICENSE: list the new zip under CC BY-NC-SA 4.0, not covered by Apache 2.0 - benchmarks/m3/data/NOTICE: source/creators, sampling balance, modifications, license, and disclaimer, mirroring small_train.zip's entry - .pre-commit-config.yaml: exclude benchmarks/m3/data/*.zip from check-added-large-files (this dataset is ~22MB; small_train.zip never needed the exception at ~80KB) Part of #125
benchmarks/m3/config/m3_registry_m3_data.yaml previously only listed m3_task_2/m3_task_3, so --m3-data --capability m3_task_4 could not resolve any services even though the capability_4_multiturn container already existed and worked. - Add the m3_task_4 service block using the same mcp_dispatch.py stdio pattern as m3_task_2/m3_task_3 (verified mcp_dispatch.py exists in the capability_4_multiturn container, same as capability_2_dashboard_apis) Part of #125
Add benchmarks/m3/scripts/generate_policy_turn_keys.py, which inspects any M3 --m3-data zip and generates ready-to-use --eval-key shortcuts for the natural breakdowns (single-turn, multi-turn, with-policy, without-policy, and non-degenerate intersections) rather than requiring these to be hand-curated per dataset. - eval_config.toml: cap4_single_turn (133), cap4_multi_turn (167), cap4_with_policy (150), cap4_without_policy (150), cap4_multi_turn_with_policy (17) - the non-degenerate set for capability_4_multiturn_policy_sampled.zip; single-turn-with/without- policy and multi-turn-without-policy were dropped as degenerate (empty or identical to a base bucket) - eval_config.toml: cap4_single_turn_s1 / cap4_multi_turn_with_policy_s1 / cap4_multi_turn_without_policy_s1 single-task probe keys, one per group, for quick manual verification Part of #125
VAKRA's additional_instructions field (per-sample "special instructions" for tasks with a policy attached) was loaded by M3DataLoader but never reached the agent - only the static, once-per-domain special_instructions rider did. - evaluate_multiturn_task: pass sample["additional_instructions"] through as user_context to evaluate_multiturn_task_with_langfuse, which flows to CugaAgent's `pi` state field and gets appended as a "## User Context" block on the first human message. This is additive to the standing special_instructions rider, not a replacement. - Confirmed working end-to-end via Langfuse trace inspection: the policy text appears in the rendered prompt for with-policy samples. Part of #125
…ability4-m3-dataset
…l follow-up VAKRA's capability_4 dialogue samples with 2+ turns hold every turn except the last one already answered (each prior turn carries a gold "answer"). Previously the eval loop live-invoked the agent on every turn in sequence, so the agent had to actually re-solve turns whose answers were already known - never a genuine follow-up question, always a fresh multi-step task. - sdk_eval_helpers.py: add optional history_messages param to evaluate_task_with_langfuse; when given, prepended to the live turn's message so agent.invoke() receives the whole primed conversation in one call (the agent only ever answers the last message - everything before it is context, not re-solved) - eval_m3.py: evaluate_multiturn_task splits samples with 2+ turns into prior turns (query + gold answer, stringified into synthetic HumanMessage/AIMessage history) and the final live turn; only the live turn is invoked and scored - eval_m3.py: cuga-agent's "## User Context" injection (the mechanism additional_instructions/user_context rides on) only fires on a thread's very first message - a primed thread never satisfies that, so it would silently never reach the model. Embed it directly on the live turn's message instead, in the same format, so the policy still applies - eval_m3.py: expected_output now reads gold_sequence/answer_per_turn/ tool_response_per_turn from index -1 (the live/last turn) instead of 0 (the first turn) - equivalent for single-turn samples, correct for multi-turn; _annotate_tool_call_diffs only compares the live turn since primed turns were never actually invoked Part of #125
Different VAKRA exports use different key names for the same shape: small_train.zip uses top-level "ground_truth" (list) with per-turn "gold_sequence"; capability_4_multiturn_policy_sampled.zip uses top-level "output" (list) with per-turn "sequence". load_domain() only recognized the small_train.zip names, so gold_sequence/answer_per_turn/ tool_response_per_turn came out silently empty for every capability_4 sample - surfaced while validating the dialogue-priming feature, whose scoring depends on this data being present. - Accept "output" as a fallback for "ground_truth", and "sequence" as a fallback for "gold_sequence", per turn Part of #125
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughCapability 4 M3 data is registered, documented, and partitioned into evaluation keys. M3 loading accepts alternate gold formats. Multiturn evaluation now primes prior turns and evaluates the final turn with policy context. ChangesCapability 4 M3 evaluation integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant M3Evaluator
participant evaluate_task_with_langfuse
participant Agent
M3Evaluator->>M3Evaluator: Build prior HumanMessage and AIMessage history
M3Evaluator->>evaluate_task_with_langfuse: Send history_messages and final intent
evaluate_task_with_langfuse->>Agent: Invoke with primed messages
Agent-->>evaluate_task_with_langfuse: Return final-turn result
evaluate_task_with_langfuse-->>M3Evaluator: Return evaluation response
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
…abilities test_m3_data_loader_loads_bundled_default_matching_registry looped over every task_id declared in the registry YAML, but small_train.zip only ever covered capabilities 2 and 3 (asserted right above the loop). Adding m3_task_4 to the registry for capability_4_multiturn_policy_sampled.zip made the loop assert small_train.zip has capability-4 domains it was never meant to have. - Only check the capabilities loader.available_capabilities() actually returns, not every capability the registry happens to declare Part of #125
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
benchmarks/helpers/sdk_eval_helpers.py (1)
843-876: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGenericReactAgent drops primed history
history_messagesare ignored on the React path becauseGenericReactAgent.invoke()only usesmessages[-1]. Follow-up tasks won’t see the intended conversation context; either thread the full message list into_build_initial_messages()or disable priming for--agent react.🤖 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 `@benchmarks/helpers/sdk_eval_helpers.py` around lines 843 - 876, Update the React execution path around GenericReactAgent.invoke() so history_messages are preserved as conversation context instead of passing or processing only messages_to_send[-1]. Thread the complete messages_to_send list through _build_initial_messages() or disable history priming when the selected agent is react, ensuring follow-up tasks do not claim unsupported primed context.
🧹 Nitpick comments (1)
benchmarks/m3/m3_data_loader.py (1)
220-221: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDefensively check that
gtis a dictionary.While the current capability 4 datasets are well-formed, it's safer to ensure that items in
gt_listare dictionaries before invoking.get(). This prevents anAttributeErrorif a future or malformed dataset containsnullor string entries in the array, aligning with the defensive type-checking used elsewhere in this function.♻️ Proposed refactor
if isinstance(gt_list, list): - gt_by_turn = {gt.get("turn_id", i): gt for i, gt in enumerate(gt_list)} + gt_by_turn = {gt.get("turn_id", i): gt for i, gt in enumerate(gt_list) if isinstance(gt, dict)} for i, _turn in enumerate(turns):🤖 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 `@benchmarks/m3/m3_data_loader.py` around lines 220 - 221, Update the gt_by_turn construction in the gt_list handling to call get only for dictionary entries, while safely ignoring or otherwise handling null and non-dictionary items without raising AttributeError. Preserve the existing turn_id fallback behavior for valid dictionary entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@benchmarks/helpers/sdk_eval_helpers.py`:
- Around line 843-876: Update the React execution path around
GenericReactAgent.invoke() so history_messages are preserved as conversation
context instead of passing or processing only messages_to_send[-1]. Thread the
complete messages_to_send list through _build_initial_messages() or disable
history priming when the selected agent is react, ensuring follow-up tasks do
not claim unsupported primed context.
---
Nitpick comments:
In `@benchmarks/m3/m3_data_loader.py`:
- Around line 220-221: Update the gt_by_turn construction in the gt_list
handling to call get only for dictionary entries, while safely ignoring or
otherwise handling null and non-dictionary items without raising AttributeError.
Preserve the existing turn_id fallback behavior for valid dictionary entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 94680029-61ed-41fa-94a6-e360514dc910
⛔ Files ignored due to path filters (1)
benchmarks/m3/data/capability_4_multiturn_policy_sampled.zipis excluded by!**/*.zip
📒 Files selected for processing (15)
.gitignore.pre-commit-config.yamlLICENSEbenchmarks/helpers/sdk_eval_helpers.pybenchmarks/m3/config/m3_registry_m3_data.yamlbenchmarks/m3/data/NOTICEbenchmarks/m3/data/address.jsonbenchmarks/m3/data/hockey.jsonbenchmarks/m3/data/olympics.jsonbenchmarks/m3/data/olympics_multiturn.jsonbenchmarks/m3/eval_config.tomlbenchmarks/m3/eval_m3.pybenchmarks/m3/m3_data_loader.pybenchmarks/m3/scripts/generate_policy_turn_keys.pyconfig/global.env
💤 Files with no reviewable changes (4)
- benchmarks/m3/data/olympics_multiturn.json
- benchmarks/m3/data/olympics.json
- benchmarks/m3/data/address.json
- benchmarks/m3/data/hockey.json
- sdk_eval_helpers.py: history_messages (dialogue priming) silently dropped every primed turn when the agent is GenericReactAgent - its invoke() only ever reads messages[-1], building one templated prompt rather than a message-list conversation like CugaAgent. Not currently reachable (M3's --agent react routes through the separate eval_m3_react.py, which never calls evaluate_multiturn_task), but this is a shared helper other benchmarks also call. Fail loudly instead of silently dropping context if this combination is ever hit. - m3_data_loader.py: guard gt_by_turn construction against non-dict entries in gt_list, so a malformed/null ground-truth entry raises a clear KeyError-free skip instead of AttributeError on .get() Part of #125
haroldship
changed the title
Collaborator
Author
|
Replying to the CodeRabbit review (both items were addressed in 66735fe; comments were posted body-only because they fell outside the diff, so replying here):
Fixed in 66735fe by taking the "disable priming for react" option:
Fixed in 66735fe exactly as suggested: the Verified at the current tip: |
AnkitaNaik
reviewed
Jul 21, 2026
AnkitaNaik
left a comment
AnkitaNaik
left a comment
Collaborator
There was a problem hiding this comment.
Specific questions listed around the implementation in eval_m3.py
| "response": _stringify_gt_answer(answers[0]) if answers else "", | ||
| "tool_calls": gold_seq[0] if gold_seq else [], | ||
| "tool_responses": tool_resps[0] if tool_resps else [], | ||
| "response": _stringify_gt_answer(answers[-1]) if answers else "", |
Collaborator
There was a problem hiding this comment.
This comment would keep only the last element of the answer list "response": _stringify_gt_answer(answers[-1]) whereas the whole answer should be retained
| "tool_calls": gold_seq[0] if gold_seq else [], | ||
| "tool_responses": tool_resps[0] if tool_resps else [], | ||
| "response": _stringify_gt_answer(answers[-1]) if answers else "", | ||
| "tool_calls": gold_seq[-1] if gold_seq else [], |
Collaborator
There was a problem hiding this comment.
Similarly, if I understand the implementation correctly, the whole tool_calls and tool_responses should be retained. Not sure how this process would work for multi-hop queries where more than 1 tool call is expected as a ground-truth tool call.
Collaborator
Author
|
needs update and merge |
This was referenced Aug 26, 2026
Collaborator
Author
|
Splitting into two focused PRs:
Also filed #174 for a regression the legacy-file removal introduces The |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Feature Pull Request
Related Issue
Closes #125
Description
M3 only had one small dataset (
small_train.zip) and no capability_4 data at all, so there was no way to run the M3 (Vakra) benchmark against capability_4. On top of that, the eval loop couldn't test real follow-up questions: for a multi-turn sample it re-invoked the agent on every turn in sequence, starting from nothing each time. That means a "follow-up" question was never actually a follow-up — the agent had to re-solve earlier turns whose answers were already known, instead of treating them as established context and answering only the new question.This PR adds the missing dataset and fixes that gap:
capability_4_multiturn_policy_sampled.zip(300 samples, VAKRA-licensed the same way assmall_train.zip), and removes four old JSON files frombenchmarks/m3/datathat had no documented source or license.--eval-keyshortcuts (single-turn, multi-turn, with-policy, without-policy) for any M3 zip, and uses it for this dataset.m3_task_4entry to the registry config — without it,--capability m3_task_4couldn't find any containers even though the container itself worked fine.additional_instructions) into the agent for that task only.output/sequence, while the loader only recognizedsmall_train.zip'sground_truth/gold_sequencenames, so the correct-answer data for scoring was silently empty. Fixed to accept both.test_m3_data_loader_loads_bundled_default_matching_registry) that broke as a side effect of addingm3_task_4to the registry — it was checking every capability in the registry againstsmall_train.zip, which never covered capability 4 in the first place. Scoped it to only check the capabilitiessmall_train.zipactually has.Test & Results
Ran the same task 5 times in a row with
compare.sh, to see whether the follow-up question could actually be answered correctly and how consistently:Result: 2 out of 5 runs passed (Pass@1 = 40%, pass@5 = 100% — at least one clean pass).
Checked the raw trace on a passing run to confirm this wasn't just "the plumbing runs without crashing" — the earlier turn's answer really does show up as prior assistant history, the live turn's message carries the per-task policy text, and the agent calls the real domain tool (
books_get_order_dates_by_customer) with the correct arguments and gets back the result that matches the gold answer exactly. The 3 failing runs are ordinary model misses on this task, not the dataset/priming code doing anything wrong — before the registry fix (#124) was merged into this branch, every run failed the same way regardless of the question, because the agent had no tools at all.Type of Changes
Testing
just lint,pytest benchmarks/m3/tests/ -m sanity)Documentation
Checklist
Summary by CodeRabbit
New Features
Documentation
Bug Fixes