Skip to content

Pr 2982 ci branch #3046

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

Merged
merged 10 commits into from
May 1, 2025
Merged

Pr 2982 ci branch #3046

merged 10 commits into from
May 1, 2025

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
    }
  }

@drbh drbh force-pushed the pr-2982-ci-branch branch from 26ccffe to 8d16071 Compare March 14, 2025 18:10
@drbh
Copy link
Collaborator Author

drbh commented Mar 17, 2025

this PR has been updated to handle response_format.type == "json_schema" formats as shown in the example below.

# model id: meta-llama/Meta-Llama-3.1-8B-Instruct
import requests
import json

# simple person JSON schema
person_schema = {
    "type": "object",
    "properties": {
        "firstName": {
            "type": "string",
            "description": "The person's first name.",
            "minLength": 3,
        },
        "lastName": {
            "type": "string",
            "description": "The person's last name.",
            "minLength": 3,
        },
        "age": {"type": "integer", "minimum": 0},
    },
    "required": ["firstName", "lastName", "age"],
}

response = requests.post(
    "http://localhost:3000/v1/chat/completions",
    json={
        "model": "model-name",
        "messages": [
            {
                "role": "user",
                "content": "John Smith is a 32-year-old software engineer.",
            }
        ],
        "temperature": 0.0,
        "response_format": {
            "type": "json_schema",
            "value": {"name": "person", "strict": True, "schema": person_schema},
        },
    },
    headers={"Content-Type": "application/json"},
)

print(json.dumps(response.json(), indent=2))

with the response:

{
  "object": "chat.completion",
  "id": "",
  "created": 1742224552,
  "model": "model-name",
  "system_fingerprint": "3.1.1-dev0-native",
  "choices": [
    {
      "index": 0,
      "message": {
        "role": "assistant",
        "content": "{ \"firstName\": \"John\", \"lastName\": \"Smith\", \"age\": 32 }"
      },
      "logprobs": null,
      "finish_reason": "stop"
    }
  ],
  "usage": {
    "prompt_tokens": 46,
    "completion_tokens": 23,
    "total_tokens": 69
  }
}

@drbh drbh force-pushed the pr-2982-ci-branch branch from 4e4d2bc to 65c6008 Compare March 21, 2025 15:15
Copy link

@jorado jorado left a comment

Choose a reason for hiding this comment

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

Other than this fix, everything looks good to me! Let me know if there’s anything else I can help with.

assert called == '{ "unit": "fahrenheit", "temperature": [ 72, 79, 88 ] }'
assert chat_completion == response_snapshot

json_payload["response_format"]["type"] = "json_schema"
Copy link

@jorado jorado Apr 29, 2025

Choose a reason for hiding this comment

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

Suggested change
json_payload["response_format"]["type"] = "json_schema"
json_payload["response_format"] = {
"type": "json_schema",
"value": {"name": "weather", "strict": True, "schema": Weather.schema()},
}

@drbh drbh force-pushed the pr-2982-ci-branch branch from 65c6008 to 1d63285 Compare April 30, 2025 16:46
Copy link
Collaborator

@Narsil Narsil left a comment

Choose a reason for hiding this comment

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

LGTM

@drbh drbh merged commit 12ea8d7 into main May 1, 2025
33 checks passed
@drbh drbh deleted the pr-2982-ci-branch branch May 1, 2025 14:17
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.

4 participants