[Tidy] Bump to Pydantic V2 #917

maxschulz-COL · 2024-12-03T15:51:42Z

Description

In draft so far - I tried to keep as clear as possible commit messages - they should tell the whole story.

Screenshot

Notice

I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":
- I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
- I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorized to submit this contribution on behalf of the original creator(s) or their licensees.
- I certify that the use of this contribution as authorized by the Apache 2.0 license does not violate the intellectual property rights of anyone else.
- I have not referenced individuals, products or companies in any commits, directly or indirectly.
- I have not added data or restricted code in any commits, directly or indirectly.

…feature/pydantic_v2

…edCallable fields This for now involves using `from pydantic.json_schema import SkipJsonSchema` which seems like a recommended and stable solution. We can try a more advanced approach if needs be

For now this does not include any arguments, as in the documentation the parent model works on a string type description, and presumably infers the correct model by name. Will need to investigate this behaviour closer, but for now this commit allows unit tests to run!

…lity

…ern for Pydantic v2 compatibility

…y code and use Pydantic v2 imports This does not include models yet where unit test instantiation fails, mainly due to use of validator etc. It also does not include tests yet

…rts to Pydantic v2

…et checks This replaces the each_item validation of the old style validator. Tested manually if it generally works

… use field_validator for Pydantic v2

… fields

…tic imports

…foreValidator to avoid validator So far did not fully eliminate validator due to it being present still for Layout - it needs to be determined whether we want a model validator here

This is a larger change that refactors the previous methods of __get_valdators__ The crux of this change is that it was not anymore possible to pass the field info to the validator without having acces to the model class. See more here: pydantic/pydantic#10903

…datate on entire targets list Additionally change the dev apps a little

At this point, all running code should be free of V1 nad only draw upon V2. This does not means that all docs have been cleaned, or that there is not usage of deprecated V2 functionality The next big challenge will be to get the add_type correct

This mostly involves fixing changed validation messages. There was also the case that we do NOT coerce numbers into strings anymore (this is technically slightly breaking)

Similar as before, with the change to not coercing strings as the main change

for more information, see https://pre-commit.ci

github-actions · 2024-12-03T15:52:57Z

View the example dashboards of the current commit live on PyCafe ☕ 🚀

Updated on: 2024-12-03 15:53:17 UTC
Commit: ee6722e

Link: vizro-core/examples/dev/

Link: vizro-core/examples/scratch_dev

Link: vizro-core/examples/visual-vocabulary/

Link: vizro-ai/examples/dashboard_ui/

antonymilne

In general it looks 💯 A few comments for now. I'll look at types.py and _base.py tomorrow morning (or whenever you think it's ready).

antonymilne · 2024-12-04T20:41:28Z

vizro-core/examples/visual-vocabulary/custom_components.py

I guess these demo apps need manually checking because we don't have unit tests for them. It looks like they don't do anything complicated with pydantic though so hopefully they just work already.

antonymilne · 2024-12-04T20:42:04Z

vizro-core/src/vizro/models/__init__.py

-Dashboard.update_forward_refs(Page=Page, Navigation=Navigation)
-NavBar.update_forward_refs(NavLink=NavLink)
-NavLink.update_forward_refs(Accordion=Accordion)
+Tabs.model_rebuild()


If these are removed entirely I presume things don't work?

antonymilne · 2024-12-04T20:46:14Z

vizro-core/src/vizro/models/_base.py

    def set_id(cls, id) -> str:
        return id or model_manager._generate_id()

    @_log_call
-    def __init__(self, **data: Any):
+    def __init__(self, **data: Any):  # TODO: model_post_init


Yesss 👍 After we do this then hopefully we get much better tab completion of model fields in IDEs. It would be worth checking this and advertising somewhere in the docs (maybe you need to install some plugin).

vizro-core/src/vizro/models/_base.py

antonymilne · 2024-12-04T20:51:54Z

vizro-core/src/vizro/models/_components/_form.py

-    components: list[_FormComponentType]
-    layout: Layout = None  # type: ignore[assignment]
+    components: conlist(
+        Annotated[_FormComponentType, BeforeValidator(check_captured_callable), Field(...)], min_length=1


Is the Field(...) really needed here?

(same comment applies elsewhere)

antonymilne · 2024-12-04T20:52:21Z

vizro-core/src/vizro/models/_components/_form.py

+    components: conlist(
+        Annotated[_FormComponentType, BeforeValidator(check_captured_callable), Field(...)], min_length=1
+    )  # since no default, can skip validate_default
+    layout: Optional[Layout] = None  # type: ignore[assignment]


Suggested change

layout: Optional[Layout] = None # type: ignore[assignment]

layout: Optional[Layout] = None

hopefully?

antonymilne · 2024-12-04T20:54:43Z

vizro-core/src/vizro/models/_components/container.py

@@ -31,15 +31,13 @@ class Container(VizroBaseModel):
    """

    type: Literal["container"] = "container"
-    components: list[ComponentType]
+    components: conlist(


Why is this (and others) done as conlist while others like RangeSlider.value and Container.tabs aren't?

antonymilne · 2024-12-04T20:56:31Z

vizro-core/src/vizro/models/_controls/parameter.py

@@ -30,45 +67,20 @@ class Parameter(VizroBaseModel):
    """

    type: Literal["parameter"] = "parameter"
-    targets: list[str] = Field(..., description="Targets in the form of `<target_component>.<target_argument>`.")
+    targets: Annotated[  # TODO: check if the double annotation is the best way to do this


This looks weird now because the each_item=True is handled differently, right?

Ideally there would be a nicer way to do this that leaves the check_dot_notation function etc. inside the Parameter class.

antonymilne · 2024-12-04T20:57:18Z

vizro-core/src/vizro/models/_models_utils.py

@@ -24,7 +24,7 @@ def validate_min_length(cls, value):
    return value


-def check_captured_callable(cls, value):
+def check_captured_callable(value):


Suggested change

def check_captured_callable(value):

def check_captured_callable_mode(value):

The name of this (which I originally chose) confused me - I think we should rename it.

antonymilne · 2024-12-04T20:57:47Z

vizro-core/src/vizro/models/_components/tabs.py

@@ -26,9 +26,7 @@ class Tabs(VizroBaseModel):
    """

    type: Literal["tabs"] = "tabs"
-    tabs: list[Container]
-
-    _validate_tabs = validator("tabs", allow_reuse=True, always=True)(validate_min_length)


Remember to remove this validate_min_length (and any other no longer used functions) from the source file.

antonymilne

A few small comments on CapturedCallable but generally looks great 👍

antonymilne · 2024-12-05T09:34:36Z

vizro-core/src/vizro/models/types.py

-        # At this point captured_callable is CapturedCallable or invalid type. Check it is in fact CapturedCallable
-        # and do final checks:
-        yield cls._check_type
+    def __get_pydantic_core_schema__(cls, source: Any, handler: Any) -> cs.core_schema.CoreSchema:


Can you remind me why we need this at all?

antonymilne · 2024-12-05T09:35:46Z

vizro-core/src/vizro/models/types.py

+        return cs.core_schema.no_info_plain_validator_function(cls.core_validation)
+
+    @staticmethod
+    def core_validation(value: Any):


I see this is called in the above method but when does it actually get used?

antonymilne · 2024-12-05T09:36:23Z

vizro-core/src/vizro/models/types.py

+        if not isinstance(value, CapturedCallable):
+            raise ValueError(f"Expected CustomType, got {type(value)}")


Suggested change

if not isinstance(value, CapturedCallable):

raise ValueError(f"Expected CustomType, got {type(value)}")

if not isinstance(value, CapturedCallable):

raise ValueError(f"Expected CapturedCallable, got {type(value)}")

antonymilne · 2024-12-05T09:38:38Z

vizro-core/src/vizro/models/types.py

+def validate_captured_callable(cls, value, info: ValidationInfo):
+    # TODO: We may want to double check on the mechanism of how field info is brought to
+    field_info = cls.model_fields[info.field_name]
+    return CapturedCallable._validate_captured_callable(value, field_info)


Suggested change

return CapturedCallable._validate_captured_callable(value, field_info)

return CapturedCallable._validate_captured_callable(value, field_info.json_schema_extra])

And rewrite signatures for the validators accordingly (we could even do a TypedDict annotation to show the expected format of json_schema_extra). Unless you think this is a bad idea for some reason. Generally speaking I would pass the minimum required information between these functions.

maxschulz-COL added 30 commits November 12, 2024 12:15

Run bump-pydantic

6f11390

Un-v1 actions.py

2cdb045

Do not fail on warnings

bbaeae5

Run bump-pydantic

b90f165

Un-v1 actions.py

7ee047c

Do not fail on warnings

b47fd4b

Merge branch 'feature/pydantic_v2' of github.com:mckinsey/vizro into …

48957af

…feature/pydantic_v2

Refactor action and component models to use SkipJsonSchema for Captur…

28b3ffe

…edCallable fields This for now involves using `from pydantic.json_schema import SkipJsonSchema` which seems like a recommended and stable solution. We can try a more advanced approach if needs be

Refactor id validator to field_validator

c77df9b

Refactor validator usage in Dashboard model for Pydantic v2 compatibi…

554afd3

…lity

Refactor layout model to use field_validator and update regex to patt…

ffef6dc

…ern for Pydantic v2 compatibility

Refactor imports in various models to remove Pydantic v1 compatibilit…

03f9b09

…y code and use Pydantic v2 imports This does not include models yet where unit test instantiation fails, mainly due to use of validator etc. It also does not include tests yet

Remove Pydantic v1 compatibility code from test files and update impo…

4094e32

…rts to Pydantic v2

Refactor Parameter model to use AfterValidator via Annotated for targ…

b0f79f7

…et checks This replaces the each_item validation of the old style validator. Tested manually if it generally works

Refactor accordion model to remove Pydantic v1 compatibility code and…

a15e325

… use field_validator for Pydantic v2

Refactor Dashboard and Navigation models to use Optional for nullable…

b7dcd67

… fields

Refactor Page model to use Optional for layout field and update Pydan…

3663293

…tic imports

Refactor form, container, tabs, and page models to use conlist and Be…

5052693

…foreValidator to avoid validator So far did not fully eliminate validator due to it being present still for Layout - it needs to be determined whether we want a model validator here

Fix Parameter model twith nested Annotated and AfterValidator to vali…

e93d6da

…datate on entire targets list Additionally change the dev apps a little

Merge branch 'main' into feature/pydantic_v2

180e75e

Refactor remaining v1 out of code base

7158c01

At this point, all running code should be free of V1 nad only draw upon V2. This does not means that all docs have been cleaned, or that there is not usage of deprecated V2 functionality The next big challenge will be to get the add_type correct

Fix unit tests for Action and Checklist

286b039

Fix form component unit tests

335c0b8

This mostly involves fixing changed validation messages. There was also the case that we do NOT coerce numbers into strings anymore (this is technically slightly breaking)

Merge branch 'main' into feature/pydantic_v2

4af444c

Fix more form component unit tests

6b77fbf

Similar as before, with the change to not coercing strings as the main change

Fix remaining unit tests plus the Filter optional argument

8bff295

Fix remaining unit tests

f05c474

Align unit tests for CapturedCallable with new way of validating it

906a448

[pre-commit.ci] auto fixes from pre-commit.com hooks

ee6722e

for more information, see https://pre-commit.ci

antonymilne reviewed Dec 4, 2024

View reviewed changes

antonymilne reviewed Dec 5, 2024

View reviewed changes

antonymilne approved these changes Dec 6, 2024

View reviewed changes

maxschulz-COL added 2 commits December 6, 2024 19:49

Refactor BaseModel.add_type

46035b3

Fix unit tests for types

d6b11d9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Tidy] Bump to Pydantic V2 #917

[Tidy] Bump to Pydantic V2 #917

maxschulz-COL commented Dec 3, 2024

github-actions bot commented Dec 3, 2024 •

edited

Loading

antonymilne left a comment

antonymilne Dec 4, 2024

antonymilne Dec 4, 2024

antonymilne Dec 4, 2024

antonymilne Dec 4, 2024

antonymilne Dec 4, 2024

antonymilne Dec 4, 2024

antonymilne Dec 4, 2024

antonymilne Dec 4, 2024

antonymilne Dec 4, 2024

antonymilne Dec 4, 2024

antonymilne left a comment

antonymilne Dec 5, 2024

antonymilne Dec 5, 2024

antonymilne Dec 5, 2024

antonymilne Dec 5, 2024

	layout: Optional[Layout] = None # type: ignore[assignment]
	layout: Optional[Layout] = None

	def check_captured_callable(value):
	def check_captured_callable_mode(value):

		if not isinstance(value, CapturedCallable):
		raise ValueError(f"Expected CustomType, got {type(value)}")

	return CapturedCallable._validate_captured_callable(value, field_info)
	return CapturedCallable._validate_captured_callable(value, field_info.json_schema_extra])

[Tidy] Bump to Pydantic V2 #917

Are you sure you want to change the base?

[Tidy] Bump to Pydantic V2 #917

Conversation

maxschulz-COL commented Dec 3, 2024

Description

Screenshot

Notice

github-actions bot commented Dec 3, 2024 • edited Loading

View the example dashboards of the current commit live on PyCafe ☕ 🚀

antonymilne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonymilne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 3, 2024 •

edited

Loading