Croway commented on code in PR #29066:
URL: https://github.com/apache/camel/pull/29066#discussion_r4453752037
##########
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:
Following up on Claus's remark about this default: CAMEL-26219 also reports
that each history grows without bound, and with the default `0` it still does:
up to 1000 histories, each with no size limit, and each turn re-serializes the
whole history. In a test with no options set, turn 200 on one memory ID sends
400 messages to the model. A long conversation eventually outgrows the context
window of the model; depending on the model, its turns then fail or the input
is truncated, until `CamelChatClearHistory` is sent. Would a finite default,
for example 50 messages, be acceptable? It would need a line in the upgrade
guide.
##########
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:
A `null` output becomes `""` and the method returns. DJL's
`Output.getAsString("data")` returns `null` when there is no `data` entry, so
text-generation, summarization, question-answering and zero-shot (with the
default `autoSelect`) now complete with a null body and no error. Before, the
same output failed with `NullPointerException: Cannot invoke
"String.contains(java.lang.CharSequence)" because "result" is null`, which was
not a good error but was a failure. Throwing a `RuntimeCamelException` for a
null output here would keep it a failure, with a clear message.
##########
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:
`maxHistoryMessages=1` keeps no history at all. After the first turn the
history is `[system, user1, assistant1]`. `from` is `3 - 1 = 2`, which points
at `assistant1`, so the loop moves on to 3 and only the system prompt is
stored. Every turn then reaches the model without earlier context, while the
option says it keeps one message. Odd values keeping one message less is
documented, but 1 is a case where nothing is kept. Could values below 2 be
rejected, or the option counted in user/assistant turns? A test with
`maxHistoryMessages=1` would pin it down.
##########
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:
Before, the `Content-Type` was overwritten with `application/octet-stream`.
Now a `Content-Type` that is already on the message stays. Behind
platform-http, a `text/plain` request gets its `Audio` result with
`Content-Type: text/plain`, which does not match "it sets no `Content-Type`" in
the doc. `exchange.getMessage().removeHeader(Exchange.CONTENT_TYPE)` would fix
it. The new test starts from a fresh exchange; sending the request with a
`Content-Type` header would cover this.
##########
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:
With the default factory this is a `SimpleLRUCache`, whose `get()` does not
update the order; only writes do. So this evicts the history that was least
recently updated, not least recently used. With `maxChatHistories=2`, the
sequence write A, write B, read A (`prepareInput` only), write C evicts A, not
B. The two are close in practice, since every successful turn writes, but the
option description, the component doc and the upgrade guide all say "least
recently used". Either reword those, or touch the entry on read.
##########
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:
This sentence also covers the custom task in the table above, but a
`predictorBean` fails on the error payload only if it calls
`failOnPythonError`, which is not documented. Also, the chat row at line 109
does not list the new `maxChatHistories` and `maxHistoryMessages` options.
--
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]
