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

json: restore default additionalProperties to false, fix some pattern escapes #8180

Merged
merged 4 commits into from
Jun 28, 2024

Conversation

ochafik
Copy link
Collaborator

@ochafik ochafik commented Jun 28, 2024

Testing more schemas after #7840, I'm getting an alarming rate of hallucinations on property names. The change of default for additionalProperties may have been somewhat principled but it seems bad in practice (@HanClinto you did warn me, should've listened 😅).

Switching back the default of additionalProperties to false (despite the spec defaulting it to true) requires less caveating in the docs (to avoid slower grammars), and produces less hallucinations (otherwise models can be tempted to add props that look much like known properties and just add some chars to them). Also, additionalProperty keys don't currently ensure their newlines are properly escaped 🫣.

This PR makes the following example from #5978 to work well again (needed extra lil fix of pattern escapes to work again):

./llama-cli --grammar-file \                                          
  <( python examples/json_schema_to_grammar.py https://json.schemastore.org/tsconfig.json )  \
  -p "Write a tsconfig.json for a simple project with strict types incremental compiler/build options:" \
  -mu https://huggingface.co/bartowski/Phi-3-medium-128k-instruct-GGUF/resolve/main/Phi-3-medium-128k-instruct-Q8_0.gguf

@ochafik ochafik marked this pull request as ready for review June 28, 2024 01:41
@github-actions github-actions bot added testing Everything test related examples python python script changes server labels Jun 28, 2024
Copy link
Collaborator

@HanClinto HanClinto left a comment

Choose a reason for hiding this comment

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

Sounds good, looks good!

To be fair, I didn't know that it would increase hallucinations, but in hindsight this makes sense. Nice work!

BTW, there is trailing whitespace in test-grammar-integration.cpp that is failing the CI -- otherwise this looks great!

@ochafik ochafik merged commit 139cc62 into ggerganov:master Jun 28, 2024
52 checks passed
arthw pushed a commit to arthw/llama.cpp that referenced this pull request Jun 30, 2024
…rn escapes (ggerganov#8180)

* json: expand ESCAPED_IN_REGEXPS_BUT_NOT_IN_LITERALS charset

* json: revert default of additionalProperties to false

* Update README.md
MagnusS0 pushed a commit to MagnusS0/llama.cpp-normistral-tokenizer that referenced this pull request Jul 1, 2024
…rn escapes (ggerganov#8180)

* json: expand ESCAPED_IN_REGEXPS_BUT_NOT_IN_LITERALS charset

* json: revert default of additionalProperties to false

* Update README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
examples python python script changes server testing Everything test related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants