-
Notifications
You must be signed in to change notification settings - Fork 342
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
[Inference] Implement a "1 class = 1 provider<>task pair" logic to isolate provider-specific code #1315
Conversation
i think you mean .ts, otherwise i'm a bit concerned by this PR 😆 |
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.
Dunno where the "self-contained logic for each provider <> task"- idea comes from but I really like it! 😄
I tried to review the PR the best I can but hard to be exhaustive 😬 Overall very nice improvement
packages/inference/src/tasks/audio/automaticSpeechRecognition.ts
Outdated
Show resolved
Hide resolved
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 a big PR - I would like to do some other rounds of review later to ensure I did not miss something :)
Great work overall - it will make contributing a provider much easier 💪
I'm a bit disappointed by the type narrowing (there is too much need for type casts to my taste) - especially when calling providerHelper.getResponse()
Perhaps we need an additional asbtract class
per task to narrow down types in getProviderHelper
..
I have left some comments on the form too (mostly code style)
…ce.js into refactor-providers
@hanouticelina I have opened #1332 with a proposal to improve the typing of Let me know what you think |
This PR introduces task helpers to achieve better typing of the `getProviderHelper`, this is inspired by @SBrandeis demonstration PR #1332, without reversing the mapping. There is a lot of diff (sorry for that) because I isolated every HF inference API-specific code into the provider file and there are a lot of supported tasks 😬 --------- Co-authored-by: SBrandeis <[email protected]>
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.
Let's go! 🚀
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.
Let get this merged! 🎉
merging! |
Woohoo! 🎉 |
The Goal of this PR is to isolate provider-specific logic into its own class/file. Similar to what we have in the Python client, this gives a clear boundary between generic task implementation and provider-specific implementation details.
This makes updating or adding a new provider much easier, adding support for a new provider will be as much as easier as:
packages/inference/src/providers/{provider_name}.ts
.Main changes
TaskProviderHelper
abstract class that defines a common interface (makeBody
,preparePayload
makeRoute
,getResponse
,prepareHeaders
, etc.) that all provider-specific helpers must implement.PROVIDERS
registry that maps any provider<>task to the correspondingTaskProviderHelper
subclass that implements the provider<>task logic.getProviderHelper
) that takes a provider and task and returns the appropriate TaskProviderHelper instance from thePROVIDERS
registry.🙏 Any feedback is welcome as the implementation is quite biased towards the Python but we might want to do some things differently here.