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

Tests: Added integration tests for GBNF parser #6472

Merged
merged 6 commits into from
Apr 6, 2024

Conversation

HanClinto
Copy link
Collaborator

Added integration tests for GBNF parser to validate correctness of grammar parsing, as well as correctness of string matching.

If we are going to increase the performance of the grammar matcher, then this will help us pin end-to-end behavior while we work on speed and memory improvements.

Might conflict a little bit with #5948, but if that one gets merged first then I'll adjust this one to match.

@HanClinto HanClinto marked this pull request as draft April 4, 2024 13:07

This comment was marked as off-topic.

…rsing, as well as correctness of string matching. Intended for use to pin behavior while working on performance improvements.
… that needed functions are available via internal API.
@HanClinto
Copy link
Collaborator Author

HanClinto commented Apr 4, 2024

Okay, all cleaned up. Was able to get rid of a hacky #include "llama.cpp" that was in the first version of this test thanks to small internal API changes made in #5948, so these tests are a bit cleaner now. test-llama-grammar.cpp still has that same hacky #include:

#include "llama.cpp" // TODO: not great

But because that test uses more internal functions that I don't think we should expose via the internal API, I think that one is still fine for now.

Usage / expected-behavior:

./tests/test-grammar-integration
Expected error:  parse: error parsing grammar: Undefined rule identifier 'numero'
End of expected error. Test successful.

Note: This error message is a leftover from a hardcoded error message printed in the grammar parser. Rather than change that, I just added a message to alert the user that the error message is expected. Not the cleanest, but it's probably fine (?).

Otherwise, this test feels reasonably clean, and I think I'm happy with it. Ready for review / merge whenever.

Thank you!

@HanClinto HanClinto marked this pull request as ready for review April 4, 2024 14:12
@HanClinto
Copy link
Collaborator Author

@ochafik Pinging you on this -- if you have interest I'd love to get your feedback on the usability of the string-vs.-grammar code in here. Does this work for your JSON test needs, or is there anything else we should do to clean it up more?

I think there might be room to add some helper functions in here (or even in the core), but I didn't want to go crazy with that at first. "Duplicated code is better than the wrong abstraction", as they say.

@HanClinto HanClinto merged commit 57dd02c into ggerganov:master Apr 6, 2024
46 checks passed
@ochafik
Copy link
Collaborator

ochafik commented Apr 8, 2024

if you have interest I'd love to get your feedback on the usability of the string-vs.-grammar code in here.

@HanClinto Sorry I'm late to the party, will look into integrating your code to the json tests this week ✌️

@HanClinto
Copy link
Collaborator Author

if you have interest I'd love to get your feedback on the usability of the string-vs.-grammar code in here.

@HanClinto Sorry I'm late to the party, will look into integrating your code to the json tests this week ✌️

No rush -- whenever you get to it. Mainly wanted to make sure you knew I was following up on stuff we'd talked about earlier.

Thanks so much!

tybalex pushed a commit to rubra-ai/tools.cpp that referenced this pull request Apr 17, 2024
* Added integration tests for GBNF parser to validate correctness of parsing, as well as correctness of string matching. Intended for use to pin behavior while working on performance improvements.

* Fixing whitespace errors and cleaning error message alert to be clearer.

* Removing hacky include to llama.cpp from grammar integration test now that needed functions are available via internal API.

* Comment cleanup.

* Reorganizing tests for readability.

* Cleaning up debug message to make a bit more sense.
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.

None yet

3 participants