-
Notifications
You must be signed in to change notification settings - Fork 5
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
Initial mypy implementation #405
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is neat, thanks for getting this going! This is all pretty new to me, but I have some comments below. A few more general ones:
- You mention "complete type-hinting for the registry agent", but there are still variables that aren't type hinted, what do you mean by "complete"?
- I added a mypy hook to pre-commit, so it'll run on PRs and locally if developers use pre-commit (highly recommend!). This caught a few more errors in the example code -- can you address those too? Namely:
example/miniobs/run_acq.py:7: error: "OCSClient" has no attribute "acq" [attr-defined]
example/miniobs/run_acq.py:13: error: "OCSClient" has no attribute "acq" [attr-defined]
example/miniobs/run_acq.py:19: error: "OCSClient" has no attribute "acq" [attr-defined]
mypy.ini
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get this when running:
mypy.ini: No [mypy] section in config file
Seems like a valid config needs a global [mypy]
section, even if empty.
That said, I also think we should add disable_error_code = annotation-unchecked
, else I get the following "notes" (which just seem like warnings):
$ mypy ocs/
mypy.ini: No [mypy] section in config file
ocs/agents/influxdb_publisher/agent.py:44: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs [annotation-unchecked]
ocs/agents/aggregator/drivers.py:567: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs [annotation-unchecked]
ocs/agents/aggregator/drivers.py:569: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs [annotation-unchecked]
ocs/agents/aggregator/agent.py:46: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs [annotation-unchecked]
Success: no issues found in 36 source files
Any thoughts on this --check-untyped-defs
option? Running with that produces 125 errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We talked about this yesterday, but I agree disabling the annotation-unchecked error code seems like the right thing to do hear. I don't believe we want to enforce checking of untyped defs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have another comment related to this file and our typing "completeness". I had been looking for some specification on what makes a py.typed
library, and didn't find it until last night in the typing.readthedocs.io
docs. What stuck out to me is the examples in "Examples of known and unknown types":
# Class with known type
class MyClass:
height: float = 2.0
def __init__(self, name: str, age: int):
self.age: int = age
@property
def name(self) -> str:
...
# Class with partially unknown type
class MyClass:
# Missing type annotation for class variable
height = 2.0
# Missing input parameter annotations
def __init__(self, name, age):
# Missing type annotation for instance variable
self.age = age
# Missing return type annotation
@property
def name(self):
...
I don't think we have enough of ocs
typed in this PR to claim it's a py.typed
package.
There are some interesting discussions on the python/typing
repo, especially this one on how to gradually type a library. It seems like type stubs can be partial, and marked as such by containing partial\n
. I haven't seen anywhere that says this is valid for a package with inline type hinting though.
We have a pretty limited downstream user base, but what is the implication for typing efforts in socs
if we don't include py.typed
here in ocs
yet?
Thoughts on more thoroughly typing
I also found this discussion about how typing simple examples like:
# Class with partially unknown type
class BaseClass:
# Missing type annotation
height = 2.0
as
class BaseClass:
height: float = 2.0
feels redundant. The response to that post comes from one of the maintainers of pyright, Microsoft's static type checker -- this seems to be the core for Pylance, the default LSP for VS Code. There I learned about pyright --verifytypes
, which can be used to check the "completeness" of a py.typed
package.
Running that we score a 17.3% in this branch currently.
Symbols exported by "ocs": 519
With known type: 90
With ambiguous type: 52
With unknown type: 377
Other symbols referenced but not exported by "ocs": 688
With known type: 614
With ambiguous type: 6
With unknown type: 68
Symbols without documentation:
Functions without docstring: 322
Functions without default param: 29
Classes without docstring: 78
Type completeness score: 17.3%
Thinking about our approach to enforcing PEP8 through autopep8
, I wondered if there were automated ways to annotate these obvious types. I found a list of tools in the mypy docs. They seem to only focus on function arguments and return values, but they get us a bit closer.
On top of that, a lot of the modules in ocs
should really probably be private (pyright only reports missing types for public modules), as ocs plugin authors shouldn't be importing code from core agents, nor any of the "scripts" like agent_cli.py
. If I mark modules that probably should be private as such and run one of the autotyping tools we get up to 22.4%, with 175 "unknown" types and 15 "ambiguous" types out of 245 symbols exported by ocs
.
Symbols exported by "ocs": 245
With known type: 55
With ambiguous type: 15
With unknown type: 175
Other symbols referenced but not exported by "ocs": 576
With known type: 520
With ambiguous type: 5
With unknown type: 51
Symbols without documentation:
Functions without docstring: 239
Functions without default param: 24
Classes without docstring: 62
Type completeness score: 22.4%
Adds a basic working mypy setup, and types some important widely used functions
Description
This adds the following to serve as a foundation for static typing in ocs/socs:
# type: ignore
statements needed to get it to compileinit_site_agent
, andregister_task/process
, and theparam
decorator.There are a couple of nice things about the type hinting here...
Types of changes
Checklist: