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

Pr 2982 ci branch #3046

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Pr 2982 ci branch #3046

wants to merge 6 commits into from

Conversation

drbh
Copy link
Collaborator

@drbh drbh commented Feb 20, 2025

This PR reopens #2982 to run CI

@drbh drbh force-pushed the pr-2982-ci-branch branch from 262b6b1 to 36fb0d7 Compare February 20, 2025 23:32
@drbh
Copy link
Collaborator Author

drbh commented Feb 26, 2025

This PR correctly adds the response type json_schema as a {"response_format": {"type: "...", requires a value of the expected schema.

This change emphasizes an existing issue with json_object which currently functions the same as json_schema (requires a schema) but should be updated to produce JSON with no expected schema.

As this PR is non breaking an only adds the correct functionality. It may make sense to follow up with the changes to the json_object in another PR and include these changes on main

Issues related to enabling json_object vs json_schema

#3058
#2899
#2900

@jorado
Copy link

jorado commented Feb 27, 2025

Thanks for adding the json_schema response type!

While this PR does implement it, I'm wondering if it might miss an opportunity to better align with OpenAI's specification, which was a key point in issue #3058. The key difference between OpenAI and TGI isn't just about response_format json_schema, but also the structure of the response_format itself. OpenAI uses a nested json_schema structure, while TGI currently uses json_object (and now json_schema in this PR) with a value field. Also, if we wanted to improve json_schema compatibility further down the line, changing it from your proposed implementation will introduce breaking changes.

Instead of making another alias, I'd propose to change the required response format for json_schema to the following, while keeping the structure of json_object/json for compatibility reasons. I'd agree that making the value field optional for json_object/json can be done in another PR.

  response_format: {
    "type": "json_schema",
    "json_schema": {
    "name": "some_name",
    "strict": true,
    "schema":  ... // the actual json schema
    }
  }

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