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

fix-issue-206 #246

Merged
merged 2 commits into from
Sep 9, 2024
Merged

fix-issue-206 #246

merged 2 commits into from
Sep 9, 2024

Conversation

Guocork
Copy link
Collaborator

@Guocork Guocork commented Sep 6, 2024

I solved the issue by removing the model-generated reply from the corresponding array when the user clicks the cancel button. However, this creates a situation where the user's questions may appear next to each other. I'm not sure if this is the best solution.
截图 2024-09-06 11-25-59

@Guocork Guocork self-assigned this Sep 6, 2024
@jmbejar
Copy link
Collaborator

jmbejar commented Sep 6, 2024

@Guocork Thanks for working on this issue.

I see a big problem with this approach: the cancel button is mostly used to interrupt the model streaming of the response, allowing to keep part of the generated output. It is true that the response should be deleted if the streaming has not started yet.

I have been thinking on this problem but in a separate pull request where we are integrating with another backend for agents.

Since there is no immediate plans to integrate all this PR, maybe you want to help to port some changes in the chat state management from this PR to this one?

If you're interested, please look at how in this PR there is a change in the Chat struct from having a bool is_streaming to state (which is an enum with a representation from the cancelled state among others).
https://github.com/moxin-org/moxin/pull/218/files#diff-cce49c6f311f98aee6e76e747d766858047b8a1711514e95ba9fc5344daa1eabL92-R120
Once the chat has this new state field, it would be possible to model the cancelled state, specially before the initial chunk of the response comes from the backend model.

Please let me know if you need more guidance or ideas.

@Guocork
Copy link
Collaborator Author

Guocork commented Sep 9, 2024

@Guocork Thanks for working on this issue.

I see a big problem with this approach: the cancel button is mostly used to interrupt the model streaming of the response, allowing to keep part of the generated output. It is true that the response should be deleted if the streaming has not started yet.

I have been thinking on this problem but in a separate pull request where we are integrating with another backend for agents.

Since there is no immediate plans to integrate all this PR, maybe you want to help to port some changes in the chat state management from this PR to this one?

If you're interested, please look at how in this PR there is a change in the Chat struct from having a bool is_streaming to state (which is an enum with a representation from the cancelled state among others). https://github.com/moxin-org/moxin/pull/218/files#diff-cce49c6f311f98aee6e76e747d766858047b8a1711514e95ba9fc5344daa1eabL92-R120 Once the chat has this new state field, it would be possible to model the cancelled state, specially before the initial chunk of the response comes from the backend model.

Please let me know if you need more guidance or ideas.

OK,I got it. i will try to fix that.

@Guocork
Copy link
Collaborator Author

Guocork commented Sep 9, 2024

I added a piece of logic, and it seems to have achieved the desired effect.

@jmbejar jmbejar merged commit 32bed45 into moxin-org:dev Sep 9, 2024
5 checks passed
@Guocork Guocork deleted the fixed-issue-206 branch September 10, 2024 02:36
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