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.

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

fix: Google Vertex - fix the content type of ChatMessage content from function #1242

Merged
merged 1 commit into from
Dec 11, 2024

Conversation

anakin87
Copy link
Member

@anakin87 anakin87 commented Dec 10, 2024

Related Issues

Proposed Changes:

  • fix the content type
  • use text instead of content to access the text

How did you test it?

CI

Notes for the reviewer

  • This is a bug fix but also a breaking change. To make things clearer, I think that we can release a major version
  • In the future, when new ChatMessage + Tool are adopted, we should refactor the tool calling section of this Chat Generator and I'm confident that we can handle it better

Checklist

@github-actions github-actions bot added type:documentation Improvements or additions to documentation integration:google-vertex and removed type:documentation Improvements or additions to documentation labels Dec 10, 2024
@anakin87 anakin87 changed the title fix vertex fix: Google Vertex - fix the content type of ChatMessage content from function Dec 10, 2024
@anakin87 anakin87 marked this pull request as ready for review December 10, 2024 12:15
@anakin87 anakin87 requested a review from a team as a code owner December 10, 2024 12:15
@anakin87 anakin87 requested review from Amnah199 and vblagoje and removed request for a team December 10, 2024 12:15
Copy link
Member

@vblagoje vblagoje 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! One change is that we previously, for some reason, dumped dict to content directly (which was a mistake) but now are properly json string serializing it. Isn't this going to break some existing clients? Even if it does - it's for the better, right?

@anakin87
Copy link
Member Author

Yes, this can potentially break the existing behavior. But it is actually a bug fix.
That is why I will release a new major version.

@anakin87 anakin87 merged commit 0811b3b into main Dec 11, 2024
16 checks passed
@anakin87 anakin87 deleted the vertex-text branch December 11, 2024 13:54
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.

2 participants