-
Notifications
You must be signed in to change notification settings - Fork 342
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
Do not make any extra API call when endpoint URL is directly specified #1322
Conversation
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.
Pull Request Overview
This PR updates the inference API request logic to avoid making unnecessary API calls when a direct endpoint URL is provided. Key changes include:
- Making the "model" property optional in BodyParams.
- Adding a fast-path in makeRequestOptions to bypass model resolution when an endpoint URL is supplied.
- Adjusting the signature and usage of makeRequestOptionsFromResolvedModel to accommodate the optional model input.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
packages/inference/src/types.ts | Changed "model" from required to optional in BodyParams. |
packages/inference/src/lib/makeRequestOptions.ts | Added branch logic for endpoint URL and updated request options construction accordingly. |
Comments suppressed due to low confidence (2)
packages/inference/src/types.ts:130
- Making the 'model' field optional may lead to cases where a required model is not provided when an endpoint URL is absent. Ensure that downstream logic either supplies a valid model ID or handles the undefined case appropriately.
model?: string;
packages/inference/src/lib/makeRequestOptions.ts:210
- When using the endpointUrl branch, 'input.resolvedModel' might be undefined. Consider adding validation or a fallback to guarantee that a model value is provided when needed by providerConfig.makeBody.
model: input.resolvedModel,
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.
ty
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! Just a nit regarding the potentially undefined model
.
a greater refactoring would need to take place (eg to remove all concept of providerConfig when endpointUrl is provided, use a separate default provider config maybe not relying on model being defined)
What we do on the Python side is to use the "hf-inference"
provider when an endpoint is passed. And in the hf-inference logic we take care of handling things correctly. If a user passes provider="<any-provider-except-hf-inference>", endpoint="https://..."
we raise an exception.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
I'm a bit afraid this PR and #1315 will highly conflict 😕 cc @hanouticelina |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
0c9cbe6
to
712ed1f
Compare
Fix #1321 (cc @Kakulukian for viz)
The fix is not perfect, a greater refactoring would need to take place (eg to remove all concept of
providerConfig
whenendpointUrl
is provided, use a separate default provider config maybe not relying onmodel
being defined)