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 task model and params #1066

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ellnix
Copy link
Contributor

@ellnix ellnix commented Jan 24, 2025

Minor refactoring to convert the TaskResults model to inherit from CamelBase. In addition, allows for tuples in addition to lists in several parameters.

DocumentResults and Document remain the only models that do not inherit from CamelBase. Since Document's attributes are dynamic, I don't know if that can be achieved.

@ellnix ellnix changed the title Task refactoring Refactor task model and params Jan 24, 2025
@sanders41 sanders41 added the breaking-change The related changes are breaking for the users label Jan 24, 2025
Copy link
Collaborator

@sanders41 sanders41 left a comment

Choose a reason for hiding this comment

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

It should also work to do this in IndexStats with a model like

class IndexStats(CamelBase):
    number_of_documents: int
    is_indexing: bool
    field_distribution: dict[str, int]

DocumentResults and Document remain the only models that do not inherit from CamelBase. Since Document's attributes are dynamic, I don't know if that can be achieved.

This is good. The way I have handled this elsewhere is with generics, but that would be a big breaking change so better to leave it like you have it.

@ellnix
Copy link
Contributor Author

ellnix commented Jan 24, 2025

It should also work to do this in IndexStats with a model like

class IndexStats(CamelBase):
    number_of_documents: int
    is_indexing: bool
    field_distribution: dict[str, int]

Done. It's a pretty big breaking change if you ask me, since field_distribution is now a dict rather than an object.

I ended up deleting a test file entirely since it seemed to just be a test of the metaprogramming behind the old IndexStats. Presumably that behavior is guaranteed by the tests on CamelCase.

@ellnix ellnix requested a review from sanders41 January 24, 2025 15:29
@sanders41
Copy link
Collaborator

Done. It's a pretty big breaking change if you ask me, since field_distribution is now a dict rather than an object.

That is fair. What I think we could do is make the field_distribution type a FieldDistribution class that is dynamic like before and use a pre-validator to populate it. If you know how to do that and want to give it a try go for it, otherwise I'll test it out as soon as I get a chance to see if it works.

@ellnix
Copy link
Contributor Author

ellnix commented Jan 24, 2025

That is fair. What I think we could do is make the field_distribution type a FieldDistribution class that is dynamic like before and use a pre-validator to populate it. If you know how to do that and want to give it a try go for it, otherwise I'll test it out as soon as I get a chance to see if it works.

I can give that a shot in a couple of hours. I've never done it before but I'll read documentation until I figure it out 😆

@sanders41
Copy link
Collaborator

I can give that a shot in a couple of hours. I've never done it before but I'll read documentation until I figure it out 😆

If you work on it, for now just focus on Pydantic 2. Since this will be a breaking change anyway I'm considering removing Pydantic 1 support also in this release. Pydantic 2 has been out for over a year and as far as I'm aware all the major libraries support it now so it shouldn't be an issue to remove support for v1.

Copy link
Contributor Author

@ellnix ellnix left a comment

Choose a reason for hiding this comment

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

Did some reading, turned out to be pretty straightforward. I can make it pydantic 1 compatible too if you want.

Comment on lines +37 to +38
if not isinstance(v, dict):
raise TypeError('"field_distribution" in IndexStats must be a dict')
Copy link
Contributor Author

@ellnix ellnix Jan 24, 2025

Choose a reason for hiding this comment

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

I made a test to see what happens if field_distribution was an int and without this line you get the mysterious:

FAILED tests/index/test_index_stats_meilisearch.py::test_make_trouble - TypeError: '
int' object is not iterable

versus the new

FAILED tests/index/test_index_stats_meilisearch.py::test_make_trouble - TypeError: "
field_distribution" in IndexStats must be a dict



class IndexStats(CamelBase):
model_config = ConfigDict(arbitrary_types_allowed=True)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I'm understanding the documentation correctly, this is necessary.

field_distribution: Dict[str, int]
field_distribution: FieldDistribution

@field_validator("field_distribution", mode="before")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw that there was an Annotated pattern and a functional pattern.

The functional pattern made more sense to me, it looked more descriptive and everything was gonna stay in class anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change The related changes are breaking for the users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants