Repository navigation
Python: perf(foundry-hosting): parallelize pre-model storage reads - #8363
Conversation
The non-workflow response handler loaded the caller's input items, the prior conversation history, and the agent session with three strictly-sequential awaits before the model call. These are independent storage round-trips with no data dependency, so on warm/multi-turn requests they added avoidable serial latency to the request critical path. Load them concurrently: the input/history reads are gathered in a new _load_request_messages helper and kicked off (via ensure_future) so they overlap the session load. Result ordering (history precedes input) and the existing error semantics are preserved; the read task is cancelled if session preparation fails. Stateless single-turn requests are unaffected (history and session loads short-circuit without a round-trip). Co-authored-by: Copilot <[email protected]> Copilot-Session: 3d900395-13d0-4698-bf9d-f6670f9e545c
There was a problem hiding this comment.
🟡 Changes recommended
Task cleanup is incomplete on failures and cancellation, allowing orphaned storage operations.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Parallelizes pre-model storage reads to reduce Foundry-hosted agent latency.
Changes:
- Concurrently loads input messages and conversation history.
- Overlaps message loading with session retrieval.
- Attempts to cancel message loading when session preparation fails.
File summaries
| File | Description |
|---|---|
_responses.py |
Adds concurrent request-message loading and session-read overlap. |
Review details
Suppressed comments (1)
python/packages/foundry_hosting/agent_framework_foundry_hosting/_responses.py:792
- This cleanup misses
CancelledErrorandGeneratorExitbecause they are notException. If the request is cancelled or the stream is closed duringsession_storage.get, the newly spawned message-loading task keeps running. Even for an ordinary session error, callingcancel()without awaiting the task returns before its cleanup finishes. CatchBaseException, cancel and drain the task, and retain logging only for ordinary exceptions.
if request_messages_task is not None:
request_messages_task.cancel()
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- _load_request_messages now owns both read tasks and, on any BaseException (including CancelledError/GeneratorExit from gather's first-failure or an outer cancellation), cancels and drains the sibling so no storage read is orphaned. - The helper returns the assembled [*history, *input] list, so the message ordering rule lives only in the helper (per review); the caller no longer reverses the tuple. - Session-prep cleanup now catches BaseException, cancels AND awaits (drains) the in-flight read task, and logs only ordinary Exceptions. - Add TestParallelRequestReads: overlap, message ordering, history-skip when not agent-server history, and sibling-cancellation on a failed read. Co-authored-by: Copilot <[email protected]> Copilot-Session: 3d900395-13d0-4698-bf9d-f6670f9e545c
|
Thanks for the review — addressed in b27b81b:
Note: my local env can't reach PyPI, so the tests were validated via CI rather than locally. |
|
Thanks for the update. Before this is ready, could you please:
Once those are addressed, please re-request review. Thanks! |
…eads Adds a handler-level regression test proving that when session preparation fails, the concurrently-launched input/history read task started by _handle_inner_agent is cancelled and drained rather than orphaned. Completes the no-orphan coverage requested in review (the helper-level read-failure path was already covered). Co-authored-by: Copilot <[email protected]> Copilot-Session: 3d900395-13d0-4698-bf9d-f6670f9e545c
Closes #8365
What
Parallelize the three pre-model storage reads in the foundry_hosting non-workflow response handler (
ResponsesHostServer._handle_inner_agent). Previously the caller's input items, the prior conversation history, and the agent session were loaded with three strictly-sequentialawaits before the model call. They are independent storage round-trips with no data dependency, so on warm / multi-turn requests they added avoidable serial latency to the request critical path.Change
_load_request_messageshelper gathers the input-items and history reads withasyncio.gather.asyncio.ensure_futureright after the stores are built, so it overlaps the session load instead of running after it.Why
A latency waterfall of the warm baseline showed the pre-model reads as a chain of serialized private-endpoint round-trips (~100-260ms each). Overlapping the independent ones removes serial wait on warm / multi-turn requests with no behavior change.
Testing
:stockimage (byte-identical baseline otherwise).Ran a multi-turn load of 500 cycles at concurrency 50, where turn-2 is the measured request (it carries
previous_response_id, so it loads both the session and the prior history — the path this PR changes). At the median (p50), turn-2 time-to-last-byte improved slightly from 2,803 ms on the stock baseline to 2,799 ms with this change (about 4 ms faster). At the 99th percentile (p99), it improved from 5,305 ms to 4,902 ms — roughly 403 ms, or 7.6%, faster.