feat: Adding code for sending parameter "response_format" as request payload #2317
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2317 +/- ##
=======================================
Coverage 84.50% 84.50%
=======================================
Files 326 326
Lines 16755 16810 +55
Branches 1480 1498 +18
=======================================
+ Hits 14158 14205 +47
- Misses 2597 2605 +8 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
| @@ -55,11 +132,20 @@ class OpenAIChatCompletion(override val uid: String) extends OpenAIServicesBase( | |||
| override def responseDataType: DataType = ChatCompletionResponse.schema | |||
|
|
|||
| private[this] def getStringEntity(messages: Seq[Row], optionalParams: Map[String, Any]): StringEntity = { | |||
| val mappedMessages: Seq[Map[String, String]] = messages.map { m => | |||
| var mappedMessages: Seq[Map[String, String]] = messages.map { m => | |||
There was a problem hiding this comment.
nit: please no vars in code unless required. In scala any kind of mutability is highly discouraged. Here in this case you can return either the original or the original plus your addition in the if statement
There was a problem hiding this comment.
made mappedMessages val again
Comment on lines
112
to
142
| val responseFormat = new Param[String]( | ||
| this, "responseFormat", "The response format from the OpenAI API.") | ||
|
|
||
| def getResponseFormat: String = $(responseFormat) | ||
|
|
||
| def setResponseFormat(value: String): this.type = { | ||
| if (value.isEmpty) { | ||
| this | ||
| } else { | ||
| val normalizedValue = value.toLowerCase match { | ||
| case "json" => "json_object" | ||
| case other => other | ||
| } | ||
|
|
||
| // Validate the normalized value using the OpenAIResponseFormat enum | ||
| if (!OpenAIResponseFormat.values | ||
| .map(_.asInstanceOf[OpenAIResponseFormat.ResponseFormat].name) | ||
| .contains(normalizedValue)) { | ||
| throw new IllegalArgumentException("Response format must be valid for OpenAI API. " + | ||
| "Currently supported formats are " + OpenAIResponseFormat.values | ||
| .map(_.asInstanceOf[OpenAIResponseFormat.ResponseFormat].name) | ||
| .mkString(", ")) | ||
| } | ||
|
|
||
| set(responseFormat, normalizedValue) | ||
| } | ||
| } | ||
|
|
||
| def setResponseFormat(value: OpenAIResponseFormat.ResponseFormat): this.type = { | ||
| this.setResponseFormat(value.name) | ||
| } |
There was a problem hiding this comment.
this code seems duplicated, can it be abstracted and shared?
There was a problem hiding this comment.
I am now using shared code.
| assert(nonNullCount == 4) | ||
| } | ||
|
|
||
| test("Basic Usage - Gpt 4o with response format text") { |
Comment on lines
295
to
311
| test("setResponseFormat should set the response format correctly for valid values") { | ||
| val prompt = new OpenAIPrompt() | ||
| prompt.setResponseFormat("text") | ||
| prompt.getResponseFormat should be ("text") | ||
|
|
||
| prompt.setResponseFormat("json") | ||
| prompt.getResponseFormat should be ("json_object") | ||
|
|
||
| prompt.setResponseFormat("json_object") | ||
| prompt.getResponseFormat should be ("json_object") | ||
|
|
||
| prompt.setResponseFormat("jSoN") | ||
| prompt.getResponseFormat should be ("json_object") | ||
|
|
||
| prompt.setResponseFormat("TEXT") | ||
| prompt.getResponseFormat should be ("text") | ||
| } |
There was a problem hiding this comment.
these tests look duplicated from above, if the core parameter flexibility is shared between the two we can keep just 1 copy and still have same basic coverage
|
Awesome! Only minor nits! |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…ts can be reused, in this iteration, I am removing this possibility. Now each test create a new OpenAIPrompt
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related Issues/PRs
For newer models, openai accepts
response_format, which can betextorjson_object. This PR adds the parameter for this. Additionally, when developer setpostprocessingoptions, inOpenAIPromptclass, post processing field is reduncdant and can be made optional. We cannot infer post processing from response format because json as response format is not supported by many models including GPT-4 and GPT-3.5-Turbo. All changes made in this PR as backward compatible and will not break any existing code.What changes are proposed in this pull request?
In this PR, we are adding:
response_formatinOpenAIPromptclass andOpenAIChatCompletionclass.postprocessingfrompostprocessingoptionsand hence making it optional.How is this patch tested?
Unit tests and integration tests are added to validate this change
Does this PR change any dependencies?
Does this PR add a new feature? If so, have you added samples on website?