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

perf: remove TypeAdapter if has use validate_assignment. #5623

Merged
merged 4 commits into from
Feb 2, 2024

Conversation

vvanglro
Copy link
Contributor

@vvanglro vvanglro commented Feb 1, 2024

Simple test example:

from datetime import date, datetime
from timeit import timeit

from pydantic import TypeAdapter


def parse_obj(annot, value):
    ta = TypeAdapter(annot)

    result1 = timeit(lambda: ta.validate_python(value), number=10000)
    result2 = timeit(lambda: TypeAdapter(annot).validate_python(value), number=10000)

    print(f'{annot.__name__:<8} {result2 / result1:.2f}')


if __name__ == '__main__':
    for t, v in (
        (int, "0"),
        (float, '0.5'),
        (str, b"123"),
        (bool, 'true'),
        (date, '2024-01-01'),
        (datetime, '2024-01-01T00:00:00'),
    ):
        parse_obj(t, v)

output:

int      131.77
float    107.20
str      109.12
bool     128.88
date     283.73
datetime 272.90

Refer to: https://docs.pydantic.dev/latest/concepts/performance/#typeadapter-instantiated-once

@github-actions github-actions bot added python PRs that change python files services PRs that change app services labels Feb 1, 2024
Copy link
Collaborator

@psychedelicious psychedelicious left a comment

Choose a reason for hiding this comment

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

Is there a measurable impact on the application's performance with this change? We don't call parse_args() very often - just at startup I believe.

It's hard to say no to faster startup, but I'm curious if there this does anything in practice besides add a few lines of code.

@vvanglro
Copy link
Contributor Author

vvanglro commented Feb 2, 2024

Is there a measurable impact on the application's performance with this change? We don't call parse_args() very often - just at startup I believe.

It's hard to say no to faster startup, but I'm curious if there this does anything in practice besides add a few lines of code.

If the TypeAdapter has to be instantiated every time it is used, there will be a performance impact, and as stated in the documentation, it is possible to modify the number of times the test script is run to see this change.

For now, it's just a startup call, so it's not a big performance hit. But I think this is a usage specification, so if you want to use TypeAdapter in a function, you can just introduce it into the function.

@psychedelicious
Copy link
Collaborator

I understand that that the change is objectively going to be a performance improvement, but I'm asking about the magnitude of benefit.

Copy link
Collaborator

@psychedelicious psychedelicious left a comment

Choose a reason for hiding this comment

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

I don't think TypeAdapter is even necessary here.

This model uses validate_assignment=True, so setting the CLI args is already automatically validated by pydantic. The TypeAdapters are extraneous.

@vvanglro
Copy link
Contributor Author

vvanglro commented Feb 2, 2024

I understand that that the change is objectively going to be a performance improvement, but I'm asking about the magnitude of benefit.

Depends on how many times the TypeAdapter is called in the code. Take 100 times as an example. Here are the gain multipliers for different types

int      0.15
float    15.17
str      19.41
bool     18.27
date     30.53
datetime 45.37

@vvanglro
Copy link
Contributor Author

vvanglro commented Feb 2, 2024

I don't think TypeAdapter is even necessary here.

This model uses validate_assignment=True, so setting the CLI args is already automatically validated by pydantic. The TypeAdapters are extraneous.

I'm sorry, but I have not investigated the use of this model in the project and cannot give a conclusion.

@psychedelicious
Copy link
Collaborator

Depends on how many times the TypeAdapter is called in the code. Take 100 times as an example. Here are the gain multipliers for different types

Sorry, to be clear, I'm asking if the change has a measurable impact on this project. It's OK if you haven't profiled it, was just curious.

I'm sorry, but I have not investigated the use of this model in the project and cannot give a conclusion.

I believe it is not needed, because calling setattr on a pydantic model with validate_assignment=True will get runtime type checking.

If you can explain why it is needed, then we can accept this PR as a minor performance enhancement.

Otherwise, the actual change that is needed is to remove the usage of TypeAdapter entirely bc it isn't doing anything. Thanks to your PR that this deeper issue was recognized.

@vvanglro
Copy link
Contributor Author

vvanglro commented Feb 2, 2024

I believe it is not needed, because calling setattr on a pydantic model with validate_assignment=True will get runtime type checking.

I tested the behavior of validate_assignment.

Otherwise, the actual change that is needed is to remove the usage of TypeAdapter entirely bc it isn't doing anything. Thanks to your PR that this deeper issue was recognized.

Yes, I agree.

@vvanglro vvanglro changed the title perf: TypeAdapter instantiated once perf: remove TypeAdapter if has use validate_assignment. Feb 2, 2024
Copy link
Collaborator

@psychedelicious psychedelicious left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the deeper issue here

@psychedelicious psychedelicious merged commit f972fe9 into invoke-ai:main Feb 2, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python PRs that change python files services PRs that change app services
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants