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.

Sign up for GitHub

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

[HF][streaming][1/n] Text Summarization #851

Merged
merged 1 commit into from
Jan 10, 2024
+10 −3
Merged

[HF][streaming][1/n] Text Summarization #851

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...HuggingFace/python/src/aiconfig_extension_hugging_face/local_inference/text_generation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ async def run_inference(
not "stream" in completion_data or completion_data.get("stream") != False
)
if should_stream:
tokenizer : AutoTokenizer = AutoTokenizer.from_pretrained(model_name)
tokenizer: AutoTokenizer = AutoTokenizer.from_pretrained(model_name)
streamer = TextIteratorStreamer(tokenizer)
completion_data["streamer"] = streamer

Expand Down
11 changes: 9 additions & 2 deletions ...gingFace/python/src/aiconfig_extension_hugging_face/local_inference/text_summarization.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,13 +128,18 @@ def construct_stream_output(
"metadata": {},
}
)

accumulated_message = ""
for new_text in streamer:
if isinstance(new_text, str):
# For some reason these symbols aren't filtered out by the streamer
new_text = new_text.replace("</s>", "")
new_text = new_text.replace("<s>", "")
Comment on lines +136 to +137
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these just signify the start and end of the stream?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not quite! I'm really not sure why they show up for a few models while for TextGeneration this isn't needed.

I saw something like </s><s>Streaming Text</s> But yea just removed so it looks clean


accumulated_message += new_text
options.stream_callback(new_text, accumulated_message, 0)

output.data = accumulated_message

return output


Expand Down Expand Up @@ -245,7 +250,9 @@ async def run_inference(self, prompt: Prompt, aiconfig: "AIConfigRuntime", optio

# if stream enabled in runtime options and config, then stream. Otherwise don't stream.
streamer = None
should_stream = (options.stream if options else False) and (not "stream" in completion_data or completion_data.get("stream") != False)
should_stream = (options.stream if options else False) and (
not "stream" in completion_data or completion_data.get("stream") != False
)
Comment on lines +253 to +255
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a very complicated logic. Ideally would prefer to break this out into if/else or create a helper function for this, but not ship-blocking if it works. I just am struggling to think how this would work.

For e.g. if no options is specified, stream will be false, even if there is stream in completion_data. Is that correct behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if no options is specified, stream will be false, even if there is stream in completion_data. Is that correct behavior?

That is correct! The reasoning for this is that stream isn't really a model setting, it's an inference setting which we would need to pass in a stream callback, so streaming without it doesn't really make sense if it's not included (where would you stream out to?).

Sure I made a task to split this into a helper function: #861

if should_stream:
tokenizer: AutoTokenizer = AutoTokenizer.from_pretrained(model_name)
streamer = TextIteratorStreamer(tokenizer)
Expand Down