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

Support min/max number of digits for numbers in JSON Schema #932

Merged
merged 1 commit into from
Jun 11, 2024

Conversation

smagnan
Copy link
Contributor

@smagnan smagnan commented May 31, 2024

This would allow to mitigate some repetition issues that certain LLMs have. This should resolve #847

@rlouf
Copy link
Member

rlouf commented May 31, 2024

Thank you for contributing! Would you mind adding some unit tests?

@smagnan
Copy link
Contributor Author

smagnan commented May 31, 2024

Thank you for contributing! Would you mind adding some unit tests?

Will do!

Should I also address the fixme I mentioned here? (ValueError not being raised when string minLength and maxLength are invalid), or should we have an other behavior? (Ignore the keys and add a warning for example)

@smagnan
Copy link
Contributor Author

smagnan commented May 31, 2024

@rlouf Added unit tests for integer bounding and number (base and bounded).

Also one fix: first digit of integers was not counted towards the bounding.
And one adjustment: base and bounded number use the same rule for decimal places: (\\.[0-9]+) instead of (\\.[0-9]*). Either can probably argued for but I am now using the same as the default one.

@rlouf
Copy link
Member

rlouf commented Jun 2, 2024

Thanks! Looks like you need to fix the code style. Also could you rebase the branch on main?

@smagnan
Copy link
Contributor Author

smagnan commented Jun 3, 2024

Thanks! Looks like you need to fix the code style. Also could you rebase the branch on main?

@rlouf Should be up to date with main again (did so with the github "update to main" button , which it seems merges main into my branch instead of doing a rebase).
Also ran black for proper formatting. Didn't realize I did not setup that part properly.

Hope it is all good now. Can't run the pipeline manually.

@smagnan
Copy link
Contributor Author

smagnan commented Jun 7, 2024

No conflicts, up to date with main and all checks are passing. Should be ready to merge unless further review is necessary @rlouf

@rlouf
Copy link
Member

rlouf commented Jun 7, 2024

Yes sorry, i haven’t had the time to pull your changes and rebase them yet will do as soon as I can. Thank you for your patience!

@smagnan
Copy link
Contributor Author

smagnan commented Jun 7, 2024

No worries. Just wasn't sure what the process for getting PRs merged (since it seems I can add reviewers myself etc). Thanks for the fast feedback!

@rlouf rlouf force-pushed the solve-847 branch 2 times, most recently from b637744 to 07af5b5 Compare June 11, 2024 09:12
@rlouf rlouf changed the title adds min/max number of digits control for integers and parts of numbers in JSON. Support min/max number of digits for numbers in JSON Schema Jun 11, 2024
@rlouf rlouf added structured generation Linked to structured generation JSON labels Jun 11, 2024
@rlouf rlouf merged commit 11af6ce into dottxt-ai:main Jun 11, 2024
6 checks passed
@rlouf
Copy link
Member

rlouf commented Jun 11, 2024

Thank you for contributing!

lapp0 pushed a commit to lapp0/outlines that referenced this pull request Jun 12, 2024
…i#932)

This would allow to mitigate some repetition issues that certain LLMs
have. This should resolve dottxt-ai#847

Co-authored-by: Samuel Magnan <[email protected]>
lapp0 pushed a commit to lapp0/outlines that referenced this pull request Jun 12, 2024
…i#932)

This would allow to mitigate some repetition issues that certain LLMs
have. This should resolve dottxt-ai#847

Co-authored-by: Samuel Magnan <[email protected]>
fpgmaas pushed a commit to fpgmaas/outlines that referenced this pull request Jun 14, 2024
…i#932)

This would allow to mitigate some repetition issues that certain LLMs
have. This should resolve dottxt-ai#847

Co-authored-by: Samuel Magnan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JSON structured generation Linked to structured generation
Projects
None yet
2 participants