fix(config): bound the total size of one rendered value 🤖🤖🤖 - #102
Conversation
event_format bounds rendering per value — max_string, max_length, max_depth — but nothing bounded the total, and those limits multiply rather than compose. At the shipped defaults, max_length ** max_depth is 200**5 leaf slots at up to 10,000 chars each. A list of depth 4 and width 30, inside every FormatConfig limit, rendered 25,102,293 chars. That output goes into the trajectory, and per truncation_config's own docstring event_format "renders every turn for the rest of the run", so the cost recurs. Eviction cannot help: max_event_tokens defaults to None, and even when set, L4 eviction runs at assembly, after the string has been materialised. The guard already existed. pformat's docstring says to use truncating_pformat "preventing OOM on huge objects", and reflexion.py had already reached for it — but with no max_chars to pass, so it took the uncapped branch anyway. The missing piece was a configured budget, not the choice of function. Adds TruncationConfig.max_render_chars (default 50_000, mirroring capture.max_stdout which bounds the analogous raw-text path; None = unlimited) and passes it at the three render sites: generated_code._pformat, codeact_errors._pformat, and reflexion._format_result_for_reflection. The field is on TruncationConfig, not FormatConfig, deliberately. FormatConfig documents its field names as matching pformat()'s kwargs exactly so model_dump() can be splatted straight in, and pformat() accepts no **kwargs — a field there raises TypeError at every splat site, of which there are six for bare pformat plus several more that merge a dump into a kwargs dict. Scope of the bound, stated plainly: this caps the rendered output, not peak allocation. TruncatingStringIO discards overflow as it arrives, but the renderer still walks the whole structure and allocates each chunk before the sink drops it — measured, a value that renders to 72 MB uncapped still peaks near 65 MB with the cap. What it removes is the multi-megabyte string entering the trajectory and being re-sent every turn. Bounding allocation means aborting the traversal once the budget is spent, which belongs inside the renderer, and is left as follow-up. The test module says so rather than implying a stronger guarantee. context_block_format stays unlimited: those values are author-curated and documented as meant to render in full, so the cap is applied at the event/error render helpers rather than globally. Two tests needed a real TruncationConfig instead of a MagicMock. They assert sampling-param forwarding and llm-kwarg rejection, and mocked the config object; a new numeric field made the mock's auto-attribute fail a comparison and mask the error each test was actually about. A real default config is more faithful to what they check. Refs NVIDIA-NeMo#96 Signed-off-by: hotragn <hotragn.pettugani_2024@woxsen.edu.in>
|
@Hotragn, curious: where did you run into this? |
|
Straight answer: I did not hit this in a live run — it came out of reading, and the numbers are from a synthetic fixture. Saying so plainly because I assume you're calibrating how much to care. The path there was #90. Your issue asked me to reuse the existing The thing that moved it from "arithmetic curiosity" to worth filing was For triggers I'd expect in practice, none exotic: a parsed JSON API response, any nested One thing relevant if you're weighing whether operators can already mitigate this: they can't. And the honest limit of this PR, which is in the description but worth repeating since you asked: it bounds the rendered output, not peak allocation. The renderer still walks the whole structure and allocates each chunk before Happy to drop it if you read the risk as too low to be worth a config field — the measurement was cheap and I'd rather you have the number than the patch. 🤖🤖🤖 |
|
@furgalep — honest answer: not from a production OOM. It came out of your own #90. While implementing #90 in #94 (bounding the So this was found by auditing for that pattern, then measured rather than assumed. The 25 MB figure is a synthetic value — depth 4, width 30, every field inside the shipped Two things turned that from a curiosity into something worth fixing. It recurs — The part that convinced me it was a real gap rather than my own misreading: Trail, for what it's worth: I filed #96 with the measurements and a proposed fix on Happy to add the reproducer as a test if you'd want it pinned — I left it out because it allocates ~72 MB to prove a point that the char-count assertion already covers more cheaply. |
What does this PR do?
Bounds the total size of one rendered value.
event_formatbounds rendering per value —max_string,max_length,max_depth— but nothing bounded the total, and those limits multiply rather than compose.At the shipped defaults,
max_length ** max_depthis200 ** 5leaf slots at up tomax_stringchars each. A list of depth 4, width 30 — inside everyFormatConfiglimit — rendered:Two things made that more than a one-off allocation. It recurs: per
truncation_config's own docstring,event_format"renders every turn for the rest of the run". And eviction can't help —max_event_tokensdefaults toNone, and even when set, L4 eviction runs at assembly, after the string is already materialised.The guard already existed.
pformat's docstring says to usetruncating_pformat"preventing OOM on huge objects", andreflexion.py:307had already reached for it — but with nomax_charsto pass, so it took the uncapped branch anyway. The missing piece was a configured budget, not the choice of function.Changes
TruncationConfig.max_render_chars, default50_000,None= unlimited.50_000mirrorscapture.max_stdout, which bounds the analogous raw-text path.generated_code._pformat,codeact_errors._pformat, andreflexion._format_result_for_reflection(a one-line addition there — it was already calling the right function).context_block_formatstays unlimited. Those values are author-curated and documented as meant to render in full, so the cap is applied at the event/error render helpers rather than globally.Why the field is on
TruncationConfigand notFormatConfigFormatConfigdocuments its field names as matchingpformat()'s kwargs exactly somodel_dump()can be splatted straight in, andpformat()accepts no**kwargs:That would fire at six bare-
pformatsplat sites (events.py:235,codeact_errors.py:173,current_call.py:161,generated_code.py:30,predict.py:315,plain_formatter.py:49) plus several more that merge a dump into a kwargs dict. I proposed theFormatConfigroute in #96 first and it was wrong; putting the total budget beside the other cross-cutting totals keeps the sub-configs meaning "per-value structural bounds" and leaves every splat site untouched.Scope of the bound — stated plainly
This caps the rendered output, not peak allocation.
TruncatingStringIOdiscards overflow as it arrives, but the renderer still walks the whole structure and allocates each chunk before the sink drops it. Measured: a value that renders to 72 MB uncapped still peaks near 65 MB with the cap applied.What the cap removes is the multi-megabyte string entering the trajectory and being re-sent every turn — the recurring cost. Bounding allocation as well means aborting the traversal once the budget is spent, which belongs inside the renderer rather than in config, and I've left it as follow-up. My first version of the test asserted a memory bound, failed, and that's how I found this; the test module now documents the limitation instead of implying a stronger guarantee.
Two unrelated tests touched
test_sampling_params_forwarded.pyandtest_method_llm_callable.pymockedruntime.truncation_configwithMagicMock. A new numeric field made the mock's auto-attribute fail a comparison insidetruncating_pformat, which masked the error each test was actually asserting (unexpected keyword argument 'llm'became aMagicMock/intTypeError). Both now useDEFAULT_TRUNCATION_CONFIG, which is more faithful to what they check — neither test is about truncation. Flagging it because "your change edited unrelated tests" is a fair thing to want explained.Related issues
Refs #96 — filed with the measurements and a design question about the default. Opening this as the concrete-diff option I offered there rather than leaving the proposal sitting;
50_000is still the open question, and it's a one-line change if you want a different figure.Checklist
uv run ruff check .anduv run ruff format --check .pass)uv run pytest)Field(description=...), and the reasoning for its placement is in a comment next to itscripts/check_license_headers.py: 882 files OK)Verification
Full CI unit suite on Linux with
uv sync --all-extras --no-extra sandbox: 6568 passed, 4 skipped.ruff check .,ruff format --check ., andscripts/check_license_headers.pyclean.11 new tests in
tests/config/test_max_render_chars.py. They're built so the cap is what does the work rather than the fixture: every fixture stays inside the per-value limits, andtest_none_means_unlimitedasserts the same value renders past 1 MB withmax_render_chars=None, so the pair fails if the cap stops being applied. Also covered: the boundary validator,merge_withcarrying the field, truncation being visible rather than silent, small values rendering untouched, strings still passing through verbatim, andcontext_block_formatstill unlimited.One note on the base: this branch is cut from
2197e61rather than currentmain. My token lacksworkflowscope, and basing on currentmainwould carry upstream'sci.ymlcommits into my fork, which GitHub refuses. No overlap with the files those commits touched, and the suite is green either way — I verified on both bases.🤖🤖🤖