Repository navigation
Conversation
jimmyzhuu
requested review from
Rod-Christensen,
jmaionchi and
stepmikhaylov
as code owners
May 27, 2026 14:11
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Baidu Qianfan (ERNIE) LLM node: runtime with config validation and lifecycle, a Chat adapter, services.json profiles, model-sync provider, docs, and tests. ChangesBaidu Qianfan LLM Node Integration
Sequence DiagramsequenceDiagram
participant Framework
participant IGlobal
participant ChatAdapter
participant QianfanAPI
Framework->>IGlobal: validateConfig()
IGlobal->>ChatAdapter: build OpenAI-compatible client
ChatAdapter->>QianfanAPI: chat.completions.create (probe)
QianfanAPI-->>ChatAdapter: response / error
ChatAdapter-->>IGlobal: return or mapped exception
IGlobal-->>Framework: emit warning or success
Framework->>IGlobal: beginGlobal()
IGlobal->>ChatAdapter: instantiate Chat and store in bag
Framework->>IGlobal: endGlobal()
IGlobal->>ChatAdapter: clear chat reference
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
docs
Contributor
|
No description provided. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nodes/src/nodes/llm_baidu_qianfan/qianfan_client.py`:
- Around line 69-97: The APIConnectionError checks in is_retryable_error and
map_exception are unreachable because APIConnectionError subclasses APIError;
update both methods (is_retryable_error and map_exception) to check for
APIConnectionError before checking for APIError, preserving the existing return
values/messages for AuthenticationError, RateLimitError, APIConnectionError, and
APIError so connection-specific logic runs as intended.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7130dc6d-1b61-4157-9c57-faedc6e54c80
⛔ Files ignored due to path filters (1)
packages/shared-ui/src/assets/nodes/baidu-qianfan.svgis excluded by!**/*.svg
📒 Files selected for processing (17)
docs/README-nodes.mdnodes/src/nodes/llm_baidu_qianfan/IGlobal.pynodes/src/nodes/llm_baidu_qianfan/IInstance.pynodes/src/nodes/llm_baidu_qianfan/README.mdnodes/src/nodes/llm_baidu_qianfan/__init__.pynodes/src/nodes/llm_baidu_qianfan/qianfan_client.pynodes/src/nodes/llm_baidu_qianfan/requirements.txtnodes/src/nodes/llm_baidu_qianfan/services.jsonnodes/test/framework/pipeline.pypackages/shared-ui/src/components/canvas/util/get-icon-path.tstools/SYNC_MODELS.mdtools/src/providers/baidu_qianfan.pytools/src/sync_models.config.jsontools/src/sync_models.pytools/test/markers.pytools/test/test_baidu_qianfan_provider.pytools/test/test_sync_live.py
asclearuc
requested changes
Jun 1, 2026
asclearuc
left a comment
asclearuc
left a comment
Collaborator
There was a problem hiding this comment.
Thanks @jimmyzhuu — this is a clean, complete provider integration (node runtime, ERNIE profiles, model-sync tooling, tests, and icon), and it follows the existing OpenAI-compatible node pattern well.
Before it can merge it needs a rebase onto develop, because #926 changed how node icons work and it rewrote the exact files your PR touches under packages/. Points to pay attention to during the rebase:
packages/shared-ui/src/assets/nodes/no longer exists. Move yourbaidu-qianfan.svgtonodes/src/nodes/llm_baidu_qianfan/baidu-qianfan.svg(next toservices.json). Keep"icon": "baidu-qianfan.svg"inservices.jsonunchanged — only the file location moves.get-icon-path.tsis gone, replaced byIcon.tsx, which auto-discoversnodes/src/nodes/**/*.svgat build time. Drop all your changes to it — there is no manual icon registration anymore.THEME_DYNAMIC_ICONSis gone. Theming is now automatic: a build step rewrites single-color SVGs tocurrentColor(so they follow the light/dark theme) and passes multicolor logos through unchanged. Your note about keeping Baidu out of the dynamic set no longer applies — instead, make sure the Baidu SVG is genuinely multicolor if you want it to keep its brand blue, otherwise it will be theme-tinted.docs/README-nodes.mdwas also edited by #926 — expect a conflict there and resolve it.
After rebasing, please run builder shared-ui:test and the icon audit (node scripts/audit-icons.mjs); the audit asserts every services.json icon exists inside its node folder.
There are also a few code findings to address for a clean PR — see the inline comments below. CodeRabbit's notes are worth a pass too.
jimmyzhuu
force-pushed
the
feat/RR-1005-add-baidu-qianfan-llm
branch
from
June 2, 2026 00:12
89a5bc7 to
07af0f2
Compare
Contributor
Author
|
@asclearuc Thanks for your detailed review! Rebased this PR onto the latest
Verification run locally:
|
asclearuc
requested changes
Jun 2, 2026
asclearuc
left a comment
asclearuc
left a comment
Collaborator
There was a problem hiding this comment.
Thanks @jimmyzhuu — good progress: the rebase onto develop is clean (icon co-located, no packages/ changes), and the validateConfig exception-ordering fix from the last round is correct and now has a focused regression test.
This PR can be merged only once CI is fully green. Right now it's red:
-
🔴 CI — new validation tests fail on all 3 platforms (macOS / Windows / Linux). Check how other nodes run tests. As a clue -
builder teststarts from the root folder. -
🔴 CI —
ruff format --checkfails.
Non-blocking nit: please run (and confirm that it works) one save-time validation against ernie-5.0-thinking-preview with a real key, since the max_tokens=1 probe can false-fail on reasoning models.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nodes/src/nodes/llm_baidu_qianfan/IGlobal.py`:
- Around line 68-80: The validation for modelTotalTokens in the IGlobal logic
currently compares config-derived modelTotalTokens directly (modelTotalTokens <=
0) which can raise TypeError for non-numeric strings; update the checks in the
block handling modelTotalTokens (and the identical logic around lines 122-124)
to normalize/coerce the value to an int first (e.g., attempt
int(modelTotalTokens) inside a safe try/except), treat non-coercible values as
invalid and call warning('Token limit must be greater than 0'), and only perform
the <= 0 numeric check on the successfully parsed integer to avoid TypeError and
surface a clean validation warning instead of an exception.
- Around line 141-155: The _format_error function can return just "Error
{status}:" when emsg/etype are missing; update _format_error (parameters status,
etype, emsg, fallback) so that if only a status was parsed (i.e., parts contains
only the status token) you include the provided fallback message (either replace
the bare status output with fallback or append fallback to parts) before
collapsing whitespace and truncating; keep the existing regex whitespace
normalization and 500-char truncation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 453f382c-b97b-4c8c-8813-70b14075a16b
⛔ Files ignored due to path filters (1)
nodes/src/nodes/llm_baidu_qianfan/baidu-qianfan.svgis excluded by!**/*.svg
📒 Files selected for processing (17)
docs/README-nodes.mdnodes/src/nodes/llm_baidu_qianfan/IGlobal.pynodes/src/nodes/llm_baidu_qianfan/IInstance.pynodes/src/nodes/llm_baidu_qianfan/README.mdnodes/src/nodes/llm_baidu_qianfan/__init__.pynodes/src/nodes/llm_baidu_qianfan/qianfan_client.pynodes/src/nodes/llm_baidu_qianfan/requirements.txtnodes/src/nodes/llm_baidu_qianfan/services.jsonnodes/test/framework/pipeline.pynodes/test/test_baidu_qianfan_global_validation.pytools/SYNC_MODELS.mdtools/src/providers/baidu_qianfan.pytools/src/sync_models.config.jsontools/src/sync_models.pytools/test/markers.pytools/test/test_baidu_qianfan_provider.pytools/test/test_sync_live.py
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
nodes/src/nodes/llm_baidu_qianfan/IGlobal.py (2)
69-80:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize
modelTotalTokensbefore comparing it.
Config.getNodeConfig(...)returns untyped input, so values like'128000'or'abc'still makemodelTotalTokens <= 0raiseTypeError. That falls through to the outerexcept Exceptionand surfaces a raw Python error instead of the intended validation warning. Parse tointfirst and reject non-numeric values with the same user-facing warning.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nodes/src/nodes/llm_baidu_qianfan/IGlobal.py` around lines 69 - 80, The value returned into modelTotalTokens (from config.get('modelTotalTokens')) must be normalized to an integer before comparing; update the validation in IGlobal.py so that you attempt to coerce modelTotalTokens to int (catching ValueError/TypeError) and if coercion fails or the resulting integer is <= 0 emit the same warning('Token limit must be greater than 0') and return, otherwise proceed using the parsed int; reference the modelTotalTokens variable and the existing warning call to locate where to add the int() conversion and error handling.
142-152:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the fallback text when parsing yields only an HTTP status.
If
APIStatusError.response.json()has no structurederror/message,_format_error()still returns onlyError 4xx:/Error 5xx:here and dropsstr(e). That makes provider-side validation failures much less actionable than the raw fallback you already have available.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nodes/src/nodes/llm_baidu_qianfan/IGlobal.py` around lines 142 - 152, The _format_error function currently drops the fallback when only a status is present; change the final assembly so that if only status is available (status set but both etype and emsg are falsey) the returned message includes the fallback text (e.g., "Error {status}: {fallback}") instead of just "Error {status}:"; update the logic around variables status, etype, emsg, and fallback in _format_error to append or merge fallback when no structured error/message exists so callers still see the raw exception text.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@nodes/src/nodes/llm_baidu_qianfan/IGlobal.py`:
- Around line 69-80: The value returned into modelTotalTokens (from
config.get('modelTotalTokens')) must be normalized to an integer before
comparing; update the validation in IGlobal.py so that you attempt to coerce
modelTotalTokens to int (catching ValueError/TypeError) and if coercion fails or
the resulting integer is <= 0 emit the same warning('Token limit must be greater
than 0') and return, otherwise proceed using the parsed int; reference the
modelTotalTokens variable and the existing warning call to locate where to add
the int() conversion and error handling.
- Around line 142-152: The _format_error function currently drops the fallback
when only a status is present; change the final assembly so that if only status
is available (status set but both etype and emsg are falsey) the returned
message includes the fallback text (e.g., "Error {status}: {fallback}") instead
of just "Error {status}:"; update the logic around variables status, etype,
emsg, and fallback in _format_error to append or merge fallback when no
structured error/message exists so callers still see the raw exception text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7b958ae3-e6ae-4345-8ab1-29826783b088
📒 Files selected for processing (2)
nodes/src/nodes/llm_baidu_qianfan/IGlobal.pynodes/test/test_baidu_qianfan_global_validation.py
Contributor
Author
|
@asclearuc Hi Alexandru, addressed the CI blockers and validation nit.
Local checks passed: Qianfan validation tests, ruff check/format, contract/provider tests, py_compile, and icon audit. Thanks! |
asclearuc
approved these changes
Jun 4, 2026
Collaborator
|
@jimmyzhuu Thank you for the contribution! |
kwit75
pushed a commit
that referenced
this pull request
Jul 2, 2026
) (#1234) embedding_transformer (miniLM) and embedding_video (openai-patch16) download model weights from huggingface.co while test_dynamic.py runs, so when the HF hub is unreachable or rate-limited they fail and turn the only required check (ci-ok) red — on PRs unrelated to embeddings. This has hit docs-only PRs, the Baidu Qianfan #1006 run, and the 2026-06-04 release prep. Add both to the existing skip_nodes set in conftest.py, matching the embedding_image precedent (same heavy-model category). They remain runnable on demand via ROCKETRIDE_INCLUDE_SKIP=embedding_transformer,embedding_video. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Sep 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds first-class Baidu Qianfan / ERNIE support as a dedicated RocketRide LLM node.
Instead of requiring users to configure Baidu through the generic OpenAI-compatible node, the new
llm_baidu_qianfannode exposes Qianfan-specific defaults, provider branding, model profiles, runtime validation, sync-models support, and targeted tests.Closes #1005
Motivation
RocketRide already includes dedicated nodes for major LLM providers such as OpenAI, Anthropic, Gemini, Mistral, DeepSeek, Perplexity, xAI, and Qwen. Baidu Qianfan is an important provider for Chinese-language and enterprise AI workflows, and Qianfan now exposes OpenAI-compatible chat-completions endpoints.
A dedicated node improves the user experience in three ways:
What Changed
New node runtime
nodes/src/nodes/llm_baidu_qianfan/.IGlobal,IInstance, andqianfan_client.Chat.langchain_openai.ChatOpenAIagainst Qianfan's OpenAI-compatible API.https://qianfan.baidubce.com/v2.serverbaseconfigurable so users can switch endpoints, including Baidu's international endpoint when needed.Node metadata and profiles
services.jsonforllm_baidu_qianfan://.ernie-4.5-turbo-128kernie-4.5-turbo-32kernie-5.0-thinking-previewcustommodelSource: "provider".llm_baidu_qianfan.Shared UI
baidu-qianfan.svgin the shared UI icon resolver.THEME_DYNAMIC_ICONSbecause it is a colored brand asset and should not be filtered as a single-color icon.Model sync tooling
BaiduQianfanProvider.llm_baidu_qianfanin_PROVIDER_REGISTRYand_SERVICES_JSON_PATHS.sync_models.config.json.ROCKETRIDE_BAIDU_QIANFAN_KEYlive-test marker.tools/SYNC_MODELS.md.Implementation Notes
sk-. The runtime only validates that a non-empty key is configured because Baidu keys can use other formats.customremains protected from sync-models deprecation.Testing
Local focused tests:
PYTHONPATH=tools/src pytest tools/test/test_baidu_qianfan_provider.py -vPYTHONPATH=nodes/src uv run --with pytest --with pytest-asyncio --with pytest-timeout --with pytest-xdist --with pytest-mock --with json5 --with requests --with httpx --with openai pytest nodes/test/test_contracts.py -k llm_baidu_qianfan -vpython -m py_compile nodes/src/nodes/llm_baidu_qianfan/IGlobal.py nodes/src/nodes/llm_baidu_qianfan/qianfan_client.py tools/src/providers/baidu_qianfan.pygit diff --checkLive/API validation:
PYTHONPATH=tools/src uv run --with pytest --with json5 --with openai pytest tools/test/test_sync_live.py -k baidu_qianfan -vVerified this skips cleanly without credentials.
ROCKETRIDE_BAIDU_QIANFAN_KEY=*** PYTHONPATH=tools/src uv run --with pytest --with json5 --with openai pytest tools/test/test_sync_live.py -k baidu_qianfan -vVerified this passes with a real Qianfan key.
ernie-4.5-turbo-128k.ROCKETRIDE_BAIDU_QIANFAN_KEY=*** PYTHONPATH=tools/src uv run --with json5 --with openai python tools/src/sync_models.py --provider llm_baidu_qianfan --model-source providerVerified dry run reports no changes.
Local Environment Note
./builder nodes:test-contractswas not runnable in this local environment because the defaultpnpmis9.15.9, while this repository requirespnpm >=10.0.0. Direct pytest contract coverage for the new node passed.Security
Summary by CodeRabbit
New Features
Documentation
Tests
Chores