-
Notifications
You must be signed in to change notification settings - Fork 683
Plugin/databricks #2196
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Plugin/databricks #2196
Conversation
- Corrected inputs to include databricks_token and databricks_host - Remove customizable-model configuration method
…tion - Fix provider class name from DatabricksModelProvider to DatabricksProvider - Implement provider credential validation using model instance - Add OpenAI client creation for Databricks endpoints - Implement validate_credentials with actual API call - Add _convert_prompt_messages helper for message format conversion - Add required imports: OpenAI, Stream, ChatCompletion, message types - Remove unused imports (cast, json)
…pport - Implement _chat_generate method for chat completions - Add _handle_generate_response for non-streaming responses - Update _invoke to call _chat_generate - Add tool calling support in request preparation - Add assistant message extraction with tool calls - Add usage calculation for token tracking This commit enables basic LLM invocation with non-streaming responses and tool calling support.
- Implement _handle_generate_stream_response for streaming responses - Add content streaming with LLMResultChunk generation - Add tool call accumulation during streaming - Add usage tracking in final chunk - Handle streaming deltas for incremental content delivery This commit enables streaming responses with real-time content delivery and tool calling support.
- Add databricks-claude-3-7-sonnet.yaml with full feature support - Add __init__.py for Python module structure - Update manifest.yaml with proper model permissions - Improve description in manifest.yaml This commit adds the predefined Claude 3.7 Sonnet model configuration with support for agent-thought, vision, tool-call, stream-tool-call, and document features.
Model Additions: - Add databricks-claude-sonnet-4, databricks-claude-opus-4-1, databricks-claude-sonnet-4-5 model configuration Parameter Fixes: - Remove unsupported response_format from all models, top_p from databricks-claude-sonnet-4-5 Bug Fixes: - Add missing _invoke_error_mapping property to fix plugin initialization - Fix icon references in provider configuration - Remove GPT-3.5 model file (llm.yaml) - Update .gitignore to exclude test scripts
- Update max_tokens limits for all Claude models based on Anthropic documentation:
- Claude 3.7 Sonnet, Sonnet 4, Sonnet 4.5: 64K
- Claude Opus 4.1: 32K
- Replace template icons with Databricks-branded assets
- Update help documentation URL to Databricks PAT docs
- Clean up manifest.yaml removing unnecessary fields
- Improve README with configuration instructions
Summary of ChangesHello @joyaung, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new official Databricks model plugin for Dify, enabling users to integrate with models deployed on Databricks Model Serving endpoints. The initial implementation focuses on supporting various Claude models, providing robust functionality for chat completions, tool calling, and streaming responses. The plugin also includes a comprehensive set of configuration files, Python logic for API interaction and credential validation, and an automated GitHub Actions workflow to facilitate its publishing and maintenance. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new Dify plugin for Databricks models. The implementation correctly uses the OpenAI library to interface with Databricks endpoints. However, there are several critical and high-severity issues that need to be addressed. The streaming tool call handling is flawed and will not work correctly. Token counting is not implemented, which will lead to incorrect usage metrics. Additionally, a referenced icon file is missing, which will cause a broken image in the UI. There are also several medium-severity suggestions to improve the robustness of the CI/CD workflow, ensure consistency in model definitions, and provide better error handling.
| if delta.tool_calls: | ||
| for tool_call_delta in delta.tool_calls: | ||
| if tool_call_delta.id: | ||
| tool_calls.append({ | ||
| "id": tool_call_delta.id, | ||
| "type": "function", | ||
| "function": { | ||
| "name": tool_call_delta.function.name if tool_call_delta.function else "", | ||
| "arguments": tool_call_delta.function.arguments if tool_call_delta.function else "", | ||
| } | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic for handling streaming tool calls is incorrect. It appends a new tool call for each chunk that contains a tool_call_delta.id, but it doesn't correctly accumulate the arguments for a single tool call which may be streamed across multiple chunks. This will result in incomplete or malformed tool calls.
The correct approach is to build up the tool calls based on their index from the stream.
| if delta.tool_calls: | |
| for tool_call_delta in delta.tool_calls: | |
| if tool_call_delta.id: | |
| tool_calls.append({ | |
| "id": tool_call_delta.id, | |
| "type": "function", | |
| "function": { | |
| "name": tool_call_delta.function.name if tool_call_delta.function else "", | |
| "arguments": tool_call_delta.function.arguments if tool_call_delta.function else "", | |
| } | |
| }) | |
| if delta.tool_calls: | |
| for tc_chunk in delta.tool_calls: | |
| if len(tool_calls) <= tc_chunk.index: | |
| # A new tool call is being streamed, prepare a placeholder | |
| tool_calls.append({"id": "", "type": "function", "function": {"name": "", "arguments": ""}}) | |
| # Accumulate parts of the tool call | |
| if tc_chunk.id: | |
| tool_calls[tc_chunk.index]["id"] = tc_chunk.id | |
| if tc_chunk.function: | |
| if tc_chunk.function.name: | |
| tool_calls[tc_chunk.index]["function"]["name"] += tc_chunk.function.name | |
| if tc_chunk.function.arguments: | |
| tool_calls[tc_chunk.index]["function"]["arguments"] += tc_chunk.function.arguments |
| :param tools: tools for tool calling | ||
| :return: | ||
| """ | ||
| return 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The get_num_tokens method is implemented to always return 0. This will result in incorrect token usage calculation and reporting within Dify, which can affect cost tracking and usage limits. A proper implementation should use a tokenizer to accurately count the tokens for the given messages and tools. If a tokenizer is not available, this limitation should be clearly documented.
| return 0 | |
| # TODO: Implement accurate token counting. Currently, this will cause incorrect usage reporting. | |
| return 0 |
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| mkdir -p $RUNNER_TEMP/bin | ||
| cd $RUNNER_TEMP/bin | ||
|
|
||
| wget https://github.com/langgenius/dify-plugin-daemon/releases/download/0.0.6/dify-plugin-linux-amd64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| tree || ls -R | ||
|
|
||
| - name: Checkout target repo | ||
| uses: actions/checkout@v3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
| git branch -a | ||
| echo "Waiting for branch to sync..." | ||
| sleep 10 # Wait 10 seconds for branch sync |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a fixed sleep is an unreliable way to handle synchronization. The time required for the branch to sync can vary, which could lead to intermittent workflow failures. A more robust solution would be to poll for the branch's existence or use a dedicated action that waits for the branch to become available.
| parameter_rules: | ||
| - name: temperature | ||
| use_template: temperature | ||
| - name: max_tokens | ||
| use_template: max_tokens | ||
| required: true | ||
| min: 1 | ||
| max: 64000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The top_p parameter rule is missing from this model's definition, while it is present in other Claude models in this PR (e.g., databricks-claude-sonnet-4). For consistency and to allow users to control this parameter, it should be added.
parameter_rules:
- name: temperature
use_template: temperature
- name: top_p
use_template: top_p
- name: max_tokens
use_template: max_tokens
required: true
min: 1
max: 64000|
|
||
| :return: Invoke error mapping | ||
| """ | ||
| return {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The _invoke_error_mapping property currently returns an empty dictionary. It's a good practice to map provider-specific exceptions (from the openai library in this case) to Dify's standard InvokeError subclasses. This provides more granular and user-friendly error information.
For example:
from openai import APIConnectionError, RateLimitError
from dify_plugin.errors.model import ModelUnavailableError, RateLimitError as DifyRateLimitError
return {
ModelUnavailableError: [APIConnectionError],
DifyRateLimitError: [RateLimitError],
}| @@ -0,0 +1 @@ | |||
| dify_plugin>=0.4.0,<0.7.0 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
will address the gemini comments soon |
crazywoola
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the PRIVACY.md as well
Related Issues or Context
This PR adds a new official Databricks model plugin that enables integration with models deployed on Databricks Model Serving endpoints. Currently, only Claude models are added. Gemini, OpenAI and custom models will be added in the future.
PR will close #2119.
This PR contains Changes to Non-Plugin
This PR contains Changes to Non-LLM Models Plugin
This PR contains Changes to LLM Models Plugin
Version Control (Any Changes to the Plugin Will Require Bumping the Version)
VersionField, Not in Meta Section)Dify Plugin SDK Version
dify_plugin>=0.3.0,<0.6.0is in requirements.txt (SDK docs)Environment Verification (If Any Code Changes)
Local Deployment Environment
SaaS Environment