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

Refactor the type system #1441

Merged
merged 22 commits into from
Mar 11, 2025
Merged

Refactor the type system #1441

merged 22 commits into from
Mar 11, 2025

Conversation

rlouf
Copy link
Member

@rlouf rlouf commented Feb 23, 2025

Following the discussion in #1433. Closes #1421. Closes #1420. Closes #1218. Closes #1223.

Todo

  • Update OpenAI tests (JsonType and test TypeAdapter in isolation)
  • Capture OpenAI's error when the user passes a feature that is not supported.
  • Update Gemini tests (JsonType, Choice, List and test TypeAdapter in isolation)
  • Capture Gemini's error when the user passes a feature that is not supported
  • Update Ollama integration
  • Update Ollama tests (JsonType and test TypeAdapter in isolation)
  • Update Dottxt tests (JsonType and test TypeAdapter in isolation)
  • Make sure that the JSON Schema passed by the user is correct with jsonschema.Draft7Validator.check_schema(json.loads(output_type))
  • Test python_type_to_terms
  • Support GenSON
  • Support Callable
  • Support enums of Callables
  • Support dict type for json mode
  • Support Interegular FSM

@rlouf
Copy link
Member Author

rlouf commented Feb 24, 2025

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: is_enum, is_typeddict, is_pydantic_basemodel, etc that will be re-used across the library.

@rlouf rlouf force-pushed the types branch 10 times, most recently from 4059bf1 to 1f1b2ce Compare February 27, 2025 23:04
@Martins6 Martins6 mentioned this pull request Feb 28, 2025
@RobinPicard RobinPicard force-pushed the types branch 2 times, most recently from a2f3985 to 2b60057 Compare March 6, 2025 14:55
@RobinPicard RobinPicard marked this pull request as ready for review March 6, 2025 15:09
@RobinPicard
Copy link
Contributor

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.

Copy link
Member Author

@rlouf rlouf left a 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:

  1. We should simplify the code by defining is_X where X is a type, and use this for open source and closed models.
  2. We forgot GenSON in a couple of places, including tests
  3. For "JSON Mode" make sure that is dict excludes things like dict[str, int]. We could also add it for OSS models by loading the JSON grammar that's on the repo.

@RobinPicard RobinPicard linked an issue Mar 11, 2025 that may be closed by this pull request
Comment on lines 63 to 66
# If CFG -> CFG
# if dict -> CFG
# if Any -> None
# Else -> Regex
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this comment necessary?

Copy link
Member Author

@rlouf rlouf left a 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.

@RobinPicard RobinPicard merged commit b679269 into dottxt-ai:v1.0 Mar 11, 2025
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants