Parse each LLM candidate once in ai.Agent - #5118
Conversation
_attempt_turn accepted a turn by parsing its terminal text with baml.sap.parse and threw the value away; run then parsed the same text again for the result. The accepting parse now travels with the turn (_AcceptedTurn.text_output, a ParsedOutput), and run uses it, the provider-decoded output, or the turn's media, whichever accepted the turn. On canary a small LLM call allocates 8.5 MB instead of 13.5 MB, and each unrelated class in the program costs half as much per call, because each call builds one parse model instead of two. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe runner now returns an accepted model turn together with any terminal-text parse result. It uses that retained parse during final result construction when provider-decoded output is unavailable. ChangesTurn output acceptance and construction
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Refactor Merge Risk: ⚪ Minimal · up to This change avoids parsing the same LLM output twice and lowers allocation per call. No merge-blocking risk was identified. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🧪 Generate unit tests (beta)
🛠️ Fix failing CI checks 💡
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. A rabbit watched the turn arrive, Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Binary size checks failed❌ 2 violations · ✅ 1 passed
Details & how to fixViolations:
Add/update baselines:
[artifacts.bridge_wasm]
file_bytes = 26057520
gzip_bytes = 7394778
[artifacts.packed-program]
file_bytes = 38454256
gzip_bytes = 15619183Generated by |
This branch was successfully deployed
2 participants
Issue Reference
No issue filed. Found while profiling LLM-call memory (see #5111 and #5113, which this PR does not depend on).
Changes
ai.Agentparsed each accepted candidate twice:_attempt_turndecided whether to accept a turn with_parses, which ranbaml.sap.parse<Out>(candidate)and discarded the value.runthen ranbaml.sap.parse<Out>(candidate)again on the same text to get the result.On
canaryevery parse builds a parse model, so the second parse doubled that cost. This PR keeps the accepting parse:_parsessplits into_fits_without_text<Out>(turn) -> bool?(the provider-decoded output or the turn's media decides;nullwhen only the text can tell) and_parse_text<Out>(candidate) -> ParsedOutput?(the parsed value, wrapped so a parsednullis distinct from no parse)._attempt_turnreturns_AcceptedTurn { turn, text_output }.runtakes the value fromturn.parsed_output ?? accepted.text_output, or builds it from the turn's media. Exactly one of the three accepted the turn, so the duplicated three-way logic inrungoes away._media_valuestill runs inrun, after the turn is on the record, as its doc requires.canary)(Measured with
llm_call_memory.rsfrom #5111. With #5111 the parse model is small, so the saving there is the cost of one parse instead of most of the call.)Testing
baml-cli test --from crates/baml_tests/baml_src): 5082 passed, 2 tolerated (the deliberate test-runner tests). This covers the runner's repair re-ask, provider-decoded outputs, and theimage?/image[]media cases.cargo insta test --test-runner nextest --dnd -p baml_tests -p baml_cli -p baml_pack_host --all-features --unreferenced=reject): 1615 passed, no snapshot changes.runner.bamlisbaml fmt-clean.Additional Notes
No new test asserts the parse count: nothing in the test harness observes how many times SAP runs, and the memory numbers above show the change.
🤖 Generated with Claude Code
Note
Medium Risk
Touches the default agent loop and final output resolution (SAP, provider decode, media), but behavior is covered by the existing BAML corpus and snapshot tests.
Overview
ai.Agentnow parses each accepted terminal reply once instead of runningbaml.sap.parsein_attempt_turnfor acceptance and again inrunfor the final value._parsesis replaced by_fits_without_text(provider-decoded output or turn media vs. needs text;nullwhen only text can decide) and_parse_text(returns a retainedParsedOutput?)._attempt_turnreturns_AcceptedTurnwith optionaltext_outputfrom that single parse.runbuilds the result fromturn.parsed_output ?? accepted.text_output, or_media_valuewhen media satisfied acceptance—removing duplicated three-way branching and the second SAP pass._fits_without_textkeeps media-shaped outputs that have no blocks on the turn on the text-parse path so optional/absent media (image?,image[]) still work instead of failing early.Reviewed by Cursor Bugbot for commit 080e786. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit