Reorganize `BaseModel` annotations by kc0506 · Pull Request #10110 · pydantic/pydantic

kc0506 · 2024-08-12T14:53:36Z

Change Summary

As per microsoft/pylance-release#6253, attribute docstrings are recommended to be placed in the top level of the class. I added several changes to #10104 to make tests pass.

The only tests that need to be modified are in test_type_hints, because typing.get_type_hints does not have access to types imported inside if TYPE_CHECKING blocks. Otherwise, we will have to import pydantic_core and pydantic.fields in pydantic.main only for type hint, which seems not ideal.

Related issue number

#10104

Checklist

The pull request title is a good summary of the changes - it will be used in the changelog
Unit tests for the changes exist
Tests pass on CI
Documentation reflects the changes where applicable
My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

codspeed-hq · 2024-08-12T15:00:37Z

CodSpeed Performance Report

Merging #10110 will not alter performance

_{Comparing kc0506:reorganize-basemodel-attributes (c988e2e) with main (58abc3d)}

Summary

✅ 17 untouched benchmarks

kc0506 · 2024-08-12T16:15:45Z

Coudn't figure out why tests for python 3.8 failed 👀
Followed every steps from ci.yml in my python 3.8 virtualenv but no error occurs

(pydantic-3.8) (base) kc0506@kc0506:~/pydantic$ make test
PDM, version 2.17.3
pdm run coverage run -m pytest --durations=10
======================================================= test session starts =======================================================
platform linux -- Python 3.8.18, pytest-8.3.2, pluggy-1.5.0
codspeed: 2.2.1 (callgraph: not supported)
benchmark: 4.0.0 (defaults: timer=time.perf_counter disable_gc=False min_rounds=5 min_time=0.000005 max_time=1.0 calibration_precision=10 warmup=True warmup_iterations=100000)
rootdir: /home/kc0506/pydantic
configfile: pyproject.toml
testpaths: tests
plugins: benchmark-4.0.0, Faker-26.1.0, examples-0.0.12, mock-3.14.0, pretty-1.2.0, codspeed-2.2.1
collected 5814 items / 1 skipped

sydney-runkle · 2024-08-12T17:33:12Z

Looks similar to https://github.com/pydantic/pydantic/pull/10104/files, which @Viicos closed. Let's get his feedback here given that he was recently working on this 👍

Viicos

Thanks for giving a shot at this. I decided to close the previous PR as I saw it would require too much changes for a small added value.

However, I think I like the approach taken here.

Note that this is not a full review, I'll go a bit more in depth tomorrow morning.

Regarding the failing test, this is because Python 3.10 changed the way __annotations__ works. Prior to 3.10, __annotations__ would inherit the defined annotations from the base(s) class(es).

I'll take a look at a hopefully easy way to handle this.

Co-authored-by: Victorien <65306057+Viicos@users.noreply.github.com>

…kc0506/pydantic into reorganize-basemodel-attributes

kc0506 · 2024-08-13T06:54:58Z

I forgot we could just clear BaseModel.__annotations__ at runtime 😅
Seems much cleaner than manually excluding BaseModel when evaluating type notations.

github-actions · 2024-08-13T07:34:19Z

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
pydantic
main.py
pydantic/_internal
_fields.py
_model_construction.py
_typing_extra.py
Project Total

_{This report was generated by python-coverage-comment-action}

Viicos

Thanks this is looking great. A few more comments. Would be awesome if you could update the description in the Attributes: section as well.

Ideally we should avoid the duplication but we seem to be limited by how mkdocstrings-python handle things :/

Co-authored-by: Victorien <65306057+Viicos@users.noreply.github.com>

kc0506 · 2024-08-16T03:35:37Z

@Viicos Thanks for the detailed suggestions!

sydney-runkle

Great work! Thanks @Viicos for giving some detailed line by line feedback.

Viicos

Awesome @kc0506

reorganize BaseModel attributes

1ea578a

github-actions bot added the relnotes-fix Used for bugfixes. label Aug 12, 2024

Viicos self-requested a review August 12, 2024 15:21

Viicos requested changes Aug 12, 2024

View reviewed changes

Comment thread pydantic/main.py Outdated

Comment thread pydantic/_internal/_model_construction.py Outdated

Comment thread pydantic/_internal/_typing_extra.py Outdated

Comment thread pydantic/main.py Outdated

Viicos changed the title ~~Reorganize BaseModel attributes~~ Reorganize BaseModel annotations Aug 13, 2024

kc0506 and others added 5 commits August 13, 2024 14:47

Update pydantic/main.py

5158250

Co-authored-by: Victorien <65306057+Viicos@users.noreply.github.com>

Update pydantic/_internal/_typing_extra.py

43dfdc1

Co-authored-by: Victorien <65306057+Viicos@users.noreply.github.com>

Update pydantic/main.py

eac1969

Co-authored-by: Victorien <65306057+Viicos@users.noreply.github.com>

clear BaseModel annotations at runtime

5e40230

Merge branch 'reorganize-basemodel-attributes' of https://github.com/…

16779d5

…kc0506/pydantic into reorganize-basemodel-attributes

fix mypy tests

19a3f58

Update test_type_hints.py

c7353b8

Viicos requested changes Aug 13, 2024

View reviewed changes

sydney-runkle added the awaiting author revision awaiting changes from the PR author label Aug 13, 2024

kc0506 and others added 2 commits August 16, 2024 11:32

Update pydantic/_internal/_model_construction.py

eaa126a

Co-authored-by: Victorien <65306057+Viicos@users.noreply.github.com>

Apply suggestions from code review

41df541

Co-authored-by: Victorien <65306057+Viicos@users.noreply.github.com>

kc0506 added 2 commits August 16, 2024 11:55

update class docstring

0920d80

merge main

c988e2e

sydney-runkle approved these changes Aug 16, 2024

View reviewed changes

Viicos approved these changes Aug 16, 2024

View reviewed changes

Viicos merged commit 9e3d461 into pydantic:main Aug 16, 2024

samuelcolvin mentioned this pull request Aug 26, 2024

v2.9.0b1 is breaking napari #10239

Closed

sydney-runkle mentioned this pull request Aug 27, 2024

Fix a crash when cleaning the namespace in ModelMetaclass #10242

Merged

5 tasks

Viicos mentioned this pull request Sep 20, 2024

Remove unused mro argument to get_cls_type_hints_lenient #10451

Merged

5 tasks

kc0506 deleted the reorganize-basemodel-attributes branch October 14, 2024 10:05

Uh oh!

Conversation

kc0506 commented Aug 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change Summary

Related issue number

Checklist

Uh oh!

codspeed-hq bot commented Aug 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CodSpeed Performance Report

Merging #10110 will not alter performance

Summary

Uh oh!

kc0506 commented Aug 12, 2024

Uh oh!

sydney-runkle commented Aug 12, 2024

Uh oh!

Viicos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kc0506 commented Aug 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Aug 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage report

Uh oh!

Viicos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kc0506 commented Aug 16, 2024

Uh oh!

sydney-runkle left a comment

Choose a reason for hiding this comment

Uh oh!

Viicos left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kc0506 commented Aug 12, 2024 •

edited

Loading

codspeed-hq bot commented Aug 12, 2024 •

edited

Loading

kc0506 commented Aug 13, 2024 •

edited

Loading

github-actions bot commented Aug 13, 2024 •

edited

Loading