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

Jump to bottom

fix: assign streaming_callback to OpenAIGenerator in run() method #8054

Merged
merged 11 commits into from
Jul 24, 2024
Merged

fix: assign streaming_callback to OpenAIGenerator in run() method #8054

merged 11 commits into from
Jul 24, 2024

Conversation

Amnah199
Copy link
Contributor

@Amnah199 Amnah199 commented Jul 22, 2024 •
edited
Loading

Related Issues

Proposed Changes:

Allow passing streaming_callback parameter to run() method to OpenAIGenerator and OpenAIChatGenerator. This allows passing streaming_callback during the pipeline run and prevents the need to recreate the pipeline within streaming callbacks.

How did you test it?

  • Ran the unit tests.
  • Testing locally by running generating streaming callbacks on FastAPI server.

Notes for the reviewer

N/A

Checklist

@github-actions github-actions bot added the type:documentation Improvements on the docs label Jul 22, 2024
@coveralls
Copy link
Collaborator

coveralls commented Jul 22, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 10077478588

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 14 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.002%) to 90.143%
Files with Coverage Reduction New Missed Lines %
components/generators/openai.py 2 96.39%
components/generators/chat/openai.py 12 79.09%
Totals Coverage Status
Change from base Build 10077384627: 0.002%
Covered Lines: 6859
Relevant Lines: 7609
💛 - Coveralls

@Amnah199 Amnah199 changed the title fix: Adds invoke() method in OpenAIGenerator and OpenAIChatGenerator fix: assign streaming_callback to OpenAIGenerator in run() method Jul 23, 2024
Amnah199
Amnah199 commented Jul 23, 2024
View reviewed changes
haystack/components/generators/chat/openai.py Outdated
Comment on lines 203 to 207
streaming_callback = generation_kwargs.pop("streaming_callback", None)
# check if streaming_callback is passed to run()
if streaming_callback:
self.streaming_callback = streaming_callback

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. This is one possible and simple fix in current run() method.
  2. Other option is to separate the run() and invoke() as implemented for OpenAIGenerator. I proposed this for code modularity but if its unnecessary, we can just add the above checks for both generators.

In any case, we can choose the same approach for both, and I'll update the PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not a great idea, this is quite dangerous.

If you call run passing a streaming_callback all subsequents calls will reuse that same callback even if not explicitly set. That's extremely confusing in my opinion.

Best choice would be something like this:

streaming_callback = streaming_callback or self.streaming_callback

This also mean adding streaming_callback as another input with a None default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point!

@Amnah199 Amnah199 marked this pull request as ready for review July 23, 2024 12:10
@Amnah199 Amnah199 requested review from a team as code owners July 23, 2024 12:10
@Amnah199 Amnah199 requested review from dfokina and anakin87 and removed request for a team July 23, 2024 12:10
@anakin87
Copy link
Member

In general, I think generation_kwargs should only contain parameters that influence text generation, such as temperature, top_p...

I would also like @silvanocerza to take a look and validate/suggest other approaches.
This PR seems to have a significant impact: it could be an example for future similar PRs for all generators.

@silvanocerza
Copy link
Contributor

I agree with @anakin87 here, adding another input is the best choice.

It also has the best UX in my opinion. With the current solution one would have to run a Pipeline like so:

pipe.run({"llm": {"generation_kwargs": {"streaming_callback": my_callback}}})

Having an extra input would be simpler:

pipe.run({"llm": {"streaming_callback": my_callback}})

@Amnah199
Copy link
Contributor Author

That too was an option but as mentioned in the #7836 , that would be a breaking change?
Anyways, I'll update the PR with suggestions 👍

@anakin87
Copy link
Member

That too was an option but as mentioned in the #7836 , that would be a breaking change?

No, adding a new Optional parameter to the run method would not be a breaking change.

@silvanocerza
Copy link
Contributor

@Amnah199 adding optional inputs is not considered a breaking change since it won't change the behaviour of existing code.

@anakin87
Copy link
Member

I would suggest adding tests for the new parameter.
You can take inspiration from

haystack/test/components/generators/test_openai.py

Line 179 in 0c9dc00

def test_run_with_params_streaming(self, mock_chat_completion_chunk):

anakin87
anakin87 approved these changes Jul 24, 2024
View reviewed changes
Copy link
Member

@anakin87 anakin87 left a comment

Choose a reason for hiding this comment

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

Looks good.

@silvanocerza could you take a final look?

silvanocerza
silvanocerza requested changes Jul 24, 2024
View reviewed changes
Copy link
Contributor

@silvanocerza silvanocerza left a comment

Choose a reason for hiding this comment

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

Small important change to make, the rest is good.

@Amnah199 Amnah199 merged commit b374c52 into main Jul 24, 2024
17 checks passed
@Amnah199 Amnah199 deleted the feat_openai_streaming_callback branch July 24, 2024 13:49
@sataycat sataycat mentioned this pull request Aug 1, 2024
Closed
@vblagoje vblagoje mentioned this pull request Aug 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@silvanocerza silvanocerza silvanocerza approved these changes

@anakin87 anakin87 anakin87 approved these changes

@dfokina dfokina Awaiting requested review from dfokina dfokina is a code owner automatically assigned from deepset-ai/documentation

Assignees
No one assigned
Labels
topic:tests type:documentation Improvements on the docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

OpenAIGenerator and OpenAIChatGenerator streaming_callback as kwargs
4 participants
@Amnah199 @coveralls @anakin87 @silvanocerza