Skip to content

Add OVHcloud as an inference provider #1303

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

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

Conversation

fabienric
Copy link

What

Adds OVHcloud as an inference provider.

Test Plan

Added new tests for OVHcloud both with and without streaming.

What Should Reviewers Focus On?

I used the Cerebras PR as an example.

@julien-c julien-c added the inference-providers integration of a new or existing Inference Provider label Apr 2, 2025
@julien-c
Copy link
Member

julien-c commented Apr 2, 2025

Hi @fabienric we are currently finishing a refactoring of Inference Providers integration code in #1315, this should be merged soon, but we will need to rewrite part of your implementation (should be even simpler to integrate), will ping again after it's been merged.

@hanouticelina
Copy link
Contributor

Hi @fabienric,
We've merged a refactoring for Inference Provider integration into main, which should make adding new providers much easier.
Could you merge main into your branch and update your PR accordingly? it should be relatively straightforward with the new structure:
1 - You have to update the PROVIDERS mapping here: inference/src/lib/getProviderHelper.ts#L49 and add ovhcloud (let's ensure we respect the alphabetical order) :

import * as OvhCloud from "../providers/ovhcloud";
...
export const PROVIDERS: Record<InferenceProvider, Partial<Record<InferenceTask, TaskProviderHelper>>> = {
	...
	"ovhcloud": {
		"conversational": new OvhCloud.OvhCloudConversationalTask(),
	},
        ...

2 - Update packages/inference/src/providers/ovhcloud.ts to implement OvhCloudConversationalTask that inherits BaseConversationalTask:

import { BaseConversationalTask, BaseTextGenerationTask } from "./providerHelper";

export class OvhCloudConversationalTask extends BaseConversationalTask {
	constructor() {
		super("ovhcloud", "https://oai.endpoints.kepler.ai.cloud.ovh.net");
	}
}

and that's it :) let us know if you need any help! you can find more details in the documentation : https://huggingface.co/docs/inference-providers/register-as-a-provider#2-js-client-integration.

@julien-c
Copy link
Member

julien-c commented Apr 8, 2025

(sorry for the moving parts @fabienric – we can help move this PR over the finish line if needed)

Fabien Ric added 2 commits April 14, 2025 16:12
- ovhcloud inference provider: use new base tasks and provider helpers, fix issues with inference parameters, add support for text generation task
@fabienric
Copy link
Author

fabienric commented Apr 14, 2025

Hi @hanouticelina and @julien-c,

Thank you for the feedback, refactoring and updated documentation.

I've implemented our provider ; it required more work than I expected to get the payload right for an OpenAI compatible endpoint (make sure that the seed, max_tokens and other generation parameters are correctly mapped: the base implementation put them in a parameters dictionary, which is ignored on our end). Maybe other providers are impacted by this issue?

I've also implemented the text generation task, but I've found that the streaming case is not covered by the base task (the getResponse returns a Promise<TextGenerationOutput> where we would need a way to return a TextGenerationStreamOutput). My test case passes using ChatCompletionStreamOutput but this doesn't seem right. Maybe I missed something here.

Available to discuss the matter further if required.

@Wauplin
Copy link
Contributor

Wauplin commented Apr 22, 2025

Maybe other providers are impacted by this issue?

Actually most (if not all) providers are implemented only for the conversational task which is by far the most used task. It covers both text-generation and image-text-to-text pipelines as long as the model is tagged as conversational (i.e. it has a chat template, etc.). That is why the text-generation implementation has been left aside a bit for now. Let us know if the intention is indeed to cover text-generation AND conversational, in which case we can think about better improvements for text-generation (streaming option, OAI-compatible API, etc.). I do think it'll better to ship conversational-only for now to get the integration running and then go back to "non-conversational text-generation"

@fabienric
Copy link
Author

fabienric commented Apr 22, 2025

Hi @Wauplin and thanks for your feedback.

Actually my remark on the OpenAI compatible parameters also applies to the conversational case ; let me know if it's not the case.

I agree with you on the fact that the priority on our side is to get the conversational use case running. I can remove the code related to text-generation if needed but I think we can leave it as is.

Let me know when you're ready to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inference-providers integration of a new or existing Inference Provider
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants