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

top_k and top_p transposed in vertexai #5673

Merged

Conversation

mheguy-stingray
Copy link
Contributor

Fix transposed properties in vertexai model

Models

@mheguy-stingray
Copy link
Contributor Author

@khallbobo - you might be interested in this since you just fixed something related. :)

Copy link
Contributor

@hwchase17 hwchase17 left a comment

Choose a reason for hiding this comment

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

good catch, thanks

@hwchase17 hwchase17 added the lgtm PR looks good. Use to confirm that a PR is ready for merging. label Jun 3, 2023
@mheguy-stingray
Copy link
Contributor Author

Added a test to cover that args are sent to the library as expected.
The call against the LLM is mocked, but I couldn't figure out how to mock out the environment check.
So it does make the call to validate your credentials.

@dev2049 dev2049 merged commit b64c39d into langchain-ai:master Jun 4, 2023
@mheguy-stingray mheguy-stingray deleted the fix-transposed-properties branch June 5, 2023 13:51
@khallbobo
Copy link
Contributor

Wow. This will certainly change results! Without my change, these are ignored anyway. With my change, we will start seeing very different results.

@bstrawson
Copy link

Google is now returning an error if you are using an older version of langchain. For anyone else wondering why their project has suddenly broken when nothing has changed their end (and they may not even be setting top_p), you may be seeing this message:

google.api_core.exceptions.InvalidArgument: 400 40.000000 is out of supported range [0, 1]; for top_p.

@mheguy-stingray
Copy link
Contributor Author

@bstrawson Can you give a little more detail about the issue you're running into?

Is it a case where you had saved an instance of an llm model in a previous version and then when you ran it in this version, you hit that issue?

To give a little more context, there are 2 changes that would have caused this.
1 is my change where top_p and top_k were transposed.
2 is that the arguments sent to the model were always ignored. #5566

@bstrawson
Copy link

@mheguy-stingray to be clear this change fixes the issue I was seeing. I just wanted to highlight the error message so others can discover this issue and find a resolution quickly.

I was using 0.188 which was working for for me up until yesterday. I then started seeing the error message I've referenced above and everything stopped working. I assumed it was my code - but even rolling back changes and deleting indexes didn't fix the issue. I spent a lot of time scratching my head and trying to work out why it suddenly broke for me.

My assumption is that the VertexAI was previously ignoring an invalid top_p setting, hence everything was previously working. It is an assumption, but I see no other explanation. Either way, I thought it might be useful to highlight the error that the API returns in case others come across it.

@mheguy-stingray
Copy link
Contributor Author

@bstrawson

My assumption is that the VertexAI was previously ignoring an invalid top_p setting, hence everything was previously working. It is an assumption, but I see no other explanation.

If you were using vertex in langchain, your llm settings (top_p, top_k, max_tokens, temperature) were never leaving langchain's code. They were never given to the vertex lib, nor the google servers.

#5566 fixed that issue.

The good news is that your settings are now actually being used.
The bad news is any assumptions you previously made about the effects your settings were having, are invalid. :(

@bstrawson
Copy link

@mheguy-stingray I've looked at #5566 but that appears to only apply to chat. Your fix applies to all VertexAI calls as _default_params is used in _predict.

Undertone0809 pushed a commit to Undertone0809/langchain that referenced this pull request Jun 19, 2023
Fix transposed properties in vertexai model


Co-authored-by: Dev 2049 <dev.dev2049@gmail.com>
This was referenced Jun 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm PR looks good. Use to confirm that a PR is ready for merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants