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

Stop generation button (closes #86) #88

Merged
merged 11 commits into from
Apr 26, 2023
Merged

Conversation

Grsmto
Copy link
Contributor

@Grsmto Grsmto commented Apr 25, 2023

Need to see if we want to have it for the release.
It's using @gary149 design instead of the one in the issue!

closes #86

Copy link
Member

@coyotte508 coyotte508 left a comment

Choose a reason for hiding this comment

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

Code LGTM, but I think we should also not save the message in the backend in this case? Or should we?

src/lib/components/StopGeneratingBtn.svelte Outdated Show resolved Hide resolved
src/routes/r/[id]/+page.svelte Outdated Show resolved Hide resolved
src/lib/components/StopGeneratingBtn.svelte Outdated Show resolved Hide resolved
@Grsmto
Copy link
Contributor Author

Grsmto commented Apr 25, 2023

Code LGTM, but I think we should also not save the message in the backend in this case? Or should we?

Yeah so that's why I mentioned the AbortController, I suppose that way we could avoid saving/save only the portion the user saw. I think it's OK for now tho because the main point of this feature is to allow users to quickly send a new request without having to wait until the end.

I suppose ideally also we would save the aborted message + not count it for further generations. But I think we would need to mark it as aborted or something. I'll create an issue to discuss this maybe.

@gary149
Copy link
Collaborator

gary149 commented Apr 25, 2023

Miss alignement on mobile:
image

@coyotte508
Copy link
Member

ok then, ok for me to merge and create new issue (after mobile UI is fixed)

@Grsmto Grsmto force-pushed the stop-generation-button branch from 719fc00 to 25fabae Compare April 25, 2023 11:45
@julien-c
Copy link
Member

no strong opinion, but imo let's only launch after launch (can be later today...)

@Grsmto Grsmto marked this pull request as draft April 25, 2023 13:09
@coyotte508
Copy link
Member

yea let's make it 100%, stopping on frontend = stopping on backend

@Grsmto
Copy link
Contributor Author

Grsmto commented Apr 26, 2023

Thinking more about this and the backend part, the only way to "catch" the abort afaik is to send an extra API call from the client. However even with that, since the server is stateless I don't see how you could cancel the on-going request. Any idea @coyotte508?

@coyotte508
Copy link
Member

Thinking more about this and the backend part, the only way to "catch" the abort afaik is to send an extra API call from the client.

Ok I looked more into it and at first glance this is right: https://www.reddit.com/r/SvelteKit/comments/yjdx13/detect_aborted_request_on_the_server/

But then I looked at websocket implementations:

And so using a similar approach - having our own custom server file that reuses' the adapter's handler function - it should be possible for us to monitor request.on('close') for each request and put that info in a global WeakMap for example.

Though this is advanced stuff, not sure we want to dip our toes yet into the internals

However even with that, since the server is stateless I don't see how you could cancel the on-going request. Any idea @coyotte508?

We could... add an "abortedRequests" function in a mongodb collection, listen to it across all instances with a changestream (or just a periodic db.abortedReqs.find().toArray() to be simpler to update a global list of aborted reqs)

We could base the abort on the convo id

@coyotte508 coyotte508 marked this pull request as ready for review April 26, 2023 11:48
coyotte508
coyotte508 previously approved these changes Apr 26, 2023
@coyotte508 coyotte508 dismissed their stale review April 26, 2023 12:20

new commit

@@ -57,6 +64,27 @@

if (!data || conversationId !== $page.params.id) break;
Copy link
Member

@coyotte508 coyotte508 Apr 26, 2023

Choose a reason for hiding this comment

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

Should remove this, it reappeared in the last commit but not valid anymore

@coyotte508 coyotte508 merged commit 2772555 into main Apr 26, 2023
@coyotte508 coyotte508 deleted the stop-generation-button branch April 26, 2023 12:57
ice91 pushed a commit to ice91/chat-ui that referenced this pull request Oct 30, 2024
Co-authored-by: coyotte508 <coyotte508@gmail.com>
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.

"Stop generating" button
4 participants