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

[BUG] Flytekit python unable to parse int | None style type hints on python3.11 #4706

Closed
2 tasks done
kumare3 opened this issue Jan 10, 2024 · 2 comments · Fixed by flyteorg/flytekit#2298
Closed
2 tasks done
Labels
backlogged For internal use. Reserved for contributor team workflow. bug Something isn't working flytekit FlyteKit Python related issue

Comments

@kumare3
Copy link
Contributor

kumare3 commented Jan 10, 2024

Describe the bug

3.11 support int|None as a way of specifying optional and union types.
But flytekit TypeEngine resolves this to pickle as it does not understand that it is infact a union.

But if you use typing.Union[int, None] or typing.Optional[int] these work fine.

On 3.9 python has an error and flytekit bubbles this error

def double(x: int | None) -> int:
TypeError: unsupported operand type(s) for |: 'type' and 'NoneType'

Flytekit bubble

Failed with Unknown Exception <class 'TypeError'> Reason: unsupported operand type(s) for |: 'type' and 'NoneType'

and also an associated Long stack trace, that originates in import lib. We can simplify this and simply show the same error as python

Expected behavior

def foo(t: int|None): or def foo(t: Optional[int]) both should work

Additional context to reproduce

No response

Screenshots

No response

Are you sure this issue hasn't been raised already?

  • Yes

Have you read the Code of Conduct?

  • Yes
@kumare3 kumare3 added bug Something isn't working untriaged This issues has not yet been looked at by the Maintainers labels Jan 10, 2024
Copy link

dosubot bot commented Jan 10, 2024

🤖

Hello @kumare3! 👋 I'm Dosu, your friendly bot helper here. Although I might not be a real person, I'm here to assist you in solving bugs, answering questions, and even guiding you on how to be a contributor while we wait for a human maintainer! Just consider me your digital sidekick in this coding adventure. 🚀

Thank you for bringing this to our attention. It seems like the issue is related to the way Flytekit's TypeEngine handles the new int | None style type hints introduced in Python 3.11. Currently, Flytekit is able to understand typing.Union[int, None] or typing.Optional[int], but it treats int | None as a pickle type because it doesn't recognize it as a union.

To address this, we can update Flytekit to support int | None style type hints. This would involve modifying the TypeTransformer's to_literal method to fail with a specific error class, allowing the typing.Union transformer to distinguish between user or programmer error and actual failure to convert type.

Here's a proposed implementation for the typing.Union type transformer in Python FlyteKit:

# Implement a `typing.Union` type transformer in Python FlyteKit:
# `get_literal_type`:
def get_literal_type(t):
    return LiteralType(sum=_type_models.SumType(summands=[TypeEngine.to_literal_type(x) for x in t.__args__]))

# `to_literal`
# Iterate through the types in `python_type.__args__` and try `TypeEngine.to_literal` for each. The first succeeding type is accepted
def to_literal(python_val, python_type, expected):
    for x in python_type.__args__:
        try:
            return TypeEngine.to_literal(python_val, x, expected)
        except SpecificErrorClass:
            pass
    raise ValueError("Value is not an instance of any of the types in the Union")

# `to_python_value`
# Use the `TypeTransformer` for the `lv.sum.type` to transform `lv.sum.value`
def to_python_value(ctx, lv, expected_python_type):
    return TypeEngine.to_python_value(ctx, lv.sum.value, TypeEngine.guess_python_type(lv.sum.type))

# `guess_python_type`
# Return `TypeEngine.guess_python_type(lv.sum.type)`
def guess_python_type(lv):
    return TypeEngine.guess_python_type(lv.sum.type)

This code is based on the rfc/core language/sum-types.md document in the Flyte repository.

As for the error handling, we can simplify the error message to match the one Python throws when it encounters unsupported operand types. This would involve catching the TypeError and re-throwing it with a more user-friendly message.

Please note that this is a proposed solution and might require adjustments based on the specific requirements and constraints of your project. If you have any further questions or need more clarification, feel free to ask.

Sources


This response is meant to be useful and save you time. It is not meant to be a precise solution, but rather a starting point for your own research.

Help me be more useful! Please leave a 👍 if this is helpful and 👎 if it is irrelevant.

If you want to continue the conversation, start your reply with @dosu-bot.

@eapolinario eapolinario added backlogged For internal use. Reserved for contributor team workflow. flytekit FlyteKit Python related issue and removed untriaged This issues has not yet been looked at by the Maintainers labels Jan 25, 2024
@thomasjpfan
Copy link
Member

Closing because this feature was implemented in flyteorg/flytekit#2298

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlogged For internal use. Reserved for contributor team workflow. bug Something isn't working flytekit FlyteKit Python related issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants