Skip to content

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
Contributor

@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!

@pydantic-hooky pydantic-hooky bot added the awaiting author revision awaiting changes from the PR author label Apr 14, 2024
@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
Contributor

@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
Contributor

@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
Contributor

Looks fantastic. Thank you for your contribution!!

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

Successfully merging this pull request may close these issues.

Cannot use model_copy(deep=True) on a model instance which was created using model_construct, when there is a model_post_init in the class hierarchy.
3 participants