[3/n][pythonic config] Layer in Pydantic compat abstractions #16436

benpankow · 2023-09-12T03:07:46Z

Summary

PR stack which re-implements #16257, in hopes of making it easier to review.

This PR builds out a Pydantic 2 compat layer, which abstracts away some of the model accesses. The next PR in the stack will actually enable Pydantic 2 tests + turn on compatibility.

Test Plan

Existing unit tests.

benpankow · 2023-09-12T03:07:59Z

Current dependencies on/for this PR:

master
- PR [1/n][pythonic config] Restructure Pythonic config & resources into modules #16434
  - PR [2/n][pythonic config] Update config conversion logic to rely on annotation rather than Pydantic model type #16435
    - PR [3/n][pythonic config] Layer in Pydantic compat abstractions #16436 👈
      - PR [4/n][pythonic config] Run Pydantic 2 tests #16437
        
        PR [helm] Update helm package to use Pydantic 2.0 #16326

This comment was auto-generated by Graphite.

github-actions · 2023-09-12T03:27:45Z

Deploy preview for dagster-docs ready!

Preview available at https://dagster-docs-1mvswt01j-elementl.vercel.app
https://benpankow-restructure-pythonic-config-module-3.dagster.dagster-docs.io

Direct link to changed pages:

schrockn · 2023-09-12T12:53:35Z

python_modules/dagster/dagster/_config/pythonic_config/config.py

+            if "Instance is frozen" in str(  # Pydantic 2.x error
+                e
+            ) or "is immutable and does not support item assignment" in str(  # Pydantic 1.x error
+                e
+            ):


is there any less fragile way of doing this?

Still relevant comment

Sadly no, not that I can tell. These values aren't set or exported as constants anywhere, just passed directly to the error constructor.

I've pulled this out into a separate fn for the moment. Our tests should at least catch if this changes.

schrockn · 2023-09-12T12:54:35Z

python_modules/dagster/dagster/_config/pythonic_config/config.py

+def ensure_env_vars_set_post_init(set_value: T, input_value: Any) -> T:
+    """Pydantic 2.x utility. Ensures that Pydantic field values are set to the appropriate
+    EnvVar or IntEnvVar objects post-model-instantiation, since Pydantic 2.x will cast
+    EnvVar or IntEnvVar values to raw strings or ints as part of the model instantiation process.
+    """
+    if isinstance(set_value, dict) and isinstance(input_value, dict):


do we have any context as to why Pydantic 2 does this? Are we going to break anything by doing this?

schrockn · 2023-09-12T12:55:17Z

python_modules/dagster/dagster/_config/pythonic_config/config.py

                    invalid_type=e.current_value,
                    is_resource=model_cls is not None
                    and safe_is_subclass(model_cls, ConfigurableResourceFactory),
                )

-    shape_cls = Permissive if model_cls.__config__.extra == Extra.allow else Shape
+    shape_cls = Permissive if model_config(model_cls).get("extra") == "allow" else Shape


is there a first-class way of doing this in pydantic 2?

It can be extracted directly (outside of the compat layer), but still relies on a .get call (it's an unstructured dictionary):

>>> MyModel2(a_str='foo').model_config {'extra': 'allow'}

python_modules/dagster/dagster/_config/pythonic_config/pydantic_compat_layer.py

schrockn · 2023-09-12T12:56:42Z

python_modules/dagster/dagster/_config/pythonic_config/pydantic_compat_layer.py

+        # Pydantic 1.x
+        field_info = getattr(self.field, "field_info", None)
+        if field_info:
+            return field_info.description
+
+        # Pydantic 2.x
+        return getattr(self.field, "description", None)
+
+    def is_required(self):
+        # Pydantic 2.x
+        if hasattr(self.field, "is_required"):
+            return self.field.is_required()
+
+        # Pydantic 1.x
+        return self.field.required
+
+    @property
+    def discriminator(self):
+        # Pydantic 2.x
+        if hasattr(self.field, "discriminator"):
+            return self.field.discriminator
+
+        # Pydantic 1.x
+        return getattr(self.field, "discriminator_key", None)


should we just explicitly have different codepaths for each version here? I think that will be more bulletproof

Yes, that's a better approach I think. Reworked.

python_modules/dagster/dagster/_config/pythonic_config/pydantic_compat_layer.py

schrockn · 2023-09-12T12:58:31Z

python_modules/dagster/dagster/_config/pythonic_config/pydantic_compat_layer.py

+from pydantic import BaseModel
+
+from .attach_other_object_to_context import (
+    IAttachDifferentObjectToOpContext as IAttachDifferentObjectToOpContext,


sidenote: should we just kill this feature?

We still use it in a few places, unfortunately. I think it would be a bit of work to strip it out, but can look into it once this lands.

schrockn · 2023-09-28T08:26:33Z

python_modules/dagster/dagster/_config/pythonic_config/config.py

+            if "Instance is frozen" in str(  # Pydantic 2.x error
+                e
+            ) or "is immutable and does not support item assignment" in str(  # Pydantic 1.x error
+                e
+            ):


Still relevant comment

python_modules/dagster/dagster/_config/pythonic_config/config.py

python_modules/dagster/dagster/_config/pythonic_config/conversion_utils.py

python_modules/dagster/dagster/_config/pythonic_config/config.py

python_modules/dagster/dagster/_config/pythonic_config/pydantic_compat_layer.py

schrockn

benpankow · 2023-10-11T23:24:01Z

Merge activity

Oct 11, 4:24 PM: @benpankow started a stack merge that includes this pull request via Graphite.
Oct 11, 4:24 PM: Graphite rebased this pull request as part of a merge.
Oct 11, 4:25 PM: @benpankow merged this pull request with Graphite.

benpankow mentioned this pull request Sep 12, 2023

[1/n][pythonic config] Restructure Pythonic config & resources into modules #16434

Merged

benpankow mentioned this pull request Sep 12, 2023

[2/n][pythonic config] Update config conversion logic to rely on annotation rather than Pydantic model type #16435

Merged

benpankow force-pushed the benpankow/restructure-pythonic-config-module-3 branch from ee5378b to 70a48e7 Compare September 12, 2023 03:08

benpankow mentioned this pull request Sep 12, 2023

[4/n][pythonic config] Run Pydantic 2 tests #16437

Merged

benpankow requested review from alangenfeld, smackesey and schrockn September 12, 2023 04:02

benpankow changed the title ~~[3/n][pythonic config] Layer in Pydantic compat abstraction layer"~~ [3/n][pythonic config] Layer in Pydantic compat abstractions Sep 12, 2023

schrockn requested changes Sep 12, 2023

View reviewed changes

benpankow force-pushed the benpankow/restructure-pythonic-config-module-2 branch from 203badb to 6c0fe9b Compare September 14, 2023 23:21

benpankow requested a review from erinkcochran87 as a code owner September 14, 2023 23:21

benpankow force-pushed the benpankow/restructure-pythonic-config-module-3 branch from 49b59df to 73e9411 Compare September 14, 2023 23:21

benpankow force-pushed the benpankow/restructure-pythonic-config-module-2 branch 3 times, most recently from b6e711d to 039c24b Compare September 28, 2023 03:33

benpankow force-pushed the benpankow/restructure-pythonic-config-module-3 branch from 73e9411 to 8924ae9 Compare September 28, 2023 05:07

schrockn requested changes Sep 28, 2023

View reviewed changes

benpankow force-pushed the benpankow/restructure-pythonic-config-module-2 branch from 039c24b to a627a90 Compare October 1, 2023 22:03

benpankow force-pushed the benpankow/restructure-pythonic-config-module-3 branch from 8924ae9 to 4d29663 Compare October 1, 2023 22:39

benpankow requested a review from schrockn October 1, 2023 23:40

schrockn reviewed Oct 2, 2023

View reviewed changes

python_modules/dagster/dagster/_config/pythonic_config/pydantic_compat_layer.py Show resolved Hide resolved

schrockn approved these changes Oct 2, 2023

View reviewed changes

benpankow force-pushed the benpankow/restructure-pythonic-config-module-2 branch from e9127ce to 2c5ee94 Compare October 10, 2023 16:51

benpankow force-pushed the benpankow/restructure-pythonic-config-module-3 branch 4 times, most recently from 35f9b86 to 4c102b0 Compare October 10, 2023 21:02

benpankow force-pushed the benpankow/restructure-pythonic-config-module-2 branch from 28fc043 to a6aba5f Compare October 11, 2023 22:45

benpankow force-pushed the benpankow/restructure-pythonic-config-module-3 branch from 4c102b0 to afebf53 Compare October 11, 2023 22:45

Base automatically changed from benpankow/restructure-pythonic-config-module-2 to master October 11, 2023 23:24

benpankow added 3 commits October 11, 2023 23:24

[3/n][pythonic config] Layer in Pydantic compat abstraction layer"

f34524a

respond to some review comments

2c18a18

pyright

b7beca2

benpankow force-pushed the benpankow/restructure-pythonic-config-module-3 branch from afebf53 to b7beca2 Compare October 11, 2023 23:24

benpankow merged commit f46a70a into master Oct 11, 2023

benpankow deleted the benpankow/restructure-pythonic-config-module-3 branch October 11, 2023 23:25

benpankow mentioned this pull request Oct 13, 2023

[helm] Update helm package to use Pydantic 2.0 #16326

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[3/n][pythonic config] Layer in Pydantic compat abstractions #16436

[3/n][pythonic config] Layer in Pydantic compat abstractions #16436

benpankow commented Sep 12, 2023 •

edited

Loading

benpankow commented Sep 12, 2023 •

edited

Loading

github-actions bot commented Sep 12, 2023 •

edited

Loading

schrockn Sep 12, 2023

schrockn Sep 28, 2023

benpankow Oct 1, 2023 •

edited

Loading

schrockn Sep 12, 2023

schrockn Sep 12, 2023

benpankow Oct 1, 2023

schrockn Sep 12, 2023

benpankow Oct 1, 2023

schrockn Sep 12, 2023

benpankow Oct 1, 2023

schrockn Sep 28, 2023

schrockn left a comment

benpankow commented Oct 11, 2023 •

edited

Loading

[3/n][pythonic config] Layer in Pydantic compat abstractions #16436

[3/n][pythonic config] Layer in Pydantic compat abstractions #16436

Conversation

benpankow commented Sep 12, 2023 • edited Loading

Summary

Test Plan

benpankow commented Sep 12, 2023 • edited Loading

github-actions bot commented Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benpankow Oct 1, 2023 • edited Loading

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

schrockn left a comment

Choose a reason for hiding this comment

benpankow commented Oct 11, 2023 • edited Loading

Merge activity

benpankow commented Sep 12, 2023 •

edited

Loading

benpankow commented Sep 12, 2023 •

edited

Loading

github-actions bot commented Sep 12, 2023 •

edited

Loading

benpankow Oct 1, 2023 •

edited

Loading

benpankow commented Oct 11, 2023 •

edited

Loading