Replies: 2 comments 5 replies
-
|
Hey @mjpieters! Working towards type completeness is very important to Prefect, so we're thrilled you opened this discussion! As you noted, we run For concrete next steps, we can add a step to our CI pipeline to run and publish a type completeness report on How does that sound to you? Is there anything else you would recommend? |
Beta Was this translation helpful? Give feedback.
-
|
I'm so glad to hear this! Thanks for being so fantastically proactive on this! Yes, a CI pipeline job that highlights the current state of type completeness and guards against regressions would be a great first step. I'd actually monitor all of A pinned issue to track progress is also a great way to help coordinate efforts, and to help communicate the importance of this task. It can be used to break down what can seem to be too large a job into manageable goals where the community can jump in and help. I'm sure this is not something Prefect engineers need to tackle alone. I think such a pinned issue is also more visible than this discussion area, and so make it more likely the community can jump in on this. As for monitoring type completeness in the pipeline: I'm not aware of any existing re-usable Github Action components that can do this, but luckily pyright can output the report in json format: I've directed this to a file for more leisurely analysis. The JSON this produces is an object with a {
"version": "1.1.389",
"time": "1733223282038",
"generalDiagnostics": [],
"summary": {
"filesAnalyzed": 570,
"errorCount": 0,
"warningCount": 0,
"informationCount": 0,
"timeInSec": 3.846
},
"typeCompleteness": {
// ...
}
}To me, the most immediately interesting
"completenessScore": 0.6773732510907177,so the project, overall, is 67.7% type complete. This value is immediately useful in reporting pipelines, providing there is a good way of storing that number somewhere so PRs can be compared against it.
Pyright treats modules that start with an underscore as private and won't include symbols in such modules in the report unless referenced by exported symbols. I am reasonably sure that the migrations module is only of use to the database, not to any other parts of the codebase nor to code importing from prefect. There are 570 unique module names in the prefect namespace, so being able to reduce this by 196 is a big win!
// symbol:
{
"category": "<string>", // enum: class|constant|function|method|module|symbol|type alias|type variable|variable
"name": "<string>", // fully qualified import name
"referenceCount": <int>, // how many places reference this symbol
"isExported": <bool>, // is this symbol part of the public API or merely a dependency?
"isTypeKnown": <bool>, // does the symbol have a type annotation or can it only be inferred?
"isTypeAmbiguous": <bool>, // if the type is unknown, would inference be ambiguous?
"diagnostics": [<issue>] // list of diagnostic issues found for this symbol, can be empty, see below
}
// issue:
{
"severity": <string>, // enum: warning|error
"message": <string>, // error details, with newlines and all but the first line indented with two no-break spaces.
"file": <string>, // Full path to the file containing this symbol
"range": <null> or <object> // Exact range within the file where this issue was found, see below
}
// range:
{ // Line and character counts are zero-based (!)
"start": {"line": <int>, "character": <int>},
"end": {"line": <int>, "character": <int>}
}There are 4315 symbols with an unknown type in this report, but 400 are related to the above mentioned migrations: I'm not yet that familiar with the prefect namespace, so this is one for the dev team to answer: How many more (exported) symbols are there in this report that should not be there at all? I'd start with seeing if these could be easily hidden, e.g. by renaming the module or a parent package to start with an underscore. From there on I'd look at the symbols that have an unknown type ( produces [
{
"category": "class",
"name": "prefect.server.database.interface.PrefectDBInterface",
"referenceCount": 179,
"isExported": true,
"isTypeKnown": false,
"isTypeAmbiguous": false,
"diagnostics": [
{
"file": "",
"severity": "error",
"message": "Type of metaclass \"[object Object]\" is partially unknown"
}
]
},
{
"category": "class",
"name": "prefect._internal.schemas.bases.PrefectBaseModel",
"referenceCount": 129,
"isExported": false,
"isTypeKnown": false,
"isTypeAmbiguous": false,
"diagnostics": []
},
{
"category": "class",
"name": "prefect.server.schemas.states.State",
"referenceCount": 104,
"isExported": true,
"isTypeKnown": false,
"isTypeAmbiguous": false,
"diagnostics": [
{
"file": "",
"severity": "error",
"message": "Type of base class \"prefect.server.schemas.states.StateBaseModel\" is partially unknown"
}
]
}
]Note that message of the first entry, The reference count is just an indication, but it should be indicative of the impact fixing that symbol would have. It's easiest to see this when the symbol name is referenced in the diagnostic issue message for other issues, indicating that their problem might be entirely due to the issues with the referenced symbol. E.g. and when I fix the issues with just |
Beta Was this translation helpful? Give feedback.
-
|
A quick troll through the Github Actions documentation shows there is a way to share pyright reports across workflow runs:
This would download the 'nearest' report artifact for the target branch, for the most recent completed workflow that was triggered by a push to that branch that has that artifact available. The |
Beta Was this translation helpful? Give feedback.
-
|
Thanks for that info @mjpieters! I've opened #16194 to add a type completeness check to our CI. In the next day or two, I'll open a tracking issue for addressing typing errors raised by I like your idea of making more of our modules private by prefixing them with an underscore. Since we don't use that pattern much right now (except in our |
Beta Was this translation helpful? Give feedback.
-
|
@desertaxle Fantastic! I found out you had added this to the pipeline because I opened a (draft) PR that fell short of the goal 🫠! In fact, I am pretty sure that my PR is falling short of the goal because I added Alembic migrations. I can add annotations to my migrations to hit the metric, but that does feel like busywork here. You may want to start with the migrations module sooner rather than later? |
Beta Was this translation helpful? Give feedback.
-
Woah, you move way faster than I realised. The migrations module has already been renamed, so clearly the score changed for other reasons. I'll see about fixing that tomorrow then! |
Beta Was this translation helpful? Give feedback.
-
Pyright 1.1.391 fixed the bug that produced the confusing output. |
Beta Was this translation helpful? Give feedback.
-
Hi!
I wanted to kick of a discussion about improving Prefect's type completeness; how important type completeness is to Prefect and if it is considered important, how to reach and maintain type completeness.
I'm in the process of evaluating workflow orchestration frameworks for a company I currently consult for, and Prefect is one of our front-runners. One of prefect's strong points is that the project makes use of type annotations, where those annotations actively help you validate your code. In other words, the annotations are not just there to help the framework serialise and deserialise input and output data.
However, the SDK type annotations are not type complete, meaning that if you want to rely on the provided annotations to validate projects using prefect, then the type annotations fall short in several ways:
Where the codebase uses generic types such as
TaskRunner,ResultSerializer,StateorTask, the annotations fail to provide type arguments.E.g. the
Flow.on_completiondecorator is annotated to take an argumentfn, annotated asCallable[["Flow", FlowRun, State]. Both theFlowandStatetypes are generic types but no arguments are provided. A complete type annotation would beCallable[["Flow[P, R]", FlowRun, State[R]], because the function will be passed the current flow and aStatethat, if the flow produced a return value of typeR, would reference that return value.Some public methods lack annotations altogether; pyright reports 48 cases where public types reachable from
prefect.main, lack a type annotation for a function argument, and 103 cases where a return type annotation is missing.Some aspects of the
prefectpublic API are not as straightforward to express using might be desired, e.g. the fact thatprefect.Flow.from_sourceis copied over to thefrom_sourceattribute of theprefect.flow()function is not reflected in the type annotations. Unless this is fixed, my developers would have to either useprefect.Flow.from_source()or silence the type checker for perfectly normal and documented code patterns.See below for the
pyright --verifytypesreport for more details.I also note that there is very little in the way of type annotation testing. The only validation is running mypy via the pre-commit hook, where mypy is configured to run in almost the default configuration with additional overrides to ignore missing imports. There are no type annotation-specific tests, e.g. tests that cover how generic type variables are propagated. This is, unfortunately, too weak a check, as the default configuration ignores unannotated code and doesn't enforce type annotation rules important for type completeness.
Now, if we do go with Prefect, I can budget some of my time in helping improve the annotations in this project. I really want developers working with Prefect to be able to rely on the type annotations, both when writing workflow code in their IDE and when linting the code in the CI. But I really want to know first if this is something this project actually wants to support!
So:
How important is type completeness for Prefect?
and if the answer is 'it is very important', then the follow-up questions are:
pytest --verifytypes prefect.main --ignoreexternal report
Type completeness report
This report focuses on just
prefect.main, which defines the public API used by workflow developers. Running the same check on all ofprefectis much more verbose, because this then also covers code from outside the SDK and even the alembic migration steps for the server.To reproduce this report, first set up a virtualenv with both
pyrightandprefectinstalled; e.g. when in a checkout of this Github project:uv venv uv pip install --upgrade pyright -e .and then run pyright to produce the report:
I removed a confusing series of
Could not resolve module: "prefect.main.NAME"errors from the start of the report that are most likely due to a pyright bug connected to running this report against theprefect.mainpackage instead ofprefect; these errors did not seem to affect the accuracy of the report.I also removed the full path of the virtualenv on my local machine for privacy reasons.
Additional resources
Beta Was this translation helpful? Give feedback.
All reactions