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

Added Support for the new /fim/completions route on Mistral AI API #467

Merged
merged 10 commits into from
Sep 19, 2024

Conversation

keshavkrishna
Copy link
Contributor

Support new /fim/completions route on Mistral AI API:

Description:

  • /fim/completions endpoint work in the same way as the /chat/completions route. It handles extra parameters such as suffix, prompt also.
  • Implemented fimCompletionsHandler to handle requests to the /fim/completions endpoint.
  • Updated the API configuration to include the new fimComplete endpoint.
  • implemented response and stream chunk transformations for the fimComplete function in Mistral AI.
  • Modified index.ts to register the new route v1//fim/completions with request validation.

Motivation:

  • To add support in portkey gateway for the new /fim/completions route on Mistral AI API

Related Issues:

@narengogi
Copy link
Contributor

Thank you for the contribution @keshavkrishna will review this soon

Copy link
Contributor

@narengogi narengogi left a comment

Choose a reason for hiding this comment

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

Looks good to me, will have to checkout in local and test the changes.

@VisargD do you think we should refactor the handlers to be more generic?
I think this is okay because we will not be seeing new types of inference endpoints that often, but new handlers feel redundant

src/providers/types.ts Outdated Show resolved Hide resolved
src/providers/types.ts Outdated Show resolved Hide resolved
@VisargD
Copy link
Collaborator

VisargD commented Aug 20, 2024

Hey @keshavkrishna! Will you be able to take a look at the comments? And please update your branch with the latest main branch changes

@keshavkrishna
Copy link
Contributor Author

Yes I will take care of these comments by this weekend. Is that timeline okay?

@VisargD VisargD requested a review from narengogi August 27, 2024 05:14
@VisargD
Copy link
Collaborator

VisargD commented Aug 28, 2024

Hey. Is it not possible to reuse the existing /chat/completions route? We can passthrough all the extra key as it is

@keshavkrishna
Copy link
Contributor Author

keshavkrishna commented Aug 30, 2024

Requests to the existing /chat/completions route are redirected to the provider's base URL with the endpoint chat/completions. In this scenario, we need to direct requests to the provider's base URL with the endpoint fim/completions.

Copy link
Contributor

@narengogi narengogi left a comment

Choose a reason for hiding this comment

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

Approving only need to make one last change as mentioned in prev comment

@VisargD
Copy link
Collaborator

VisargD commented Sep 2, 2024

We can accept a new header x-portkey-mistral-fim-completion: true and route it conditionally to the the mistral fim completions route. Adding a new router for provider specific routes is not the most feasible solution here because it will increase the number of routers in future.

EDIT: Adding extra details

  • In this function, gateway already passes providerOptions. Using it, you can check for a new param called providerOptions.mistralFimCompletion. If its true, you can change the endpoint to the mistral fim endpoint.
    getEndpoint: ({ fn }) => {
  • In the chatComplete config, you can add the extra fields which are required (like suffix, prompt, etc.). The response structure seems to be OpenAI compliant. So that should not be a problem

@keshavkrishna
Copy link
Contributor Author

Sure, I'll implement this

@keshavkrishna
Copy link
Contributor Author

@VisargD

If I am not getting mistralFimCompletion in providerOptions. following is provider option i am getting

{
  provider: 'mistral-ai',
  apiKey: '  SOME_API_KEY_HERE',
  overrideParams: {},
  retry: {},
  cache: {}
} 

so i have passed requestHeaders in getEndpioint, and i am checking by
if (requestHeaders['x-portkey-mistral-fim-completion'] === 'true')

Is that okay??

@VisargD
Copy link
Collaborator

VisargD commented Sep 11, 2024

@VisargD

If I am not getting mistralFimCompletion in providerOptions. following is provider option i am getting

{
  provider: 'mistral-ai',
  apiKey: '  SOME_API_KEY_HERE',
  overrideParams: {},
  retry: {},
  cache: {}
} 

so i have passed requestHeaders in getEndpioint, and i am checking by if (requestHeaders['x-portkey-mistral-fim-completion'] === 'true')

Is that okay??

Hey! specific headers checks might not be required. Here is what you can do:

  • In this function, add one more condition for mistral-ai provider and add the mapping for mistralFimCompletion in it. Check the existing conditions for other providers for reference. Once this is done, you will always get the mistralFimCompletion param in providerOptions.
    export function constructConfigFromRequestHeaders(

Please let me know if you want an example

Copy link
Contributor

@narengogi narengogi left a comment

Choose a reason for hiding this comment

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

Is this working? I've noticed you've not made any changes in providers/mistral-ai/index.ts for request and response transformers

@keshavkrishna
Copy link
Contributor Author

Yes it is. We are using chatComplete transforms as per visarg's comment.

@VisargD
Copy link
Collaborator

VisargD commented Sep 18, 2024

@keshavkrishna - Can you please run npm run format and also update with main branch? I can merge it now

@keshavkrishna
Copy link
Contributor Author

@VisargD as requested i have updated my branch and also formatted it

@VisargD VisargD merged commit 114895d into Portkey-AI:main Sep 19, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants