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

Rework typing in the repo #164

Open
mkmkme opened this issue Dec 19, 2024 · 7 comments
Open

Rework typing in the repo #164

mkmkme opened this issue Dec 19, 2024 · 7 comments
Assignees
Labels
help wanted Extra attention is needed typing

Comments

@mkmkme
Copy link
Collaborator

mkmkme commented Dec 19, 2024

As was pointed in #162, the typing problems go quite deep and will likely require some refactoring.

The biggest issue is the fact that sync and async clients inherit from the same class, which causes issues in type system.

For instance, this code:

from typing import *

class Parent:
    def set(self) -> int:
        return 1
        
class Child(Parent):
    async def set(self) -> int:
        return 1

raises this mypy error:

main.py:8: error: Return type "Coroutine[Any, Any, int]" of "set" incompatible with return type "int" in supertype "Parent"  [override]

Attempting to have a union there won't help either. That is, this code

from typing import *

class Parent:
    def set(self) -> int | Coroutine[Any, Any, int]:
        return 1
        
class Child(Parent):
    async def set(self) -> int | Coroutine[Any, Any, int]:
        return 1

causes this error

main.py:8: error: Return type "Coroutine[Any, Any, int | Coroutine[Any, Any, int]]" of "set" incompatible with return type "int | Coroutine[Any, Any, int]" in supertype "Parent"  [override]

There should be a way to fix this without completely reworking the class hierarchy in the repo. This should be investigated.

@mkmkme mkmkme added help wanted Extra attention is needed typing labels Dec 19, 2024
@mkmkme mkmkme self-assigned this Dec 19, 2024
@amirreza8002
Copy link
Contributor

amirreza8002 commented Dec 19, 2024

hi sorry for the mess :)

as with the discussion we had in discord with graingert and fix error, this could be solved with python's generic types
tho it would need some work to fully adopt it

basically:

Async = Literal[True]
Sync = Literal[False]
IsAsynchronous = TypeVar("IsAsynchronous", Async, Sync)

class A(Generic[IsAsynchronous]):
    @overload
    def a(self: A[Sync]) -> int: ...

    @overload
    def a(self: A[Async]) -> Coroutine[int, Any, Any]: ...
    
    def a(self) -> Union[int, Coroutin[int, Any, Any]:
        # do stuff

but i have a feeling writing a separate async client would end up with smaller code base :p, who perhaps harder to maintain.

as refrence:
https://inspector.pypi.io/project/coiled/1.69.1.dev29/packages/04/17/74ef3c8799f6d3aa4a3d36e54d687b6b03cd98af7e60c54a21b04a764828/coiled-1.69.1.dev29-py3-none-any.whl/coiled/core.py#line.589

as a side note:

async def a() -> int:
    return 1

def b() -> Coroutine[int, Any, Any]:
    return a()

@mkmkme
Copy link
Collaborator Author

mkmkme commented Dec 19, 2024

Hey,

Thanks for your input! I'm also now looking towards generics but with slightly different approach. I'll post updates in this issue.

@mkmkme
Copy link
Collaborator Author

mkmkme commented Dec 19, 2024

Used Gemini to see if it can come up with a solution for this. It actually suggested this which I think is quite nice:

from typing import *
from abc import ABC, abstractmethod

S = TypeVar('S')

# Helper type to represent either a value or a coroutine that produces a value
MaybeAwaitable = Union[S, Awaitable[S]]

class Parent(ABC):
    @abstractmethod
    def set(self) -> MaybeAwaitable[int]:
        pass

    @abstractmethod
    def get_value(self) -> MaybeAwaitable[str]:
        pass

    # ... other methods with different return types ...


class SyncChild(Parent):
    def set(self) -> int:
        return 1

    def get_value(self) -> str:
        return "sync_value"

    # ... other synchronous methods ...


class AsyncChild(Parent):
    async def set(self) -> int:
        return 1

    async def get_value(self) -> str:
        return "async_value"

    # ... other asynchronous methods ...

This code keeps mypy happy

@mkmkme
Copy link
Collaborator Author

mkmkme commented Dec 19, 2024

It actually resembles quite a bit what had already been in the code base, namely ResponceT is Union[Awaitable[Any], Any].

What I'd do is I'd make it less generic. Returning Any is not optimal.

@amirreza8002
Copy link
Contributor

well, in this case you still have two separate classes for sync and async, what is the parent doing?

@amirreza8002
Copy link
Contributor

there's also a package called unasync, which might be of interest?

from a glance it looks like it can ease up the work here

@mkmkme
Copy link
Collaborator Author

mkmkme commented Dec 19, 2024

thanks, I'll have a look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed typing
Projects
None yet
Development

No branches or pull requests

2 participants