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

fix: reject grammars without properties #2309

Merged
merged 1 commit into from
Jul 29, 2024
Merged

Conversation

drbh
Copy link
Collaborator

@drbh drbh commented Jul 25, 2024

This PR is adds a new check to grammar validation that ensures that the grammar contains properties

For a grammar to be valid it needs to 1. be a valid JSON Schema, and 2. have properties that are used to constrain the output.

This PR is related to
pr: #2282
issue: #2280

req/res example

The following request would crash TGI due to outlines failing to parse the grammar, now an error is returned because value does not include properties

curl localhost:3000/generate \
    -X POST \
    -H 'Content-Type: application/json' \
    -d '{
        "inputs": "My name is Olivier and I",
        "parameters": {
            "grammar": {"type": "json", "value": {"abc":"def"}}
            }
        }'

now returns

{
    "error": "Input validation error: grammar is not valid: Grammar must have a 'properties' field",
    "error_type": "validation"
}

@ErikKaum
Copy link
Member

Nice 👍 this is the 80% catch that I wasn't able to find!

@ErikKaum ErikKaum self-assigned this Jul 29, 2024
@ErikKaum ErikKaum self-requested a review July 29, 2024 13:48
Copy link
Member

@ErikKaum ErikKaum 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 f15e808 into main Jul 29, 2024
9 checks passed
@drbh drbh deleted the ensure-grammars-have-properties branch July 29, 2024 14:07
yuanwu2017 pushed a commit to yuanwu2017/tgi-gaudi that referenced this pull request Sep 26, 2024
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.

2 participants