667 langchain4j tools - #720
Conversation
This reverts commit 0af5788.
# Conflicts: # dil/src/main/java/org/assimbly/dil/blocks/connections/ai/LangChain4jAgentConnection.java # dil/src/main/resources/kamelets/langchain4jagent-action.kamelet.yaml
…rds are porvided on env vars
… health urls requires it
# Conflicts: # camelBase/pom.xml # dil/pom.xml # dil/src/main/resources/kamelets/langchain4jagent-action.kamelet.yaml
This reverts commit 0af5788.
# Conflicts: # camelBase/pom.xml # commonBase/pom.xml # dil/pom.xml # dil/src/main/java/org/assimbly/dil/blocks/connections/ai/LangChain4jAgentConnection.java # dil/src/main/resources/kamelets/langchain4j-agent-action.kamelet.yaml # dil/src/main/resources/kamelets/langchain4j-web-search-action.kamelet.yaml # dil/src/main/resources/kamelets/langchain4jagent-action.kamelet.yaml # pom.xml
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 9 |
| Duplication | 7 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
| .maxMessages(100) | ||
| .chatMemoryStore(chatMemoryStore) | ||
| .build(); | ||
| java.util.Map<Object, dev.langchain4j.memory.ChatMemory> memories = new java.util.concurrent.ConcurrentHashMap<>(); |
There was a problem hiding this comment.
Possible memory leak here:
The new implementation does:
memories.computeIfAbsent(memoryId, id -> ...)
but I don't see any eviction/removal mechanism. Therefore every distinct memoryId encountered by the agent remains in the ConcurrentHashMap for the lifetime of the connection.
| @@ -0,0 +1,20 @@ | |||
| apiVersion: "camel.apache.org/v1" | |||
There was a problem hiding this comment.
Is this Kamelet need, or just used for local testing?
| @@ -0,0 +1,64 @@ | |||
| package org.assimbly.dil.blocks.connections.broker; | |||
There was a problem hiding this comment.
Why is this in the broker package? Probably should be in the ai package like the other ai connections
| log.debug("Imaps connection will be configured on the component"); | ||
|
|
||
| case "langchain4j-agent" -> | ||
| case "langchain4j-agent", "langchain4jagent" -> |
There was a problem hiding this comment.
Why support both naming convnentions. Better choose one (that fits the other kamelets)
| new RabbitMQConnection(context, decryptedProperties, connectionId, "spring-rabbitmq").start(); | ||
|
|
||
| case "springai" -> | ||
| case "springai-chat", "springaichat" -> |
There was a problem hiding this comment.
Why support both naming convnentions. Better choose one (that fits the other kamelets)
# Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
pedrocatalao
left a comment
pedrocatalao
left a comment
There was a problem hiding this comment.
I'm not familiar with how this works but here goes, probably worth checking:
-
The camel-log dependency was deleted. In camelBase/pom.xml the line for camel-log was replaced by camel-langchain4j-agent, was that by accident?
-
Messages without a memory ID now share one conversation. Before, a missing memoryId fell back to the exchange ID but now falls back to "default".
-
The memory leak fix seems incomplete. Evicting a ChatMemory from the new bounded map doesn't clear its messages from the shared InMemoryChatMemoryStore. The store is a plain map that only removes an entry when clear() is called on the memory. So the old ids' messages still stay forever, just via a different object. The eviction hook needs to call clear() on the evicted memory.
Also got this when I asked AI to check:
- Unrelated changes bundled in. Switching the Gemini model on for returnThinking and sendThinking, changing the default model name, and adding camel-mustache have nothing to do with tools. They change cost and behaviour for every existing agent user. Ask to split them out or justify them. Also ask them to confirm "gemini-3.6-flash" is a real model name.
- Tavily library version is stale. It's pinned to 0.36.2 while the rest of LangChain4j is 1.19.0. The matching release is 1.19.0-beta29. Mixing 0.x and 1.x LangChain4j artifacts is a classpath problem waiting to happen.
- Two names for the same setting, again. The agent connection reads websearchapikey and falls back to tavilyapikey. Raymond already asked for one naming convention. Same for the dash-stripping template lookup in RouteTemplate, which exists only so both langchain4j-agent and langchain4jagent resolve. Pick one name and delete the fallback code.
- Web search is wired twice. There's a standalone web-search connection and kamelet, and separately the agent connection builds its own Tavily engine from an API key. The agent should reuse the registered web-search connection bean instead of duplicating the setup.
- The weather tool is a demo. It always answers "sunny, 25°C". Same question Raymond asked about the hello tool: is it meant to ship, or is it test scaffolding? If it ships, name and describe it as a sample.
Finally:
- Does this need any unit tests? (maybe not in scope)
- There are merge conflicts in the PR that need to be resolved carefully before a final review.
5 participants
No description provided.