Skip to content

Conversation

uriyyo
Copy link
Contributor

@uriyyo uriyyo commented Mar 28, 2021

Change Summary

Try to evaluate forward refs after model created.

So it will not be required to use update_forward_refs in the case of self-referencing model:

from __future__ import annotations

from pydantic import BaseModel
from typing import Optional


class Foo(BaseModel):
    foo: Optional[Foo]


assert Foo.__fields__['foo'].type_ is Foo

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)

Copy link
Collaborator

@PrettyWood PrettyWood left a comment

Choose a reason for hiding this comment

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

Can you please rebase/merge with master to trigger the new CI?

@PrettyWood PrettyWood added the awaiting author revision awaiting changes from the PR author label Apr 5, 2021
@uriyyo uriyyo force-pushed the feature/auto-forward-refs branch from fc89c5a to 286b730 Compare April 5, 2021 18:06
@PrettyWood PrettyWood added ready for review and removed awaiting author revision awaiting changes from the PR author labels Apr 5, 2021
@samuelcolvin
Copy link
Member

you have conflicts and the wrong change.

Please update.

@github-actions github-actions bot added awaiting author revision awaiting changes from the PR author and removed ready for review labels May 9, 2021
Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

looking good, but this also needs a note in the docs.

@uriyyo uriyyo force-pushed the feature/auto-forward-refs branch from 286b730 to cb5ee00 Compare May 9, 2021 15:20
@uriyyo
Copy link
Contributor Author

uriyyo commented May 9, 2021

@samuelcolvin I have updated PR. Could you please review it again?

@uriyyo uriyyo requested a review from samuelcolvin May 9, 2021 15:23
@github-actions github-actions bot added ready for review and removed awaiting author revision awaiting changes from the PR author labels May 9, 2021
@github-actions github-actions bot assigned PrettyWood and samuelcolvin and unassigned uriyyo May 9, 2021
Copy link
Collaborator

@PrettyWood PrettyWood left a comment

Choose a reason for hiding this comment

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

Cool feature! Nice to be able to do everything automatically

@@ -374,6 +373,8 @@ def is_untouched(v: Any) -> bool:
cls = super().__new__(mcs, name, bases, new_namespace, **kwargs)
# set __signature__ attr only for model class, but not for its instances
cls.__signature__ = ClassAttribute('__signature__', generate_model_signature(cls.__init__, fields, config))
cls.__try_update_forward_refs__()
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the impact on perf for models without forward refs? Do we have a way to know whether or not we should try to update?

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 wrote a simple benchmark:

import timeit
from pydantic import BaseModel


def callback():
    class Order(BaseModel):
        a: int
        b: float
        c: str


t = timeit.timeit(
    callback,
    number=10_000,
    globals={"BaseModel": BaseModel},
)

print(f'{t:.5f}')

Results:

  • With __try_update_forward_refs__ - 3.17182
  • Without __try_update_forward_refs__ - 3.15772

@PrettyWood
Copy link
Collaborator

Hi @uriyyo
Can you please rebase/merge and update? 🙏

@PrettyWood PrettyWood assigned uriyyo and unassigned samuelcolvin and PrettyWood Sep 4, 2021
@PrettyWood PrettyWood added awaiting author revision awaiting changes from the PR author and removed ready for review labels Sep 4, 2021
uriyyo and others added 3 commits September 4, 2021 21:20
Co-authored-by: Eric Jolibois <em.jolibois@gmail.com>
Co-authored-by: Eric Jolibois <em.jolibois@gmail.com>
Copy link
Collaborator

@PrettyWood PrettyWood left a comment

Choose a reason for hiding this comment

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

Yes I run the benchmark seems ok performance-wise

This branch

▶ BENCHMARK_REPEATS=20 make benchmark-pydantic
python benchmarks/run.py pydantic-only
testing pydantic, 20 times each
    pydantic ( 1/20) time=0.622s, success=48.65%
    pydantic ( 2/20) time=0.617s, success=48.65%
    pydantic ( 3/20) time=0.598s, success=48.65%
    pydantic ( 4/20) time=0.596s, success=48.65%
    pydantic ( 5/20) time=0.597s, success=48.65%
    pydantic ( 6/20) time=0.606s, success=48.65%
    pydantic ( 7/20) time=0.595s, success=48.65%
    pydantic ( 8/20) time=0.598s, success=48.65%
    pydantic ( 9/20) time=0.591s, success=48.65%
    pydantic (10/20) time=0.597s, success=48.65%
    pydantic (11/20) time=0.595s, success=48.65%
    pydantic (12/20) time=0.605s, success=48.65%
    pydantic (13/20) time=0.603s, success=48.65%
    pydantic (14/20) time=0.601s, success=48.65%
    pydantic (15/20) time=0.611s, success=48.65%
    pydantic (16/20) time=0.603s, success=48.65%
    pydantic (17/20) time=0.600s, success=48.65%
    pydantic (18/20) time=0.596s, success=48.65%
    pydantic (19/20) time=0.592s, success=48.65%
    pydantic (20/20) time=0.605s, success=48.65%
    pydantic best=0.591s, avg=0.601s, stdev=0.008s

    pydantic best=98.503μs/iter avg=100.240μs/iter stdev=1.335μs/iter version=1.8.2

1.8

▶ BENCHMARK_REPEATS=20 make benchmark-pydantic
python benchmarks/run.py pydantic-only
testing pydantic, 20 times each
    pydantic ( 1/20) time=0.608s, success=48.65%
    pydantic ( 2/20) time=0.588s, success=48.65%
    pydantic ( 3/20) time=0.616s, success=48.65%
    pydantic ( 4/20) time=0.632s, success=48.65%
    pydantic ( 5/20) time=0.599s, success=48.65%
    pydantic ( 6/20) time=0.588s, success=48.65%
    pydantic ( 7/20) time=0.591s, success=48.65%
    pydantic ( 8/20) time=0.595s, success=48.65%
    pydantic ( 9/20) time=0.589s, success=48.65%
    pydantic (10/20) time=0.612s, success=48.65%
    pydantic (11/20) time=0.599s, success=48.65%
    pydantic (12/20) time=0.594s, success=48.65%
    pydantic (13/20) time=0.623s, success=48.65%
    pydantic (14/20) time=0.591s, success=48.65%
    pydantic (15/20) time=0.592s, success=48.65%
    pydantic (16/20) time=0.595s, success=48.65%
    pydantic (17/20) time=0.595s, success=48.65%
    pydantic (18/20) time=0.600s, success=48.65%
    pydantic (19/20) time=0.625s, success=48.65%
    pydantic (20/20) time=0.587s, success=48.65%
    pydantic best=0.587s, avg=0.601s, stdev=0.014s

    pydantic best=97.825μs/iter avg=100.158μs/iter stdev=2.254μs/iter version=1.8

Copy link
Collaborator

@PrettyWood PrettyWood left a comment

Choose a reason for hiding this comment

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

LGTM!

@PrettyWood PrettyWood added ready for review and removed awaiting author revision awaiting changes from the PR author labels Sep 5, 2021
@samuelcolvin samuelcolvin merged commit 415eb54 into pydantic:master Dec 5, 2021
@samuelcolvin
Copy link
Member

this is great, thank you so much.

PrettyWood added a commit to PrettyWood/pydantic that referenced this pull request Dec 6, 2021
* Try to evaluate forward refs after model created

* Upadate docs and remove code duplication

* Update changes/2588-uriyyo.md

Co-authored-by: Eric Jolibois <em.jolibois@gmail.com>

* Update docs/usage/postponed_annotations.md

Co-authored-by: Eric Jolibois <em.jolibois@gmail.com>

* Remove unused import

Co-authored-by: Eric Jolibois <em.jolibois@gmail.com>
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.

3 participants