langchain4j / langchain4j

Java version of LangChain
https://docs.langchain4j.dev
Apache License 2.0
4.37k stars 846 forks source link

[BUG] baseUrl for OllamaChatModel or OllamaStreamingChatModel is not working when having additional path #1455

Closed pangzixiang closed 1 month ago

pangzixiang commented 1 month ago

Describe the bug The value of the APIs path in OllamaApi.class is wrong, which are starting with '/'. Therefore, if the baseUrl ends with path, like http://host:port/ollama-api/, the retrofit2 would ignore the path and the final URL would become http://host:port/api/chat. According to the retrofit2 document, we should remove the '/' at the beginning to fix this bug!!!

Log and Stack trace

dev.langchain4j.model.ollama.OllamaRequestLoggingInterceptor.log()
DEBUG: Request:
- method: POST
- url: https://xxxxxxx/api/chat
- headers: 
- body: {"model":"llama3","messages":[{"role":"USER","content":"hello"}],"options":{"temperature":0.7,"top_p":1.0},"stream":false}
2024-07-13 21:42:26 [main] dev.langchain4j.model.ollama.OllamaResponseLoggingInterceptor.log()
DEBUG: Response:
- status code: 405
- headers: Server: nginx/1.26.1
Date: Sat, 13 Jul 2024 13:42:25 GMT
Content-Type: application/json
Content-Length: 31
Connection: keep-alive
access-control-allow-origin: *
access-control-allow-credentials: true
x-process-time: 0

To Reproduce

OllamaChatModel ollamaChatModel = OllamaChatModel.builder()
                .baseUrl("http://xxxxxx/ollama-api/")
                .modelName("llama3")
                .customHeaders(headers)
                .topP(1D)
                .temperature(0.7)
                .logRequests(true)
                .logResponses(true)
                .build();

System.out.println(ollamaChatModel.generate("hello"));

Expected behavior

dev.langchain4j.model.ollama.OllamaRequestLoggingInterceptor.log()
DEBUG: Request:
- method: POST
- url: https://xxxxxx/ollama-api/api/chat
- headers: 
- body: {"model":"llama3","messages":[{"role":"USER","content":"hello"}],"options":{"temperature":0.7,"top_p":1.0},"stream":false}
2024-07-13 21:51:40 [main] dev.langchain4j.model.ollama.OllamaResponseLoggingInterceptor.log()
DEBUG: Response:
- status code: 200
- headers: Server: nginx/1.26.1
Date: Sat, 13 Jul 2024 13:51:39 GMT
Content-Type: application/json; charset=utf-8
Content-Length: 389
Connection: keep-alive
Access-Control-Allow-Origin: http://localhost:11434
Vary: Origin

Please complete the following information:

pangzixiang commented 1 month ago

PR #1456

zambrinf commented 1 month ago

I don't think this is a bug, try to remove the final "/" from the base url

pangzixiang commented 1 month ago

I don't think this is a bug, try to remove the final "/" from the base url

no, we can't. I did try before, and found retrofit would validate the base url and throw exception if the base url not ends with "/".

and for this issue, here is the reference from the source code https://github.com/square/retrofit/blob/trunk/retrofit%2Fsrc%2Fmain%2Fjava%2Fretrofit2%2FRetrofit.java#L570-L594. I think the root cause is we are not using retrofit in a correct way.

zambrinf commented 1 month ago

I could replicate it but this problem happens only if your base url has an additional path like "http://example.com/ollama-api". I believe the PR is not suitable because would fail other cases like "http://example.com" when there is no need for "/" in the end of the url, did you run the tests? Probably should have another solution for this problem instead of removing the slash of the OllamaApi

pangzixiang commented 1 month ago

I could replicate it but this problem happens only if your base url has an additional path like "http://example.com/ollama-api". I believe the PR is not suitable because would fail other cases like "http://example.com" when there is no need for "/" in the end of the url, did you run the tests? Probably should have another solution for this problem instead of removing the slash of the OllamaApi

yes, i have run all the tests, and tried few new tests for different base url in my branch:

  1. http://example.com retrofit would automatically convert to http://example.com/

  2. http://example.com/ happy case

  3. http://example.com/path throw IllegalArgumentException, as the base url with additonal path must end in /

  4. http://example.com/path/ happy case

Actually, the examples in the retrofit2 document also showing that the first slash is no need.

zambrinf commented 1 month ago

I think it is best to maintain OllamaApi convention of using the beginning slash for defining the endpoints. Maybe change something in OllamaClient, If it doesnt work maybe there is something in Retrofit we can try.

pangzixiang commented 1 month ago

I think it is best to maintain OllamaApi convention of using the beginning slash for defining the endpoints. Maybe change something in OllamaClient, If it doesnt work maybe there is something in Retrofit we can try.

No, we can't do anything as this is the behavior of Retrofit. And this issue is only related to how we use Retrofit. What we can do is just to follow the doc to use Retrofit in standard way.

Endpoint values which contain a leading / are absolute.

Absolute values retain only the host from baseUrl and ignore any specified path components.

Base URL: http://example.com/api/
Endpoint: /foo/bar/
Result: http://example.com/foo/bar/

Base URL: http://example.com/
Endpoint: /foo/bar/
Result: http://example.com/foo/bar/

Is there any reason why you want to keep the leading slash?

zambrinf commented 1 month ago

Is there any reason why you want to keep the leading slash?

I thought lc4j was using this convention, but I took a look there are not many classes (probably they will require this change too), I think this is ok, only would suggest adding some tests, also you could add the same idea of OpenAiClient that appends the trailing slash when it doesn't exists, so "http://example.com/path" would not fail.

pangzixiang commented 1 month ago

Is there any reason why you want to keep the leading slash?

I thought lc4j was using this convention, but I took a look there are not many classes (probably they will require this change too), I think this is ok, only would suggest adding some tests, also you could add the same idea of OpenAiClient that appends the trailing slash when it doesn't exists, so "http://example.com/path" would not fail.

test cases added (as I am not able to config the base url of ollama dev container, I use mock server to test it), and the same logic of OpenAiClient applied to OllamaClient.