[Inference] Move provider-specific logic away from makeRequestOptions (1 provider == 1 module)

[Wauplin]

Goal of this PR is to move away any provider-specific logic from makeRequestOptions.ts. In theory no tests should be updated as we only want to modify the internal logic without modifying input/output behavior.

Feedback around structure, TS convention, naming, etc. is welcome.

How it works ?

Each provider must define a providerConfigconsisting of:

export interface ProviderConfig {
	baseUrl: string;
	makeBody: (params: BodyParams) => unknown;
	makeHeaders: (params: HeaderParams) => Record<string, string>;
	makeUrl: (params: UrlParams) => string;
}

(with HeaderParams, UrlParams, BodyParams the parameters required to build these)

[SBrandeis]

I wonder if a class with static method would be more idiomatic - wdyt @coyotte508 @julien-c ?

[Wauplin]

As an example, here is how a class would look like (transcribed using hf.co/chat). In Python I usually tend to avoid static methods as it's more boilerplate and usually don't benefit from being a class. But happy to change if it's more idiomatic in JS!

export abstract class ProviderConfig {
	static baseUrl: string;
	static makeBody(params: BodyParams): unknown {
		throw new Error("Not implemented");
	}
	static makeHeaders(params: HeaderParams): Record<string, string> {
		throw new Error("Not implemented");
	}
	static makeUrl(params: UrlParams): string {
		throw new Error("Not implemented");
	}
}

(...)

export class ReplicateConfig extends ProviderConfig {
	static baseUrl = "https://api.replicate.com";

	static makeBody({ args, model }: BodyParams): unknown {
		return {
			input: args,
			version: model.includes(":") ? model.split(":")[1] : undefined,
		};
	}

	static makeHeaders({ accessToken }: HeaderParams): Record<string, string> {
		return {
			Authorization: `Bearer ${accessToken}`,
			"Content-Type": "application/json",
			Prefer: "Wait",
		};
	}

	static makeUrl({ baseUrl, model }: UrlParams): string {
		if (model.includes(":")) {
			return `${baseUrl}/v1/predictions`;
		}
		return `${baseUrl}/v1/models/${model}/predictions`;
	}
}

[Wauplin]

Note that in huggingface_hub we use classes because we decided to go with "1 class == 1 provider<>task pair" and we benefit from inheritance (example).

[SBrandeis]

Love it!!
I think using classes instead of interfaces might be slightly more idiomatic - would love opinion from @coyotte508 and @julien-c

[Wauplin]

I completed all the providers. Now hf-inference is treated the same way as the other ones.

There is still a topic around the data structure to adopt (interface vs class with static methods - see #1208 (comment)). Happy to adapt once agreed on it.

[SBrandeis]

LGTM, I have a few code style suggestions.

Subsequently to this PR, I thinl we should also factor task-specific logic within the provider modules.

[SBrandeis]

Independent from the changes introduced here: I dislike this binary data special case, it's not obvious from an external pov that passing data in arguments will result in the payload being passed as raw bytes in the body.

[Wauplin]

(addressed the code style comments, thanks for the advice)

[SBrandeis]

LGTM, thanks!
Would love to have another review from @coyotte508 and/or @julien-c

[coyotte508]

Regarding static classes etc, I think current code is ok, it's already typed correctly:

export const SAMBANOVA_CONFIG: ProviderConfig = {

Alternative solution would be to type the file itself, I think it's possible (with declare) and that sveltekit does something similar. It's more complicated but would allow users importing specfic functions (makeBody, ...) directly while still preserving typing, and possibly reduce overhead.

Or it's possible to use abstract classes and export local class instances inheriting the base class in each provider file - without static.

But tbh I think the way it's currently done is the right one, at least for my personal preferences, it's what I would have done too.

Maybe maybe, exporting a typed namespace if at all possible would be my preferred choice but not worth spending time on it

[julien-c]

reviewing right after lunch 🍔

[Wauplin]

I just pushed 6bd79c6 to fix the conflict issue + switch from taskHint to task (from #1204)

[julien-c]

this is nice!

personally re. static class vs. interface, intuitively i would have done a non-static instance i.E. inheritance. But that's just personal bias, i think your solution is perfectly fine.

Let's 🚢 !! (and ping our partners so they see we refactored things a bit? i can do it if needed)

[julien-c]

we don't have any provider that takes a binary body but needs to have it transformed, i guess

[julien-c]

(linting errors are not my fault AFAIK :)

[Wauplin]

CI fails with

packages/hub test: ⎯⎯⎯⎯⎯⎯⎯ Failed Tests 1 ⎯⎯⎯⎯⎯⎯⎯
packages/hub test:  FAIL  src/utils/WebBlob.spec.ts > WebBlob > should lazy load a LFS file hosted on Hugging Face
packages/hub test: Error: Test timed out in 5000ms.
packages/hub test: If this is a long-running test, pass a timeout value as the last argument or configure it globally with "testTimeout".

which seems unrelated to this PR. Let's get it merged!