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

Remove apparently-unnecessary model_rebuild #5371

Merged
merged 5 commits into from
Apr 5, 2023

Conversation

dmontagu
Copy link
Contributor

@dmontagu dmontagu commented Apr 4, 2023

Partially addresses the performance issues noted in #5351

@dmontagu
Copy link
Contributor Author

dmontagu commented Apr 4, 2023

I'll note here that this doesn't have as dramatic of an effect on overall pytest runtime as it does on hot-loop generic model creation.

I ran pytest tests/test_generics.py --durations 20 on this branch and on the pre-refactor commit af73124e, and it seems to attribute most of the difference in runtime to the "setup" of the tests:

# Pre-refactor:
0.46s call     tests/test_generics.py::test_generics_work_with_many_parametrized_base_models
0.10s call     tests/test_generics.py::test_caches_get_cleaned_up
0.02s setup    tests/test_generics.py::test_generic_config
0.02s setup    tests/test_generics.py::test_classvar
0.01s setup    tests/test_generics.py::test_enum_generic
0.01s call     tests/test_generics.py::test_caches_get_cleaned_up_with_aliased_parametrized_bases
0.01s call     tests/test_generics.py::test_generic_name
0.01s call     tests/test_generics.py::test_generic_recursive_models_complicated
0.01s call     tests/test_generics.py::test_generic_recursive_models_triple
0.01s setup    tests/test_generics.py::test_generic_recursive_models_with_a_concrete_parameter
0.01s setup    tests/test_generics.py::test_generic_recursive_models_in_container
0.01s setup    tests/test_generics.py::test_generic_with_user_defined_generic_field
0.01s setup    tests/test_generics.py::test_construct_other_generic_model_with_validation
0.01s setup    tests/test_generics.py::test_multilevel_generic_binding
0.01s setup    tests/test_generics.py::test_generic_subclass
0.01s setup    tests/test_generics.py::test_generic_model_as_parameter_to_generic_type_alias
0.01s setup    tests/test_generics.py::test_generic_recursive_models_complicated
0.01s setup    tests/test_generics.py::test_generic_recursive_models_triple
0.01s setup    tests/test_generics.py::test_multi_inheritance_generic_defaults
0.01s setup    tests/test_generics.py::test_double_typevar_substitution

# This branch:
0.56s call     tests/test_generics.py::test_generics_work_with_many_parametrized_base_models
0.09s call     tests/test_generics.py::test_caches_get_cleaned_up
0.03s setup    tests/test_generics.py::test_generic_enum
0.03s setup    tests/test_generics.py::test_generic_annotated
0.03s setup    tests/test_generics.py::test_parent_field_parametrization
0.03s setup    tests/test_generics.py::test_nested
0.03s setup    tests/test_generics.py::test_optional_value
0.03s setup    tests/test_generics.py::test_construct_generic_model_with_validation
0.03s setup    tests/test_generics.py::test_multilevel_generic_binding
0.03s setup    tests/test_generics.py::test_generic_recursive_models
0.03s setup    tests/test_generics.py::test_generic_recursion_contextvar
0.03s setup    tests/test_generics.py::test_generic_enums
0.03s setup    tests/test_generics.py::test_partial_specification_instantiation
0.03s setup    tests/test_generics.py::test_generic_subclass_with_extra_type
0.03s setup    tests/test_generics.py::test_custom_generic_naming
0.03s setup    tests/test_generics.py::test_schema_is_valid
0.03s setup    tests/test_generics.py::test_construct_other_generic_model_with_validation
0.03s setup    tests/test_generics.py::test_generic_recursive_models_triple
0.03s setup    tests/test_generics.py::test_generic_recursive_models_in_container
0.03s setup    tests/test_generics.py::test_double_typevar_substitution

I also checked and there doesn't seem to be any meaningful changes to fixtures in test_generics.py or conftest.py, so I'm not sure where the extra setup time is coming from, or how to profile it.

I also checked, adding time.sleep(0.02) as a fixture brings the runtimes in line, so it at least seems consistent with something slow happening in a fixture or similar, but it would still be nice to confirm/resolve that.

@adriangb
Copy link
Member

adriangb commented Apr 4, 2023

Some profiling for the record

from typing import Generic, List, Type, TypeVar

from pydantic import BaseModel

T = TypeVar('T')
C = TypeVar('C')


class A(BaseModel, Generic[T, C]):
    x: T
    y: C


class B(A[int, C], BaseModel, Generic[C]):
    pass


models: List[Type[BaseModel]] = []
for i in range(1_000):
    models.append(type(f'M{i}', (BaseModel,), {}))

for m in models:
    B[m]

A pyinstrument report:
report.html.zip

JSON data for https://www.speedscope.app/:
CPU_profile_for_test.speedscope.json.zip

@dmontagu
Copy link
Contributor Author

dmontagu commented Apr 4, 2023

It turns out the garbage collection fixture was the one that had increased in runtime from ~10ms to ~30ms. Perhaps worth looking into more, but removing from most tests, the runtime becomes much more similar pre- and post-refactor.

@dmontagu
Copy link
Contributor Author

dmontagu commented Apr 4, 2023

@samuelcolvin I think unless this change seems suspicious to you we should merge this, and if the garbage collection stuff concerns you at all let's just create a separate issue for that.

Given the massive simplifications I realized were possible in #5361 I just think that the improvements to model creation eliminated the need for some of the hacks that I had to use to get it working in the first place.

@samuelcolvin samuelcolvin merged commit d1b42d5 into main Apr 5, 2023
@samuelcolvin samuelcolvin deleted the generics-performance-improvement branch April 5, 2023 07:27
@samuelcolvin
Copy link
Member

Amazing, thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants