Skip to content

fix(core): process streaming inputs once - #1008

Open
xihaian251 wants to merge 2 commits into
apache:mainfrom
xihaian251:f2-stream-inputs-once
Open

xihaian251 wants to merge 2 commits into
apache:mainfrom
xihaian251:f2-stream-inputs-once

Conversation

@xihaian251

@xihaian251 xihaian251 commented Oct 29, 2026 •

Copy link
Copy Markdown

stream_result() and astream_result() process the action inputs twice when the action matched by halt_after is not a streaming action: they build action_inputs before branching, then delegate to _step() / _astep(), which process the same inputs again. An injected dependency such as __context or __tracer is 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 both stream_result() and astream_result(). The non-streaming path already delegates to a function that processes inputs, so it now processes them exactly once. On that path action_inputs was previously computed and then unused — _step() / _astep() receive the raw inputs.
  • tests/core/test_stream_input_processing.py (+76): one parametrised regression test asserting the __context factory runs exactly once for both entry points.

Ordering on the streaming path is unchanged: _process_inputs() still runs before pre_start_stream, and the generators receive the same action_inputs as before.

Current state of the branch

Head 902c27de is a normal merge of main @ 222b32c3 into the original commit 4479bbbd — no history rewrite, no force-push, no rebase. The diff against current main is still exactly the two files above, and the patch content is identical to what was first submitted.

Integrating main was necessary rather than cosmetic. The base commit's matrix still scheduled Python 3.9, which actions/setup-python cannot install now that ubuntu-latest is 26.04; on 6741dac2 the Build Burr run 40639768734 fails on exactly the four (3.9) jobs and passes every 3.10 / 3.11 / 3.12 job. Upstream fixed that in 200ab860 (pin to ubuntu-24.04) and 222b32c3 (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_required is not a test result:

Workflow Run Status
Build Burr 41300255322 action_required, 0 jobs
Release Validation 41300255145 action_required, 0 jobs
documentation 41300256236 startup_failure — pre-existing and repo-wide, see Notes

How 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):

$ pytest tests/core/test_stream_input_processing.py -q
main + this test file, without the fix:  2 failed  (assert 2 == 1, both parametrisations)
this branch:                             2 passed

$ pytest tests/core -q
main:                                   9 failed, 425 passed
main + this test file, without the fix: 11 failed, 425 passed
this branch:                            9 failed, 427 passed

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 unpatched main, 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:

  • the dependency is constructed exactly once on the non-streaming halt path for stream_result, astream_result, stream_iterate and astream_iterate, sync and async (2 → 1); step() / astep() stay at 1;
  • streaming event order is identical on both trees: 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_stream counts match before and after across all 15 scenarios;
  • exception types and complete messages match in all 6 error cases (reserved __-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-folder args) and flake8 (repo setup.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 --check is clean.

Notes

  • One observable side effect, only relative to main as it stands. A missing-required-input error raised through astream_result() additionally emits the standard astep() ERROR log line, because the error now surfaces from inside _astep()'s try (whose handler calls logger.exception) rather than from stream_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: main 0 → 1, #995 0 → 0, #1000 0 → 1 (the extra line only disappears on #995, which moves _process_inputs() ahead of the try block).
  • Lifecycle hooks are unchanged, as measured above.
  • Composes with fix(core): process and validate inputs before pre_run_step in astep (#979) #995 and fix(core): only end a stream that was actually started #1000. Cherry-picking 4479bbbd onto 7a58bba5 (fix(core): process and validate inputs before pre_run_step in astep (#979) #995) and e323c836 (fix(core): only end a stream that was actually started #1000) applies cleanly, the regression test passes 2/2 on both, and each branch's tests/core gains 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.
  • Independent of astep() passes unprocessed inputs to pre_run_step hooks #979 and astream_result() masks action exceptions with a tracker KeyError when the action is non-streaming #999. This does not change what pre_run_step receives, and it does not touch the post_end_stream pairing.
  • The documentation startup_failure is not related to this change. This PR modifies no file under .github/ (the docs.yml blob on this head is identical to the one on main). That workflow fails before starting a single job for every branch: of its 917 retained runs, the last 684 consecutive ones are all startup_failure, spanning 2026-03-21 to today — including 161 push runs on main and 523 pull_request runs — and the last non-startup_failure run is a success on main at 9da8f294 on 2026-03-19. The current main head 222b32c3 and 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; and Build Sphinx Documentation passes on main, so docs building itself is healthy. GitHub publishes no reason for a startup_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.
  • One caveat about the test, stated plainly so it is not a surprise to a reviewer: counting the constructions requires replacing app._dependency_factory["__context"], and that mapping is private. ApplicationBuilder exposes no public way to install a dependency factory, so there is no supported handle for making the duplication observable; no other test in tests/ 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

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered
  • Any change in functionality is tested
  • New functions are documented — N/A, no new functions
  • Placeholder code is flagged / future TODOs are captured in comments — N/A
  • Project documentation has been updated if adding/changing functionality — N/A, this restores the documented behaviour rather than changing the API

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 902c27de and current main 222b32c3 (Python 3.13.1, pytest 9.2.0 and 9.1.1); the original submission was measured on 6741dac2 and its numbers still reproduce on current main. I can provide the full pytest output or re-run any step on request.

`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`.
@github-actions github-actions Bot added the area/core Application, State, Graph, Actions label Oct 29, 2026
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).
@xihaian251

Copy link
Copy Markdown
Author

Updating this PR to current main and asking for one small thing: workflow approval.

CI here is gated, not failing. On head 902c27de both substantive workflows are action_required with 0 jobs — they have never executed:

Could a maintainer approve those two runs? Only Auto-label (a pull_request_target workflow) has run so far.

Why the branch just moved. The previous head was based on 6741dac2, whose CI matrix still scheduled Python 3.9, which setup-python cannot install now that ubuntu-latest is 26.04. That is not hypothetical: on 6741dac2, Build Burr run 40639768734 fails on exactly the four (3.9) jobs and passes every 3.10 / 3.11 / 3.12 job. So approving the runs on the old head would have produced red for a reason unrelated to this change. I merged main in (200ab860 + 222b32c3) as a normal merge — 4479bbbd is preserved as-is, no rebase, no force-push, and no new content: the diff against main is still burr/core/application.py +8/−2 plus the 76-line test, byte-identical to the patch originally submitted.

The documentation startup_failure is pre-existing and repo-wide — please don't read it as a signal about this PR. Run 41300256236 fails again with 0 jobs, as it does for every branch and for pushes to main too. Of that workflow's 917 retained runs, the last 684 are all startup_failure (523 pull_request + 161 push on main), from 2026-03-21 to today; the last run that was not startup_failure was a success on main at 9da8f294, 2026-03-19. docs.yml is byte-identical (573046ac) between that success and the first failing run, so no edit to the file caused it; all four referenced actions resolve; this PR touches nothing under .github/; and Build Sphinx Documentation passes on main, so docs building itself is healthy. GitHub exposes no reason for a startup_failure (0 jobs, /logs unavailable), so this needs the Actions UI or runner-side logs. I would rather open a separate infra issue for it than fold it into this PR — say the word.

Re-verified on the merged head against current main 222b32c3 (Python 3.13.1; pytest 9.2.0 / pytest-asyncio 1.4.1, then again with pytest 9.1.1 / 1.4.0 — same result both times):

  • the regression test fails 2/2 on main (assert 2 == 1 for both entry points) and passes 2/2 here;
  • tests/core: main = 9 failed / 425 passed → this branch = 9 failed / 427 passed, and the two failure sets are identical — all 9 are test_graphviz_display.py on this machine only (no graphviz binary; CI installs the system dependency). Nothing else changes state;
  • a 15-scenario probe (wrapping _process_inputs, recording the four lifecycle hooks, capturing log records) confirms the dependency is now constructed exactly once on the non-streaming path for stream_result, astream_result, stream_iterate and astream_iterate, sync and async, while streaming event order (pre_run_step → process_inputs → pre_start_stream → stream_run → post_run_step → post_end_stream), all four hook counts, and exception types and complete messages in 6 error cases are unchanged;
  • black / isort / flake8 pass with the repo's own pre-commit pins (23.11.0 / 5.12.0 / 6.1.0) and args; git diff --check is clean;
  • it composes with the sibling work: cherry-picking 4479bbbd onto 7a58bba5 (fix(core): process and validate inputs before pre_run_step in astep (#979) #995) and e323c836 (fix(core): only end a stream that was actually started #1000) applies cleanly, the test passes 2/2 on both, and each branch's suite gains exactly its own 2 tests. The duplicate processing is still present on both of those heads, so this PR isn't superseded by either.

Two things I want to flag rather than have you find them:

  1. One disclosed behaviour nuance. An input-processing ValueError raised through astream_result() now emits one extra astep() ERROR log line, because the error surfaces from inside _astep()'s try instead of from stream_result()'s own _process_inputs(). Exception type, message and hook sequence are unchanged; synchronous paths are unaffected; and on top of fix(core): process and validate inputs before pre_run_step in astep (#979) #995 the extra line does not appear at all.
  2. The test reaches into a private attribute. Counting the constructions requires replacing app._dependency_factory["__context"] — ApplicationBuilder has no public way to install a dependency factory, and no other test in tests/ uses that mapping. The behaviour under test (__context injection, and halting on a non-streaming action) is documented and public, but the measurement handle is not, so the test could break on a rename even though the fix stays correct. If you would prefer a different observable, I will happily change it.

The PR description has been refreshed to the current baseline, so the numbers above are reproducible from main as it stands now. Thanks for your time — and apologies for the notification noise.

@xihaian251

Copy link
Copy Markdown
Author

Hi maintainers,
PR #1008 is now synchronized with the latest upstream main (222b32c3) and is ready for CI validation.
Could a maintainer with write access please approve the two pending fork pull request workflows?

@xihaian251

Copy link
Copy Markdown
Author

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 _process_inputs() call on a non-streaming halt by moving the call below that branch. #1018 moves it above pre_run_step instead, which removes the same duplicate and additionally makes pre_run_step see the processed dictionary (the #979 symptom on stream_result/astream_result, which #980/#995 do not cover).

I verified the coverage using this PR's own test rather than my own:

tests/core/test_stream_input_processing.py on main       2 failed  (stream_result, astream_result)
same test on #1018's branch                              2 passed

#1018 does not touch _step/_astep, so it stays disjoint from #980/#995, and it leaves #1000 untouched. Happy to close this one on request, or to re-open it if maintainers prefer to keep the two concerns separate.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/core Application, State, Graph, Actions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

stream_result()/astream_result() process inputs twice when halting on a non-streaming action

1 participant