Skip to content

fix multiphysics medium attribute lookup#2468

Merged
momchil-flex merged 1 commit into
developfrom
yaugenst-flex/fix-attr-lookup
May 15, 2025
Merged

fix multiphysics medium attribute lookup#2468
momchil-flex merged 1 commit into
developfrom
yaugenst-flex/fix-attr-lookup

Conversation

@yaugenst-flex
Copy link
Copy Markdown
Collaborator

The custom __getattr__ from #2431 intercepts every missing-attribute lookup, including internals such as _cached_properties. During model construction or when a @cached_property is accessed, Pydantic tries to read that attribute, hits this handler, and raises, breaking any code path that touched cached properties. This surfaced in #2433 because that PR makes the _cached_properties fields a PrivateAttr, but this problem is also present on develop for any Pydantic internal field.

This PR addresses that by delegating all unresolved lookups (fields, cached props, dunders) back to Pydantic before our own logic runs.

@yaugenst-flex yaugenst-flex self-assigned this May 15, 2025
@yaugenst-flex yaugenst-flex added 2.8 will go into version 2.8.* .4 labels May 15, 2025
@frederikschubertflex
Copy link
Copy Markdown
Contributor

That this was not caught by the unit tests should be motivation for us to improve on that 😊

@yaugenst-flex yaugenst-flex force-pushed the yaugenst-flex/fix-attr-lookup branch from 648268e to a25dcb4 Compare May 15, 2025 07:53
@yaugenst-flex
Copy link
Copy Markdown
Collaborator Author

yaugenst-flex commented May 15, 2025

Noticed another issue: if self.optical is None (which is the default), then DELEGATED_ATTRIBUTES[name] is None and getattr(None, name) raises. Fixed that case too (well not "fixed" but the error message should be clear).

Copy link
Copy Markdown
Collaborator

@daquinteroflex daquinteroflex left a comment

Choose a reason for hiding this comment

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

Thanks a lot Yannick!

Comment thread tidy3d/components/material/multi_physics.py
@frederikschubertflex
Copy link
Copy Markdown
Contributor

frederikschubertflex commented May 15, 2025

Can we add some tests for this? I will add some tests!

@yaugenst-flex yaugenst-flex force-pushed the yaugenst-flex/fix-attr-lookup branch from a25dcb4 to b869a15 Compare May 15, 2025 09:07
@yaugenst-flex
Copy link
Copy Markdown
Collaborator Author

@frederikschubertflex oh sorry didnt see your edit, I added some tests now and force pushed! feel free to add more, I got a little bit confused. actually I still don't understand the root cause of the error I was seeing on my pydantic v2 branch. there, the _cached_properties lookup fails for MultiPhysicsMedium, but on develop it doesn't. I added a test to catch this but it currently passes on develop. still good to have probably. I did add tests for the other fix too though.

@frederikschubertflex
Copy link
Copy Markdown
Contributor

No, worries! I just figured that you have other things on your plate :) I will look into this in more depth later!

@momchil-flex momchil-flex merged commit de6c2ce into develop May 15, 2025
9 checks passed
@momchil-flex momchil-flex deleted the yaugenst-flex/fix-attr-lookup branch May 15, 2025 09:30
@yaugenst-flex
Copy link
Copy Markdown
Collaborator Author

ok maybe just to conclude this discussion:
it looks like pydantic v1 handled PrivateAttr in __getattribute__, v2 does not, so it falls back to __getattr__ which now gets caught by MultiPhysicsMedium's override. another difference is that __dir__ in v1 includes private attrs, and v2 omits them..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2.8 will go into version 2.8.* .4

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants