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

Typing for sync/async decorators #298

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

shughes-uk
Copy link
Contributor

This is a first pass draft, fairly sure it's broken for python versions lower than 3.8.

Seems to work but i'm not sure how to test it right this moment, i'm used to using pyright and will have to investigate how to use mypy

One thing that confused me a bit is that the async_to_sync function accepts awaitable but it actually seems to be looking for a callable that returns an awaitable? I could use some clarification there.

I'm also not sure what naming convention to use for the TypeVar/Paramspec pairs.

Looks like I also introduce some formatting errors to correct!

@shughes-uk
Copy link
Contributor Author

Seems I have some work to do with mypy and this!

@andrewgodwin
Copy link
Member

No worries - take the time you need to get everything working! I might suggest you mark the PR as a draft, though, so that way you can move it to "ready for review" when you think you've got it all ready to go.

@shughes-uk shughes-uk marked this pull request as draft September 26, 2021 19:20
@shughes-uk
Copy link
Contributor Author

mypy seems not to support paramspec yet so this might take a little while unless you want to switch to pyright or pyre python/mypy#8645

@andrewgodwin
Copy link
Member

I'm not really feeling like switching typechecker right at this moment, unfortunately. Sorry for delaying this yet further!

@graingert
Copy link
Contributor

graingert commented Oct 15, 2021

This is a first pass draft, fairly sure it's broken for python versions lower than 3.8.

typing_extensions.ParamSpec has been backported to python 3.6 so it should still work fine.

Seems to work but i'm not sure how to test it right this moment, i'm used to using pyright and will have to investigate how to use mypy

mypy on pypi currently doesn't support ParamSpec, but now that python/mypy#10883 and python/mypy#10866 and python/mypy#10862 have been merged the version on m*ster at least treats ParamSpec as Any so as soon as mypy releases this change will be no worse for mypy users.

One thing that confused me a bit is that the async_to_sync function accepts awaitable but it actually seems to be looking for a callable that returns an awaitable? I could use some clarification there.

That should definitely be a Callable[P, Awaitable[R]]

I'm also not sure what naming convention to use for the TypeVar/Paramspec pairs.

you can just use:

P = ParamSpec("P")
R = TypeVar("R")

you don't need define new ParamSpec placeholder variables for every use - you can use the same one over and over.

Looks like I also introduce some formatting errors to correct!

asgiref/sync.py Outdated
Comment on lines 488 to 492
loop,
source_task,
exc_info,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably worth defining an _ExcInfo type alias here:

_ExcInfo = Tuple[Optional[Type[BaseException], None], Optional[BaseException], Optional[types.TracebackType]]
Suggested change
loop,
source_task,
exc_info,
loop: asyncio.AbstractEventLoop,
source_task: asyncio.Task[object],
exc_info: _ExcInfo,

asgiref/sync.py Outdated
self,
args,
kwargs,
call_result: a_cls_return,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
call_result: a_cls_return,
call_result: Future[a_cls_return],

asgiref/sync.py Outdated
Comment on lines 285 to 288
args,
kwargs,
Copy link
Contributor

@graingert graingert Oct 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you need to type annotate all the parameters for mypy to check this function

It's probably worth making all the parameters positional only using a closure so you can use *args: P.args and **kwargs: P.kwargs:

def _main_wrap(self, call_result: Future[R], source_thread: threading.Thread, exc_info: ExcInfo, context: Optional[Context]) -> Callable[P, Coroutine[Any, Any, None]:
    async def wrap(*args: P.args, **kwargs: P.kwargs) -> None:
        ...

    return wrap

you'd then need to call it like this

            awaitable = self._main_wrap(
                call_result, source_thread, sys.exc_info(), context
            )(*args, **kwargs)

@shughes-uk
Copy link
Contributor Author

Made a little more progress.

  • Switching to using the main mypy branch for now (to be resolved before merging).
  • Mypy seems to not like combining paramspec with generics
  • Future is only generic in 3.9+ and will throw exceptions in previous versions, removed it for now

@shughes-uk
Copy link
Contributor Author

Not entirely sure whats going on but the checks appears to have been running for many hours 😱

@graingert
Copy link
Contributor

Future is only generic in 3.9+ and will throw exceptions in previous versions, removed it for now

You'll need to use a forward reference with "Future[R]"

asgiref/sync.py Outdated
@@ -319,6 +318,10 @@ async def main_wrap(
_restore_context(context[0])

current_task = SyncToAsync.get_current_task()
if not current_task:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably best to use an assertion instead eg:

assert current_task is not None

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way it's not considered an un covered branch, and is idiomatic mypy type assertion

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah perfect thank you

@shughes-uk
Copy link
Contributor Author

shughes-uk commented Oct 19, 2021

3.9 Tests are passing and these are the mypy issues left. Most of which seem to be caused by mypy not recognizing Generic's can accept ParamSpecs.

asgiref/sync.py:126: error: Free type variable expected in Generic[...]
asgiref/sync.py:159: error: "Callable[..., Awaitable[R]]" has no attribute "__self__"
asgiref/sync.py:183: error: Name "P.args" is not defined
asgiref/sync.py:183: error: Name "P.kwargs" is not defined
asgiref/sync.py:310: error: Name "P.args" is not defined
asgiref/sync.py:311: error: Name "P.kwargs" is not defined
asgiref/sync.py:348: error: Free type variable expected in Generic[...]
asgiref/sync.py:432: error: Name "P.args" is not defined
asgiref/sync.py:432: error: Name "P.kwargs" is not defined
asgiref/sync.py:441: error: Argument 1 to "get" of "ContextVar" has incompatible type "None"; expected "Union[ContextVar[str], str]"
asgiref/sync.py:498: error: Returning Any from function declared to return "R"
asgiref/sync.py:506: error: Function is missing a return type annotation
asgiref/sync.py:512: error: Name "P.args" is not defined
asgiref/sync.py:513: error: Name "P.kwargs" is not defined
asgiref/sync.py:569: error: "SyncToAsync" expects 1 type argument, but 2 given
asgiref/sync.py:569: error: Invalid location for ParamSpec "P"
asgiref/sync.py:569: note: You can use ParamSpec as the first argument to Callable, e.g., 'Callable[P, int]'
asgiref/sync.py:578: error: "SyncToAsync" expects 1 type argument, but 2 given
asgiref/sync.py:578: error: Invalid location for ParamSpec "P"
asgiref/sync.py:578: note: You can use ParamSpec as the first argument to Callable, e.g., 'Callable[P, int]'

@graingert
Copy link
Contributor

3.9 Tests are passing and these are the mypy issues left. Most of which seem to be caused by mypy not recognizing Generic's can accept ParamSpecs.

asgiref/sync.py:126: error: Free type variable expected in Generic[...]
asgiref/sync.py:159: error: "Callable[..., Awaitable[R]]" has no attribute "__self__"
asgiref/sync.py:183: error: Name "P.args" is not defined
asgiref/sync.py:183: error: Name "P.kwargs" is not defined
asgiref/sync.py:310: error: Name "P.args" is not defined
asgiref/sync.py:311: error: Name "P.kwargs" is not defined
asgiref/sync.py:348: error: Free type variable expected in Generic[...]
asgiref/sync.py:432: error: Name "P.args" is not defined
asgiref/sync.py:432: error: Name "P.kwargs" is not defined
asgiref/sync.py:441: error: Argument 1 to "get" of "ContextVar" has incompatible type "None"; expected "Union[ContextVar[str], str]"
asgiref/sync.py:498: error: Returning Any from function declared to return "R"
asgiref/sync.py:506: error: Function is missing a return type annotation
asgiref/sync.py:512: error: Name "P.args" is not defined
asgiref/sync.py:513: error: Name "P.kwargs" is not defined
asgiref/sync.py:569: error: "SyncToAsync" expects 1 type argument, but 2 given
asgiref/sync.py:569: error: Invalid location for ParamSpec "P"
asgiref/sync.py:569: note: You can use ParamSpec as the first argument to Callable, e.g., 'Callable[P, int]'
asgiref/sync.py:578: error: "SyncToAsync" expects 1 type argument, but 2 given
asgiref/sync.py:578: error: Invalid location for ParamSpec "P"
asgiref/sync.py:578: note: You can use ParamSpec as the first argument to Callable, e.g., 'Callable[P, int]'

Is there a ticket for this in mypy? I think it's supposed to be ignored

@shughes-uk
Copy link
Contributor Author

shughes-uk commented Oct 19, 2021

Is there a ticket for this in mypy? I think it's supposed to be ignored

python/mypy#11362 created one with minimal reproduction code

@shughes-uk
Copy link
Contributor Author

3.6 tests are failing with a circular import caused by the context shenanigans on Local. Looks kinda tricky to solve

@shughes-uk shughes-uk force-pushed the resolve-270 branch 5 times, most recently from f729c0b to e555890 Compare November 25, 2021 22:42
@shughes-uk
Copy link
Contributor Author

shughes-uk commented Nov 25, 2021

@andrewgodwin @graingert I've updated this with a workaround based on the current (unreleased) mypy version. Temporarily installing mypy from the git version. Once mypy cuts a release this should be good to go. No generics required, but maybe a little hacky.

Other option is to wait for generic support in mypy and a release. Which who knows maybe that'll happen before the next release rendering the workaround redundant.

@andrewgodwin
Copy link
Member

Will current versions of mypy-in-the-wild choke on the new stuff, btw, or will they ignore it and not typecheck it? Considering the type annotations here are mostly for the benefit of users rather than asgiref itself, I want to ensure it's not going to torpedo current users.

@graingert
Copy link
Contributor

Sadly mypy 0.910 will choke on this. I think the work-around is to provide these aliases in a new module eg asgiref.sync_paramspec

@shughes-uk
Copy link
Contributor Author

Yes this will blow up on wild mypy. Once they roll out a new release this could be merged without too much worry unless you intend for backwards compatibility with older mypy versions.

@andrewgodwin
Copy link
Member

Ugh, yeah, this is tricky because end-users mypy installations is the thing we care about at the end of the day. No way to somehow erase ParamSpec and fall back to Any or something on those older versions, I'm guessing?

setup.cfg Outdated Show resolved Hide resolved
Co-authored-by: Thomas Grainger <[email protected]>
@graingert
Copy link
Contributor

graingert commented Dec 22, 2021

mypy 0.930 is out with experimental support for ParamSpec! https://mypy-lang.blogspot.com/2021/12/mypy-0930-released.html?m=1

This is particularly good as it has support for Generic[P, R]

@lexicalunit
Copy link

Given that Python 3.6 is no longer supported as of asgiref version 3.5.0, is now a good time to revisit this discussion? Sorry, I've just been following along here and am really excited to see this feature land at some point 😄

@andrewgodwin
Copy link
Member

Maybe? I don't keep tight track of typing stuff right now so I'm not quite sure where things sit in terms of this working with 3.7.

@graingert
Copy link
Contributor

All versions of python support ParamSpec, even Python 2.7 all you need to do is import it in an "if TYPE_CHECKING:" block, or use the typing_extensions backport

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants