Add a dedicated OpenAI-compatible LLM adapter#1895
Open
jimmyzhuu wants to merge 5 commits intoZipstack:mainfrom
Open
Add a dedicated OpenAI-compatible LLM adapter#1895jimmyzhuu wants to merge 5 commits intoZipstack:mainfrom
jimmyzhuu wants to merge 5 commits intoZipstack:mainfrom
Conversation
|
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
Summary by CodeRabbit
WalkthroughAdds support for OpenAI-compatible LLM providers: new Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Adapter as OpenAI Compatible Adapter
participant Validator as Parameter Validator
participant LLM as LLM Instance
participant TokenCounter as Token Counter
participant Audit as Audit System
App->>Adapter: create/instantiate with config
Adapter->>Validator: validate(adapter_metadata)
Validator->>Validator: normalize model -> custom_openai/{model}
Validator-->>Adapter: return validated params
App->>LLM: trigger _record_usage(usage_data)
alt usage_data contains prompt_tokens
LLM-->>TokenCounter: (skipped)
else usage_data missing prompt_tokens
LLM->>TokenCounter: token_counter(model, messages)
alt TokenCounter succeeds
TokenCounter-->>LLM: prompt token count
else TokenCounter raises
TokenCounter-->>LLM: exception
LLM->>LLM: catch, set prompt_tokens = 0, log warning
end
end
LLM->>Audit: push_usage_data(with prompt_tokens)
Audit-->>LLM: acknowledge
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
for more information, see https://pre-commit.ci
Contributor
|
| Filename | Overview |
|---|---|
| unstract/sdk1/src/unstract/sdk1/adapters/llm1/openai_compatible.py | New adapter class — clean implementation following existing patterns; all required static methods present |
| unstract/sdk1/src/unstract/sdk1/adapters/base1.py | Adds OpenAICompatibleLLMParameters with correct custom_openai/ prefix guard and optional api_key |
| unstract/sdk1/src/unstract/sdk1/llm.py | Adds graceful token_counter fallback in _record_usage; logic is correct but final usage_data.get call is redundant |
| unstract/sdk1/src/unstract/sdk1/adapters/llm1/static/custom_openai.json | Well-formed schema; api_key correctly typed as nullable, model has a sensible gpt-4o-mini default |
| unstract/sdk1/tests/test_openai_compatible_adapter.py | Good coverage of registration, prefix normalisation, schema loading, and usage-recording fallback; _load_llm_class() is dead code |
| unstract/sdk1/src/unstract/sdk1/adapters/llm1/init.py | Import and all entry added correctly for the new adapter |
| README.md | Table updated to include OpenAI Compatible; last row now has an empty right cell due to the odd provider count |
Class Diagram
%%{init: {'theme': 'neutral'}}%%
classDiagram
class BaseAdapter {
<<abstract>>
+get_id()* str
+get_name()* str
+get_description()* str
+get_provider()* str
+get_icon()* str
+get_json_schema() str
+get_adapter_type()* AdapterTypes
}
class BaseChatCompletionParameters {
+model: str
+temperature: float
+n: int
+timeout: float
+max_tokens: int
+max_retries: int
+validate()* dict
+validate_model()* str
}
class OpenAICompatibleLLMParameters {
+api_key: str | None
+api_base: str
+validate(adapter_metadata) dict
+validate_model(adapter_metadata) str
}
class OpenAICompatibleLLMAdapter {
+get_id() str
+get_name() str
+get_provider() str
+get_adapter_type() AdapterTypes
}
class OpenAILLMParameters {
+api_key: str
+api_base: str
+api_version: str
}
BaseChatCompletionParameters --|> BaseAdapter : inherits
OpenAICompatibleLLMParameters --|> BaseChatCompletionParameters : inherits
OpenAICompatibleLLMAdapter --|> OpenAICompatibleLLMParameters : inherits
OpenAICompatibleLLMAdapter --|> BaseAdapter : inherits
OpenAILLMParameters --|> BaseChatCompletionParameters : inherits
Prompt To Fix All With AI
This is a comment left during a code review.
Path: unstract/sdk1/tests/test_openai_compatible_adapter.py
Line: 28-29
Comment:
**`_load_llm_class()` is unused dead code**
This helper is defined but never called — every test function reaches into `_load_llm_module().LLM` directly. If it was meant to replace those inline accesses, the tests should use it; otherwise it can be removed to avoid confusion for future readers.
```suggestion
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: unstract/sdk1/src/unstract/sdk1/llm.py
Line: 556-557
Comment:
**Redundant dict lookup obscures intent**
After the `if prompt_tokens is None:` block, `prompt_tokens` is already guaranteed to be non-`None` (either from `usage_data`, the `token_counter` result, or the `= 0` fallback). Re-fetching from `usage_data` with a default is therefore redundant and slightly obscures the logic.
Simplifying to the local variable makes the intent clearer:
```suggestion
all_tokens = TokenCounterCompat(
prompt_tokens=prompt_tokens or 0,
```
How can I resolve this? If you propose a fix, please make it concise.Greploops — Automatically fix all review issues by running /greploops in Claude Code. It iterates: fix, push, re-review, repeat until 5/5 confidence.
Use the Greptile plugin for Claude Code to query reviews, search comments, and manage custom context directly from your terminal.
Reviews (2): Last reviewed commit: "[pre-commit.ci] auto fixes from pre-comm..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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
🧹 Nitpick comments (1)
unstract/sdk1/src/unstract/sdk1/llm.py (1)
542-557: Avoid unconditional token estimation when usage already includes prompt tokens.This currently computes
token_counter()even when provider usage already has prompt tokens, which can create repeated warnings/noise for unmapped models without improving recorded usage.♻️ Proposed refinement
- try: - prompt_tokens = token_counter(model=model, messages=messages) - except Exception as e: - prompt_tokens = 0 - logger.warning( - "[sdk1][LLM][%s][%s] Failed to estimate prompt tokens: %s", - model, - llm_api, - e, - ) usage_data: Mapping[str, int] = usage or {} + prompt_tokens = usage_data.get("prompt_tokens") + if prompt_tokens is None: + try: + prompt_tokens = token_counter(model=model, messages=messages) + except Exception as e: + prompt_tokens = 0 + logger.warning( + "[sdk1][LLM][%s][%s] Failed to estimate prompt tokens: %s", + model, + llm_api, + e, + ) all_tokens = TokenCounterCompat( - prompt_tokens=usage_data.get("prompt_tokens", 0), + prompt_tokens=usage_data.get("prompt_tokens", prompt_tokens or 0), completion_tokens=usage_data.get("completion_tokens", 0), total_tokens=usage_data.get("total_tokens", 0), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@unstract/sdk1/src/unstract/sdk1/llm.py` around lines 542 - 557, The code unconditionally calls token_counter(model, messages) even when usage already contains prompt token counts; change the logic in the block around token_counter and TokenCounterCompat so you first check usage (usage_data = usage or {}) and if usage_data.get("prompt_tokens") is present use that value for prompt_tokens instead of calling token_counter; only call token_counter(model, messages) inside the try/except when usage_data lacks prompt_tokens, preserving the existing exception handling and the logger.warning path, and then construct TokenCounterCompat using the values from usage_data (falling back to the estimated prompt_tokens when used).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@unstract/sdk1/src/unstract/sdk1/adapters/llm1/static/custom_openai.json`:
- Around line 15-20: The schema for the "api_key" property currently only allows
a string which fails when runtime metadata contains null; update the "api_key"
entry in the JSON schema (the "api_key" property in custom_openai.json) to
permit null values by changing its type to accept both string and null (or add a
nullable:true equivalent) so stored configs with null pass validation and
editing flows.
---
Nitpick comments:
In `@unstract/sdk1/src/unstract/sdk1/llm.py`:
- Around line 542-557: The code unconditionally calls token_counter(model,
messages) even when usage already contains prompt token counts; change the logic
in the block around token_counter and TokenCounterCompat so you first check
usage (usage_data = usage or {}) and if usage_data.get("prompt_tokens") is
present use that value for prompt_tokens instead of calling token_counter; only
call token_counter(model, messages) inside the try/except when usage_data lacks
prompt_tokens, preserving the existing exception handling and the logger.warning
path, and then construct TokenCounterCompat using the values from usage_data
(falling back to the estimated prompt_tokens when used).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bf841637-54b7-4802-9156-7f56e899ca54
📒 Files selected for processing (7)
README.mdunstract/sdk1/src/unstract/sdk1/adapters/base1.pyunstract/sdk1/src/unstract/sdk1/adapters/llm1/__init__.pyunstract/sdk1/src/unstract/sdk1/adapters/llm1/openai_compatible.pyunstract/sdk1/src/unstract/sdk1/adapters/llm1/static/custom_openai.jsonunstract/sdk1/src/unstract/sdk1/llm.pyunstract/sdk1/tests/test_openai_compatible_adapter.py
Author
|
Addressed the review follow-ups.
Validation re-run:
|
|
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 a dedicated
OpenAI CompatibleLLM adapter for OpenAI-style chat completion endpoints that are not the official OpenAI service.The implementation is intentionally small in scope:
OpenAI CompatibleLLM adapter backed by LiteLLM'scustom_openaipathOpenAIadapter unchangedWhy
Users may already have access to OpenAI-compatible endpoints behind a private gateway or third-party provider, but the current
OpenAIadapter is specifically shaped around official OpenAI semantics.Using a separate adapter keeps those semantics explicit and avoids broadening the meaning of the existing
OpenAIadapter.Refs #1894
Refs #856
Refs #1443
Scope
This PR is limited to:
OpenAIadapter behaviorNotes
For custom models that are not mapped in LiteLLM's token metadata, usage recording now falls back gracefully instead of failing while estimating prompt tokens.
This keeps the adapter usable for custom OpenAI-compatible endpoints without changing pricing semantics in this PR.
Validation
UV_SKIP_WHEEL_FILENAME_CHECK=1 uv run pytest tests/test_openai_compatible_adapter.pyUV_SKIP_WHEEL_FILENAME_CHECK=1 uv run ruff check src/unstract/sdk1/adapters/base1.py src/unstract/sdk1/adapters/llm1/__init__.py src/unstract/sdk1/adapters/llm1/openai_compatible.py src/unstract/sdk1/llm.py tests/test_openai_compatible_adapter.py