-
Notifications
You must be signed in to change notification settings - Fork 121
Make validate_credentials max_tokens configurable (preserve stream default 10) #275
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
Conversation
Summary of ChangesHello @Yeuoly, 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 enhances the flexibility of credential validation for OpenAI-compatible LLMs by making the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 configurability for max_tokens during credential validation, which is a good improvement for provider compatibility. The changes correctly handle defaults for both streaming and non-streaming modes. My review focuses on improving the robustness of handling the max_tokens value from credentials. I've identified that the value is not being converted to an integer, which could lead to API errors if it's provided as a string. I've provided suggestions to enforce type conversion to int and ensure consistent behavior for default fallbacks. These changes will make the validation logic more resilient.
|
|
||
| # prepare the payload for a simple ping to the model | ||
| data = {"model": credentials.get("endpoint_model_name", model), "max_tokens": 5} | ||
| validate_credentials_max_tokens = credentials.get("validate_credentials_max_tokens", 5) or 5 |
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 value for max_tokens is read from credentials but it's not guaranteed to be an integer. If validate_credentials_max_tokens is a string (e.g., "10"), it will be passed as a string in the JSON payload, which will likely cause an API error. The value should be explicitly cast to int. The current logic also handles integer 0 and string "0" differently.
The suggested change ensures the value is an integer and that any falsy value (like 0, "0", or None) correctly falls back to the default of 5.
Note: This will raise a ValueError if a non-numeric string is provided. This is generally good for failing fast, but a try-except block could be used for more graceful error handling.
| validate_credentials_max_tokens = credentials.get("validate_credentials_max_tokens", 5) or 5 | |
| validate_credentials_max_tokens = int(credentials.get("validate_credentials_max_tokens") or 0) or 5 |
| # ADD stream validate_credentials | ||
| stream_mode_auth = credentials.get("stream_mode_auth", "not_use") | ||
| if stream_mode_auth == "use": | ||
| stream_validate_max_tokens = credentials.get("validate_credentials_max_tokens") or 10 |
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.
Similar to the non-streaming case, the max_tokens value here should be converted to an integer to avoid potential API errors when the value in credentials is a string.
This change ensures the value is an int and that falsy values like 0, "0", or None correctly fall back to the default of 10, providing consistent behavior.
Note: This will raise a ValueError if a non-numeric string is provided. This is generally good for failing fast, but a try-except block could be used for more graceful error handling.
| stream_validate_max_tokens = credentials.get("validate_credentials_max_tokens") or 10 | |
| stream_validate_max_tokens = int(credentials.get("validate_credentials_max_tokens") or 0) or 10 |
Mairuis
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.
LGTM
Motivation
max_tokensused during credentials validation must be configurable.max_tokens=10to ensure a token chunk is emitted, so that behavior should be preserved by default.pyproject.tomlversion bump as requested to avoid changing the published SDK version.Description
validate_credentials_max_tokensfromcredentialsand use it as the pingmax_tokensfor non-stream validation inpython/dify_plugin/interfaces/model/openai_compatible/llm.pywith a default fallback of5.max_tokens=10but allow override whenvalidate_credentials_max_tokensis explicitly provided incredentials, and add an inline comment explaining the rationale.llm.pyto apply the configurable values in both validation flows.python/pyproject.tomlas requested (the lockfile changes were not reverted here).Testing
python/dify_plugin/interfaces/model/openai_compatible/llm.pyandpython/pyproject.toml(revert).git commit) were used to record the change and no runtime validation was performed.Codex Task