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

TextIteratorStreamer & generate graceful interruption #29536

Closed
wants to merge 2 commits into from

Conversation

paulcjh
Copy link

@paulcjh paulcjh commented Mar 8, 2024

What does this PR do?

This PR adds in functionality to stop the generate function for LLMs when used in conjunction with a streamer. The TextIteratorStreamer typically runs in a separate thread , per the example provided in its docstring, and if you need to terminate the generation there's currently no way to do this. This makes operating the models in an API environment challenging when things such as client disconnects occur.

note: the file was also formatted with black, happy to remove and just keep the new interruption changes

Before submitting

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

@gante

@dmarx
Copy link

dmarx commented Mar 8, 2024

FYI: it looks like you're including a lot of changes to generation/utils.py that are just formatting changes. I'm guessing this sort of thing is discouraged since it makes it difficult to identify the substantive changes you are proposing, and also is likely to cause merge conflicts for others. Concretely: I was curious to see what changes you had made in utils.py, but between the formatting changes and collapsing all of your work into a single commit, your substantive changes in that file are a needle in a haystack.

@paulcjh
Copy link
Author

paulcjh commented Mar 11, 2024

@dmarx just removed all of the cleanup - the changes in utils.py are very light

@paulcjh
Copy link
Author

paulcjh commented Mar 12, 2024

@gante do you have any thoughts on this feature?

@gante
Copy link
Member

gante commented Mar 14, 2024

Hi @paulcjh 👋 Thank you for opening the PR!

We're planning to rework streaming soon, so I'm avoiding adding short-lived changes. Have a look at the conversation with @dmarx on this PR, I think it covers the reasons to not add changes and the roadmap for streaming 🤗

Copy link

github-actions bot commented Apr 8, 2024

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the contributing guidelines are likely to be ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants