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

Use ParamSpec to preserve function signatures of tasks #75

Merged
merged 2 commits into from May 29, 2022

Conversation

henribru
Copy link
Contributor

@henribru henribru commented May 8, 2022

Currently when you decorate a function as a task its signature is completely lost to typecheckers. This PR tries to improve on that with the help of ParamSpec (https://docs.python.org/3/library/typing.html#typing.ParamSpec, https://peps.python.org/pep-0612/). There are some caveats to this approach though, so if you decide that the downsides are bigger than the upsides and decline to merge this for that reason, that's okay. Let me cover the positives first and then the negatives.

Say you have the following tasks:

@app.task
def add(x: int, y: int) -> int:
    return x + y

With this PR, a typechecker can verify that the argument types are correct in these cases:

add(1, 2)
add.delay(1, 2)

And wrong in these cases:

add(1, "2")
add.delay(1, "2")

They can verify that the return types allow doing these operations:

add(1, 2) + 3
add.delay(1, 2).get() + 3
add.apply((1, 2)).get() + 3
add.apply_async((1, 2)).get() + 3
add.s((1, 2)).apply_async().get() + 3
add.s((1, 2)).apply().get() + 3

And that they don't allow these:

add(1, 2) + "3"
add.delay(1, 2).get() + "3"
add.apply((1, 2)).get() + "3"
add.apply_async((1, 2)).get() + "3"
add.s(1, 2).apply_async().get() + "3"
add.s(1, 2).apply().get() + "3"

They can verify that this is correct usage with bind:

@app.task(bind=True)
def add(task: Task[Any, Any], x: int, y: int) -> int:
    return x + y

add.delay(1, 2)

And that this is an incorrect call:

add.delay(some_task, 1, 2)

And that this is an incorrect definition:

@app.task(bind=True)
def add(x: int, y: int) -> int:
    return x + y

Now for the caveats:

Conspicuously absent from my very first example were these:

add.apply((1, 2))
add.apply_async((1, 2))
add.s(1, 2).apply()
add.s(1, 2).apply_async()

Unfortunately we can't verify the argument types in these cases. The two first ones don't work because they don't take their arguments as *args, **kwargs, which is required for ParamSpec. The two latter don't work because you can pass partial arguments to s and there's no way to represent a partial ParamSpec. In other words only direct calls and delay actually give you type-checked arguments.

To pass around and store the type variables representing the signature I've had to make Task and Signature generic. Since they're not generic at runtime, this is a bit misleading. Anyone who wants to annotate these with generics in their code will have to either use from __future__ import annotations or wrap the type in quotes. Anyone who uses Mypy with disallow_any_generics and has previously annotated these without generics will be forced to specify the generics now, which I guess technically makes this a breaking change.

There's also no attempt to support chains, groups, chords etc. I suspect it's not really possible with the current capabilities of the Python type system, but I haven't looked into it.

@John98Zakaria
Copy link

As much as I love typing. Wouldn't that limit the package to python 3.10 and above?
Is it possible to use modern stubs in older versions of python?

@henribru
Copy link
Contributor Author

henribru commented May 29, 2022

As much as I love typing. Wouldn't that limit the package to python 3.10 and above?
Is it possible to use modern stubs in older versions of python?

That's the reason I've imported from typing_extensions instead of typing. It contains backports of features from typing. This should work fine in Python 3.7. You just need an up-to-date type checker to take advantage of it.

@sbdchd
Copy link
Owner

sbdchd commented May 29, 2022

Somehow missed the notification for this PR, it looks really useful!

With the cases you noted as not working:

add.apply((1, 2))
add.apply_async((1, 2))
add.s(1, 2).apply()
add.s(1, 2).apply_async()

What does the type checker say? does it allow it through?

one concern I have is that adding generics to existing types will break people's code, but I think that would be okay since it's pretty beneficial

Also I think it's okay that the types aren't generic at runtime, we can add some notes about using annotations or monkey patching the types to allow passing generic args

@henribru
Copy link
Contributor Author

henribru commented May 29, 2022

With the cases you noted as not working:
[...]
What does the type checker say? does it allow it through?

They'll work exactly as before, i.e. accept any tuple for args and any dict for kwargs. I haven't made any changes to those arguments.

one concern I have is that adding generics to existing types will break people's code, [...]

Yep, if they've enabled disallow_any_generics in Mypy (which is included in strict mode) they will absolutely get Mypy errors any place they've explicitly annotated Task, Signature, AsyncResult and EagerResult, until they've added the appropriate generics. Not sure if this is common. Also not sure if other typecheckers behave similarly. It's definitely a pretty unfortunate downside of this change.

Also I think it's okay that the types aren't generic at runtime, we can add some notes about using annotations or monkey patching the types to allow passing generic args

Yeah, personally for me I use from __future__ import annotations pretty much everywhere anyway so it doesn't feel like a huge problem. No clue how common that is, though. But there is some precedence for type stubs shipping a runtime component to monkeypatch generics, django-stubs does that: https://github.com/typeddjango/django-stubs/tree/master/django_stubs_ext

@sbdchd sbdchd added the automerge auto merge PRs using kodiak label May 29, 2022
@kodiakhq kodiakhq bot merged commit 2de4a0a into sbdchd:main May 29, 2022
@henribru henribru deleted the paramspec branch October 21, 2022 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge auto merge PRs using kodiak
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants