Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Epic] Python: Improved Typing #12689

Closed
17 of 26 tasks
justinvp opened this issue Apr 17, 2023 · 5 comments
Closed
17 of 26 tasks

[Epic] Python: Improved Typing #12689

justinvp opened this issue Apr 17, 2023 · 5 comments
Assignees
Labels
kind/epic Large new features or investments language/python resolution/fixed This issue was fixed

Comments

@justinvp
Copy link
Member

justinvp commented Apr 17, 2023

For Python codegen we generate classes for inputs of object type to enable strong typing for these inputs. However, this leads to somewhat verbose syntax, requiring to instantiate the "args classes". See #11500 for a motivating example.

With PEP 589 – TypedDict and PEP 655 – Marking individual TypedDict items as required or potentially-missing there is now a more concise way to achieve the same result.

For each input object type we can generate a Args class as before, as well as a ArgsDict TypedDict based type. To support backwards compatibility, inputs of object type will accept the union of the two types.

class MyTypeArgsDict(TypedDict):
    my_prop: pulumi.Input[str]
    my_other_prop: NotRequired[pulumi.Input[float]]

Related issues: #11732

Example with Args

from pulumi_kubernetes.apps.v1 import Deployment
from pulumi_kubernetes.apps.v1 import Deployment, DeploymentSpecArgs
from pulumi_kubernetes.meta.v1 import LabelSelectorArgs, ObjectMetaArgs
from pulumi_kubernetes.core.v1 import ContainerArgs, PodSpecArgs, PodTemplateSpecArgs

app_labels = {"app": "nginx"}
deployment = Deployment(
    "nginx",
    spec=DeploymentSpecArgs(
        selector=LabelSelectorArgs(match_labels=app_labels),
        replicas=1,
        template=PodTemplateSpecArgs(
            metadata=ObjectMetaArgs(labels=app_labels),
            spec=PodSpecArgs(containers=[ContainerArgs(name="nginx", image="nginx")]),
        ),
    ),
)

Example with ArgsDict

from pulumi_kubernetes.apps.v1 import Deployment

app_labels = {"app": "nginx"}

deployment = Deployment(
    "nginx",
    spec={
        "selector": {"match_labels": app_labels},
        "replicas": 1,
        "template": {
            "metadata": {"labels": app_labels},
            "spec": {"containers": [{"name": "nginx", "image": "nginx"}]},
        },
    },
)

Design (M103)

Implementation Plan

M104

  • Get started on implementation
  • Verify mypy/pyright/pycharm performance for TypedDict
  • Review typing error messages for TypedDict

M106

M107

Future Iteration

Rollout & Announce

@justinvp justinvp added language/python kind/epic Large new features or investments labels Apr 17, 2023
@justinvp justinvp self-assigned this Apr 17, 2023
@jf
Copy link

jf commented May 17, 2023

hi Justin, for folks interested in the progress of this, where would we go to observe the progress and/or help with things?

@justinvp
Copy link
Member Author

@jf, keeping an eye on this issue and #11732 is the best place to observe progress. When we have a fleshed out design proposal, we'll post it here.

@julienp
Copy link
Contributor

julienp commented May 17, 2024

We have a working implementation in #15957, however we ran into some performance issues when using MyPy 1.

To work around this, we currently generate the TypedDict based args to behave differently depending on the used type checker. MyPy supports a special variable MYPY that is always considered true, and we can use this to hide the TypedDict types from it, and use Mapping[str, Any] instead. The PyDantic library currently uses a similar workaround, although for a different performance issue 2.

Here's an example from the AWS provider:

if not MYPY:
    class InstanceCpuOptionsArgsDict(TypedDict):
        amd_sev_snp: NotRequired[pulumi.Input[str]]
        core_count: NotRequired[pulumi.Input[int]]
        threads_per_core: NotRequired[pulumi.Input[int]]
elif False:
    InstanceCpuOptionsArgsDict: TypeAlias = Mapping[str, Any]

When using Pyright, you get the full type checking experience within dictionary arguments, but not with MyPy.

The MyPy behaviour matches the previous implementation where these input types could either take an Args class or a Mapping[str, Any]. This means that we technically don't have a regression here, and when using Pyright you get a better experience.

However the question then becomes what style of arguments should we encourage? Currently we encourage the Args style classes over passing in dictionaries. It is the default for generated code and in documentation. This is not the most pythonic approach and fairly verbose 3, but does give strong typing and better IDE discoverability.

I'll also note that Pyright does not produce very readable error messages

Example

For the following code, Pyright correctly notes an error, but MyPy checking passes.

from pulumi_kubernetes.apps.v1 import Deployment

deployment = Deployment("nginx",
    spec={
        "selector":{
            "match_labels": { "app": "nginx" }
        },
        "replicas": "one", # This should be a Input[int]
        "template": { "spec": { "containers": [{ "name": "nginx", "image": "nginx" }] }
        }
    }
)

MyPy reports no issues

python3 -m mypy __main__.py
Success: no issues found in 1 source fil

Pyright reports an error, but the error message is not very friendly, and it is not obvious what the actual error is.

__main__.py:30:10 - error: Argument of type "dict[str, dict[str, dict[str, str]] | str | dict[str, dict[str, list[dict[str, str]]]]]" cannot be assigned to parameter "spec" of type "Input[DeploymentSpecArgs | DeploymentSpecArgsDict] | None" in function "__init__"
    Type "dict[str, dict[str, dict[str, str]] | str | dict[str, dict[str, list[dict[str, str]]]]]" is incompatible with type "Input[DeploymentSpecArgs | DeploymentSpecArgsDict] | None"
      "dict[str, dict[str, dict[str, str]] | str | dict[str, dict[str, list[dict[str, str]]]]]" is incompatible with "DeploymentSpecArgs"
      "dict[str, dict[str, dict[str, str]] | str | dict[str, dict[str, list[dict[str, str]]]]]" is incompatible with "DeploymentSpecArgsDict"
      "dict[str, dict[str, dict[str, str]] | str | dict[str, dict[str, list[dict[str, str]]]]]" is incompatible with protocol "Awaitable[DeploymentSpecArgs | DeploymentSpecArgsDict]"
        "__await__" is not present
      "dict[str, dict[str, dict[str, str]] | str | dict[str, dict[str, list[dict[str, str]]]]]" is incompatible with "Output[DeploymentSpecArgs | DeploymentSpecArgsDict]"
      "dict[str, dict[str, dict[str, str]] | str | dict[str, dict[str, list[dict[str, str]]]]]" is incompatible with "None" (reportArgumentType)
1 error, 0 warnings, 0 informations

Footnotes

  1. https://github.com/python/mypy/issues/17231

  2. https://github.com/pydantic/pydantic-core/pull/528

  3. https://github.com/pulumi/pulumi/discussions/11500

@julienp
Copy link
Contributor

julienp commented Jun 12, 2024

It looks like PyCharm also picks Mapping[str, Any] when confronted with the if not MYPY/elif False workaround.

github-merge-queue bot pushed a commit that referenced this issue Jun 18, 2024
Epic: Improved Typing #12689

This PR adds the flag `Languages.Python.InputTypes` to the schema, which
can take the values `classes` or `classes-and-dicts`. In the first
iteration of the feature, this defaults to `classes`, which leaves code
generation as is and does not change input types.. If the flag is set to
`classes-and-dicts`, `TypedDict` based types are generated next to the
current `<Type>Args` classes. In the future this could be extended to
support `dicts` to generate only `TypedDict` types.

The generated types are conditional on the used type checker to work
around perf issues in MyPy and PyCharm, see
#12689 (comment)

```python
if not MYPY:
    class DeploymentArgsDict(TypedDict):
        api_version: NotRequired[Input[str]]
        kind: NotRequired[Input[str]]
        metadata: NotRequired[Input['ObjectMetaArgsDict']]
        ...
elif False:
    DeploymentArgsDict: TypeAlias = Mapping[str, Any]
```

Removing the workaround is tracked in
#16408

---------

Co-authored-by: Anthony King <anthony@datapane.com>
Co-authored-by: Justin Van Patten <jvp@justinvp.com>
julienp added a commit to pulumi/pulumi-kubernetes that referenced this issue Jun 25, 2024
### Proposed changes

This PR requires pulumi/pulumi 3.121.0 and is stacked on top of
#3066.

Enable TypedDict input types for the Python SDK. The only manual change
is to set `inputTypes` flag in the python language features of the
schema. This is done here
20314f5.
This flag is currently opt in, but will default to `classes-and-dicts`
in the future.

I ran `make build` afterwards.

### Related issues (optional)

Epic pulumi/pulumi#12689
julienp added a commit to pulumi/pulumi-aws that referenced this issue Jun 25, 2024
Enable TypedDict input types for the Python SDK. The only manual change
is to set the InputTypes flag in the python language features of the
schema. This is done here
25a4cec.
This flag is currently opt in, but will default to classes-and-dicts in
the future.

I ran `make tfgen build_sdks` afterwards.

Epic pulumi/pulumi#12689
julienp added a commit to pulumi/pulumi-azure-native that referenced this issue Jul 1, 2024
julienp added a commit to pulumi/pulumi-azure-native that referenced this issue Jul 1, 2024
julienp added a commit to pulumi/pulumi-azure-native that referenced this issue Jul 3, 2024
Enable TypedDict input types for the Python SDK. The only manual change
is to set the InputTypes flag in the python language features of the
schema. This is done here
5a99a47.
This flag is currently opt in, but will default to classes-and-dicts in
the future.

Epic pulumi/pulumi#12689
julienp added a commit to pulumi/pulumi-azuredevops that referenced this issue Jul 8, 2024
Enable TypedDict input types for the Python SDK. The only manual change
is to set the InputTypes flag in the python language features of the
schema. This is done here
f11e86d.
This flag is currently opt in, but will default to classes-and-dicts in
the future.

Epic pulumi/pulumi#12689
julienp added a commit to pulumi/pulumi-azuread that referenced this issue Jul 8, 2024
Enable TypedDict input types for the Python SDK. The only manual change
is to set the InputTypes flag in the python language features of the
schema. This is done here
b7001de.
This flag is currently opt in, but will default to classes-and-dicts in
the future.

Epic pulumi/pulumi#12689
julienp added a commit to pulumi/pulumi-azure that referenced this issue Jul 8, 2024
Enable TypedDict input types for the Python SDK. The only manual change
is to set the InputTypes flag in the python language features of the
schema. This is done here
e0ccec8.
This flag is currently opt in, but will default to classes-and-dicts in
the future.

Epic pulumi/pulumi#12689
@justinvp justinvp added the resolution/fixed This issue was fixed label Jul 9, 2024
@justinvp
Copy link
Member Author

justinvp commented Jul 9, 2024

Closing this out as fixed! 🎉

Blog announcement here: https://www.pulumi.com/blog/pulumi-loves-python/

There is some remaining work to rollout fully across the long tail of providers, docs, and examples, but we'll track the remaining work via the individual issues.

@justinvp justinvp closed this as completed Jul 9, 2024
julienp added a commit to pulumi/templates that referenced this issue Aug 27, 2024
With type checking for dictionary literals, we now prefer this style over args classes pulumi/pulumi#12689
julienp added a commit to pulumi/templates that referenced this issue Aug 28, 2024
With type checking for dictionary literals, we now prefer this style over args classes pulumi/pulumi#12689
julienp added a commit to pulumi/templates that referenced this issue Aug 28, 2024
With type checking for dictionary literals, we now prefer this style over args classes pulumi/pulumi#12689
julienp added a commit to pulumi/templates that referenced this issue Aug 30, 2024
With type checking for dictionary literals, we now prefer this style over args classes pulumi/pulumi#12689
julienp added a commit to pulumi/docs that referenced this issue Aug 30, 2024
With type checking for dictionary literals, we now prefer this style over args classes pulumi/pulumi#12689
justinvp pushed a commit to pulumi/docs that referenced this issue Aug 30, 2024
With type checking for dictionary literals, we now prefer this style over args classes pulumi/pulumi#12689
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/epic Large new features or investments language/python resolution/fixed This issue was fixed
Projects
None yet
Development

No branches or pull requests

3 participants