Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement asynch iterator for Open AI stream #14920

Conversation

colin-grant-work
Copy link
Contributor

@colin-grant-work colin-grant-work commented Feb 13, 2025

What it does

Fixes #14902 with an alternative to #14914 based on the implementation of the chunk iterator in the Open AI client itself - I couldn't find a clean way to piggyback off of that implementation while adding messages, as we want to, but it did seem worth following. The main differences between the existing implementation and this one are

  • the introduction of queues for reads and a cache for messages received. I think this is preferable to the current system of pinning resolve / reject in the closure, because I believe that that system requires that clients operate synchronously on the data they receive, which isn't guaranteed. If the client did asynch work on a chunk they'd received and our iterator received more than one chunk in the meantime, it would resolve those chunks on the same promise as before, which would cause them to be lost. It also assumes that the client won't request more than one chunk before waiting on them, which is how the client should behave in the asynch iterator protocol, but isn't strictly required. In the current implementation, if the client did so, we could create new promises for each request, but only retain a reference to the resolver for the last, so the promises in the middle would be irresolvable.
  • I've converted some of the emitted calls to once calls. Internally, emitted delegates to once, so I think we should still get the same results, but please correct me if I'm wrong.
  • Added logic to dispose of our listeners when we're done.

How to test

  1. Listener memory leak in AI system #14902 should be solved: no more warnings when interacting with Open AI.
  2. Other operations should behave normally.

NB: today or yesterday, it seems that on master, the progress display for tool calls is a bit different - they mostly appear once they're already complete. I don't believe it's a symptom of any change in message delivery implemented here.

Follow-ups

Breaking changes

  • This PR introduces breaking changes and requires careful review. If yes, the breaking changes section in the changelog has been updated.

Attribution

Review checklist

Reminder for reviewers

@colin-grant-work colin-grant-work force-pushed the bugfix/async-iterator-implementation branch from 8cdad62 to 86207e4 Compare February 13, 2025 22:28
- adds openai async iterator test cases
- adapts end of stream handling
- log aborts with debug severity
Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @colin-grant-work! Thank you for your great work ❤️

Although the problematic conditions did not (yet) occur in the Theia codebase, an adopter who themselves invoked the API might have run into them. Also in general the code is much cleaner encapsulated this way. So, thanks for the initiative!

I added a number of unit tests and noticed a weird issue with the 'end' handling. We always handed over finalChatCompletion, however that is actually a promise. The code did not handle that correctly and therefore emitted an undefined before completing.

Now I don't know why we ever used finalChatCompletion as the result of that promise is actually the full completion, but we handled the chunks before anyway. So I don't think we need to handle it at all.

Please have a look whether you agree and are fine with the code change. Thanks!

@colin-grant-work
Copy link
Contributor Author

@sdirix, thanks for adding the tests for such a variety of cases. That certainly helps increase confidence that things should work correctly. I seem to have left one log in that I didn't mean to, so I'll remove that, but otherwise, it looks good to me.

@colin-grant-work colin-grant-work force-pushed the bugfix/async-iterator-implementation branch from be72949 to 8c4c0bf Compare February 14, 2025 16:03
@colin-grant-work colin-grant-work merged commit b0f91ae into eclipse-theia:master Feb 14, 2025
10 of 11 checks passed
@colin-grant-work colin-grant-work deleted the bugfix/async-iterator-implementation branch February 14, 2025 22:10
@github-actions github-actions bot added this to the 1.59.0 milestone Feb 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Listener memory leak in AI system
2 participants