-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
google-genai [feature]: Support Gemini system messages #5069
Comments
Hi @afirstenberg @jacoblee93 we are students from the University of Toronto and we are interested in contributing to this issue. Would it be possible for us to take it on? If so, are there any specific guidelines or suggestions you would like us to follow as we work on it? |
Hey @shannon-ab! I'm not sure the status of this but you can check out general guidelines here: https://github.com/langchain-ai/langchainjs/blob/main/CONTRIBUTING.md But in general feel free to give it a your best shot and let us refine it! |
Hi @jacoblee93! Thank you for your response and the guidelines. We have an issue analysis document that outlines the code changes for this issue which you can find here: https://docs.google.com/document/d/1YDUxJVI9NPu7nPObFppPfkdn1ZSR5BsosZdUkuDNB6I/edit?tab=t.0#heading=h.6w32w3mbbyvh |
@shannon-ab - Great to see your group taking this on, and it is good to see you have a preliminary design document. Some notes on your analysis:
Looks good so far! Looking forward to reviewing the work. |
Hi @afirstenberg, thanks for taking the time to take a look into our draft. We will update our planned implementation in the next days after discussing with the rest of the group and we will send the next draft under this issue too. Once again, thank you and have a great rest of your day! |
Hi @afirstenberg, I'm a groupmate of @shannon-ab and we were wondering what is the acceptance criteria for implementing this feature. We tried to reproduce the error message in langchain-ai/langchain-google#129 but we are not sure if this is still reproducible given that the issue is already closed |
Hi @afirstenberg, I just wanted to follow up on my previous message regarding the acceptance criteria for implementing this feature. I understand you might be busy, but my groupmates and I are still trying to determine if the issue in langchain-ai/langchain-google#129 is reproducible, as we're unsure due to its closure. We’d appreciate any guidance or clarification you can provide when you have the time. Thanks so much for your time and help! |
langchain-ai/langchain-google#129 references the Python version of LangChain. As far as I know system messages using the @langchain/google-genai package is still unimplemented. (It is already implemented in the @langchain/google-common package and it's sub-packages, but google-genai is separate.) |
Hi @jacoblee93, our group (together with @shannon-ab) is looking to take on this issue now that we confirmed the status of this, could you assign this issue to me? |
Hi @afirstenberg, I hope you've been having a great week. I have some follow-up questions regarding this issue,
|
No, you should leave the behavior as is.
The langchainjs/libs/langchain-google-common/src/connection.ts Lines 402 to 415 in 2e19277
The langchainjs/libs/langchain-google-common/src/chat_models.ts Lines 136 to 165 in 2e19277
|
Hi @afirstenberg, thanks for the detailed response! Here's the updated implementation plan for this feature. We are targeting to have the initial PR ready by next week: However, our group still has some questions about the following statement from the issue description, particularly the requirement for convertSystemMessageToHumanContent to be compatible with Python:
Could you clarify how this would apply in the context of google-genai? Comparing the two chat_models.ts, we noticed that the genai version does not include a ChatConnection class that extends AbstractGoogleLLMConnection. We were wondering if this statement is in scope for this issue or if it was a carryover from the google-common equivalent of this issue. Once again, we appreciate the time you have taken to guide us in fulfilling this feature. Have a great rest of your week! |
@chrisnathan20 - I'll review the implementation plan shortly. The paragraph you quoted has two parts:
|
Hi @afirstenberg, I hope you are doing well. I have updated the implementation plan to better match the Gen AI package on Python. Key Updates:
Given that the PR will be ready soon, you can just make your review on the PR directly if that works better for you. However, if you see anything from the implementation plan that needs to be revised, please do not hesitate to let us know and we will make the change prior to making the PR. Thanks! |
Privileged issue
Issue Content
See langchain-ai/langchain-google#129
https://ai.google.dev/docs/system_instructions
https://cloud.google.com/vertex-ai/generative-ai/docs/model-reference/gemini#request_body (for reference only)
This isn't implemented for all models, so also add
convertSystemMessageToHumanContent
to be compatible with Python. Also see the warning message generated at https://github.com/langchain-ai/langchain-google/blob/62ba6ca5e39174d467e47c83119c8bb4275128f6/libs/vertexai/langchain_google_vertexai/chat_models.py#L175The text was updated successfully, but these errors were encountered: