Open afirstenberg opened 7 months ago
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 We plan to have a PR ready by early to mid-November. I saw from the guidelines attached that we should assign the issue to ourselves once we start working on it, however I am unable to assign it to myself, could you please assign this issue to me? Thanks again!
@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:
GeminiRole
is used anywhere in the genai package. Is it?formatSystemInstruction()
will need to collapse system instructions found anywhere in the chat, since other models support more than one.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 https://github.com/langchain-ai/langchain-google/issues/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!
https://github.com/langchain-ai/langchain-google/issues/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,
In the current behavior of genai's integration, system messages are currently prepended to the next user message, which I assume is a temporary stopgap assuming all models lack system instruction support. For models that don't support system instructions, would it be more consistent to align with the Google Common approach, where system messages are converted into a user message followed by an 'OK' model response or should I leave the behavior as is?
From what I understand, system messages are treated as just another role and concatenated to contents until they are finally removed. https://github.com/langchain-ai/langchainjs/blob/37e21d281a4fdc6f58bbd233af4371888bd5d9c8/libs/langchain-google-common/src/chat_models.ts#L99-L134 I assume that before removal, they are saved as part of the systemInstructions field, as it influences the model’s behavior as expected. Could you point me to where this is implemented in the Google Common library?
@chrisnathan20
- For models that don't support system instructions, would it be more consistent to align with the Google Common approach, where system messages are converted into a user message followed by an 'OK' model response or should I leave the behavior as is?
No, you should leave the behavior as is. (The GenAI and Google Common packages are separate and take different approaches.)
- I assume that before removal, they are saved as part of the systemInstructions field, as it influences the model’s behavior as expected. Could you point me to where this is implemented in the Google Common library?
The formatData()
method in the AbstractGoogleLLMConnection
class is what creates all the different fields that are being sent in the request to Gemini. It calls this.formatSystemInstruction()
to create the system instruction fields itself. https://github.com/langchain-ai/langchainjs/blob/2e192776e1190ce56fd5b70cd7698ffea088fd2c/libs/langchain-google-common/src/connection.ts#L402-L415
The formatSystemInstruction()
method in the ChatConnection
class is the actual implementation that is typically used.
https://github.com/langchain-ai/langchainjs/blob/2e192776e1190ce56fd5b70cd7698ffea088fd2c/libs/langchain-google-common/src/chat_models.ts#L136-L165
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: https://docs.google.com/document/d/1UolNPrfg9QfXvdH698bkpWaqt6BYBYFOZnaGbLE_Zbo/edit?usp=sharing
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:
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#L175
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:
convertSystemMessageToHumanContent
should be followed, since it will match the Generative AI package on Python.ChatConnection
class.Hi @afirstenberg, I hope you are doing well.
I have updated the implementation plan to better match the Gen AI package on Python. https://docs.google.com/document/d/1UolNPrfg9QfXvdH698bkpWaqt6BYBYFOZnaGbLE_Zbo/edit?usp=sharing
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 https://github.com/langchain-ai/langchain-google/issues/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#L175