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

bugfix: convert several providers stop_reason to valid openai finish_reason #363

Closed

Conversation

unsync
Copy link

@unsync unsync commented May 14, 2024

Description:
Currently, we forward the anthropic stop_reason, which is not compatible with the openai finish_reason.

This can cause warnings from some OpenAI sdk.

The conversion function returns stop as a default value in case new stop_reason are added.

There was also a bug in the groq and google providers for the finish_reason

Side note:
It would probably be beneficial to update the types to we can get compilation-time validation :

/**
* The structure of a completion response for the 'complete' function.
* @interface
*/
export interface CompletionResponse extends CResponse {
choices: {
text: string;
index: number;
logprobs: null;
finish_reason: string;
}[];
}
/**
* The structure of a choice in a chat completion response.
* @interface
*/
export interface ChatChoice {
index: number;
message: Message;
finish_reason: string;
logprobs?: object | null;
}

Proposed change :

export interface ChatChoice {
  index: number;
  message: Message;
  finish_reason: 'stop' | 'length' | 'function_call' | 'content_filter' | null;
  logprobs?: object | null;
}
export interface CompletionResponse extends CResponse {
  choices: {
    text: string;
    index: number;
    logprobs: null;
    finish_reason: 'stop' | 'length' | 'function_call' | 'content_filter' | null;
  }[];
}

see https://platform.openai.com/docs/guides/text-generation/chat-completions-response-format

@unsync unsync force-pushed the feat/improve-anthropic-stop-reason branch 2 times, most recently from 1e5a6ee to ae228b9 Compare May 16, 2024 15:16
@unsync unsync changed the title Fix: convert anthropic stop_reason to finish_reason bugfix: convert several providers stop_reason to valid openai finish_reason May 16, 2024
@unsync unsync force-pushed the feat/improve-anthropic-stop-reason branch 3 times, most recently from 351fff8 to ff93296 Compare May 27, 2024 08:22
@unsync unsync force-pushed the feat/improve-anthropic-stop-reason branch from ff93296 to b5a650e Compare June 5, 2024 14:33
@unsync
Copy link
Author

unsync commented Jun 6, 2024

hello @vrushankportkey @VisargD, i'm not sure if there is a process after opening a PR to request a review, any chance this PR could be considered ?

@vrushankportkey
Copy link
Collaborator

Thank you so much @unsync! Yes, we're checking the PR and will share review if any in some time! Really appreciate you pushing this bugfix 🙌

@unsync unsync force-pushed the feat/improve-anthropic-stop-reason branch from b5a650e to 05eff29 Compare June 27, 2024 13:49
@roh26it roh26it requested review from narengogi and VisargD July 5, 2024 06:16
@@ -345,6 +345,22 @@ export const AnthropicErrorResponseTransform: (
return undefined;
};

// this converts the anthropic stop_reason to an openai finish_reason
const getFinishReason = (stopReason?: string): string | null => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Recommend creating a type

type AnthropicStopReason = 'max_tokens' | 'stop_sequence' | 'end_turn' | ....

Copy link
Contributor

Choose a reason for hiding this comment

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

also recommend using a switch/case

@@ -394,7 +410,8 @@ export const AnthropicChatCompleteResponseTransform: (
},
index: 0,
logprobs: null,
finish_reason: response.stop_reason,
finish_reason:
getFinishReason(response.stop_reason) || response.stop_reason,
Copy link
Contributor

Choose a reason for hiding this comment

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

This should just be getFinishReason(response.stop_reason), Otherwise it defeats the purpose of strictly typing the response

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.

Thanks for the Pull Request! Requested a few changes

@@ -226,6 +226,28 @@ interface GoogleGenerateContentResponse {
};
}

// this converts the google stop_reason to an openai finish_reason
// https://ai.google.dev/api/python/google/ai/generativelanguage/Candidate/FinishReason
const getFinishReason = (stopReason?: string): string | null => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comments as in the getFinishReason transformer function above.

  1. Use switch/case
  2. Define types for GoogleStopReason

@@ -291,7 +313,9 @@ export const GoogleChatCompleteResponseTransform: (
return {
message: message,
index: generation.index,
finish_reason: generation.finishReason,
finish_reason:
getFinishReason(generation.finishReason) ??
Copy link
Contributor

Choose a reason for hiding this comment

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

should only be

finish_reason:
           getFinishReason(generation.finishReason) 

default case should be handled inside getFinishReason

@narengogi
Copy link
Contributor

@unsync I've created a fork of your branch to do some more thorough changes, it has your exisiting commits #468

@unsync
Copy link
Author

unsync commented Jul 22, 2024

@unsync I've created a fork of your branch to do some more thorough changes, it has your exisiting commits #468

@narengogi that is great !
I think your PR is a better candidate for merge than mine, i think my PR can be close in favour of yours ?

cc @vrushankportkey

@vrushankportkey
Copy link
Collaborator

Thanks @unsync & @narengogi!

@unsync unsync closed this Jul 22, 2024
@unsync unsync deleted the feat/improve-anthropic-stop-reason branch July 22, 2024 07:53
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