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

Address case where model_construct on a class which defines model_post_init fails with AttributeError: __pydantic_private__ when subsequently model_copy'd #9168

Merged

Conversation

babygrimes
Copy link
Contributor

@babygrimes babygrimes commented Apr 4, 2024

Change Summary

See detail as reported in Issue #9122

Note: this patch specifically addresses the handling of __pydantic_private__ in __copy__, __deepcopy__ and __eq__. It does not address usage in several other places in main.py including: __{get,set,del}attr__, __{get,set}state__. It was unclear whether these were required, as eventually the model created by model_construct will in fact have __pydantic_private__ set (possibly to None).

:## Related issue number: #9122

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 [N/A]
  • My PR is ready to review

Please Review

Fix #9122

Selected Reviewer: @alexmojaki

Copy link

codspeed-hq bot commented Apr 4, 2024

CodSpeed Performance Report

Merging #9168 will not alter performance

Comparing babygrimes:9122-deepcopy-pydantic-private (d2660e5) with main (6322b24)

Summary

✅ 13 untouched benchmarks

@sydney-runkle sydney-runkle added the relnotes-fix Used for bugfixes. label Apr 4, 2024
Copy link
Member

@sydney-runkle sydney-runkle 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, just requested some minor changes!

@babygrimes
Copy link
Contributor Author

Made another push which uses a local private: dict[str, Any] | None to quiet make typecheck but still uses the getattr() approach. Please review and let me know if there are any additional changes required here!

@sydney-runkle
Copy link
Member

@babygrimes,

Ah, I see. Let's go with your initial approach then (it feels a bit cleaner to me). I'll go ahead and approve - we can merge once you revert back to that approach. Thanks a bunch - looking forward to merging soon!

Copy link
Member

@sydney-runkle sydney-runkle left a comment

Choose a reason for hiding this comment

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

As noted above -- approved, will merge when you revert back to your initial approach with the hasattr checks followed by the direct attribute accesses (thanks for pointing out, we already use that pattern in model_construct

@alexmojaki alexmojaki removed their assignment Apr 19, 2024
@babygrimes
Copy link
Contributor Author

Changes have been reverted to the hasattr() followed by direct attribute access, also reverting ALL changes in model_construct.

@babygrimes
Copy link
Contributor Author

Please review and merge, or let me know if anything else is needed here. And, finally, THANKS for pydantic - a great solution to such a core problem :)

@alexmojaki alexmojaki removed their assignment Apr 19, 2024
@sydney-runkle
Copy link
Member

Looks fantastic. Thank you for your contribution!!

@sydney-runkle sydney-runkle merged commit 77b0e1c into pydantic:main Apr 19, 2024
53 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants