-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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-common[patch]: adding client-info #5103
google-common[patch]: adding client-info #5103
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -53,11 +53,28 @@ export abstract class GoogleConnection< | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey team, just wanted to flag the recent change in the PR for your review. The added _getclientInfo
function calls getRuntimeEnvironment
, which is expected to access or read environment variables or configuration related to the runtime environment. Please take a look and ensure everything is handled correctly. Thanks!
return { | ||
"User-Agent": clientLibraryVersion, | ||
"Client-Info": clientInfo.toString(), |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
}; | ||
} | ||
|
||
async _getclientInfo(): Promise<Record<string, string>> { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
async _getclientInfo(): Promise<Record<string, string>> { | ||
const userAgent = await this._clientLibraryVersion(); | ||
const env = await getRuntimeEnvironment(); | ||
const langchainVersion = env?.libraryVersion ?? "0"; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
async _getclientInfo(): Promise<Record<string, string>> { | ||
const userAgent = await this._clientLibraryVersion(); | ||
const env = await getRuntimeEnvironment(); | ||
const langchainVersion = env?.libraryVersion ?? "0"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, let's be aware that this is currently always "0" because getRuntimeEnvironment()
doesn't currently set libraryVersion
. (Because this is difficult to do in non-node environments.)
I don't know what the format of Client-Info is supposed to be (is it a standard HTTP header? I can't find a reference if so), but it almost certainly isn't the output See other inline notes. Might also be good to have a test to make sure these are in the format expected. See the test in llms.test.ts called "user agent header" |
const env = await getRuntimeEnvironment(); | ||
const langchainVersion = env?.libraryVersion ?? "0"; | ||
const moduleName = await this._moduleName(); | ||
let clientLibraryVersion = `${langchainVersion}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I unfortunately don't think this does what you think it does :( we don't currently have a way to extract version at runtime.
I think it looks better after these updates. Remaining thoughts:
|
Agree with @afirstenberg's comments above - the |
@@ -64,8 +64,9 @@ export abstract class GoogleConnection< | |||
clientLibraryVersion: string; | |||
}> { | |||
const env = await getRuntimeEnvironment(); | |||
const packageJson = require('../package.json'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've discussed this with @jacoblee93 before (who can correct me if I mis-state something).
Unfortunately, while this works in Node, it doesn't work in all environments. Otherwise it would have been done in getRuntimeEnvironment()
(which would be the best place to fix it, so it is available to all packages).
require
itself is node-specific, but it isn't the only problem here.
Hi @afirstenberg @jacoblee93 , For now, i think we can make this hardcoded as we have mentioned in the package.json. Any thoughts? |
Sure, @holtskinner , I will put it as zero.Thanks a lot for your response. |
expect(record.opts.headers["User-Agent"]).toMatch( | ||
/langchain-js\/[0-9.]+-ChatConnection/ | ||
); | ||
expect(record.opts.headers["Client-Info"]).toMatch( | ||
/langchain-[a-zA-Z0-9.-]+\/[0-9.]+\/[a-zA-Z0-9.-]+/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part of the test fails.
const langchain = env?.library ?? "langchain-js"; | ||
const langchainVersion = env?.libraryVersion ?? "0"; | ||
const langchainVersion = env?.libraryVersion ?? "0";; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please run yarn format
I'm just going to hardcode this to |
I would have said to keep it as But I'm at the point where it clearly doesn't matter. |
@jacoblee93 , I have added formatting too, is there anything else I should add or its good to go? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks for being patient with us. Will get this merged and included in the next @langchain/google-common
release. Thank you!
Fixes # (issue)