FXC-4111-Enable Tidy3dBaseModel.from_file() for various subclasses #2990

momchil-flex · 2025-11-12T10:38:21Z

Greptile Overview

Greptile Summary

This PR refactors polymorphic deserialization by centralizing type registry and parsing logic in Tidy3dBaseModel, enabling automatic type resolution when loading files.

Key changes:

Moved TYPE_TO_CLASS_MAP from Expression plugin to base class __init_subclass__()
Added parse_obj() override to dispatch to correct subclass based on type field
Simplified stub implementations by delegating to base class methods
Removed 100+ lines of duplicate type mapping code

Critical issue found:

Line 219 in tidy3d/components/base.py has a logical error that breaks the intended behavior. The condition compares a ModelField object to a string, which will always be False, causing parse_obj() to never use the polymorphic dispatch logic.

Minor issues:

Debug print statement left in test file

Confidence Score: 1/5

This PR has a critical logic bug that breaks core functionality
The condition on line 219 of tidy3d/components/base.py contains a logic error that compares a ModelField object to a string literal "Tidy3dBaseModel", which will always evaluate to False. This means the polymorphic parse_obj() behavior will never be triggered when calling Tidy3dBaseModel.parse_obj(), breaking the core feature this PR is intended to add. The fix is simple (change to cls.__name__ == "Tidy3dBaseModel"), but this is a critical functional bug that needs to be resolved before merging.
tidy3d/components/base.py requires immediate attention due to the critical logic bug on line 219

Important Files Changed

File Analysis

Filename	Score	Overview
tidy3d/components/base.py	2/5	Added `TYPE_TO_CLASS_MAP` registry and `parse_obj()` override with critical bug in line 219 condition check
tidy3d/web/api/tidy3d_stub.py	4/5	Simplified stub implementation by delegating to `Tidy3dBaseModel.from_file()`. Clean refactoring with minor variable naming issue
tests/test_web/test_webapi.py	3/5	Added debug print statement that should be removed before merging

Sequence Diagram

sequenceDiagram
    participant User
    participant Tidy3dBaseModel
    participant TYPE_TO_CLASS_MAP
    participant Subclass as Simulation/HeatSimulation/etc

    Note over Tidy3dBaseModel,Subclass: Registration Phase (at import time)
    Subclass->>Tidy3dBaseModel: __init_subclass__()
    Tidy3dBaseModel->>Tidy3dBaseModel: add_type_field()
    Tidy3dBaseModel->>Tidy3dBaseModel: generate_docstring()
    Tidy3dBaseModel->>TYPE_TO_CLASS_MAP: Register type -> class mapping
    Note over TYPE_TO_CLASS_MAP: {"Simulation": Simulation,<br/>"HeatSimulation": HeatSimulation, ...}

    Note over User,Subclass: Loading Phase (from_file)
    User->>Tidy3dBaseModel: from_file("sim.json")
    Tidy3dBaseModel->>Tidy3dBaseModel: dict_from_file("sim.json")
    Tidy3dBaseModel->>Tidy3dBaseModel: parse_obj({"type": "HeatSimulation", ...})
    Note over Tidy3dBaseModel: BUG: Line 219 condition always False
    Tidy3dBaseModel->>TYPE_TO_CLASS_MAP: Get class for "HeatSimulation"
    TYPE_TO_CLASS_MAP-->>Tidy3dBaseModel: HeatSimulation class
    Tidy3dBaseModel->>Subclass: HeatSimulation(**obj)
    Subclass-->>User: HeatSimulation instance

Copilot

Pull Request Overview

This PR refactors the class loading mechanism by consolidating type-to-class mapping logic from individual classes (like Expression and stub classes) into the base Tidy3dBaseModel class. This allows various simulation and data types to be loaded directly using Tidy3dBaseModel.from_file() without requiring class-specific implementations.

Key changes:

Moved TYPE_TO_CLASS_MAP dictionary and parse_obj method from Expression to Tidy3dBaseModel
Simplified Tidy3dStub.from_file() and Tidy3dStubData.from_file() to delegate to base class
Updated documentation strings to use generic type references

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
tidy3d/components/base.py	Added global `TYPE_TO_CLASS_MAP` and `parse_obj` method to enable polymorphic loading of all subclasses
tidy3d/plugins/expressions/base.py	Removed duplicate `TYPE_TO_CLASS_MAP` and `parse_obj` implementation now handled by base class
tidy3d/web/api/tidy3d_stub.py	Simplified stub loading methods to delegate to `Tidy3dBaseModel.from_file()` and updated docstrings
CHANGELOG.md	Added entry documenting the new functionality

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

tidy3d/web/api/tidy3d_stub.py

greptile-apps

_{4 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

tidy3d/components/base.py

tidy3d/web/api/tidy3d_stub.py

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>

yaugenst-flex · 2025-11-12T13:18:23Z

@marcorudolphflex could you have a look at the failing lazy loading test 🙏

momchil-flex · 2025-11-12T13:19:26Z

@marcorudolphflex could you have a look at the failing lazy loading test 🙏

I have this print statement about the types currently. In both develop and this branch I get
[<class 'tidy3d.components.base._make_lazy_proxy.<locals>._LazyProxy'>, <class 'dict'>, <class 'tuple'>]
but somehow on this branch the lazy proxy is not manifested as SimulationData in the isinstance check.

greptile-apps

_{5 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2025-11-12T13:21:51Z

tidy3d/components/base.py

+    def parse_obj(cls, obj: dict[str, Any]) -> Tidy3dBaseModel:
+        """Specialized parse_obj to handle any Tidy3D component, but only if called from
+        ``Tidy3dBaseModel``. Otherwise, call the regular pyadantic parse_obj."""
+        if cls.__fields__.get(TYPE_TAG_STR) == "Tidy3dBaseModel":


logic: This condition is always False. cls.__fields__.get(TYPE_TAG_STR) returns a ModelField object, not a string. Should be cls.__name__ == "Tidy3dBaseModel" instead.

Suggested change

if cls.__fields__.get(TYPE_TAG_STR) == "Tidy3dBaseModel":

if cls.__name__ == "Tidy3dBaseModel":

Prompt To Fix With AI

This is a comment left during a code review. Path: tidy3d/components/base.py Line: 219:219 Comment: **logic:** This condition is always False. `cls.__fields__.get(TYPE_TAG_STR)` returns a `ModelField` object, not a string. Should be `cls.__name__ == "Tidy3dBaseModel"` instead. ```suggestion if cls.__name__ == "Tidy3dBaseModel": ``` How can I resolve this? If you propose a fix, please make it concise.

greptile-apps · 2025-11-12T13:21:52Z

tests/test_web/test_webapi.py


    assert isinstance(data, list) and len(data) == 3

+    print([type(d) for d in data])


style: Remove debug print statement before merging

Suggested change

print([type(d) for d in data])

Prompt To Fix With AI

This is a comment left during a code review. Path: tests/test_web/test_webapi.py Line: 964:964 Comment: **style:** Remove debug print statement before merging ```suggestion ``` How can I resolve this? If you propose a fix, please make it concise.

momchil-flex · 2025-11-12T13:24:41Z

I guess because of this change the data[0].class.name is now 'Tidy3dBaseModelProxy'. Will wait to hear from Marco before digging through more.

marcorudolphflex · 2025-11-12T13:41:56Z

If I catch that correctly, the fundamental issue is that it tries to create a Tidy3DBaseModel instead of the "correct" simulation class.
This fails for me without lazy loading:

from tests.test_web.test_tidy3d_stub import make_sim_data
from tidy3d.web.api.tidy3d_stub import Tidy3dStubData

tmp_path = "test.hdf5"
sim_data = make_sim_data()
sim_data.to_file(tmp_path)
Tidy3dStubData.postprocess(tmp_path, lazy=False)

pydantic.v1.error_wrappers.ValidationError: 5 validation errors for Tidy3dBaseModel
data
  extra fields not permitted (type=value_error.extra)
diverged
  extra fields not permitted (type=value_error.extra)
log
  extra fields not permitted (type=value_error.extra)
simulation
  extra fields not permitted (type=value_error.extra)
type
  extra fields not permitted (type=value_error.extra)

momchil-flex · 2025-11-12T15:44:30Z

Sorry @marcorudolphflex there was a bug in that state you tried. Now the test failures are exclusively due to lazy loading.

marcorudolphflex · 2025-11-12T16:13:51Z

Sorry @marcorudolphflex there was a bug in that state you tried. Now the test failures are exclusively due to lazy loading.

No problem.

So the issue with lazy loading is the following: In the current state of Tidy3dStubData.from_file, we cannot detect the individual class when creating the lazy object as we do not look into the file when working lazy. Therefore, we simply cannot inherit from the original class which let isinstance checks fail. The type name itself should not be really critical, that is more a gimmick when you print the type - but it is failing for the same reason.

Now the question is if we want to support isinstance on the simulation data type which I would generally tend to and it looks like we do some checks like that currently.

So I see 2 alternative solutions:

check the json for the type as before - at least in the lazy loading case
materialize on isinstance checks (delete __class__ from list of non-materialize-attributes)

What do you think?

marcorudolphflex · 2025-11-12T16:28:02Z

Edit:

materialize on isinstance checks (delete class from list of non-materialize-attributes)

this option just does not work in general. The object would still be a Tidy3DBaseModel after materialization such that it would be not usable. I would prefer the other option which seems more stable.

momchil-flex · 2025-11-12T17:05:04Z

Yes if the first option works it sounds better, are you going to give it a try?

marcorudolphflex · 2025-11-12T17:30:03Z

I can try to do that, but actually it would be better to do that on the pydantic v2 branch as we already observed varied behavior there.
Maybe we should postpone that if it does not work out of the box? Will post an update tomorrow.

momchil-flex · 2025-11-12T17:32:56Z

Thanks, sounds good, this is not exactly urgent it's just a nice cleanup.

momchil-flex · 2025-11-14T09:16:40Z

tidy3d/components/base.py

+            raise ValueError(f'Missing "{TYPE_TAG_STR}" in data') from exc
+        if not isinstance(type_value, str) or not type_value:
+            raise ValueError(f'Invalid "{TYPE_TAG_STR}" value: {type_value!r}')
+        return type_value


Do we really need all these protections LLM's like to put...?

haha i guess this is more of a fundamental question on error handling and how afraid we are of more verbose code 😉

feat: Moving TYPE_TO_CLASS_MAP logic from Expressions to Tidy3dBaseModel

a8a3ecf

Copilot AI review requested due to automatic review settings November 12, 2025 10:38

momchil-flex requested a review from yaugenst-flex as a code owner November 12, 2025 10:38

Copilot started reviewing on behalf of momchil-flex November 12, 2025 10:38 View session

Copilot finished reviewing on behalf of momchil-flex November 12, 2025 10:39

Copilot AI reviewed Nov 12, 2025

View reviewed changes

tidy3d/web/api/tidy3d_stub.py Outdated Show resolved Hide resolved

tidy3d/web/api/tidy3d_stub.py Outdated Show resolved Hide resolved

tidy3d/web/api/tidy3d_stub.py Outdated Show resolved Hide resolved

greptile-apps bot reviewed Nov 12, 2025

View reviewed changes

tidy3d/components/base.py Outdated Show resolved Hide resolved

tidy3d/web/api/tidy3d_stub.py Outdated Show resolved Hide resolved

momchil-flex marked this pull request as draft November 12, 2025 13:06

momchil-flex and others added 8 commits November 12, 2025 14:06

Update tidy3d/web/api/tidy3d_stub.py

59d9394

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update tidy3d/web/api/tidy3d_stub.py

9c03c8a

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update tidy3d/web/api/tidy3d_stub.py

925f0b5

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update tidy3d/web/api/tidy3d_stub.py

619fa26

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>

tmp

0ffc1a9

tmp

3662df5

tmp

6f07da4

Tmp

cb1c994

yaugenst-flex requested a review from marcorudolphflex November 12, 2025 13:18

momchil-flex marked this pull request as ready for review November 12, 2025 13:18

greptile-apps bot reviewed Nov 12, 2025

View reviewed changes

tmp

b438fc5

marcorudolphflex and others added 4 commits November 13, 2025 09:01

fix lazy loading

92c185f

rm print

762e342

fix tests

12edabd

codex's opinion

7a9f331

momchil-flex commented Nov 14, 2025

View reviewed changes

	if cls.__fields__.get(TYPE_TAG_STR) == "Tidy3dBaseModel":
	if cls.__name__ == "Tidy3dBaseModel":


		assert isinstance(data, list) and len(data) == 3

		print([type(d) for d in data])

FXC-4111-Enable Tidy3dBaseModel.from_file() for various subclasses #2990

Are you sure you want to change the base?

FXC-4111-Enable Tidy3dBaseModel.from_file() for various subclasses #2990

Uh oh!

Conversation

momchil-flex commented Nov 12, 2025 • edited by greptile-apps bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Overview

Greptile Summary

Confidence Score: 1/5

Important Files Changed

Sequence Diagram

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yaugenst-flex commented Nov 12, 2025

Uh oh!

momchil-flex commented Nov 12, 2025

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

momchil-flex commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcorudolphflex commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

momchil-flex commented Nov 12, 2025

Uh oh!

marcorudolphflex commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcorudolphflex commented Nov 12, 2025

Uh oh!

momchil-flex commented Nov 12, 2025

Uh oh!

marcorudolphflex commented Nov 12, 2025

Uh oh!

momchil-flex commented Nov 12, 2025

Uh oh!

momchil-flex Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

yaugenst-flex Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

momchil-flex commented Nov 12, 2025 •

edited by greptile-apps bot

Loading

momchil-flex commented Nov 12, 2025 •

edited

Loading

marcorudolphflex commented Nov 12, 2025 •

edited

Loading

marcorudolphflex commented Nov 12, 2025 •

edited

Loading