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

Updated checkPromptType function to handle prompt list in completions #885

Merged

Conversation

AyushSawant18588
Copy link
Contributor

@AyushSawant18588 AyushSawant18588 commented Oct 22, 2024

Describe the change
The checkPromptType function has been updated to support validation of []interface{} slices, ensuring that each element in the slice is of type string.
Currently if list of string is provided in prompt field ErrCompletionRequestPromptTypeNotSupported error is returned.
Example:
for this completions request-

{
	"model": "gemma-vllm",
	"prompt": ["Explain transformers in atelast 100 words or more","Hello whoare you"],
	"max_tokens": 150
}

This error will occur-

Completion error: the type of CompletionRequest.Prompt only supports string and []string

This is because arrays in JSON are decoded into Go as slices of interface{} ([]interface{}), not as slices of a specific type like []string. Therefore, when a JSON request containing a list of strings is unmarshalled, the resulting value of request.Prompt is a []interface{}. The checkPromptType function only checked for string and []string, so its returning false when request.Prompt is a []interface{} even if all elements within that slice are strings, because []interface{} is not the same type as []string.

This change allows better validation of prompt types, accommodating the scenario where the prompt is provided as a list of strings in an interface{} slice.

Describe your solution
The solution involves adding a new type check for []interface{} in the checkPromptType function. The code checks if prompt is of type []interface{} and, if so, iterates over the slice to ensure all elements are strings. If any element is not a string, the validation fails. This logic is integrated alongside the existing checks for string and []string types. This enhancement ensures that the function properly handles various input types for prompts and raises the ErrCompletionRequestPromptTypeNotSupported error if an unsupported type is provided.

Copy link

codecov bot commented Oct 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.86%. Comparing base (774fc9d) to head (413dc3a).
Report is 67 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #885      +/-   ##
==========================================
+ Coverage   98.46%   98.86%   +0.40%     
==========================================
  Files          24       26       +2     
  Lines        1364     1761     +397     
==========================================
+ Hits         1343     1741     +398     
+ Misses         15       14       -1     
  Partials        6        6              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AyushSawant18588
Copy link
Contributor Author

@sashabaranov Can you please look into this?

@johnugeorge
Copy link

@sashabaranov Can you review? Currently, completions api is not working due to this bug

completion.go Outdated
}
}
}
return isString || isStringSlice || isInterfaceSlice
Copy link
Owner

Choose a reason for hiding this comment

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

@AyushSawant18588 thank you for the PR! Could we please use a less nested version of this with early returns? Something like this https://go.dev/play/p/cS9jrYnWrvN

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah sure I will check and update the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the PR @sashabaranov, can you please take a look?

@sashabaranov
Copy link
Owner

Thank you!

@sashabaranov sashabaranov merged commit 6e08732 into sashabaranov:master Oct 25, 2024
3 checks passed
@AyushSawant18588
Copy link
Contributor Author

@sashabaranov Can you please create a release? I need to upgrade my go-openai client with the above changes. Thanks!

@sashabaranov
Copy link
Owner

@AyushSawant18588 sure, I was waiting for #882 to be merged, but it seems it would take some time!

Note: you can always swap to your fork as well!

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