Skip to content

Generate: Enable easier TextStreamer customization#22516

Merged
gante merged 2 commits into
huggingface:mainfrom
vblagoje:streaming_callback
Apr 3, 2023
Merged

Generate: Enable easier TextStreamer customization#22516
gante merged 2 commits into
huggingface:mainfrom
vblagoje:streaming_callback

Conversation

@vblagoje

@vblagoje vblagoje commented Apr 2, 2023

Copy link
Copy Markdown
Contributor

What does this PR do?

Minimally adapts recently integrated (#22449) by adding a more obvious API hook for streaming tokens yet retains all the current semantics.

I am excited to use TextStreamer but I found the hook-in API not as intuitive as it could be if one needs to customize token printing. For example, I need to use specific colouring to print arriving tokens, yet achieving this without a complete TextStreamer rewrite is not as easy. We can easily create an obvious hook-in method:

def on_new_token(self, token: str, stream_end: bool = False):

This method is called by TextStreamer yet its subclasses can easily customize printing. The default implementation of on_new_token method simply prints tokens to stdout as it currently does.

I don't foresee any major documentation updates as a consequence of this PR.

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Who can review?

@gante @sgugger

@HuggingFaceDocBuilderDev

HuggingFaceDocBuilderDev commented Apr 2, 2023

Copy link
Copy Markdown

The documentation is not available anymore as the PR was closed or merged.

@gante gante left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like the structure, but I dislike the name on_new_token. It's actually a callback on "new print-ready text". Perhaps on_finalized_text? WDYT?

@vblagoje

vblagoje commented Apr 3, 2023

Copy link
Copy Markdown
Contributor Author

I like the structure, but I dislike the name on_new_token. It's actually a callback on "new print-ready text". Perhaps on_finalized_text? WDYT?

@gante I am completely indifferent regarding the name. Please adjust the name as you like!
On a second look, I'm also not 100% sure whether to put this method into TextStreamer only. Your call.

@gante

gante commented Apr 3, 2023

Copy link
Copy Markdown
Contributor

Let's keep it in TextStreamer for now. It's still early to tell how people will want to use it :)

@gante gante left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

(@sgugger with this new fn, the newer iterator streamer can inherit from the stdout streamer, sharing the text preparation logic)

@gante gante requested a review from sgugger April 3, 2023 17:26
@vblagoje vblagoje force-pushed the streaming_callback branch from 39bbf19 to 9c275b7 Compare April 3, 2023 17:26

@sgugger sgugger left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, thanks for your contribution!

@gante gante merged commit a17841a into huggingface:main Apr 3, 2023
raghavanone pushed a commit to raghavanone/transformers that referenced this pull request Apr 5, 2023
novice03 pushed a commit to novice03/transformers that referenced this pull request Jun 23, 2023
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.

4 participants