-
Notifications
You must be signed in to change notification settings - Fork 694
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
967 set useragent to lang chain4j #1046
base: main
Are you sure you want to change the base?
967 set useragent to lang chain4j #1046
Conversation
A few questions for y'all
It seems like there are several other tickets discussing HTTP client including getting rid of OkHttp3 as the default, so perhaps the cut and paste is fine for the time being before larger changes take place?
|
Hi @roytruelove,
Yes, I guess copy/paste is OK for now.
Ideally it would be nice to have a test. But if it takes too long, let's just test manually and see that existing tests are green. |
Will do, thank you
…On Thu, May 2, 2024 at 3:55 PM LangChain4j ***@***.***> wrote:
Hi @roytruelove <https://github.com/roytruelove>,
I've cut and pasted the same code to (so far) 3 of the OkHttpClient.
Continuing this way doesn't feel right but there's not a natural place
(that I've found) where I could create a singleton Interceptor and/or a
make a constant for the User Agent value. Maybe a util class in
dev.langchain4j.core but that doesn't have OkHttp as a dependency.
It seems like there are several other tickets discussing HTTP client
including getting rid of OkHttp3 as the default, so perhaps the cut and
paste is fine for the time being before larger changes take place?
Yes, I guess copy/paste is OK for now.
There are no current tests for the classes I'm changing, so I'm wondering
if as part of this change I'd need to create full coverage for all of the
client classes?
Ideally it would be nice to have a test. But if it takes too long, let's
just test manually and see that existing tests are green.
If it is quick to do though, I would do it and copy/paste it everywhere.
—
Reply to this email directly, view it on GitHub
<#1046 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA6NMLJBVAGBY4H3BVMRULZAJAUZAVCNFSM6AAAAABHAYDWKOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJQGU3DENBXGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
langchain4j/src/main/java/dev/langchain4j/code/Judge0JavaScriptEngine.java
Outdated
Show resolved
Hide resolved
Core code is complete, looking to see where I can add some tests |
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.
Added tests for new methods. Unfortunately it is a bigger lift to test that the HTTP calls contain the correct user agent, without adding mocks / inject-able HTTP clients. Please let me know your thoughts on this.
Other than that the PR is ready for review.
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.
Modeled after InternalOpenAiHelper.
@@ -95,8 +96,11 @@ public VertexAiEmbeddingModel(String endpoint, | |||
); | |||
|
|||
try { | |||
// TODO switch to use InternalVertexAiHelper.defaultPredictionServiceSettingsBuilder() once the library |
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.
Interestingly this uses a beta verson of PredictionServiceSettings
so it could not use the new InternalVertexAiHelper
class.
Apologies - I marked this as "ready" when it should still be in draft.. |
…' into 967-set-useragent-to-LangChain4j
Issue
Set User Agent to 'LangChain4J'
Change
Added the User Agent wherever it was accessible
General checklist
Checklist for adding new model integration
Checklist for adding new embedding store integration
{NameOfIntegration}EmbeddingStoreIT
that extends from eitherEmbeddingStoreIT
orEmbeddingStoreWithFilteringIT
Checklist for changing existing embedding store integration
{NameOfIntegration}EmbeddingStore
works correctly with the data persisted using the latest released version of LangChain4j