NotificationsYou must be signed in to change notification settings
Fork2.2k
Star24.6k

Allow cached properties to be altered on frozen models#11432

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

Viicos merged 2 commits intomainfromcached-property-frozen

Feb 12, 2025

Merged

Allow cached properties to be altered on frozen models#11432

Viicos merged 2 commits intomainfromcached-property-frozen

Feb 12, 2025

Conversation

Copy link

Member

Viicos commentedFeb 12, 2025

Also clarify documentation ofmodel_copy().

Fixes#11428,closes#11431.

Change Summary

Related issue number

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

Allow cached properties to be altered on frozen models

c777562

Also clarify documentation of `model_copy()`.

github-actionsbot added the relnotes-fixUsed for bugfixes. label

Feb 12, 2025

Copy link

cloudflare-workers-and-pagesbot commentedFeb 12, 2025•
edited
Loading

Deploying pydantic-docs with Cloudflare Pages

Latest commit:	`ecea8a6`
Status:	✅ Deploy successful!
Preview URL:	https://a644b8b1.pydantic-docs.pages.dev
Branch Preview URL:	https://cached-property-frozen.pydantic-docs.pages.dev

View logs

Viicos commented

Feb 12, 2025

View reviewed changes

pydantic/main.py Outdated

		if isinstance(attr, cached_property):
		return _SIMPLE_SETATTR_HANDLERS['cached_property']

		model_frozen = cls.model_config.get('frozen')

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

I inlined the logic as I couldn't find a good way to keep it in a single method as I need to raise it differently in__delattr__. A bit unfortunate, but at least this removes the_check_frozen method on theBaseModel class, so it avoids polluting the namespace.

Viicos mentioned this pull request

Feb 12, 2025

Fix cached property not cleared on model copy#11431

Closed

5 tasks

Copy link

Contributor

github-actionsbot commentedFeb 12, 2025•
edited
Loading

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
pydantic
dataclasses.py
main.py
Project Total

_{This report was generated bypython-coverage-comment-action}

Copy link

codspeed-hqbot commentedFeb 12, 2025•
edited
Loading

CodSpeed Performance Report

Merging#11432 willnot alter performance

_{Comparingcached-property-frozen (ecea8a6) withmain (a1f12c7)}

Summary

✅ 46 untouched benchmarks

ddorian reviewed

Feb 12, 2025

View reviewed changes

pydantic/main.py

		attr = getattr(cls, name, None)
		# NOTE: We currently special case properties and `cached_property`, but we might need
		# to generalize this to all data/non-data descriptors at some point. For non-data descriptors
		# (such as `cached_property`), it isn't obvious though. `cached_property` caches the value
		# to the instance's `__dict__`, but other non-data descriptors might do things differently.
		if isinstance(attr, cached_property):

Copy link

ddorianFeb 12, 2025

Choose a reason for hiding this comment

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

Can you apply the same thing that I requested in#11431 (comment) ?

Copy link

MemberAuthor

ViicosFeb 12, 2025

Choose a reason for hiding this comment

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

As per the comment above this line, we currently only special case@cached_property, and could extend to other descriptors (but it isn't obvious to do so, as per the comment).

We currently don't plan on changing the behavior unless someone has an explicit request for it

Copy link

Contributor

sydney-runkle commentedFeb 12, 2025

Can we add a test that's more in line with the initial issue report as well?

sydney-runkle suggested changes

Feb 12, 2025

View reviewed changes

Copy link

Contributor

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.

Left a few nitpicks, thanks for picking up this fix!

pydantic/main.py OutdatedShow resolvedHide resolved

pydantic-hookybot added the awaiting author revisionawaiting changes from the PR author label

Feb 12, 2025

pydantic-hookybot assignedViicos

Feb 12, 2025

refactor

ecea8a6

Viicos removed the awaiting author revisionawaiting changes from the PR author label

Feb 12, 2025

Copy link

MemberAuthor

Viicos commentedFeb 12, 2025

Can we add a test that's more in line with the initial issue report as well?

The initial issue report isn't really fixed as it's still up to the user to clear the cached properties (which this PR allows). We could also do it ourselves on model copy, but there are arguments to be made in favor ofnot clearing them (e.g. if the cached property is not depending on any other fields but rather caches something really expensive to compute).