-
Notifications
You must be signed in to change notification settings - Fork 53
feat: add async and streaming #137
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
Conversation
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🟢 Enforce conventional commitWonderful, this rule succeeded.Make sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
|
nrfulton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The overall approach looks good to me. I do wonder about everything becoming async but I think it's okay.
@nrfulton, if we want to keep synchronous versions of functions around we definitely can. |
HendrikStrobelt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, I understand the logic... but currently from the outside, .chat and .instruct act the same for streaming and non-streaming, or ?
Yes; once we introduce partial requirement checking / logging, you will be able to notice differences with streaming, but no real functional differences between the two. |
|
I still need to make a few tweaks and push my changes to fix aloras / logging. |
|
@nrfulton @HendrikStrobelt Still need to finish up and push the documentation, but the code is ready for review. I'll monitor for test failures, I believe I fixed all of the things failing due to merging main back into my branch. |
|
I am trying to debug why the tests are failing; it's some out of space issue so I'm not sure if I just happened to be the unlucky one that pushed us over the edge or if some change I made is actually causing the issue. |
|
Tested that new code works in collab by installing my specific branch of mellea. Didn't see any issues in the notebook. |
Discussion: #103
Currently, the draft PR only adds the capability to ollama to highlight what changes were necessary.
Changes
Note: I'm not happy with where/how some functions are defined (mostly the processing functions). I am planning on moving those. These changes also set us up for simplifying backends. A generic backend could define this control flow of generation and prepping a model output thunk while calling the processing methods implemented by a specific backend.