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: better support for "type" unions (e.g. nullable arrays w/ typed items) #7863

Merged
merged 8 commits into from
Jun 26, 2024

Conversation

ochafik
Copy link
Collaborator

@ochafik ochafik commented Jun 10, 2024

Adds support for the following nullable syntax I've seen in the wild (also checked properly by https://www.jsonschemavalidator.net/):

{
  "type": ["array", "null"],
  "items": { "type": "string" }
}

Before this PR, the grammar above was treated like {"anyOf": [{"type": "array"}, {"type": "null"}]} (meaning it would accept to generate [123]).

With this PR, it ensures the array is typed, e.g. ["123"] is possible but not [123]

@github-actions github-actions bot added testing Everything test related examples python python script changes server labels Jun 10, 2024
@ochafik ochafik marked this pull request as ready for review June 10, 2024 22:07
@ochafik ochafik requested a review from HanClinto June 10, 2024 22:07
@ochafik ochafik added the Review Complexity : Low Trivial changes to code that most beginner devs (or those who want a break) can tackle. e.g. UI fix label Jun 10, 2024
@HanClinto
Copy link
Collaborator

Updating the integration tests with pass/fail cases for this one would help me understand better what's going on with this.

@ochafik
Copy link
Collaborator Author

ochafik commented Jun 11, 2024

Updating the integration tests with pass/fail cases for this one would help me understand better what's going on with this.

I think we need #7790 for that ;-) In the meantime added an example to the PR's description, sorry it was a bit cryptic :-D

@ochafik ochafik changed the title json: better support for "type" unions (e.g. nullable arrays w/ typed items) json: better support for "type" unions (e.g. nullable arrays w/ typed items) Jun 11, 2024
@HanClinto
Copy link
Collaborator

Updating the integration tests with pass/fail cases for this one would help me understand better what's going on with this.

I think we need #7790 for that ;-) In the meantime added an example to the PR's description, sorry it was a bit cryptic :-D

haha, thanks. :) Noted -- I've been a bit slow on that one. It's finally updated and I think it's ready for review / merge.

@HanClinto
Copy link
Collaborator

Looks like one of the tests is failing:

Assertion failed: (false), function verify, file test-json-schema-to-grammar.cpp, line 38.

@ochafik
Copy link
Collaborator Author

ochafik commented Jun 23, 2024

Looks like one of the tests is failing:

Assertion failed: (false), function verify, file test-json-schema-to-grammar.cpp, line 38.

Fixed, thanks!

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.

Super, super clean.

To test I reverted the change to json-schema-to-grammar.cpp and the integration test failed. Brought the changes in, and the integration test passed.

The changes are small and discrete and self-contained. Love it! ❤️❤️❤️ Enthusiastic approval -- very well done!!

@ochafik ochafik merged commit 9b2f16f into ggerganov:master Jun 26, 2024
52 checks passed
arthw pushed a commit to arthw/llama.cpp that referenced this pull request Jun 30, 2024
…ed items) (ggerganov#7863)

* json: better suport for "type" arrays (e.g. `{"type": ["array", "null"], "items": {"type": "string"}}`)

* json: add test for type: [array, null] fix

* update tests
MagnusS0 pushed a commit to MagnusS0/llama.cpp-normistral-tokenizer that referenced this pull request Jul 1, 2024
…ed items) (ggerganov#7863)

* json: better suport for "type" arrays (e.g. `{"type": ["array", "null"], "items": {"type": "string"}}`)

* json: add test for type: [array, null] fix

* update tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
examples python python script changes Review Complexity : Low Trivial changes to code that most beginner devs (or those who want a break) can tackle. e.g. UI fix server testing Everything test related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants