https://docs.all-hands.dev/ logo
Join Slack
Powered by
Re: creating utils to dedup sync and async apis :t...
# proj-agent
q
Re: creating utils to dedup sync and async apis 🧵 cc @calm-artist-7206 @chilly-france-44439 @modern-fireman-99755 @fierce-account-55183
https://github.com/OpenHands/software-agent-sdk/pull/3353 One approach is to use shared utils function, its straight forward to see The problem, however, is that we get the pattern where the sync/async API is sandwiched between the utils. We get a bunch of util bloat
Copy code
<pre-call-shared-util>
<sync/async call e.g acompletion()>
<post-call-shared-util>
https://github.com/OpenHands/software-agent-sdk/pull/3354 Alternatively we can use a generator pattern, which was authored by Tim to dedup heavily. But its an unconventional pattern
c
As of now, I think the utils are still reasonable, and they may make sense even if they didn't solve this problem... at least at a quick look, it didn't seem suspicious to me 🤔
Does it feel already like too much?
m
Strategy pattern? But i should look into it more in details actually
I did a brief discussion with OH about that
Copy code
| Dimension              | A. Sans-IO (generator+driver)   | B. Async-canonical + sync shim   | C. Extract pure helpers        |
|------------------------|----------------------------------|----------------------------------|--------------------------------|
| What varies            | The I/O transport                | The execution model              | Nothing — shared pure helper   |
| Already in repo?       | Yes — RemoteWorkspaceMixin       | Yes — AsyncExecutor (ACP/MCP)    | Yes — RetryMixin, ParallelExec |
| Lines eliminated       | ~65 (LLM transport + MCP)        | ~700 (LLM, Agent, Convo, Cond.)  | ~140 (dispatch, title, utils)  |
| Subclass back-compat   | Safe — internal only             | Risky — needs __init_subclass__  | Safe — internal only           |
| Perf impact            | None                             | One thread-hop per top-level run | None                           |
| pyright friendliness   | Awkward (tagged-union yields)    | Clean                            | Cleanest                       |
| Cancellation / Ctrl-C  | Trivial — driver controls it     | Needs portal-cancel plumbing     | Trivial                        |
| Stack traces           | Slightly worse                   | Worse (portal thread frames)     | Identical to today             |
| Testability            | Best — drive with fake transport | Same as today                    | Same as today                  |
| Where it shines        | LLM transport, HTTP, MCP         | Big duplicated control loops     | Small pairs (one-line diff)    |
| Where it's wrong       | Non-I/O orchestration            | Tiny methods; nested event loops | Bodies that aren't ~1 line off |
| Effort to land         | Medium (1 contained PR)          | High (3 PRs + shim)              | Low (mechanical)               |
| Reversibility          | High                             | Medium (public API shift)        | High                           |
| Risk                   | Low                              | Medium–high                      | Very low                       |
b
I read both PRs. They solve different layers of the same problem and I think they're complementary, not competing. #3353 (shared utils): extracts non-async prep/postprocess into helpers. Sync and async become thin wrappers: shared prep → sync-or-async I/O → shared postprocess. Works across 6 files (LLM, agent, condenser, conversation). Boring, correct, broadly applicable. -254 net lines. #3354 (generator): the 
_run_loop()
generator owns all the control flow (status transitions, stuck detection, iteration cap, error handling) and yields once at the agent step. 
run()
feeds it 
agent.step()
, 
arun()
feeds it 
await agent.astep()
. Elegant, and the codebase already has this pattern in 
RemoteWorkspaceMixin
. -78 net lines. The generator is great specifically for run/arun because there's exactly one yield point — the agent step. All the hairy loop logic lives in one place. But it doesn't generalize well to functions with multiple async call sites (like the LLM layer where you have prep → transport call → process response → telemetry). So I'd say: generator for the conversation run loop (#3354), shared utils for everything else (#3353). The two PRs don't conflict. One nit on #3353: 12 new internal helpers in the LLM class is a lot of names to hold in your head. The pattern is right, but some of those could probably be grouped or simplified. 🐾
2 Views
Open in Slack
PreviousNext
Powered by