Skip to content
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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

yavens
Copy link
Collaborator

@yavens yavens commented Feb 25, 2025

Adds support for Huggingface models.
Closes #36.

@yavens yavens changed the title feat/huggingface feat: Huggingface provider integration Feb 25, 2025
Copy link
Contributor

@0xMochan 0xMochan left a 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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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 {
Copy link
Contributor

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() {
Copy link
Contributor

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

Copy link
Collaborator Author

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}));
Copy link
Contributor

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"),
// }
}
Copy link
Contributor

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: Add support for HuggingFace hosted models
2 participants