-
Notifications
You must be signed in to change notification settings - Fork 81
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
add support for tools for the ollama provider #662
Conversation
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 is really interesting.
Just so it's clear - does this work with the latest Ollama version or do we still need to wait for that feature to land?
...a/deployment/src/main/java/io/quarkiverse/langchain4j/ollama/deployment/OllamaProcessor.java
Outdated
Show resolved
Hide resolved
import io.quarkus.test.QuarkusUnitTest; | ||
|
||
@Disabled("Integration tests that need an ollama server running") | ||
public class ToolsTest { |
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.
We generally don't write such tests, but instead use Wiremock (see the OpenAI module for tools related tests)
...llama/runtime/src/main/java/io/quarkiverse/langchain4j/ollama/OllamaDefaultToolsHandler.java
Outdated
Show resolved
Hide resolved
return toolSpecifications.stream() | ||
.filter(ts -> ts.name().equals(toolResponse.tool)) | ||
.map(ts -> toToolExecutionRequest(toolResponse, ts)) | ||
.toList(); |
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.
We generally try hard to avoid lambdas in Quarkus code
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.
why (just curious)?
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.
When Quarkus started, the team found that the lambdas had a small (but not zero) impact on memory usage.
Mind you, this on Java 8, so things may have changed substantially since then, but we still try to avoid them unless the alternative is just plain terrible.
...llama/runtime/src/main/java/io/quarkiverse/langchain4j/ollama/OllamaDefaultToolsHandler.java
Outdated
Show resolved
Hide resolved
Yes it works with the latest Ollama version. |
Very nice, I'll give it a try tomorrow |
This is super interesting, but unfortunately it does not work properly :(. The issue seems to be that Ollama does not understand that the tool has been executed and keeps telling us to re-execute it. 1st request:
1st response:
After this the extension properly executed the tool:
Then the following is sent to Ollama:
The response however is now problematic:
As you can see it tells us to execute the tool again... This keep on happening without the sequence ending from the Ollama side. |
* Whether to enable the experimental tools | ||
*/ | ||
@WithDefault("false") | ||
Optional<Boolean> experimentalTools(); |
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.
Shouldn't we rather just name it tools
and mark it as experimental in a comment? To avoid having to do a breaking change once we don't consider it experimental anymore...
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 would name it enableTools
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 think this will stay experimental till ollama implements tools feature.
Yes the issue with this approach is that the llm should be aware the tool has been executed. We can have a simplified approach where we just trigger one tool without recursivity. OR the tools should always answer with a status for llm. I will try to add some example with the sendPoem. |
…avoid unnecessary multiple calls to same tool
I've updated the prompt + added some tools history in user messages. But didn't find the good way to avoid selecting twice the same tool. Perhaps @langchain4j @jmartisk or @geoand you can help here ? |
By selecting twice, do you mean the tool gets executed twice? |
yes |
We can't really do much here, the LLM is supposed to decide which tools need to get executed as complex workflows may need to have multiple tool invocations. OpenAI handles this seamlessly. |
yes, but it's weird that this one https://github.com/quarkiverse/quarkus-langchain4j/pull/662/files#diff-4cad3d1a7b72dca01c9cf8f6019dfdc9c8949b729fdafe2cbda381631db6f88bR34 seems to work correctly and it is more complex that the send Poem. I think I'm missing the correct inputs/prompt to tell LLM that the action has been executed. |
In that case, I would turn on logging of requests and responses and compare the one that works with the one that does not. |
By the way, I want to clarify if that we can get this to work properly, it's a no brainer for inclusion :) |
New approach: ask llm to create list of tools to execute and then respond with previous result. Needs some modification in core : https://github.com/quarkiverse/quarkus-langchain4j/pull/662/files#diff-2dd3bec40934ad6d175f6f14dad1af0e11c234cf5fec69739a89460d472ab55b I need to check OpenAI broken tests but they don't work on my side. Still in progress, but the main part could be done in langchain4j and then used in ollama models from langchain and quarkus-langchain. |
In order to replace some AI response containing variables with function results I've changed the order of chat memory. WDYT ? Could I change the messages order in the tests ? Or should I keep the existing order and adapt the tools executor part. |
Finally I've come back to the full implementation in quarkus-langchain4j, will discuss later with langchain4j if we want to port it. Still need to:
But if someone could test and give me some feedbacks to finalize, it could be cool. |
- cleanup parameters - doc
I will try it soon |
core/runtime/src/main/java/io/quarkiverse/langchain4j/data/AiStatsMessage.java
Show resolved
Hide resolved
.../java/io/quarkiverse/langchain4j/runtime/aiservice/AiServiceMethodImplementationSupport.java
Outdated
Show resolved
Hide resolved
.../java/io/quarkiverse/langchain4j/runtime/aiservice/AiServiceMethodImplementationSupport.java
Outdated
Show resolved
Hide resolved
core/runtime/src/main/java/io/quarkiverse/langchain4j/runtime/aiservice/ToolsResultMemory.java
Outdated
Show resolved
Hide resolved
...ders/ollama/runtime/src/main/java/io/quarkiverse/langchain4j/ollama/OllamaMessagesUtils.java
Outdated
Show resolved
Hide resolved
...ders/ollama/runtime/src/main/java/io/quarkiverse/langchain4j/ollama/QuarkusOllamaClient.java
Show resolved
Hide resolved
.../src/main/java/io/quarkiverse/langchain4j/ollama/runtime/config/LangChain4jOllamaConfig.java
Outdated
Show resolved
Hide resolved
.../runtime/src/main/java/io/quarkiverse/langchain4j/ollama/tool/ExperimentalMessagesUtils.java
Show resolved
Hide resolved
Thanks, I'll have another look tomorrow. One thing I can say right now is that when everything is ready, we will need the commits squashed and the PR rebased onto the latest main (if it's not already) |
Sure, this is just the draft PR of a pretty complex feature, i'm still thinking on some enhancements and perhaps another way to register it like a dedicated ChatLanguageModel instead of an option. Or even else a specific extension. When finalized i will create a new PR based on latest Main with clean git log. I'm already using it in my project to see if it covers my uses cases and seems ok. But It should be tested through other use cases. |
Hi @geoand , But I would need the changes in this PR of the core part for my Model to work. WDYT ? |
If Ollama is close to releasing official support for tools, it's probably best to wait |
No pb! It was just waiting for official tool.
There is some stuff to keep like variables and the fact that in one llm
request we have the tools calls and the result.
I would like to benchmark and then discuss with ollama if it's something
that could be useful.
Thanks for 783
Le ven. 26 juil. 2024, 10:48, Georgios Andrianakis ***@***.***>
a écrit :
… @humcqc <https://github.com/humcqc> Ollama 0.3.0 was released and
contains tools support!
#783 <#783> is the
change that is needed to bring it in. Compared to this change, it's much
simpler so I hope you don't mind if we close this PR in favor of the other
one.
I would however like to thank you very much for your work on this!
—
Reply to this email directly, view it on GitHub
<#662 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/A477YITGYCE72EWUKO7HNQTZOIEOHAVCNFSM6AAAAABJCSHS2GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJSGI3DQOJXGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
👍🏼 |
Proposition for : #305, tested on llama3, does not work yet with other models.
Draft to discuss the proposition.
Based on experimental python and discussion here
It's way to have tools working untill ollama fix will be available.
To discuss if we want this in langchain or quarkus-langchain or both.
@jmartisk @langchain4j @geoand WDYT ?