Repository navigation
Run schema-aligned parsing as VM natives - #5119
Merged
Merged
Conversation
SAP's three ops were sys ops although they do no IO. As sys ops each parse received T as an owned copy, built its model from definition tables the engine copied into the per-call SysOpContext, and returned a BexExternalValue tree the engine landed on the heap by looking every class and enum up by name. They returned SysOpOutput::Ready, so they already ran inline under the heap permit. _new_parse_cache<T>, _parse_final, and _parse_partial are now $rust_function natives (bex_vm/src/package_baml/sap.rs). The model is built from the declarations T reaches on the heap (reachable::all_declarations) through the unchanged TypeCtx::new; it holds tagged names only, since RustData is a GC leaf. Results are allocated directly on the heap. Error behavior is unchanged. A @Skip field no longer breaks a parse: the parser drops it and the heap landing required every field. The native fills it with its type's empty value. Supporting changes: bex_vm::definitions is the one class/enum definition reader (the engine's five readers disagreed on @Skip); BexVm::rust_data_field replaces copies in regex.rs and csv.rs; reachable::all_nominals filters the new all_declarations. Deleted: the SAP sys ops, CompiledSapModel::from_sys_op_context, TypeCtx::for_target, and the value half of to_external (now to_baml_ty). Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
⏭️ 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):
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7293ce3. Configure here.
# Conflicts: # baml_language/crates/bex_sap/src/sap_model/convert.rs
A 13 KB reply of 200 items: a final parse allocates about 665 KB (1.15 MB before the natives), and the same reply streamed in 52 batches allocates about 32.3 MB (45.4 MB before), since the parse no longer builds an external copy of each value and lands it by name. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Contributor
|
Found 3 test failures on Blacksmith runners: Failures
|
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light.svg)
# Conflicts: # baml_language/crates/bex_sap/src/sap_model/convert.rs
Binary size checks failed❌ 2 violations · ✅ 1 passed
Details & how to fixViolations:
Add/update baselines:
[artifacts.bridge_wasm]
file_bytes = 26074945
gzip_bytes = 7403159
[artifacts.packed-program]
file_bytes = 38460792
gzip_bytes = 15616477Generated by |
The parse model and the per-parse declaration map followed every head a declaration holds, including @Skip fields, which the parser drops, and field templates. Types only a skipped field names now cost a parse nothing; with 400 such classes the walk added 8.65 MB per call. reachable::declarations_reached takes the heads to follow, and all_declarations is it with every head. Co-Authored-By: Claude Opus 5.5 <[email protected]>
5 tasks done
The $rust_function directives in sap.baml moved baml.sap.parse and parse_type down four lines. Co-Authored-By: Claude Opus 5.5 <[email protected]>
6 tasks done
meefs
pushed a commit
to meefs/baml
that referenced
this pull request
Oct 3, 2026
… match arms (BoundaryML#5113) ## Issue Reference No issue filed. Found by heap-profiling an LLM call after BoundaryML#5111 (now merged). **This branch also carries BoundaryML#5119** ("Run schema-aligned parsing as VM natives"), which was merged into it: SAP as `$rust_function` natives, the shared definition reader, the `@skip` fix, and the parse memory guards. See BoundaryML#5119 for its description and measurements. ## Changes After BoundaryML#5111, one small LLM call still allocated 3.4 MB, and **each prompt message added 1.56 MB** (debug build). A heap profile (dhat) put 77% of it in runtime `match` type tests (`IsType` / `NarrowBind` → `value_matches_template` → `normalize::is_subtype`): - `openai.internal._prune_nulls` (and the Bedrock copy) runs once per node of the request JSON and tests `map<string, baml.json.json>`, then `baml.json.json[]`. - A hit is cheap (the `sub == sup` fast path). A **miss** is not: `heads_definitely_differ` decided only same-variant nominal pairs, so a string or a list tested against `map<string, json>` canonicalized both sides and expanded the recursive `json` alias through the μ automaton: about 187 KB per string, 377 KB per list. Two fixes: 1. **Cross-category refutation in the type algebra** (`baml_type/src/normalize.rs`). `heads_definitely_differ` and its interned twin now also return true when both heads have a `Category` and the categories differ, so all five entry points (`is_subtype`/`equivalent`, the interned free functions, `InternedCanonicalCache`) reject those pairs before canonicalizing. Heads that canonicalization can rewrite or that a context-driven rule relates across kinds — alias, union, projection, interface, type variable, inference variable, `unknown`, `never`, error — have no category and are never refuted. A literal and its base, and an enum variant and its enum, share a category. This follows TYPE_SYSTEM.md §Concrete Types (no concrete type is a subtype of another); interface membership, associated types, and type variables stay with the full algebra. The `Ty` and interned category tables come from one `macro_rules!` table. 2. **Tag test for narrowing container arms** (`baml_compiler2_mir/src/lower.rs`). MIR already emits a coarse `IsTypeTag` when the scrutinee's static type proves it suffices (`parametric_arm_tag_sufficient`), and a `json` parameter is already unfolded to `… | json[] | map<string, json>`. But a `let x: T` arm tests a *snapshot* of the scrutinee, and the guard compared against the match local only, so the shortcut never fired for narrowing arms. `MatchScrutinee` now records snapshot copies, and both the container-arm gate and the switch gate ask whether the tested local holds the scrutinee. An element-specific arm (`let xs: int[]` on a `json` scrutinee) stays structural. | Measure (debug build) | BoundaryML#5111 | this PR | | --- | --- | --- | | Bytes allocated per small LLM call | about 3.4 MB | about 0.70 MB | | Extra bytes per call per prompt message | about 1.56 MB | about 35 KB | ## Testing - [x] `baml_type` unit tests: an oracle over a 37-kind corpus (every `Ty` variant, plus facts that make the interface, type-variable, projection, and recursive-alias rules fire) checks that `is_subtype` and `equivalent` give the canonical relation's verdict for every pair, on the plain and the interned paths. Cost tests count the context's `alias_def` lookups and assert that cross-category pairs never ask for one (they failed before the change). A test pins that rewritable and context-driven kinds are not refuted. - [x] MIR tests: `json_container_arms_test_only_the_runtime_tag` (failed before) and `json_scrutinee_keeps_element_specific_arms_structural`. - [x] Corpus tests in `ns_json_alias/json_alias.baml`: every json kind reaches its own arm, and a recursive null-pruning function over nested json gives the right result. - [x] Regression test: `llm_call_memory.rs` `call_cost_does_not_grow_with_conversation_length` compares a 1-message and a 21-message prompt and bounds the bytes per extra message at 128 KiB (it failed at 1,559,878). The per-call bound drops from 8 MiB to 2 MiB. - [x] CI suites, locally: the general job passed 4,891 tests. The snapshot job passed 1,595; its 12 CLI-driven tests (`baml_corpus`, `pack_e2e`, `type_quiz`) stopped only at the local agent-skill check and pass with `BAML_AGENT_SKILL_CHECK=off` (14 of 14, including `type_quiz_conformance` and the new corpus tests). No snapshot changed. - [x] `cargo clippy` on the changed crates is clean. ## Additional Notes - Not done: caching the canonical form of constant type templates in the VM. After these two fixes the type tests cost about nothing in an LLM call, and a cache would hold `NormalTy<TypeHead>` (no GC head walk) built from facts that runtime impl registration can change. - Soundness of fix 2 rests on the same invariant the existing gate uses: a value's runtime container type matches its static type. `let j: json = xs` with `xs: int[]` is a type error, so a list in a `json` slot is stamped `json[]`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes type subtyping fast paths and MIR match lowering (soundness-sensitive) plus the SAP/LLM parse pipeline; broad test and allocation guards reduce regression risk. > > **Overview** > Cuts **LLM-call allocation** by fixing two hot paths and moving **schema-aligned parsing (SAP)** onto the VM. > > **Type tests on `json`:** Failed runtime `match` checks used to **canonicalize the recursive `json` alias** on every miss (e.g. when pruning request JSON). `heads_definitely_differ` now **rejects cross-category pairs** before canonicalization. MIR **`MatchScrutinee`** tracks narrowing-arm snapshots so **`json[]` / `map<string, json>` arms** can use coarse **LIST/MAP tag tests** when the scrutinee is statically `json`, while arms like `int[]` stay structural. > > **SAP:** Internal SAP entry points are **`$rust_function` VM natives** (not sys ops). Parsing builds a model from **declarations the target reaches**, writes results **directly on the heap** (no external value tree), and fills **`@skip` fields** with type empty values. Definition extraction is centralized in **`bex_vm::definitions`**. > > **Tests:** Corpus and MIR coverage for json container matching; SAP skip-field behavior; **`llm_call_memory`** bounds per-call bytes, per-message growth, large/streamed parses, and types only referenced by skipped fields. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 3691a2f. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added schema-aligned parsing that fills missing skipped fields with appropriate empty values. * **Bug Fixes** * Improved JSON matching so list and map patterns distinguish containers from scalars while preserving structural checks for typed collections. * Improved type comparisons to reject incompatible concrete types without disrupting checks that depend on aliases or context. * **Performance** * Tightened the per-call allocation limit for LLM calls and added limits for allocation growth as conversations get longer. * **Tests** * Added coverage for nested removal of null-valued JSON entries, JSON parsing and stringifying, skipped fields, and allocation limits. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5.5 <[email protected]>
This branch was successfully deployed
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
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.
Issue Reference
No issue filed. Stacked on #5113 (which is stacked on #5111): it removes
TypeCtx::for_targetfrom #5111 and extendsllm_call_memory.rs. Retarget tocanaryonce those merge.Changes
Schema-aligned parsing (SAP) was three sys ops (
$rust_io_function) although it does no IO: beforespawn, sys ops were the only way to get concurrency. As sys ops, each parse:Tas an owned copy (SapTy), and the engine copied the class and enum tables into the per-callSysOpContext(twice more for runtime-built types);TypeCtx::for_target);BexExternalValuetree that the engine landed on the heap by looking every class and enum up again by name.The SAP ops already returned
SysOpOutput::Ready, so they ran inline on the VM task under the heap permit. Moving them into the VM changes nothing about what they block.This PR makes them VM natives (
$rust_function,bex_vm/src/package_baml/sap.rs):_new_parse_cache<T>readsTfrom the call's type arguments, walks the declarations it reaches (reachable::all_declarations), reads each with the shared definition reader, and builds the sameCompiledSapModelthrough the unchangedTypeCtx::new/build_db/convert_ty. The model stays in the_ParseCache<T>RustData(one model per cache, only the typesTreaches); it holds tagged names, never heap pointers, becauseRustDatais a GC leaf. The instance now carries its realT._parse_final/_parse_partialrun jsonish and the coercer as before and allocate the result directly on the heap, mapping the model's tagged names back to the declarations the target reaches. Error behavior is unchanged: a final failure isbaml.errors.LlmClientwith the same messages, a partial that does not parse yet is_NoYield.Supporting changes:
bex_vm::definitions):class_definition/enum_definitionreplace the engine's three class readers and two enum readers, which disagreed on@skip(the runtime-type overlay dropped skipped fields; the program table and the runtime-package overlay kept them flagged). Every consumer (output-format rendering,json.schema, SAP) already filters on the flag, so the reader keeps skipped fields flagged and drops skipped enum variants.reachable::all_declarations: every declaration a type reaches, aliases included;all_nominalsnow filters it, and the walk uses aHashSetinstead ofVec::contains.BexVm::rust_data_field: one helper for "clone theArcout of a$rust_typefield", replacing the copies inregex.rsandcsv.rs(which also reported a misleadingMissingNativeFunction).sys_ops/src/sap.rsand the SAP sys-op impls,CompiledSapModel::from_sys_op_context,TypeCtx::for_target/reachable_definitions, and the value half ofbex_sap::to_external(the type half remains, renamedto_baml_ty).bex_sapno longer depends onbex_external_types;sys_opsno longer depends onbex_sap.Bug fixed:
@skipfieldsParsing into a class with a
@skipfield failed: SAP drops the field (the prompt never shows it), and the heap landing required every field (Missing field \age` in external Instance for class `user.Person`). The native fills a skipped field with its type's empty value:nullfor nullable types,0/0.0/0n,false,""`, an empty list or map, a literal's own value, an enum's first variant, and a class of its fields' empty values. A skipped field whose type has none (media, functions, a self-containing class) panics with a message naming the field.Memory
Measured with the counting allocator in
llm_call_memory.rs(debug build). The gain grows with the size of the parsed output, because the parse no longer builds an external copy of the value and lands it by name.Item { id, name, tags }[])baml.sap.parse<Item[]>)_ParseCache(asai.stream.Streamdoes)(SAP's own bytes: the totals minus a baseline function doing the same work without parsing, about 4-6 KB.)
The streamed case is still large because every batch reparses the whole text so far; incremental parsing is not in this PR.
Testing
bex_sapunit tests unchanged and passing (they buildTypeRefDbdirectly).bex_vmunit tests:definitionskeeps skipped fields flagged and drops skipped variants.sap_parse_fills_skipped_fields_with_empty_values(failed before with the missing-field error).baml_builtins2_codegen:test_sap_parse_is_a_vm_builtinreplaces the IO-builtin assertion for_parse_final.llm_call_memory.rs:large_parse_allocates_little(limit 900 KiB) andstreamed_parse_allocates_little(limit 40 MiB). Both limits sit between the numbers above, so a return to the external-copy path fails them.bex_sap,bex_vm,baml_builtins2_codegen,sys_ops, and thebaml_testslibrary plusllm_call_memoryandreflect_call_any(1616 passed); the corpus namespaces that parse (parse_companions,generic_union_returns,streaming_partial_parse,stdlib_runner_stream_batching,runtime_type_bindings_phase1_wrapped,runtime_identity_seams,provider_stdlib, and everyllm_*namespace: 498 passed). CI runs the rest.prekpasses (cargo fmt, cargo clippy, cargo stow, baml fmt).Additional Notes
YieldToCallcontinuation pathbaml.json.to<T>uses), and movingctx.output_format()/baml.json.schema, the last users of theSysOpContexttables, to natives.StreamStateTystill has no production constructor; left as is.🤖 Generated with Claude Code
Note
Medium Risk
Touches core LLM output parsing and heap allocation paths; behavior is intended to match prior SAP errors but execution moved from sys ops to VM with definition-walk semantics changes.
Overview
Schema-aligned parsing (SAP) moves from async sys ops to VM natives (
baml.sapuses$rust_functionwith//baml:mut_vm). Parsing no longer copies definition tables intoSysOpContext, builds models viaTypeCtx::for_target, or returnsBexExternalValuefor a second heap landing—_new_parse_cache<T>walks only declarationsTreaches on the heap, and_parse_final/_parse_partialallocate results directly.Supporting cleanup: shared
bex_vm::definitionsfor class/enum tables (skipped fields stay flagged),reachable::declarations_reached,BexVm::rust_data_field, removal ofsys_opsSAP andbex_sap::to_external(types live into_baml_ty).@skipfields are filled with type empty values instead of failing parse/landing. New corpus and allocation guards inllm_call_memory.rslock in lower parse memory (~42% on large final parses).Reviewed by Cursor Bugbot for commit e82af14. Bugbot is set up for automated code reviews on this repo. Configure here.