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

Return request buffer from `llm-request-async' #11

Closed
wants to merge 1 commit into from

Conversation

Stebalien
Copy link
Contributor

Return request buffer from llm-request-async, allowing the user can cancel the request by killing the buffer. This will, in turn, kill the and cancel HTTP request.

I've tested this with Ollama and it appears to kill the underlying request as well.

fixes #6

That way, the user can cancel the request by killing the buffer. This
will, in turn, kill the and cancel HTTP request.

I've tested this with Ollama and it appears to kill the underlying
request as well.

fixes ahyatt#6
@ahyatt
Copy link
Owner

ahyatt commented Dec 22, 2023

Thanks for the fix. This is part of a fix for #6, but not sufficient. I have the other part written already, which is supporting a cancel-query method in the llm library. This would be nice in the future if there are any llms that connect in a way that aren't buffer-based. There's also another problem - I've found that killing the buffer often will cause the url request library to throw an error. I've had to wrap killing the buffer with (cl-letf (((symbol-function 'url-http-async-sentiel) (lambda (_ _)))) (kill-buffer)).

So I don't mind merging this in, but you can also wait until my more complete fix is in, which I think is necessary. Sorry for the delay, though, it's been taking longer than I've though.

@Stebalien
Copy link
Contributor Author

Ah, I see. This does seem to be sufficient for ollama as canceling the HTTP request cancels the rest of the request as well.

If you want to support LLMs that connect without buffers, it's probably best not to merge this (lest users depend on that behavior).

So I don't mind merging this in, but you can also wait until my more complete fix is in, which I think is necessary. Sorry for the delay, though, it's been taking longer than I've though.

No problem, I completely understand.

@Stebalien Stebalien closed this Dec 22, 2023
ahyatt added a commit that referenced this pull request Dec 23, 2023
This closes the feature request in #6, and
is relevant to the conversation at #11.
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.

Provide a way to cancel queries
2 participants