-
Notifications
You must be signed in to change notification settings - Fork 304
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
feat: Huggingface provider integration #321
base: main
Are you sure you want to change the base?
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.
Couple of changes but mostly looks good. I think the SubProvider
methodology makes sense to me 👍
self | ||
} | ||
|
||
pub fn sub_provider(mut self, provider: SubProvider) -> Self { |
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.
pub fn sub_provider(mut self, provider: SubProvider) -> Self { | |
pub fn sub_provider(mut self, provider: impl Into<SubProvider>) -> Self { |
This makes it convenient to pass custom strings, though you still have to create impl From<&str>
since FromStr
is different 😭.
Custom(String), | ||
} | ||
|
||
impl FromStr for SubProvider { |
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.
btw, FromStr
!= From<&str>
unfortuntely
} | ||
|
||
pub fn build(self) -> Client { | ||
let route = match self.sub_provider.clone() { |
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 would implement ToString
for SubProvider
here so it's consistent with the enum, then you can use that directly
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.
Makes sense
let mut request = self.create_request_body(&completion_request)?; | ||
|
||
// Enable streaming | ||
merge_inplace(&mut request, json!({"stream": true})); |
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.
This is nice, I think our other streaming stuff could leverage this pattern in a more standardized / trait way (like in #320)
// } | ||
// _ => panic!("Expected user message"), | ||
// } | ||
} |
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.
Should this still be commented?
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.
Nope
Adds support for Huggingface models.
Closes #36.