Feat: Adds type annotations #43
Conversation
- generated using MonkeyType - added `from __future__ import annotations` - introduced imports moved into `if TYPE_CHECKING` block
This commit has `python -m mypy saq/queue.py` running without issue under current configuration.
|
@tobymao you should be able to run I'm running with default config to start with, we can ramp up the strictness settings of mypy once I'm past the initial batch of errors. Found an issue with the redis stubs in typeshed: python/typeshed#8960 |
|
Thanks for the reviews! I'll keep working at it over the weekend.
172 errors still when I run on saq and tests :) will def. add it in when I get a clean run. |
|
let me know when is hould take another look |
Ensures `max_delay` parameter to `exponential_backoff()` called with value derived from `Job.retry_backoff`.
|
A good chunk of the mypy errors revolve around attribute access on These are currently Traceback (most recent call last):
File "/home/peter/.pyenv/versions/3.10.8/lib/python3.10/code.py", line 90, in runcode
exec(code, self.locals)
File "<input>", line 1, in <module>
File "/home/peter/PycharmProjects/saq/saq/job.py", line 139, in id
return self.queue.job_id(self.key)
AttributeError: 'NoneType' object has no attribute 'job_id'One idea is to add an access method to centralise the exception logic and type narrowing, such as: class Job:
...
def get_queue(self) -> Queue:
if self.queue is None:
raise TypeError("`Job` must be associated with a `Queue` before this operation can proceed")
return self.queue
@property
def id(self) -> str:
return self.get_queue().job_id(self.key)It could also be a property and/or memoized but I doubt we'd be looking at too much overhead with the plain method. Any opinion here @tobymao @barakalon? |
A common class of mypy error is "`Optional[...]` has no attribute ...". - Adds `Job.get_queue()` which raises for no queue and narrows type. - Adds `enqueue()`, `dequeue()`, `count()` and `finish()` methods to `TestQueue` class that are queue methods commonly called in the tests and they narrow type where applicable. - Adds `TestWorker.enqueue()` for type narrowing to `Job`. - Other handling of specific instances where optional types needed to be narrowed.
that’s fine |
For py37 we `TypedDict` and `Literal` come from `typing_extensions`. Includes import handling in `compat.py`.
- uses builtin generics, e.g., `list[...]` instead of `t.List[...]`. We are able to do this due to the `__future__.annotations` and `if TYPE_CHECKING` blocks preventing any runtime parsing of annotations. - mypy config - increase strictness and warnings. - adds `py.typed` file. - adds mypy to `run_checks.sh`.
| @@ -233,7 +236,7 @@ async def update(self, **kwargs) -> None: | |||
| setattr(self, k, v) | |||
| await self.get_queue().update(self) | |||
|
|
|||
| async def refresh(self, until_complete: t.Optional[float] = None) -> None: | |||
| async def refresh(self, until_complete: float | None = None) -> None: | |||
There was a problem hiding this comment.
I was just about to write an explanation.
The from __future__ import annotations means that all annotations are strings at runtime, so no runtime errors irrespective of version, and type checkers are able to use the current syntax irrespective of the version.
I've been running the tests locally on 3.8 without issue and that syntax was introduced in 3.10. Similarly, the builtin subscription syntax, e.g., list[...] is 3.9+ but the future annotations mean we don't need to worry about the runtime errors.
There was a problem hiding this comment.
Of course, if there is an issue I'm missing here, or you'd prefer to stay syntactically valid with the min version irrespective of the future annotations, I'll be happy to roll them back. Just that the current syntax is a lot nicer than what was available in 3.7.
There was a problem hiding this comment.
I think mypy will automatically add an Optional type when the default value is None? I could have that wrong, though.
There was a problem hiding this comment.
You are correct, but reliance on it is discouraged: python/peps#689 and python/mypy#9091
| schedule: int | ||
| stats: int | ||
| sweep: int | ||
| abort: int |
There was a problem hiding this comment.
@tobymao Note in this module that I've used the typing versions of generics as it would be feasible that a user would import something from here to annotate their own code, and we can't be sure they'll use it with from __future__ import annotations, so needs to be compatible with 3.7 runtime.
This is also why "Job" and "Status" are forward refs here, as if this module is imported outside of a if TYPE_CHECKING block, their reference would otherwise fail.
There was a problem hiding this comment.
can you double check everything works fine in 3.7?
There was a problem hiding this comment.
yes I will verify
There was a problem hiding this comment.
Add 3.7 to the tests? https://github.com/tobymao/saq/blob/master/.github/workflows/python-ci.yml#L19
There was a problem hiding this comment.
IsolatedAsyncioTestCase not available in 3.7. I tried to do a compat with aiounittest for 3.7 (import aiounittest.AsyncTestCase as IsolatedAsyncioTestCase) but there were a stack of errors.
Perhaps a simple integration test script can be added to CI for 3.7 only?
3.7 EOL is 27th June 2023.
There was a problem hiding this comment.
Then maybe we ignore 3.7?
There was a problem hiding this comment.
that would certainly simplify things
There was a problem hiding this comment.
3.7 removed and added 3.11
There was a problem hiding this comment.
Note we still need to use the typing module generics here unless we go back to:
LoadType: t.TypeAlias = "Callable[[bytes | str], t.Any]"Six to one, half a dozen to the other in my mind, so I'll leave as is unless you'd prefer otherwise.
saq/types.py
Outdated
| CountKind = Literal["queued", "active", "incomplete"] | ||
| DumpType = t.Callable[[t.Dict], str] | ||
| DurationKind = Literal["process", "start", "total", "running"] | ||
| Function = t.Callable |
There was a problem hiding this comment.
This is just a stub for a better type.
I tried different configurations with Protocol and __call__ and @overload to try and satisfy requirement that these can be both sync and async, accept a single pos arg, either accept kwargs, or not, as well as having the __qualname__ attribute, but couldn't find a solution that worked.
There was a problem hiding this comment.
Going 3.8+ might help with this as positional only parameters were introduced in 3.8.
From running command `pyright --verifytypes saq --ignoreexternal`
This change addresses highlighted to bring % of public interfaced typed
up to 100%.
```
(.venv) peter@peter-Inspiron-15-5510:~/test-saq$ pyright --verifytypes saq --ignoreexternal
Module name: "saq"
Package directory: "/home/peter/PycharmProjects/saq/saq"
Module directory: "/home/peter/PycharmProjects/saq/saq"
Path of py.typed file: "/home/peter/PycharmProjects/saq/saq/py.typed"
Public modules: 8
saq
saq.__main__
saq.job
saq.queue
saq.types
saq.utils
saq.web
saq.worker
Symbols used in public interface:
Symbols exported by "saq": 212
With known type: 212
With ambiguous type: 0
With unknown type: 0
(Ignoring unknown types imported from other packages)
Functions without docstring: 50
Functions without default param: 0
Classes without docstring: 6
Other symbols referenced but not exported by "saq": 0
With known type: 0
With ambiguous type: 0
With unknown type: 0
Type completeness score: 100%
Completed in 0.631sec
```
|
I just added a commit that addresses issues with public interface types highlighted by the Pyright Most of the issues were from use of generic types without specifying type arguments. One interesting case to highlight was this:
for key in await self.redis.zrangebyscore(self._stats, now(), "inf"):
key_str = key.decode("utf-8")Therefore On the negative, it raised a few like this: Meaning it wants: logger: logging.Logger = logging.getLogger("saq")Pretty gross, and I'm not sure how that could be inferred to be anything different by other type checkers. Full output: |
E.g., `logger: logging.Logger = logging.getLogger()`. The pyright tool encouraged these, but I can't see what they add.
|
seems reasonable to me, is this ready to go? |
Close, I'm experimenting with it downstream. The top-level import aren't being recognized by mypy, e.g.,: import saq
q = saq.Queue() # Module has no attribute "Queue" [attr-defined]Might need an |
That was it (ref). Should be good to go now. |
|
@peterschutt thanks again for this, great work! |
No worries! Feel free to ping me if there are any issues. |
Related to #39