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

A few mypy fixups #3346

Conversation

jcrist
Copy link

@jcrist jcrist commented Sep 18, 2020

  • Fix __signature__ of Task types to work on both the class and on
    instances.
  • Fix task decorator to support type checking. mypy doesn't like
    decorators that accept optional arguments, to handle this we have to
    add additional annotations.
  • Almost fix resource_manager decorator to support type checking.
    This is annoying, mypy assumes (incorrectly) that the output of a
    class decorator is always an instance of the class, even if the
    decorator type signature says otherwise.

Fixes #3056.
Related to, but doesn't fix #3191.

  • adds new tests (if appropriate)
  • adds a change file in the changes/ directory (if appropriate)
  • updates docstrings for any new functions or function arguments, including docs/outline.toml for API reference docs (if appropriate)

- Fix `__signature__` of `Task` types to work on both the class and on
  instances.
- Fix `task` decorator to support type checking. mypy doesn't like
  decorators that accept optional arguments, to handle this we have to
  add additional annotations.
- *Almost* fix `resource_manager` decorator to support type checking.
  This is annoying, mypy assumes (incorrectly) that the output of a
  class decorator is always an instance of the class, even if the
  decorator type signature says otherwise.
@jcrist jcrist requested a review from joshmeek as a code owner September 18, 2020 18:09
@jcrist
Copy link
Author

jcrist commented Sep 18, 2020

The type checking this supports only checks that the output of a task decorated function is a Task (before this would be Any). This should also work for resource_manager decorated classes, but doesn't due to a bug in mypy. I'd be fine with deleting the changes to the resource_manager file, as they don't really help anything, but they should and it took a bit of experimenting to get even this far.

Demo:

from prefect import task, Flow


@task
def hello(name: str) -> str:
    print("Hello %s" % name)
    return name


@task(name="hello-2")
def hello2(name: str) -> None:
    print("Hello %s" % name)


reveal_type(hello)
reveal_type(hello2)


with Flow("greeter") as flow:
    res = hello("world")
    reveal_type(res)
    res2 = hello2(res)
    reveal_type(res2)

flow.run()
$ mypy test.py
test2.py:15: note: Revealed type is 'prefect.tasks.core.function.FunctionTask'
test2.py:16: note: Revealed type is 'prefect.tasks.core.function.FunctionTask'
test2.py:21: note: Revealed type is 'prefect.core.task.Task'
test2.py:23: note: Revealed type is 'prefect.core.task.Task'

@codecov
Copy link

codecov bot commented Sep 18, 2020

Codecov Report

Merging #3346 into master will decrease coverage by 0.03%.
The diff coverage is 84.21%.

cicdw
cicdw previously approved these changes Sep 18, 2020
Copy link
Member

@cicdw cicdw left a comment

Choose a reason for hiding this comment

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

I'll admit I'm not 100% in the loop on overload here, but the changes seem safe and valuable, so LGTM!

@jcrist
Copy link
Author

jcrist commented Sep 18, 2020

I'll admit I'm not 100% in the loop on overload here, but the changes seem safe and valuable, so LGTM!

I am also not a mypy user, but per googling on how to do this, this appears to be the recommended approach.

@jcrist jcrist merged commit c098ca3 into PrefectHQ:master Sep 18, 2020
@jcrist jcrist deleted the esoteric-mypy-incantations-to-make-decorators-work branch September 18, 2020 19:42
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.

Using static type checking as qa on a Flow library Use of @curry with task decorator breaks type checking
2 participants