Repository navigation
fix(core): process streaming inputs once - #1008
xihaian251 wants to merge 2 commits into
Conversation
`stream_result` and `astream_result` computed the processed action inputs before checking whether the halt action is streaming, and then delegated to `_step`/`_astep` for the non-streaming case. Both of those process the inputs again, so a dependency declared as `__context` or `__tracer` was constructed twice per step, and the object built by the streaming entry point was thrown away. Move the `_process_inputs` call into the streaming branch. The non-streaming path already delegates to a function that processes inputs, so it now processes them exactly once. Ordering on the streaming path is unchanged: `_process_inputs` still runs before `pre_start_stream`.
Brings in the two upstream commits since 6741dac: - 200ab86 ci: fix main after ubuntu-latest moved to 26.04 - 222b32c build: drop support for Python 3.9 (apache#900) Neither touches burr/core/application.py or the new test file. Required so the CI matrix no longer schedules Python 3.9 on ubuntu-latest, where setup-python has no 3.9 build (the four 3.9 jobs failed on the base commit).
|
Updating this PR to current CI here is gated, not failing. On head
Could a maintainer approve those two runs? Only Why the branch just moved. The previous head was based on The Re-verified on the merged head against current
Two things I want to flag rather than have you find them:
The PR description has been refreshed to the current baseline, so the numbers above are reproducible from |
|
Hi maintainers,
|
|
Superseded by #1018 — flagging so this does not sit in the queue unnecessarily; I am not closing it in case a maintainer would rather take the narrower patch. This PR removes the duplicate I verified the coverage using this PR's own test rather than my own:
|
stream_result()andastream_result()process the action inputs twice when the action matched byhalt_afteris not a streaming action: they buildaction_inputsbefore branching, then delegate to_step()/_astep(), which process the same inputs again. An injected dependency such as__contextor__traceris constructed twice per step, and the instance built by the streaming entry point is discarded.Fixes #1007
Changes
burr/core/application.py(+8 / −2): move the_process_inputs()call into the streaming branch in bothstream_result()andastream_result(). The non-streaming path already delegates to a function that processes inputs, so it now processes them exactly once. On that pathaction_inputswas previously computed and then unused —_step()/_astep()receive the rawinputs.tests/core/test_stream_input_processing.py(+76): one parametrised regression test asserting the__contextfactory runs exactly once for both entry points.Ordering on the streaming path is unchanged:
_process_inputs()still runs beforepre_start_stream, and the generators receive the sameaction_inputsas before.Current state of the branch
Head
902c27deis a normal merge ofmain@222b32c3into the original commit4479bbbd— no history rewrite, no force-push, no rebase. The diff against currentmainis still exactly the two files above, and the patch content is identical to what was first submitted.Integrating
mainwas necessary rather than cosmetic. The base commit's matrix still scheduled Python 3.9, whichactions/setup-pythoncannot install now thatubuntu-latestis 26.04; on6741dac2the Build Burr run40639768734fails on exactly the four(3.9)jobs and passes every3.10/3.11/3.12job. Upstream fixed that in200ab860(pin toubuntu-24.04) and222b32c3(drop 3.9,requires-python >=3.10). Those four jobs no longer exist on this head.CI status
Both substantive workflows are gated behind first-time-contributor approval and have not run —
action_requiredis not a test result:41300255322action_required, 0 jobs41300255145action_required, 0 jobs41300256236startup_failure— pre-existing and repo-wide, see NotesHow I tested this
Re-verified on this head against current
main(222b32c3), Python 3.13.1, pytest 9.2.0 / pytest-asyncio 1.4.1, and repeated with pytest 9.1.1 / pytest-asyncio 1.4.0 (same outcome both times):The 9 remaining failures are the same set in both trees — all
tests/core/test_graphviz_display.py, caused by no graphviz binary in this environment; they fail identically on unpatchedmain, and CI installs the system dependency. No test other than the two new ones changes state.Behaviour measured directly, not inferred from the diff: a 15-scenario probe that wraps
_process_inputs, records the four lifecycle hooks and captures log records, run unchanged against both trees and diffed:stream_result,astream_result,stream_iterateandastream_iterate, sync and async (2 → 1);step()/astep()stay at 1;pre_run_step → process_inputs → pre_start_stream → stream_run → post_run_step → post_end_stream;pre_run_step/post_run_step/pre_start_stream/post_end_streamcounts match before and after across all 15 scenarios;__-prefixed inputs and missing required inputs, sync / async / streaming).black --line-length=100,isort --profile black --line-length=100 -p burr(with the repo's--known-local-folderargs) andflake8(reposetup.cfg) pass on both changed files, using the versions pinned in.pre-commit-config.yaml(23.11.0 / 5.12.0 / 6.1.0).git diff --checkis clean.Notes
mainas it stands. A missing-required-input error raised throughastream_result()additionally emits the standardastep()ERROR log line, because the error now surfaces from inside_astep()'stry(whose handler callslogger.exception) rather than fromstream_result()'s own_process_inputs()call. Exception type, message and hook sequence are unchanged, and the synchronous paths are unaffected. Measured on the four combinations:main0 → 1,#9950 → 0,#10000 → 1 (the extra line only disappears on#995, which moves_process_inputs()ahead of thetryblock).4479bbbdonto7a58bba5(fix(core): process and validate inputs before pre_run_step in astep (#979) #995) ande323c836(fix(core): only end a stream that was actually started #1000) applies cleanly, the regression test passes 2/2 on both, and each branch'stests/coregains exactly its own two tests (430 → 432 and 426 → 428 passed, same 9 graphviz failures). The duplicate processing is still present on both of those heads, so this is not superseded by either.pre_run_stepreceives, and it does not touch thepost_end_streampairing.documentationstartup_failureis not related to this change. This PR modifies no file under.github/(thedocs.ymlblob on this head is identical to the one onmain). That workflow fails before starting a single job for every branch: of its 917 retained runs, the last 684 consecutive ones are allstartup_failure, spanning 2026-03-21 to today — including 161pushruns onmainand 523pull_requestruns — and the last non-startup_failurerun is a success onmainat9da8f294on 2026-03-19. The currentmainhead222b32c3and the base of this PR,6741dac2, both fail it too. The workflow file is byte-identical between that last success and the first failing run (573046ac), so no edit to it caused this; all four referenced actions resolve; andBuild Sphinx Documentationpasses onmain, so docs building itself is healthy. GitHub publishes no reason for astartup_failure(0 jobs, logs unavailable), so this is an infrastructure matter — I would rather open a separate issue for it than fold it into this PR.app._dependency_factory["__context"], and that mapping is private.ApplicationBuilderexposes no public way to install a dependency factory, so there is no supported handle for making the duplication observable; no other test intests/uses it. If the mapping is ever renamed, this test breaks even though the fix stays correct. If you would prefer a different observable, I will change it.Checklist
Disclosure: this patch and the accompanying test were prepared with AI assistance, then reviewed and verified by me. The results quoted above under "How I tested this" and "Notes" come from actual local runs against the merged head
902c27deand currentmain222b32c3(Python 3.13.1, pytest 9.2.0 and 9.1.1); the original submission was measured on6741dac2and its numbers still reproduce on currentmain. I can provide the full pytest output or re-run any step on request.