-
Notifications
You must be signed in to change notification settings - Fork 227
feat: add Dashscope (Qwen) LLM client support #687
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: develop
Are you sure you want to change the base?
Conversation
Seems the dashscope platform updated to sfm(bailian). Use the naming for client might not be a good idea. |
@kid1412621 In their official SDKs, they still call themselves as |
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.
thanks a lot!
lgtm!
Have you tested this client locally?
Could you also update to the latest changes from develop
branch and add the logic with additionalProperties?
* @property promptTokensDetails Breakdown of tokens used in the prompt. | ||
*/ | ||
@Serializable | ||
public class DashscopeUsage( |
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.
Is the usage of DashScope different from OpenAI?
I think they’re the same, so you could reuse OpenAIUsage
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.
I had found a slight different after testing locally.
/**
* @property audioTokens Audio input tokens generated by the model.
* @property cachedTokens Cached tokens present in the prompt.
*/
@Serializable
public class PromptTokensDetails(
public val audioTokens: Int? = null,
public val cachedTokens: Int,
)
In older versions of Koog, the audioTokens
property of PromptTokensDetails
does not have a default value and will cause SerializationException
for missing field with Dashscope endpoints.
In commit 6b2283a1fb733e95ccc28453f3c09cf30588dad1
, this property was changed nullable and having a null
default value. So I agree with your idea.
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.
OK. Refactored.
* @property maxTokens The maximum number of tokens to generate. | ||
* @property toolChoice Controls which (if any) tool is called by the model. | ||
*/ | ||
public data class DashscopeParams( |
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.
DashscopeParams
should inherit from LLMParams
. But if it doesn’t extend LLMParams
, then it’s better not to use it at all
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.
OK. Refactored.
/** | ||
* Dashscope LLM models enumeration. | ||
*/ | ||
public object DashscopeModels : LLModelDefinitions { |
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.
In the documentation I see many more models – https://modelstudio.console.alibabacloud.com/?tab=doc#/doc/?type=model&url=2840914
Could you please add the latest Qwen3 models and remove the old turbo?
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.
OK. Added.
Motivation and Context
Alibaba cloud serves Qwen models and they have a different API format. This PR adds support to use models on Alibaba dashscope platform.
Resolves #529.
Breaking Changes
No breaking change.
Type of the changes
New feature (non-breaking change which adds functionality)
Checklist
develop
as the base branchAdditional steps for pull requests adding a new feature