-
Notifications
You must be signed in to change notification settings - Fork 991
feat: add CerebrasProvider
#1867
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?
Conversation
hitting
let me know how you'd like to solve this Line 130 in 7a7ca1e
|
PR Change SummaryAdded the
Modified Files
How can I customize these reviews?Check out the Hyperlint AI Reviewer docs for more information on how to customize the review. If you just want to ignore it on this PR, you can add the Note specifically for link checks, we only check the first 30 links in a file and we cache the results for several hours (for instance, if you just added a page, you might experience this). Our recommendation is to add |
def client(self) -> AsyncOpenAI: | ||
return self._client | ||
|
||
def model_profile(self, model_name: str) -> ModelProfile | None: |
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 think we should do automatic model profile selection here based on the underlying model used, like we do in TogetherProvider
. We can still override the json_schema_transformer
unconditionally, like we do there as well.
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'm a bit puzzled on
provider, model_name = model_name.split('/', 1)
if provider in provider_to_profile:
profile = provider_to_profile[provider](model_name)
in TogetherProvider
.
I dont fully grasp the idea behind what's meant to be a "provider" in this case. Should CerebrasProvider
support alternative model name syntax like Qwen/Qwen-3-32b
instead of what's listed in their docs: qwen-3-32b
?
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.
@smallstepman What we're trying to do is to use the regular model names that Cerebras expects to choose the right model profile for the model in question, based on the prefix. In the case of TogetherProvider
their model names have a <provider>/
prefix, but for example in GroqProvider
you can see how we match on a simple qwen
prefix and then use qwen_model_provider
. We'll want to do the same here for all models Cerebras supports that match models we already have model profiles for.
@smallstepman Is this Qwen specific or do all models used with Cerebras need this? Either way we'll want a new subclass as |
@smallstepman You can add |
it's weird, but 75% of the models need this (the only exception is |
please advise on how to solve failing coverage check |
def __init__(self, schema: JsonSchema, *, strict: bool | None = None): | ||
super().__init__(schema, strict=strict, prefer_inlined_defs=True, simplify_nullable_unions=False) | ||
|
||
def transform(self, schema: JsonSchema) -> JsonSchema: |
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.
Can we please reduce the duplication here with the GoogleJsonSchemaTransformer
, perhaps by subclassing it?
def client(self) -> AsyncOpenAI: | ||
return self._client | ||
|
||
def model_profile(self, model_name: str) -> ModelProfile | None: |
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.
@smallstepman What we're trying to do is to use the regular model names that Cerebras expects to choose the right model profile for the model in question, based on the prefix. In the case of TogetherProvider
their model names have a <provider>/
prefix, but for example in GroqProvider
you can see how we match on a simple qwen
prefix and then use qwen_model_provider
. We'll want to do the same here for all models Cerebras supports that match models we already have model profiles for.
@smallstepman When we remove the duplicated code in the json schema transformer by inheriting from one of the existing ones instead, this issue should go away! |
Co-authored-by: Douwe Maan <[email protected]>
as discussed in #1826
based on
GrokProvider
from #1842json schema transformer compatibility
I found out setting
simplify_nullable_unions
toFalse
improves the compatibility, as it circumvents the following error:pydantic_ai.exceptions.ModelHTTPError: status_code: 400, model_name: qwen-3-32b, body: {'message': "Unsupported JSON schema fields: {'nullable'}", 'type': 'invalid_request_error', 'param': 'response_format', 'code': 'wrong_api_format'}
I had to manually set it for
let me know if I can either:
simplify_nullable_unions
via__init__
params with default set toTrue
(current value in upstream), orGoogleJsonSchemaTransformer
test with