Skip to content

Refactor async/sync chat#101

Closed
PiotrCzapla wants to merge 4 commits intomainfrom
refactor-async-sync-chat
Closed

Refactor async/sync chat#101
PiotrCzapla wants to merge 4 commits intomainfrom
refactor-async-sync-chat

Conversation

@PiotrCzapla
Copy link
Contributor

I've note some differences between sync, async in this case I was taking async version.
The pr is going to be divided in to few commits, so I can test on each step of refactor, to ensure I don't miss anything

I've changed yield from to for ... in yield, so the diff between async and sync is minimal.
I also have lots of changes, as cachy.jsonl was recreated for some reason, but I fix that once I flesh out the refactor.

BTW: Solveit is quite slow while editing this notebook (especially find in the editor takes a long time), is this only on my side?

Apply the same fix from #68 to AsyncChat._call() method.
Wraps get_model_info() in try-except to register unknown models
before querying their info, allowing AsyncChat to work with new
OpenRouter models not yet in litellm's model registry.
@PiotrCzapla PiotrCzapla force-pushed the refactor-async-sync-chat branch from a41c1dc to 43529c2 Compare January 29, 2026 23:03
@PiotrCzapla PiotrCzapla force-pushed the refactor-async-sync-chat branch from 43529c2 to 6f141f4 Compare January 30, 2026 00:23
@PiotrCzapla
Copy link
Contributor Author

@jph00 if you have a moment could you have a look at this early draft?
The work is incremental, so once it is finished the earlier commits won't change
The way I've extracted async /sync from the _call in the last commit adds complexity,
but It let us split it to more/better helper functions (todo tomorrow). So I hope it will make up for the complexity.

@PiotrCzapla PiotrCzapla requested a review from jph00 January 30, 2026 16:15
@PiotrCzapla
Copy link
Contributor Author

@jph00 It is still a draft, as there are few changes in how the code works now I would like to double check, besides cachy is playing tricks and the ipynb is full of changes (like tool ids, etc) (commit has only code hunks).

The things to fix once we decide if the approach is ok or not:

  • max_steps = 3 means that there will be 3+1 steps max for both AsyncChat and Chat, perviously Chat was doing 3 steps
  • 'pause_turn' - is handled differently, it was a bit tricky to add, and anthropic docs says we should keep it. And I can't get the model to emit this, so I decided to skip it and follow up with person that implemented it,
  • user provided final_prompt, result in message: {'role':'assistant', } - this is how it was before, but it stopped respecting tool_choice='none' and the last message has unexecuted tool request, it works for default _final_prompt as it is {'role':'user', ... }
  • gemini search still does not work
  • cost property does not seems to work (it is in the notebook but it wasn't exported, I exported it by accident)

@jph00
Copy link
Contributor

jph00 commented Jan 31, 2026

Thanks @PiotrCzapla . I don't fully understand all your questions - but yes asking on the AAI discord about pause_turn is a good idea. Try using fastaistyle's chkstyle to see what to change to align with our style guide. That will make it easier to review because I'll be able the see more at a glance. Since cost wasn't exported, just put it back to unexported - I guess it's not ready. I don't understand the final_prompt issue.

@PiotrCzapla
Copy link
Contributor Author

PiotrCzapla commented Feb 2, 2026

I simplified a bit the _call_as_sync /async to take less vertical space, I also added a diff to show how this two adapters differ, but I think the PR will be hard to review anyway, as it pack a lot of changes.
Screenshot 2026-02-02 at 13 41 05

So let me actually split this PR up.

  • I will first use the approach with _call adapters for sync/async, on an unmodified _call, so you see how it can be used to make AsyncChat, without any code duplication.
  • I will show as well examples for potential issues I've found (the final_prompt), so it is easier to talk about.
  • Then I will make another pr that converts tail recursion on _call to approach with for loop - this is easier for me to reason about than a recursion hope you find it useful to.

@PiotrCzapla PiotrCzapla closed this Feb 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants