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

benpankow · 2023-09-12T02:49:56Z

Summary

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

This PR holds the meat of the actual changes. It reworks some of the underlying conversion code to look at the annotation (e.g. actual Python type objects) on config classes instead of the Pydantic internal type representations, when converting to Dagster config types under the hood.

Test Plan

Existing unit tests.

benpankow · 2023-09-12T02:50:02Z

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-12T02:53:49Z

Deploy preview for dagster-docs ready!

Preview available at https://dagster-docs-p917pdokn-elementl.vercel.app
https://benpankow-restructure-pythonic-config-module-2.dagster.dagster-docs.io

Direct link to changed pages:

schrockn

This looks good. I just have some questions about some code paths I do not understand (and should probably be commented?)

schrockn · 2023-09-12T12:47:27Z

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

+        for key, field in self.__fields__.items():
+            if field.required and key not in modified_data:
+                modified_data[key] = None
+


@benpankow can you speak to what is going on with these changes? These look pretty core...

This is to accommodate the changes in Pydantic 2.0 around the meaning of required and optional fields. This change ensures that all required fields are passed a value when calling the BaseModel __init__.

In Pydantic 1.0, this is a no-op, since this is implicitly done behind the scenes.

Thanks for the context. Let's put this explanation in the code as a comment.

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

schrockn · 2023-09-12T12:51:04Z

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

+    if safe_is_subclass(get_origin(potential_dagster_type), List):
+        list_inner_type = get_args(potential_dagster_type)[0]
+        return Array(_config_type_for_type_on_pydantic_field(list_inner_type))
+    elif _is_optional(potential_dagster_type):
+        optional_inner_type = next(
+            arg for arg in get_args(potential_dagster_type) if arg is not type(None)
+        )
+        return Noneable(_config_type_for_type_on_pydantic_field(optional_inner_type))
+    elif safe_is_subclass(get_origin(potential_dagster_type), Dict) or safe_is_subclass(
+        get_origin(potential_dagster_type), Mapping
+    ):
+        key_type, value_type = get_args(potential_dagster_type)
+        return Map(
+            key_type,
+            _config_type_for_type_on_pydantic_field(value_type),
+        )
+
+    from .config import Config, infer_schema_from_config_class
+
+    if safe_is_subclass(potential_dagster_type, Config):
+        inferred_field = infer_schema_from_config_class(
+            potential_dagster_type,
+        )
+        return inferred_field.config_type
+


can you comment as to what is going on here?

Yes - here we're reworking the type case/switching in which we originally referred to the Pydantic internal type system. Now, we just check against the raw Python type annotations, which is simpler and works with Pydantic 2.0 (which eliminates the internal type system).

This logic is compacted a bit from what we had previously - it used to be done both here and in the _wrap_config_type and _get_inner_field_if_exists functions, which are no longer used.

python_modules/dagster/dagster/_config/pythonic_config/resource.py

github-actions · 2023-09-28T02:57:05Z

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-60jccysj3-elementl.vercel.app
https://benpankow-restructure-pythonic-config-module-2.core-storybook.dagster-docs.io

Built with commit a6aba5f.
This pull request is being automatically deployed with vercel-action

github-actions · 2023-09-28T02:59:37Z

Deploy preview for dagit-storybook ready!

✅ Preview
https://dagit-storybook-hounguv1v-elementl.vercel.app
https://benpankow-restructure-pythonic-config-module-2.components-storybook.dagster-docs.io

Built with commit 2c5ee94.
This pull request is being automatically deployed with vercel-action

schrockn

Ok one final pass.

It's also essential that you do a BK run across all Python versions before merging. I believe we can kick that off manually?

python_modules/dagster/dagster/_config/pythonic_config/resource.py

schrockn · 2023-09-28T08:24:58Z

python_modules/dagster/dagster/_config/pythonic_config/type_check_utils.py

+def is_optional(annotation: Type) -> bool:
+    """Returns true if the annotation is Optional[T] or Union[T, None]."""
+    return (
+        get_origin(annotation) in (Union, UnionType)
+        and len(get_args(annotation)) == 2
+        and type(None) in get_args(annotation)
+    )


This comment isn't quite accurate. This returns true if any of the generic arguments to Union is None, I believe.

I think this is worth:

Separating the cases of Union and UnionType more clearly. Without really deep knowledge of this corner of the Python type system internals it is hard to understand how your description maps to the code.

Explaining to how Optional maps to Union, linking to docs if necessary.

This is the type of code that you can easily see causing bugs across different Python versions, so I think we should bias towards being super explicit and over-explaining. (That's kind of true for a bunch of the stuff in this PR).

Fixed docstring + expanded in comments on some of the typing nuances.

benpankow · 2023-10-01T22:17:08Z

Build against py3.8-3.11: https://buildkite.com/dagster/dagster/builds/67955

schrockn

Great. Please heed final comments.

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/resource.py

schrockn · 2023-10-02T08:24:09Z

python_modules/dagster/dagster/_config/pythonic_config/type_check_utils.py

+    # The Python 3.10 pipe syntax evaluates to a UnionType
+    # rather than a Union, so we need to check for that as well


Thanks. Also I wonder why 3.10 does this. Seems like this will bite everyone.

smackesey

Cool, I don't see anything alarming

…n rather than Pydantic model type

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: @benpankow merged this pull request with Graphite.

… values (#17242) ## Summary Updates our handling of passing defaulted values back into Pydantic model constructors. Previously, we'd pass in a model's config dictionary into its constructor, but this breaks the Pydantic [validator expectation](https://docs.pydantic.dev/latest/concepts/validators/#validation-of-default-values) that validators only run when a user explicitly provides input. This was already not working as expected, but was further obfuscated by some changes in #16435. ## Test Plan Adds some unit tests to test out this behavior in various config/resource scenarios.

benpankow requested a review from erinkcochran87 as a code owner September 12, 2023 02:49

benpankow mentioned this pull request Sep 12, 2023

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

Merged

benpankow changed the title ~~[pythonic config] Update config conversion logic to rely on annotation rather than Pydantic model type~~ [2/n][pythonic config] Update config conversion logic to rely on annotation rather than Pydantic model type Sep 12, 2023

This was referenced Sep 12, 2023

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

Merged

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

Merged

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

schrockn requested changes Sep 12, 2023

View reviewed changes

benpankow force-pushed the benpankow/restructure-pythonic-config-module branch from 4168aea to f00ba77 Compare September 14, 2023 23:21

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

Base automatically changed from benpankow/restructure-pythonic-config-module to master September 14, 2023 23:48

benpankow force-pushed the benpankow/restructure-pythonic-config-module-2 branch from 6c0fe9b to 1f4df49 Compare September 28, 2023 02:53

benpankow force-pushed the benpankow/restructure-pythonic-config-module-2 branch from 1f4df49 to b6e711d Compare September 28, 2023 03:16

benpankow requested a review from schrockn September 28, 2023 03:33

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

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 requested a review from schrockn October 1, 2023 22:39

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

smackesey approved these changes Oct 11, 2023

View reviewed changes

benpankow added 3 commits October 11, 2023 15:31

[pythonic config] Update config conversion logic to rely on annotatio…

0a0b8ea

…n rather than Pydantic model type

respond to review comments test-py38 test-py39 test-py310 test-py311

29120bd

update comment a bit more

89c98e9

benpankow added 4 commits October 11, 2023 15:31

review pass

9402134

rm unnecessary early continue

286598d

add it back

dcd826e

add comment

a6aba5f

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

benpankow merged commit dcd28b7 into master Oct 11, 2023
2 of 3 checks passed

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

This was referenced Oct 13, 2023

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

Closed

[pythonic config][fix] Properly handle Pydantic validators w/ default values #17242

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

benpankow commented Sep 12, 2023 •

edited

Loading

benpankow commented Sep 12, 2023 •

edited

Loading

github-actions bot commented Sep 12, 2023 •

edited

Loading

schrockn left a comment

schrockn Sep 12, 2023

benpankow Sep 28, 2023

schrockn Sep 28, 2023

schrockn Sep 12, 2023

benpankow Sep 28, 2023

github-actions bot commented Sep 28, 2023 •

edited

Loading

github-actions bot commented Sep 28, 2023 •

edited

Loading

schrockn left a comment

schrockn Sep 28, 2023

benpankow Oct 1, 2023

benpankow commented Oct 1, 2023

schrockn left a comment

schrockn Oct 2, 2023

smackesey left a comment

benpankow commented Oct 11, 2023 •

edited

Loading

		# The Python 3.10 pipe syntax evaluates to a UnionType
		# rather than a Union, so we need to check for that as well

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

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

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

schrockn 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

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

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

schrockn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benpankow commented Oct 1, 2023

schrockn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smackesey 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

github-actions bot commented Sep 28, 2023 •

edited

Loading

github-actions bot commented Sep 28, 2023 •

edited

Loading

benpankow commented Oct 11, 2023 •

edited

Loading