-
Notifications
You must be signed in to change notification settings - Fork 570
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
Refactor the type system #1441
Refactor the type system #1441
Conversation
a41a411
to
69fb69b
Compare
I am fairly happy with where I got so far with the Gemini integration. We will obviously need a set of utility functions for the types: |
4059bf1
to
1f1b2ce
Compare
a2f3985
to
2b60057
Compare
There are still things we'll need to change regarding what we do in the type adapters of open source models, but I think it's already worth it to review the current state of the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I have a few comments:
- We should simplify the code by defining
is_X
where X is a type, and use this for open source and closed models. - We forgot GenSON in a couple of places, including tests
- For "JSON Mode" make sure that
is dict
excludes things likedict[str, int]
. We could also add it for OSS models by loading the JSON grammar that's on the repo.
outlines/generator.py
Outdated
# If CFG -> CFG | ||
# if dict -> CFG | ||
# if Any -> None | ||
# Else -> Regex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this comment necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! I only have two minor comments. We can merge after resolving the merge conflicts.
We are currently using intermediate `Json`, `Choice` and `List` classes which, in addition to being cumbersome, are not useful abstractions. This commit bypasses these abstractions to handle the output types directly at the level of `GeminiTypeAdapter`.
Following the discussion in #1433. Closes #1421. Closes #1420. Closes #1218. Closes #1223.
Todo
JsonType
and testTypeAdapter
in isolation)JsonType
,Choice
,List
and testTypeAdapter
in isolation)JsonType
and testTypeAdapter
in isolation)JsonType
and testTypeAdapter
in isolation)jsonschema.Draft7Validator.check_schema(json.loads(output_type))
python_type_to_terms
Callable
Callable
sdict
type for json mode