-
Notifications
You must be signed in to change notification settings - Fork 596
[Inference Providers] fix outputType for all providers #1919
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
Wauplin
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.
Looks good 👍
| } | ||
| if (outputType === "url") { | ||
| console.warn("hyperbolic provider does not support URL output, returning base64 data URL instead"); | ||
| return `data:image/jpeg;base64,${response.images[0].image}`; |
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.
Personally, I'd expect an exception to be thrown rather than an alternative format. IMO choosing the outputType should be left to the user, based on the formats supported by the providers
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.
indeed!
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.
addressed in ba01d73
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.
could we add a new dataUrl type then? because here we are introducing a breaking change so at least we should provide a drop-in replacement I think
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.
^yes agree, i added dataUrl type in 7e18606
regarding the breaking change, I first thought it was okay since it was kind of a bug to return base64 URLs when outputType: "url" (similar to response_format: "url" in other APIs), users would expect an actual http URL but agree about adding a drop-in replacement 👍
related to this comment (private link).
When using
textToImagewithoutputType: "url", together and nebius providers returnedbase64URLs instead of actual http URLs. Some providers don't support URL format :hf-inference,nscale,hyperbolic.cc @kefranabg