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

feat: Add chatrole tests and meta for GeminiChatGenerators #1090

Merged
merged 11 commits into from
Sep 24, 2024
Merged

feat: Add chatrole tests and meta for GeminiChatGenerators #1090

merged 11 commits into from
Sep 24, 2024

Conversation

Amnah199
Copy link
Contributor

@Amnah199 Amnah199 commented Sep 17, 2024 •
edited
Loading

Related Issues

Proposed Changes:

  • Improve tests in gemini chat generators to include checks for ChatRole of messages.
  • Also added metadata to responses of chat generators which was previously missing.
  • The function calls were not handled properly in chat generators

How did you test it?

  • Updated the existing tests
  • Also locally tested the VertexChatGenerator with examples

Notes for the reviewer

There might be some room for improvement as I wasn't sure what is the general protocol for meta in ChatMessages.

Checklist

@Amnah199 Amnah199 requested review from vblagoje and silvanocerza and removed request for vblagoje September 18, 2024 08:37
@github-actions github-actions bot added the type:documentation Improvements or additions to documentation label Sep 19, 2024
@Amnah199 Amnah199 marked this pull request as ready for review September 20, 2024 11:27
@Amnah199 Amnah199 requested a review from a team as a code owner September 20, 2024 11:27
@Amnah199 Amnah199 requested review from vblagoje and removed request for a team September 20, 2024 11:27
@Amnah199 Amnah199 changed the title feat: Add chatrole tests feat: Add chatrole tests and meta for GeminiChatGenerators Sep 20, 2024
silvanocerza
silvanocerza reviewed Sep 24, 2024
View reviewed changes
integrations/google_ai/src/haystack_integrations/components/generators/google_ai/chat/gemini.py Outdated

combined_response = "".join(responses).lstrip()
return [ChatMessage.from_assistant(content=combined_response)]
streaming_callback(StreamingChunk(content=content, meta=metadata))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the indentation correct here? We're calling streaming_callback once per candidate. Shouldn't it be called once per part? What if there are multiple parts per candidate?

Copy link
Contributor Author

@Amnah199 Amnah199 Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

You have a valid point. I looked into this, but it's difficult to find any chunk that contains multiple Parts, so for now, it produces the same result. In other generators, the entire chunk is passed to the callback function, as shown here. Because of this, I'm not entirely certain, but we can do it your way.
@anakin87 would you have an input here?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry. @vblagoje knows better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@silvanocerza I updated based on your suggestion for now which makes more sense, also this PR is blocking my other PR. So I'll just go ahead

@Amnah199 Amnah199 merged commit bca32be into main Sep 24, 2024
22 checks passed
@Amnah199 Amnah199 deleted the add-chatrole-tests branch September 24, 2024 16:35
Amnah199 added a commit that referenced this pull request Oct 2, 2024
@Amnah199
feat: Add chatrole tests and meta for GeminiChatGenerators (#1090)
9e57a57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anakin87 anakin87 anakin87 left review comments

@vblagoje vblagoje vblagoje left review comments

@silvanocerza silvanocerza silvanocerza approved these changes

Assignees
No one assigned
Labels
integration:google-ai integration:google-vertex type:documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add new tests to check roles in responses of Chat Generators
4 participants
@Amnah199 @vblagoje @silvanocerza @anakin87