https://prefect.io logo
Powered by
n

Nash Taylor

05/22/2022, 10:26 PM
I definitely sound like a broken record at this point, but I'm still stuck on trying to understand the reasoning behind [these overloads](https://github.com/PrefectHQ/prefect/blob/orion/src/prefect/tasks.py#L231-L255) on the base Task class. Namely, the use of NoReturn in the first overload. Here's a minimal example where I use NoReturn in a Task skeleton, and the resulting 
reveal_type
of a task run according to mypy: (threaded to avoid an obnoxiously long message)
✅ 1
Copy code
class Task(Generic[P, R]):
    def __init__(self, fn: Callable[P, R]) -> None:
        self.fn = fn

    @overload
    def __call__(self: "Task[P, NoReturn]", *args: P.args, **kwargs: P.kwargs) -> None:
        ...

    @overload
    def __call__(self: "Task[P, T]", *args: P.args, **kwargs: P.kwargs) -> T:
        ...

    def __call__(self, *args, **kwargs):
        return self.fn(*args, **kwargs)

@Task
def my_task(a: str, b: int) -> float:
    return len(a) + b + 0.5


reveal_type(my_task(a="a", b=1))  # None
This as far as I can tell roughly reproduces what you see in 2.0, where instead of 
None
it's 
PrefectFuture[None, Literal[False]]
Now if I replace 
NoReturn
with None, and swap the order of the overloads...
Copy code
class Task(Generic[P, R]):
    def __init__(self, fn: Callable[P, R]) -> None:
        self.fn = fn

    @overload
    def __call__(self: "Task[P, T]", *args: P.args, **kwargs: P.kwargs) -> T:
        ...

    @overload
    def __call__(self: "Task[P, None]", *args: P.args, **kwargs: P.kwargs) -> None:
        ...

    def __call__(self, *args, **kwargs):
        return self.fn(*args, **kwargs)


@Task
def my_task(a: str, b: int) -> float:
    return len(a) + b + 0.5


reveal_type(my_task(a="a", b=1))  # builtins.float
I get the correct return type
k

Kevin Kho

05/22/2022, 10:31 PM
This is beyond me. I’ll have to ping Michael during the workday tom
n

Nash Taylor

05/22/2022, 10:31 PM
If I do the same, but decorating a function whose return type is 
None
, then I get 
None
. I can't actually tell if that's matching the first or second overload... but in either case, it works
(by the way, I've seen a few discussions where it's mentioned that Mypy doesn't support PEP612 and therefore doesn't work for type checking prefect code... but that very recently seems to no longer be the case: https://github.com/python/mypy/issues/8645)
For the record, I have tried to use pyright because of its support for 612, but then I ran into too many other problems between pyright and pydantic. Namely, the fact that pyright incorrectly handles `alias`es on Pydantic models, and when it was brought to the attention of Samuel Colvin (the maintainer of Pydantic), the response was basically one of "not my problem, I don't use pyright": https://github.com/samuelcolvin/pydantic/issues/3617
(not to bash Samuel, Pydantic is genuinely one of my favourite contributions to the python open source community ever)
Basically I'm trying to resolve a conflict in the social network of open source software. Prefect likes Pyright, Prefect does not like Mypy; Pydantic does not like Pyright, Pydantic likes Mypy; Prefect likes Pydantic; I like Pydantic, and Prefect
k

Kevin Kho

05/22/2022, 10:41 PM
Damn what I deep dive 😅. Will make sure Michael sees
n

Nash Taylor

05/22/2022, 10:44 PM
Anyway, getting a little off topic for the thread... NoReturn is meant to annotate functions that never return any value at all. I.e., functions that are sure to raise an exception, or... well that's the only thing I can really think of. Is that really the kind of task we're trying to encapsulate with this overload? I'm not so sure. I think maybe 
None
is, but I also think that doesn't need to be its own overload, since 
T
has no bindings and could take on 
NoneType
. So... yeah, I'm probably just not understanding the purpose of this overload. I'll wait to see what Michael says haha
As always thanks for the quick response. Do you guys ever take time off?
k

Kevin Kho

05/22/2022, 10:46 PM
Lol. Weekend is optional but I still have COVID so I cant go out anyway.
a

Anna Geller

05/22/2022, 10:52 PM
Do you guys ever take time off?
we do! 😄 coming week I'll be actually OOO Thu-Sun entirely plus we are experimenting with Slack Office Hours to make it clearer when we are available - see my Slack status
but Kevin is a Community Hero anyway, he can answer your questions faster than I can read them
z

Zanie

05/23/2022, 4:22 PM
The use of 
NoReturn
was necessary to support functions that return 
Any
or were unannotated in Pyright. See https://github.com/microsoft/pyright/issues/2142#issuecomment-913249595
These overloads took me forever to get working
I’m very hesitant to spend any time investigating support for mypy at this moment. I suspect it will be hard for us to support both mypy and pyright without making changes upstream.
If you open a pull request that addresses all the use-cases I’d certainly be willing to review it though
n

Nash Taylor

05/23/2022, 4:28 PM
What do you make of this conflict between Pyright and Pydantic? I see you guys use Pydantic quite heavily (
State
being a 
BaseModel
, for example). I'll work up a minimal example to show you the issue I'm having
Okay, so in one of my tasks I want to make use of an OpenAPISchema pydantic model I created. It's pretty self-explanatory from the name; it defines a OpenAPI document that I can use to render an 
openapi.yaml
for a Cloud Endpoints service I'm using. OpenAPI docs have a lot of keys that use hyphens. The only way I can see to create a pydantic model which can have properties that come out in the 
.dict()
representation with hyphens is via aliases. For example:
Copy code
class OneOpenAPIField(BaseModel):
    x_google_endpoints: list[str] = Field(..., alias="x-google-endpoints")
So, I'll make a little 
test.py
and run it against mypy
Copy code
from pydantic import BaseModel, Field


class MyModel(BaseModel):
    x_google_endpoints: list[str] = Field(..., alias="x-google-endpoints")

    class Config:
        allow_population_by_field_name = True


MyModel(x_google_endpoints=["my_endpoint"])
Copy code
~> mypy test.py
Success: no issues found in 1 source file
Copy code
~> pyright test.py
...
  test.py:11:9 - error: No parameter named "x_google_endpoints" (reportGeneralTypeIssues)
  test.py:11:1 - error: Argument missing for parameter "x-google-endpoints" (reportGeneralTypeIssues)
So you can see that pyright has this inexplicably backwards. It's expecting me to use as arguments in my model constructor the 
alias
of every field, if it exists. Aliases are almost always used for keys that can't be represented as python argument names, like here with the dashes. So... I basically can't build this model with pyright
I realize this isn't a Prefect issue at all, it just feels like a disconnect, because Pydantic has a mypy plugin and fully supports it, and seems to ignore pyright, while Prefect leans on Pydantic and only supports Pyright
And ultimately I don't think I can give up Pydantic (or static analysis) for Prefect
If there's a hack for this particular issue I'd take it, although I'd be concerned that I'm just kicking the can down the road waiting for the next time they disagree
https://github.com/samuelcolvin/pydantic/discussions/3986 The discussion on this point in the pydantic repo was rather disappointing, as it ends with Samuel Colvin saying that it seems like a fundamental limitation of PEP 681, so I guess no one there will be putting time into making this work
https://github.com/microsoft/pyright/discussions/1782#discussioncomment-1920822 Meanwhile on the pyright discussion board: "this feature is very specific to pydantic" so they won't be implementing it either. Software!
Also there's still plenty of room here for the possibility that I'm just being an idiot and somewhere/somehow getting field names and aliases confused or something. Checking for that possibility (and other workarounds) now.
😂 1
eyyyyyyyy
Okay I wasn't being dumb (I think), but I did find a potential solution to this particular problem
Copy code
from pydantic import BaseModel


def alias_generator(name: str) -> str:
    if name == "x_google_endpoints":
        return "x-google-endpoints"
    else:
        return ""


class MyModel(BaseModel):
    x_google_endpoints: list[str]

    class Config:
        allow_population_by_field_name = True
        alias_generator = alias_generator


model = MyModel(x_google_endpoints=["my_endpoint"])

assert model.dict(by_alias=True)["x-google-endpoints"] == ["my_endpoint"]
assert MyModel.parse_obj({"x-google-endpoints": ["my_endpoint"]}).x_google_endpoints == [
    "my_endpoint"
]
Obviously I can make that alias generator a lot less verbose for the case of many more aliases
But yeah. Look at that. No pyright errors (because it doesn't actually know the alias), and no KeyErrors, because pydantic doesn't type the result of 
.dict()
with an actual 
TypedDict
k

Kevin Kho

05/23/2022, 5:11 PM
Nice work! What a journey
n

Nash Taylor

05/23/2022, 5:12 PM
When I ran pyright against my whole (mypy-passing) codebase, I got a list of errors that was at least 100 long. This was the first one. It took me like 2 days to figure out. The journey has just begun
😅 1
Hey while we're here, did anyone get around to adding 
py.typed
to the manifest, so that it shows up in the pip install of 
prefect>=2.0b
? I'm just adding it myself for now but it would be nice to, y'know, not do that
k

Kevin Kho

05/23/2022, 5:18 PM
Not yet, but that will happen. Don’t know if by next release (there might be one for windows support soon)
👍 1
z

Zanie

05/23/2022, 5:45 PM
You can also write a mypy plugin for Prefect if that makes things easier
🙏 1
Pyright doesn’t support plugins so we need to have support for it built-in if we want a good IDE experience with Prefect, but mypy will let you do all sorts of things
n

Nash Taylor

05/23/2022, 5:47 PM
Oooh that is an interesting option
First: yes, I basically have been doing this all day. One day I will find a life. Second, Python why. • I decided that because all of my functions are typed with no exceptions, I would fork and just remove the 
NoReturn
overload, leaving me with one overload for async and one for sync. It almost worked. But not really. • It didn't work at all just by removing the first overload; mypy started telling me that it "needs a type annotation", because 
@task
was just giving me 
Any
. I swapped the order of the coroutine overload and the regular overload, and it started giving me the correct type annotation for my synchronous function! • It gave me 
Any
for my asynchronous function. Gah. • 5-or-so hours later, after trying a bunch of things I won't even bother mentioning, I discovered just about the stupidest solution possible: I renamed 
___call___
to a regular function name, 
do
.
Copy code
from prefect import flow
from prefect.tasks import Task


@Task
def task1(a: str, b: int) -> float:
    return len(a) + b + 0.5


@Task
async def task2(f: float) -> str:
    return str(f)


@flow
async def my_flow(a: str, b: int) -> str:
    task1_out = <http://task1.do|task1.do>(a=a, b=b)
    reveal_type(task1_out) # Revealed type is "prefect.futures.PrefectFuture[builtins.float, Literal[False]]"
    task2_out = await <http://task2.do|task2.do>(task1_out.result())
    reveal_type(task2_out) # Revealed type is "prefect.futures.PrefectFuture[builtins.str, Literal[True]]"
    return await task2_out.result()
So..... uh. I fixed it? Task failed successfully, I think.
Because A) I had to fork the repo, and I'm not even sure how this would go if I tried to deploy this on Prefect Cloud (I'm guessing poorly). And B) I have to use 
.do()
instead of 
___call___
. Honestly, I would make that sacrifice for the sake of typing, but I'm pretty sure I'm alone in that
And, on number B... why in the world does that fix it? Somebody get me Guido's phone number, I have questions.
😂 1
Assuming I do have the ability to deploy a fork of Prefect, I'm not too upset about A. B is technically not that big of a deal, but I hate it. So much.
I am currently losing my mind changing the name of the method from 
do
(success) to 
dont
(success) to 
___somethingdunder___
(success) to 
___call___
(failure)
k

Kevin Kho

05/24/2022, 1:35 AM
Michael may correct me but I think it might work if you have your own fork because all the compute happens on your own infrastructure. We have people who forked 1.0 and made their own agent and run config for example. As long as the Deployment part works, it might work
n

Nash Taylor

05/24/2022, 1:35 AM
That does help a lot I think
Here's another fun question. If I did indeed have to change the interface of 
Task
so that calling it was done with 
.do()
instead of 
()
(
___call___
), would that change have to be made in a bunch of other places?
k

Kevin Kho

05/24/2022, 1:40 AM
I wouldn’t know the answer, we’d need Michael but honestly this is in the realm of unsupported already 😅
n

Nash Taylor

05/24/2022, 1:40 AM
Understandable
I'm on the python/typing gitter now seeing if anyone knows anything about this call nonsense. They seem much less responsive than here, unfortunately
Oh by the way, I had to make one more change, which might cause me some problems if I try to use it. I had to remove the 
wait_for
parameter from the interface of 
Task
, because it's not actually PEP 612 compliant. Check out the rejected case there; wait_for is 
s
. https://peps.python.org/pep-0612/
In attempting to reproduce the error, every time I added a 
wait_for
kwarg, I started getting 
P.args is not defined
and 
P.kwargs is not defined
.
fwiw, I get this in pyright as well. Different messaging, but same error on the same line
z

Zanie

05/24/2022, 2:27 AM
I spent a couple weekends on this too, it’s insane.
I can’t believe changing the name of the method changes something? What? Have you considered opening an issue? Pyright is very responsive on GitHub.
We may be able to entertain a 
submit
function on tasks as an alias of 
__call__
n

Nash Taylor

05/24/2022, 2:28 AM
This call/do thing has pushed me over the edge. I no longer care about the pydantic-mypy plugin. I feel like I'm just going to bite the bullet of converting our codebase to use Pyright so that we can use a pydantic-pyright-prefect combo. 3 P's is more symmetric anyway
z

Zanie

05/24/2022, 2:28 AM
We can’t support 
wait_for
and user function types because PEP612 explicit declares appending keyword arguments out of scope
I wish we could, but it’ll probably be in a later version of Python.
n

Nash Taylor

05/24/2022, 2:29 AM
Just to clarify, what do you mean by "user function types"?
z

Zanie

05/24/2022, 2:29 AM
The “ParamSpec” types
They allow you to concat positional parameters to a param spec but not extend it with keyword parameters.
n

Nash Taylor

05/24/2022, 2:31 AM
At the moment though, you have both don't you? 
Task._call_
(goddammit Slack) takes a 
P.args
and 
P.kwargs
, and in pyright it deals with those correctly, but also a 
wait_for
z

Zanie

05/24/2022, 2:31 AM
A fork (or even just subclass of the 
Task
class) would work fine via Prefect Cloud as long as you roll your own Docker images
Yeah but you can extend P.args with more arg types but you can’t extend P.kwargs
n

Nash Taylor

05/24/2022, 2:32 AM
yeah, I tried subclassing Task at first but mypy started complaining about 
wait_for
and 
P.args
. When I removed 
wait_for
, it told me I was violating the contract of the supertype.
Oh I didn't realize that
z

Zanie

05/24/2022, 2:33 AM
What if you write a subclass of task with a 
do
method and type-ignore your call to 
__call__
?
I’m also open to exploring a 
wait_for
interface that doesn’t require a keyword argument, but there’s nothing quite as simple for users.
n

Nash Taylor

05/24/2022, 2:36 AM
Why is 
wait_for
part of the 
__call__
signature and not part of 
with_options
?
(If the answer is too complicated feel free to just say that)
Subclassing is going poorly. I'm getting 
ParamSpec "P" is unbound
even though the init takes a 
Callable[P, R]
as a parameter, so I don't know what that's about
Along with some very unhelpful return types when calling the task
Ultimately I think trying to use mypy alongside prefect is going to be an uphill battle. I'm gonna switch gears and go back to dealing with a migration to pyright, and trust that the type safety of Prefect within that environment will only improve
And I'll just have to say goodbye to the pydantic-mypy plugin. Honestly though, pyright has a lot of nice features anyway. And it runs faster from what I can tell
z

Zanie

05/24/2022, 3:52 PM
😄 I still think a prefect-mypy plugin would be reasonable
But yeah type-checking is most definitely immature, especially in this use-case
n

Nash Taylor

05/24/2022, 3:56 PM
So I'm not too familiar with how mypy plugins work. Would it even be possible for a plugin to circumvent an issue as fundamental as this "you can't use overloads on dunder_call methods" madness?
z

Zanie

05/24/2022, 3:58 PM
Is that the specific issue? If you literally can’t use overloads on a dunder I’d open an issue because that seems ridiculous.
Aside from that, I think you can basically do anything in a plugin but I’m also not sure 🙂
n

Nash Taylor

05/24/2022, 3:58 PM
Also, have you seen this issue? https://github.com/samuelcolvin/pydantic/issues/3617 The lead maintainer of Pydantic was asked to resolve an issue related to usage with Pyright, and if you scroll to the bottom you'll find him saying (paraphrased) "I'm trying to help you and you're just making me not want to work on this project at all", and then he closed and locked the issue to maintainers
Also he opened a PR where he "added support for Pyright", but it only has thumbs down reactions and everyone is saying it's just a hack. To which he's replied, "well yeah, but it's the best we can do"
k

Kevin Kho

05/24/2022, 4:00 PM
I was in Samuel Covin’s Pydantic office hour and he said stuff like “if you need feature X, you’d need to pay me to work on that”. I think it’s really a volunteer thing for him
n

Nash Taylor

05/24/2022, 4:00 PM
Honestly I have no idea what the root cause of that call issue is. I threw it in the python/typing gitter to see if anyone has any ideas
z

Zanie

05/24/2022, 4:00 PM
Yes I did see that. It’s not really their fault either, Python typing just doesn’t have the power necessary to do these things.
n

Nash Taylor

05/24/2022, 4:01 PM
Maybe pyright will start supporting plugins and someone can contribute a pydantic plugin
It's really a symphony of conflicts though. Pyright supports PEP 612, which Prefect needs; Pydantic < 1.9 works with Pyright, but not with Python 3.10; >= 1.9 works with 3.10, but not Pyright; Pydantic works with Mypy, but Mypy doesn't support PEP 612
I really believe static typing catapulted my ability as a software engineer, which is why I'm so adamant about using it. Sometimes it feels like I'm in the wrong language. But then, I came here because I wanted to do (and am doing) ML, and at this point Python is the way to do that
😂 1
z

Zanie

05/24/2022, 4:13 PM
Pyright noted that they will not support plugins like mypy does as they’d rather encourage people to write code that follows supported Python type patterns.
4 Views
Powered by