-
Notifications
You must be signed in to change notification settings - Fork 854
fix: ordered json_schema response #2436
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
fix: ordered json_schema response #2436
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2436 +/- ##
==========================================
+ Coverage 84.41% 84.46% +0.05%
==========================================
Files 333 334 +1
Lines 17550 17591 +41
Branches 1580 1603 +23
==========================================
+ Hits 14815 14859 +44
+ Misses 2735 2732 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…psingh/SynapseML into rana/ordered_json_schema
|
/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). |
...ive/src/main/scala/com/microsoft/azure/synapse/ml/services/openai/OpenAIChatCompletion.scala
Outdated
Show resolved
Hide resolved
mhamilton723
left a comment
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.
Minor things
...src/test/scala/com/microsoft/azure/synapse/ml/services/openai/ResponseFormatOrderSuite.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/com/microsoft/azure/synapse/ml/codegen/Wrappable.scala
Show resolved
Hide resolved
core/src/main/scala/com/microsoft/azure/synapse/ml/param/JsonEncodableParam.scala
Outdated
Show resolved
Hide resolved
|
/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). |
|
/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). |
This reverts commit c6aee7d.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| ) | ||
| if (value.contains("strict")) base + ("strict" -> value("strict")) else base | ||
| } else { | ||
| val js = value("json_schema").asInstanceOf[Map[String, Any]] |
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.
Are we checking the type of json_schema upstream before we cast here? Consider pattern match instead
| val hasSchema = value.contains("schema") | ||
| if (!hasName || !hasSchema) { | ||
| throw new IllegalArgumentException( | ||
| "Bare schema Map must include 'name' and 'schema' keys.") |
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.
nit: consider indicating which keys have been specified and which are missing
| js.get("strict").map(v => base ++ Map("strict" -> v)).getOrElse(base) | ||
| } | ||
| } | ||
| // Validation helpers moved into ResponseFormatUtils |
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.
nit: unnecessary
| if (hasFlat) { | ||
| val base = Map( | ||
| "type" -> "json_schema", | ||
| "name" -> value("name"), |
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.
validation for name + schema?
| if (value.contains("strict")) base + ("strict" -> value("strict")) else base | ||
| } else { | ||
| val js = value("json_schema").asInstanceOf[Map[String, Any]] | ||
| val name = js.getOrElse("name", |
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.
does this validate that name/schema is nonempty/nonnull or just that the keys exist?
| throw new IllegalArgumentException( | ||
| "json_schema requires nested 'json_schema' or top-level 'name' and 'schema'.") | ||
| } | ||
| if (hasFlat) { |
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.
Should we error/alert if the user erroneously specifies both json_schema + name/schema?
| val payload = | ||
| if (normalized.get("type").exists(_.toString.equalsIgnoreCase("json_schema"))) { | ||
| Map( | ||
| "type" -> "json_schema", |
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.
nit: consider elevating "json_schema" "name" and "schema" to vals to reduce duplication and avoid potential typo errors if any of these strings need to be updated in the future
Related Issues/PRs
feat: Add json_schema as responseFormat option #2428
What changes are proposed in this pull request?
How is this patch tested?
Does this PR change any dependencies?
Does this PR add a new feature? If so, have you added samples on website?
website/docs/documentationfolder.Make sure you choose the correct class
estimators/transformersand namespace.DocTablepoints to correct API link.yarn run startto make sure the website renders correctly.<!--pytest-codeblocks:cont-->before each python code blocks to enable auto-tests for python samples.WebsiteSamplesTestsjob pass in the pipeline.