json: restore default additionalProperties to false, fix some pattern escapes#8180
Merged
ochafik merged 4 commits intoggml-org:masterfrom Jun 28, 2024
Merged
json: restore default additionalProperties to false, fix some pattern escapes#8180ochafik merged 4 commits intoggml-org:masterfrom
json: restore default additionalProperties to false, fix some pattern escapes#8180ochafik merged 4 commits intoggml-org:masterfrom
Conversation
4 tasks
HanClinto
approved these changes
Jun 28, 2024
Contributor
There was a problem hiding this comment.
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!
MagnusS0
pushed a commit
to MagnusS0/llama.cpp-normistral-tokenizer
that referenced
this pull request
Jul 1, 2024
…rn escapes (ggml-org#8180) * json: expand ESCAPED_IN_REGEXPS_BUT_NOT_IN_LITERALS charset * json: revert default of additionalProperties to false * Update README.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Testing more schemas after #7840, I'm getting an alarming rate of hallucinations on property names. The change of default for
additionalPropertiesmay have been somewhat principled but it seems bad in practice (@HanClinto you did warn me, should've listened 😅).Switching back the default of
additionalPropertiestofalse(despite the spec defaulting it totrue) 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):