youtype / mypy_boto3_builder

Type annotations builder for boto3 compatible with VSCode, PyCharm, Emacs, Sublime Text, pyright and mypy.
https://youtype.github.io/mypy_boto3_builder/
MIT License
518 stars 35 forks source link

Mypy-boto3-appflow 1.28.12 has inconsistent typing #209

Closed potiuk closed 11 months ago

potiuk commented 11 months ago

Describe the bug

The Mypy-boto3-appflow 1.28.12 introduces inconsistent typing for input/output parameters In airlfow, our tests started to fail in "canary" builds after that release and it seems we cannot use response from describe_flow in update_flow. without explicitly casting the types:

This code:

response = self.conn.describe_flow(flowName=flow_name)
....

self.conn.update_flow(
            flowName=response["flowName"],
            destinationFlowConfigList=response["destinationFlowConfigList"],
            sourceFlowConfig=response["sourceFlowConfig"],
            triggerConfig=response["triggerConfig"],
            description=response.get("description", "Flow description."),
            tasks=tasks,
        )

When we are passing output of appflow describe_flow to input of update flow, mypy complains:

airflow/providers/amazon/aws/hooks/appflow.py:113: error: Argument
"destinationFlowConfigList" to "update_flow" of "AppflowClient" has incompatible
type "List[DestinationFlowConfigOutputTypeDef]"; expected
"Sequence[DestinationFlowConfigTypeDef]"  [arg-type]

To Reproduce

You will see the mypy error above.

Example of it as well here: https://github.com/apache/airflow/actions/runs/5685555908/job/15411044776?pr=32882

Actual output

airflow/providers/amazon/aws/hooks/appflow.py:113: error: Argument
"destinationFlowConfigList" to "update_flow" of "AppflowClient" has incompatible
type "List[DestinationFlowConfigOutputTypeDef]"; expected
"Sequence[DestinationFlowConfigTypeDef]"  [arg-type]
                destinationFlowConfigList=response["destinationFlowConfigL...
                                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~...
airflow/providers/amazon/aws/hooks/appflow.py:114: error: Argument
"sourceFlowConfig" to "update_flow" of "AppflowClient" has incompatible type
"SourceFlowConfigOutputTypeDef"; expected "SourceFlowConfigTypeDef"  [arg-type]
                sourceFlowConfig=response["sourceFlowConfig"],
                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
airflow/providers/amazon/aws/hooks/appflow.py:115: error: Argument
"triggerConfig" to "update_flow" of "AppflowClient" has incompatible type
"TriggerConfigOutputTypeDef"; expected "TriggerConfigTypeDef"  [arg-type]
                triggerConfig=response["triggerConfig"],
                              ^~~~~~~~~~~~~~~~~~~~~~~~~
airflow/providers/amazon/aws/hooks/appflow.py:117: error: Argument "tasks" to
"update_flow" of "AppflowClient" has incompatible type
"List[TaskOutputTypeDef]"; expected "Sequence[TaskTypeDef]"  [arg-type]
                tasks=tasks,
                      ^~~~~

Expected output

No erorrs.

Additional context

It was working fine in 1.28.0. We are going to exclude 1.28.12 until this gets fixed (or maybe once we know how to adapt the code of ours other than explicitly casting the output types to the input ones - which seems like a bad idea).

cc: @o-nikolas

vemel commented 11 months ago

Thank you for the report!

Fixes for input/output TypeDefs are already implemented, and hopefully be released today. I am going to cross-check your case. Could you please prepare a short snippet to reproduce the issue?

vemel commented 11 months ago

No issues with the new version on the following snippet:

import boto3

conn = boto3.client("appflow")

response = conn.describe_flow(flowName="flow_name")

conn.update_flow(
    flowName=response["flowName"],
    destinationFlowConfigList=response["destinationFlowConfigList"],
    sourceFlowConfig=response["sourceFlowConfig"],
    triggerConfig=response["triggerConfig"],
    description=response.get("description", "Flow description."),
    tasks=[
        {
            "sourceFields": ["field_name", "field_name2"],
            "taskType": "Map",
        }
    ],
)

I am going to prepare a new release and update boto3 packages later today.

potiuk commented 11 months ago

No issues with the new version on the following snippet:

import boto3

conn = boto3.client("appflow")

response = conn.describe_flow(flowName="flow_name")

conn.update_flow(
    flowName=response["flowName"],
    destinationFlowConfigList=response["destinationFlowConfigList"],
    sourceFlowConfig=response["sourceFlowConfig"],
    triggerConfig=response["triggerConfig"],
    description=response.get("description", "Flow description."),
    tasks=[
        {
            "sourceFields": ["field_name", "field_name2"],
            "taskType": "Map",
        }
    ],
)

I am going to prepare a new release and update boto3 packages later today.

Thanks. I am travelling and had no time to check it - but yes that was precisely the snippet that caused the problems for us so it looks good.

Thanks for quick reaction.

potiuk commented 11 months ago

BTW. Once you release the new package, Airflow will run a canary build and will automatically pull and test the last version - so we will know immediately if it's been fixed :)

vemel commented 11 months ago

Well, I have good news, and I have bad news.

Unfortunately, it looks like a bug (or expected behavior) in mypy.

from typing import TypedDict

class InputTypedDict(TypedDict, total=True):
    field_name: str | int

class OutputTypedDict(TypedDict, total=True):
    field_name: str

def test(data: InputTypedDict) -> OutputTypedDict:
    return {
        "field_name": str(data["field_name"]),
    }

data: OutputTypedDict = {"field_name": "123"}
test(data)  # Argument 1 to "test" has incompatible type "OutputTypedDict"; expected "InputTypedDict"

The proper solution would be to check if input and output shapes are compatible and if they are, accept Union[MyTypeDef, MyOutputTypeDef] instead of MyTypeDef.

I will test and let you know.

vemel commented 11 months ago

Just in case, TypedDict compatibility is implemented in pyright properly. However, I cannot find a relevant mypy issue. Probably I should create one.

potiuk commented 11 months ago

Indeed. Seems like the issue is back even after the new release with 1.28.15: https://github.com/apache/airflow/actions/runs/5696388972/job/15441693475#step:6:161

Unfortunately, I cannot change it on my side - the problem is that both - producer(describe_flow) and consumer(update_flow) of the data structures come from mypy-boto3-appflow, so I cannot change the consumer to accept the union. Both methods are in the mypy-boto3-appflow realm.

For now I am going to simply limit mypy-boto3-appflow to < 1.28 and I hope it can get resolved somehow - possibly with mypy if you create an issue, but I think honestly this is a flaw of the design if you have two - from Python point of view - types and expect that one will be compatible with the other. That sounds a bit strange.

vemel commented 11 months ago

@potiuk I am preparing a new release with the changes I described above.

You can help me if you test mypy-boto3-appflow 1.28.15.post1 on your code and let me know if it works as it should for you. If everything is good - I am going to release the changes tomorrow for all packages.

potiuk commented 11 months ago

Sure. I can do that

potiuk commented 11 months ago

Build running here: https://github.com/apache/airflow/pull/32930

potiuk commented 11 months ago

All looks good. We had this one on the first pass:

airflow/providers/amazon/aws/hooks/appflow.py:103: error: Argument 1 to
"append" of "list" has incompatible type "TaskOutputTypeDef"; expected
"TaskTypeDef"  [arg-type]
                    tasks.append(task)  # List of non-filter tasks

But this is one originating from our code and I just fixed it with changing tasks: list[TaskTypeDef] = [] into tasks: list[TaskTypeDef | TaskOutputTypeDef] = []

potiuk commented 11 months ago

I re-run https://github.com/apache/airflow/pull/32930 with this fix.

potiuk commented 11 months ago

Confirmed. All static checks passed with the fix: https://github.com/apache/airflow/actions/runs/5699174893/job/15447938188?pr=32930

vemel commented 11 months ago

Thank you for testing this postrelease. I am going to leave this issue open until mypy-boto3-appflow 1.28.16 is released next Monday.

Please let me know if you find any other issues, that was a huge improvement.

vemel commented 11 months ago

Closing the issue, all packages are updated.

Please reopen in case you have any issues. And feel free to report new ones.