Skip to content

Conversation

@Amnah199
Copy link
Contributor

Related Issues

Proposed Changes:

  • Updated the role assigned to model responses from system to assistant for both GoogleGeminiChatGenerator and VertexAIGeminiChatGenerator.
  • Reviewed and verified consistency across other generators to ensure correct role assignment.

How did you test it?

Ran existing tests

Notes for the reviewer

Checklist

@Amnah199 Amnah199 requested a review from anakin87 August 27, 2024 14:15
@Amnah199 Amnah199 marked this pull request as ready for review August 27, 2024 14:15
@Amnah199 Amnah199 requested a review from a team as a code owner August 27, 2024 14:15
@Amnah199 Amnah199 requested review from silvanocerza and removed request for a team and silvanocerza August 27, 2024 14:15
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, but please also update these other occurrences

wrong usage example:

wrong tests:

messages = [ChatMessage.from_system("What is the biggest city in United States?")]

ChatMessage.from_system(content="It's an arithmetic operation."),

@Amnah199 Amnah199 marked this pull request as draft August 27, 2024 14:31
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.

The changes look good. Feel free to merge.

Having more tests would help.
See #1019 (comment)

@Amnah199 Amnah199 merged commit e1c0fc3 into main Aug 30, 2024
@Amnah199 Amnah199 deleted the fix-role-chat-gen branch August 30, 2024 09:13
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.

Check ChatRoles in responses returned by ChatGenerators

3 participants