CAMEL-24527: camel-huggingface - apply the configured token to every task, not only chat - #25899
Conversation
…task, not only chat Only ChatPredictor passed the configured token to its Python script (as a token= kwarg), so every other task (text generation, summarization, question answering, classification, embeddings, ASR, TTS, text-to-image, zero-shot) failed with HTTP 401 when loading a gated or private model even though authToken / oauthProfile was configured. Apply the token centrally in AbstractTaskPredictor.loadModel by exporting it as the standard HF_TOKEN environment variable at the top of the generated handler. transformers.pipeline() reads HF_TOKEN when no explicit token is passed, so this covers all tasks in one place. The token is taken from config.getAuthToken(), which holds either the authToken option or the value resolved from an OAuth profile. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 9 tested, 27 compile-only — current: 9 all testedMaveniverse Scalpel detected 36 affected modules (current approach: 9).
|
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Good approach — setting HF_TOKEN as an environment variable is the idiomatic way to authenticate with the Hugging Face ecosystem and avoids threading a token parameter through each Python script template individually.
Two observations after review:
1. Redundant token handling in ChatPredictor (medium)
After this PR, the chat task gets the token applied twice: once via the new central HF_TOKEN env var injection in withAuthToken(), and again via the explicit token='...' kwarg that ChatPredictor.getPythonScript() still builds at line 113. Since the PR's goal is to centralize token handling, the old per-task tokenClause in ChatPredictor is now dead code and should be removed (along with the corresponding %s placeholder in chat.py). The redundancy is not a runtime bug (token= takes precedence), but it contradicts the PR's design intent.
2. Missing empty-string test case (low)
The withAuthToken() method handles both null and "", but only null is tested in noAuthTokenLeavesTheScriptUnchanged(). Adding a second assertion for predictorWithToken("") would complete the coverage.
🔀 Backport Status
main but the same gap exists on camel-4.22.x:
camel-4.22.x— no backport PR found
Consider creating a backport PR or adding the backport-to-4.22.x label.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Reviewed against this project's rules and conventions (this does not replace CodeRabbit/Sourcery or SonarCloud-style static analysis).
Good fix overall — centralizing token injection in AbstractTaskPredictor is the right approach, and it comes with a new test (AuthTokenInjectionTest) covering the token-present/absent cases. I independently built and ran the module's tests on this branch and they pass.
Two things to address before merge:
- Inline comment below on the unescaped token interpolation.
- There's already an unresolved review thread from @gnodet on
AbstractTaskPredictor.javapointing out thatChatPredictor's explicittoken='...'clause (line 113) is now redundant with the centralizedHF_TOKENenv var — chat would pass the token twice. Since this PR's stated intent is to centralize token handling, that cleanup (removingtokenClausefromChatPredictorand the%splaceholder fromchat.py) seems worth doing here, or at least replying to close out the thread.
Minor, non-blocking: LOG.debug("Generated Python script for task {}:\n{}", ...) in loadModel() now logs the token-bearing script for all 10 tasks instead of just chat. Low severity since it's DEBUG-only, but worth considering redaction given this project's guidance on secrets in logs.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
…dant chat token clause - Escape single quotes when interpolating the token into the generated handler so a token (or OAuth-resolved value) containing a quote cannot break out of / inject into the Python string literal (raised by @davsclaus). - Remove the per-task token clause from ChatPredictor.getPythonScript and the trailing placeholder from chat.py: the token is now applied centrally as HF_TOKEN, so the chat task no longer needs to pass it twice (raised by @gnodet). - Extend AuthTokenInjectionTest with the empty-token case and a single-quote escape case, and add ChatScriptFormatTest to guard chat.py's format-argument alignment. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
|
Thanks @davsclaus and @gnodet — addressed in the latest commit:
On the backport note: I'll open a Claude Code on behalf of oscerd |
|
Raised by Claus Ibsen on the PR. loadModel() logged the generated handler after withAuthToken() had prepended os.environ['HF_TOKEN'] = '<token>' to it, so the configured token reached the log at DEBUG level. That line previously only ran for chat; centralising the token made it apply to all ten tasks. Log the script before the token is prepended, and write the token-bearing version to the handler file. The log keeps its diagnostic value: the token is a single prepended line, not part of the script being debugged. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks — the DEBUG note was worth acting on rather than filing away, so I have taken it too. You are right about the change in blast radius: that line only ran for chat before, and centralising the token made it apply to all ten tasks, with the token now sitting in the script it prints. Rather than redact the string, String pythonScript = getPythonScript();
// logged before the token is prepended: withAuthToken writes the configured token into the
// script, and this now runs for every task rather than only chat
if (LOG.isDebugEnabled()) {
LOG.debug("Generated Python script for task {}:\n{}", config.getTask(), pythonScript);
}
Files.writeString(handlerPath, withAuthToken(pythonScript));The log keeps its diagnostic value — the token is one prepended line, not part of the script anyone is debugging — and there is no redaction to get wrong later. The other two points from this round were already handled in the previous commit: the token is escaped for single quotes, and Module tests pass, and a full reactor build from the root is green with nothing left uncommitted. Claude Code on behalf of oscerd |
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Clean, well-structured bug fix that centralizes auth-token injection in AbstractTaskPredictor so all ten task predictors can load gated/private models, not only chat. The environment-variable approach (HF_TOKEN) is the standard mechanism used by the transformers library.
Good details:
- The debug log was intentionally moved before
withAuthToken()to avoid logging the secret token — nice security hygiene. - The format-placeholder realignment in
chat.pyis verified correct: one%splaceholder and one argument removed, leaving 6 matched pairs. - No other Python task scripts had a token clause, so they gain token support purely through the centralized
withAuthTokeninloadModel().
No backport needed — camel-huggingface was introduced in 4.21.0 and does not exist on camel-4.18.x.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
4 participants
Issue
CAMEL-24527
Problem
The
authTokenoption (and the token resolved fromoauthProfile) was honoured by only the chattask.
ChatPredictor.getPythonScript()builds atoken='...'clause and passes it totransformers.pipeline(); none of the other nine task predictors do. As a result, text generation,summarization, question answering, classification, sentence embeddings, ASR, TTS, text-to-image and
zero-shot classification all failed with HTTP 401 when loading a gated or private model, even with a
token configured.
Fix
Apply the token centrally in
AbstractTaskPredictor.loadModel()by exporting it as the standardHF_TOKENenvironment variable at the top of the generated handler script:transformers/huggingface_hubreadHF_TOKENfrom the environment when no explicit token ispassed, so this authenticates every task in a single place instead of threading a token clause
through nine predictors and nine Python templates. The token is read from
config.getAuthToken(),which holds either the
authTokenoption or the value resolved from an OAuth profile(
HuggingFaceProducer.resolveOAuthToken). When no token is configured, the script is unchanged.Testing
AuthTokenInjectionTestverifies the token is exported asHF_TOKENwhen configured and thatthe script is left untouched when it is not.
mvn -Psourcecheck validategreen.Claude Code on behalf of oscerd