-
Notifications
You must be signed in to change notification settings - Fork 862
Add OVHcloud AI Endpoints as an Inference Provder #3541
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
Add OVHcloud AI Endpoints as an Inference Provder #3541
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
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.
Hey @eliasto , thanks for the PR! I've made a first pass, please let us know when it's not in draft anymore or if you have any questions :)
docs/source/en/guides/inference.md
Outdated
| | --------------------------------------------------- | ----------------- | -------- | -------- | ------ | ------ | -------------- | ------------ | ---- | ------------ | ---------- | ---------------- | --------- | ------ | -------- | ---------- | --------- | --------- | --------- | -------- | --------- | ---- | | ||
| | [`~InferenceClient.audio_classification`] | ❌ | ❌ | ❌ | ❌ | ❌ | ❌ | ❌ | ❌ | ✅ | ❌ | ❌ | ❌ | ❌ | ❌ | ❌ | ❌ | ❌ | ❌ | ❌ | ❌ | ❌ | | ||
| | [`~InferenceClient.audio_to_audio`] | ❌ | ❌ | ❌ | ❌ | ❌ | ❌ | ❌ | ❌ | ✅ | ❌ | ❌ | ❌ | ❌ | ❌ | ❌ | ❌ | ❌ | ❌ | ❌ | ❌ | ❌ | | ||
| | [`~InferenceClient.automatic_speech_recognition`] | ❌ | ❌ | ❌ | ❌ | ✅ | ❌ | ❌ | ❌ | ✅ | ❌ | ❌ | ❌ | ❌ | ✅ | ❌ | ❌ | ❌ | ❌ | ❌ | ❌ | ❌ | |
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.
table to be updated accordingly to keep only conversational and text-generation
| def _prepare_payload_as_dict( | ||
| self, messages: Any, parameters: dict, provider_mapping_info: InferenceProviderMapping | ||
| ) -> Optional[dict]: | ||
| return super()._prepare_payload_as_dict(messages, parameters, provider_mapping_info) |
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 it's best to inherit from the base conversational task class instead of defining a base OVH cloud class. You can check an example for featherless-ai for instance which also defines a task provider for conversational and text-generation.
| def __init__(self, task: str): | ||
| super().__init__(provider=_PROVIDER, base_url=_BASE_URL, task=task) | ||
|
|
||
| def _prepare_route(self, mapped_model: str, api_key: str) -> str: |
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.
better to have this method defined "per task" rather than in the base one using if statements
|
Hi @Wauplin, thank you for your complete review! I just pushed the new changes, let me know if it's correct for you. 😄 |
249da97 to
01c7439
Compare
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.
Will have to test that properly later but sharing early feedback for you. Thanks for iterating, it almost looks good to me :)
| _BASE_URL = "https://oai.endpoints.kepler.ai.cloud.ovh.net" | ||
|
|
||
|
|
||
| class OVHcloudAIEndpointsConversationalTask(BaseConversationalTask): |
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.
| class OVHcloudAIEndpointsConversationalTask(BaseConversationalTask): | |
| class OVHCloudConversationalTask(BaseConversationalTask): |
(nit) could we simplify the name? ovhcloud will be the provider name that users will see
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.
You right, I edited. Thank you for your help again! 😄
| def _prepare_route(self, mapped_model: str, api_key: str) -> str: | ||
| return "/v1/chat/completions" |
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.
no need for that one (already the default route for conversational)
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.
| def _prepare_route(self, mapped_model: str, api_key: str) -> str: | |
| return "/v1/chat/completions" |
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.
Done! 😄
| def _prepare_route(self, mapped_model: str, api_key: str) -> str: | ||
| return "/v1/chat/completions" |
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.
are you sure about that one? usually /v1/chat/completions is the route for conversational endpoint. text-generation is the task for raw text generation i.e. without server-side chat template rendering
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.
Oh you right! I did a bad copy-paste. Thank you for noticing it! I edited. 😄
# Conflicts: # docs/source/en/guides/inference.md
61eb19b to
9ff4047
Compare
|
Thanks again for your wonderful help @Wauplin. Do you think I should make this PR ready when the billing integration is done on your side, or we do not need to wait for it? 😄 |
Billing is an orthogonal problem so it's fine to move forward on this PR. You'll still need billing for a proper go-live on the platform but just for the Python SDK it's fine. For now only OVH and HF users will be able to test it. |
|
I've given a try to the implementation with from huggingface_hub import InferenceClient
completion = InferenceClient(provider="ovhcloud").chat.completions.create(
model="openai/gpt-oss-20b",
messages=[{"role": "user", "content": "What is the capital of France?"}],
)
print(completion.choices[0].message)and I can confirm it works as expected! |
|
@Wauplin Sounds great! Perfect! I changed the PR to ready. 😄 |
| }, | ||
| "ovhcloud": { | ||
| "conversational": OVHcloudConversationalTask(), | ||
| "text-generation": OVHcloudTextGenerationTask(), |
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.
In practice, do you have an example of text-generation-only model that is deployed on OVH-side?
Asking because most (if not all) users are interested in conversational models, i.e. on the Chat Completion and Responses APIs. The text-generation task is only useful for models generating text but not using a chat template e.g. gpt2-like models. It is not possible to register a model both as "conversational" and as "text-generation".
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.
Looking at https://huggingface.co/api/partners/ovhcloud/models, it looks like only conversational models have been registered. If that's the plan, I would suggest to get rid of the OVHcloudTextGenerationTask class entirely. Makes everything cleaner and more aligned with most other 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.
You right, we only have conversational models. I removed the text generation capability! 😄
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.
All good! Thanks for the iterations on this :)
|
@bot /style |
|
Style bot fixed some files and pushed the changes. |
|
Failing CI is unrelated. Let's merge! |
* Add OVHcloud AI Endpoints provider * Only add text-generation and conversational task from feedback * Edit name of class and text-generation * Remove text_generation capability * Apply style fixes --------- Co-authored-by: Lucain <[email protected]> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Hi team!
You will find this PR to add OVhcloud AI Endpoints as an Inference Provider, following the PR from HuggingFace.js.
Thank you for your review! 😄