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

Make streaming not repeatedly insert the same thing #72

Merged
merged 2 commits into from
Aug 29, 2024

Conversation

ultronozm
Copy link
Contributor

  • llm.el (llm-chat-streaming-to-point): Ignore common prefix between streamed text and buffer contents.

The motivation is as follows: without this patch, when the streamed text gets long enough, the call to (delete-region start end) (insert text) causes the buffer position to reset, preventing the user from viewing parts of the buffer away from the stream. Happy to clarify or illustrate if that would help.

* llm.el (llm-chat-streaming-to-point): Ignore common prefix between
streamed text and buffer contents.
@ultronozm ultronozm temporarily deployed to Continuous Integration August 27, 2024 23:24 — with GitHub Actions Inactive
@ultronozm ultronozm temporarily deployed to Continuous Integration August 27, 2024 23:24 — with GitHub Actions Inactive
@ultronozm ultronozm temporarily deployed to Continuous Integration August 27, 2024 23:24 — with GitHub Actions Inactive
@ultronozm ultronozm temporarily deployed to Continuous Integration August 27, 2024 23:24 — with GitHub Actions Inactive
@ahyatt
Copy link
Owner

ahyatt commented Aug 28, 2024

Thanks - I think this makes sense! Would you mind adding a comment in the code about the motivation for doing this?

Also, have you done FSF copyright assignment yet? No issues either way, but if you haven't, we need to make sure your total contribution to the project doesn't exceed about a dozen lines.

* llm.el (llm-chat-streaming-to-point): Add comments, fill.
@ultronozm ultronozm temporarily deployed to Continuous Integration August 28, 2024 05:23 — with GitHub Actions Inactive
@ultronozm ultronozm temporarily deployed to Continuous Integration August 28, 2024 05:23 — with GitHub Actions Inactive
@ultronozm ultronozm temporarily deployed to Continuous Integration August 28, 2024 05:23 — with GitHub Actions Inactive
@ultronozm ultronozm temporarily deployed to Continuous Integration August 28, 2024 05:23 — with GitHub Actions Inactive
@ultronozm
Copy link
Contributor Author

Thanks, comments added. Yes, I'm all set re. FSF.

By the way, it wasn't clear to me the reason for passing the accumulated string to llm-chat-streaming-to-point (rather than the new part). Is this because the chunks could conceivably arrive out of order, or something else?

@ahyatt ahyatt merged commit 6955d8a into ahyatt:main Aug 29, 2024
4 checks passed
@ahyatt
Copy link
Owner

ahyatt commented Aug 29, 2024

I felt like both passing the accumulated version, or the complete string, could make sense, but felt that the complete string might be more useful. I didn't want the clients to have to store the accumulated string.

Thanks for the change!

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