oscerd commented on code in PR #29066:
URL: https://github.com/apache/camel/pull/29066#discussion_r4454321758
##########
components/camel-ai/camel-huggingface/src/main/java/org/apache/camel/component/huggingface/tasks/ChatPredictor.java:
##########
@@ -203,10 +208,35 @@ protected void processOutput(Exchange exchange, Output
output) throws Exception
if (node.isArray() && !node.isEmpty()) {
JsonNode first = node.get(0);
if (first.has("generated_text") &&
first.get("generated_text").isArray()) {
- String fullHistoryJson =
first.get("generated_text").toString();
+ String fullHistoryJson =
trimHistory(first.get("generated_text")).toString();
chatHistories.put(memoryId, fullHistoryJson);
LOG.debug("Updated chat history for ID: {}", memoryId);
}
}
}
+
+ /**
+ * Keeps the system message and at most maxHistoryMessages of the other
messages. The oldest messages are dropped
+ * first, and the kept messages start with a user message, as chat
templates that alternate the roles expect.
+ */
+ private JsonNode trimHistory(JsonNode history) {
+ int maxMessages = config.getMaxHistoryMessages();
+ int systemMessages = !history.isEmpty() &&
"system".equals(history.get(0).path("role").asText()) ? 1 : 0;
+ if (maxMessages <= 0 || history.size() - systemMessages <=
maxMessages) {
+ return history;
+ }
+ int from = history.size() - maxMessages;
+ while (from < history.size() &&
!config.getUserRole().equals(history.get(from).path("role").asText())) {
Review Comment:
Good catch, thanks. A value of 1 can never keep a reply, because the kept
messages have to start with a user message. The chat predictor now rejects a
`maxHistoryMessages` of 1 or below 0 when it is created, so the route fails at
start with "The maxHistoryMessages option must be 0 (no limit) or at least 2,
but was 1". I kept the option in messages rather than turns, as that is what
the history holds and what `maxHistoryMessages` of camel-openai counts, and the
description says that an odd value keeps one message less. `ChatHistoryTest`
now checks that 1 and -1 are rejected and that 2 keeps the last turn.
_Claude Code on behalf of Andrea Cosentino (oscerd)_
##########
components/camel-ai/camel-huggingface/src/main/java/org/apache/camel/component/huggingface/HuggingFaceConfiguration.java:
##########
@@ -81,6 +81,18 @@ public class HuggingFaceConfiguration implements Cloneable {
@UriParam(defaultValue = "CamelChatMemoryId", description = "Header name
for conversation memory ID (for multi-user chats)")
private String memoryIdHeader = "CamelChatMemoryId";
+ @UriParam(defaultValue = "1000",
+ description = "The maximum number of chat histories, one per
memory ID, that the chat task keeps in memory."
+ + " When there are more, the least recently used
history is dropped. Zero or a negative value"
+ + " keeps them all")
+ private int maxChatHistories = 1000;
+
+ @UriParam(defaultValue = "0",
+ description = "The maximum number of messages that the chat task
keeps in a chat history, besides the system"
+ + " prompt. The oldest messages are dropped first,
and the kept messages start with a user"
+ + " message. Zero or a negative value keeps them
all")
+ private int maxHistoryMessages;
Review Comment:
Agreed, the issue asks to bound the length of each history too.
`maxHistoryMessages` now defaults to 50 messages, besides the system prompt,
and `0` keeps every message as before. The upgrade guide says that the default
is now 50 and that `0` gives the old behaviour, and the component page points
long-running conversations to `maxHistoryMessages`. `ChatHistoryTest` checks
that with the default the 27th turn on one memory ID sends 51 messages,
starting from the second turn, and that `0` sends all 53.
_Claude Code on behalf of Andrea Cosentino (oscerd)_
##########
components/camel-ai/camel-huggingface/src/main/java/org/apache/camel/component/huggingface/tasks/TextToSpeechPredictor.java:
##########
@@ -132,8 +128,8 @@ protected void processOutput(Exchange exchange, Output
output) throws Exception
// Reconstruct DJL Audio (mono, 1 channel)
Audio audio = new Audio(audioData, samplingRate, 1);
+ // the body is the DJL Audio, not encoded audio bytes, so no
Content-Type is set
exchange.getMessage().setBody(audio);
Review Comment:
Right, thanks. The predictor now removes `Exchange.CONTENT_TYPE` from the
message, so a `text/plain` request no longer leaves its content type on the
`Audio` result. The test now sends a request with `Content-Type: text/plain`
and checks that the result has no `Content-Type`. The component page and the
upgrade guide say that the header is removed.
_Claude Code on behalf of Andrea Cosentino (oscerd)_
##########
components/camel-ai/camel-huggingface/src/main/java/org/apache/camel/component/huggingface/tasks/AbstractTaskPredictor.java:
##########
@@ -169,6 +177,34 @@ public void predict(Exchange exchange) throws Exception {
}
}
+ /**
+ * Fails when the output of the Python handler is its error payload: a
JSON object with the single field
+ * {@code error}. Any other output is a result, also a text or a JSON that
has the word error in it.
+ */
+ protected static void failOnPythonError(String output) {
+ String text = output != null ? output.strip() : "";
Review Comment:
Thanks, that turned a failure into a silent success. `failOnPythonError` now
throws `RuntimeCamelException` with "Python inference returned no output" for a
null output, so text-generation, summarization, question-answering and
zero-shot fail again, with a clear message. text-to-image fails the same way
now; it used to fail with a `NullPointerException`. New tests run each of them
with an `Output` that has no `data` entry. The JSON tasks already fail on a
missing output, as Jackson rejects null content.
_Claude Code on behalf of Andrea Cosentino (oscerd)_
##########
components/camel-ai/camel-huggingface/src/main/java/org/apache/camel/component/huggingface/tasks/ChatPredictor.java:
##########
@@ -100,10 +103,15 @@ public class ChatPredictor extends AbstractTaskPredictor {
private final ObjectMapper objectMapper = new ObjectMapper();
// In-memory chat history (key = memoryId, value = JSON string of list of
dicts)
- private final Map<String, String> chatHistories = new
ConcurrentHashMap<>();
+ private final Map<String, String> chatHistories;
public ChatPredictor(HuggingFaceEndpoint endpoint) {
super(endpoint);
+ // keeps the most recently used histories only, so the map does not
grow without bound
+ int maxChatHistories = config.getMaxChatHistories();
+ this.chatHistories = maxChatHistories > 0
+ ? LRUCacheFactory.newLRUCache(16, maxChatHistories, false)
Review Comment:
Good point: with the default `SimpleLRUCache` only a write moves an entry,
so the history that was updated least recently goes first. As every successful
turn writes its history, the two are close, so I kept the cache as it is and
reworded the option description, the component page and the upgrade guide to
say that the history updated least recently is evicted first.
_Claude Code on behalf of Andrea Cosentino (oscerd)_
##########
components/camel-ai/camel-huggingface/src/main/docs/huggingface-component.adoc:
##########
@@ -111,6 +111,13 @@ The component supports the following tasks:
| custom | TBD | TBD | predictorBean (required), revision, device | Any HF
model compatible with the custom predictor bean (e.g.,
Helsinki-NLP/opus-mt-en-fr for translation, distilgpt2 for custom generation)
|===
+A task fails with a `RuntimeCamelException` when its Python handler reports an
error, which the handler does by
Review Comment:
Thanks. The page now says that a custom predictor (`predictorBean`) fails on
the error payload only when its `processOutput` calls `failOnPythonError`, and
the custom task example shows that call. The chat row of the task table now
lists `maxChatHistories` and `maxHistoryMessages`.
_Claude Code on behalf of Andrea Cosentino (oscerd)_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
