Skip to content

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
Contributor

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
Contributor

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
Contributor

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
Contributor

@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 ggml-org:master Jun 26, 2024
arthw pushed a commit to arthw/llama.cpp that referenced this pull request Jun 30, 2024
…ed items) (ggml-org#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) (ggml-org#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