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

Initial mypy implementation #405

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Initial mypy implementation #405

wants to merge 6 commits into from

Conversation

jlashner
Copy link
Collaborator

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:

  • A basic mypy config file that ignores type requirements for specific packages
  • py.typed file
  • A couple of # type: ignore statements needed to get it to compile
  • Type hinting for a few widely-used functions, such as init_site_agent, and register_task/process, and the param decorator.
  • Complete type-hinting for the registry agent as an example.

There are a couple of nice things about the type hinting here...

  • It's immediately clear just looking at the src what types of arguments are accepted by functions and what types are stored in generic containers like lists and dicts.
  • Will catch type errors such as not checking an Optional type for None, or attempting to access non-existent member variables of a class.
  • Instances where incorrect types are returned from functions (such as an OCS operation that doesn't return (bool, str)) will be caught.
  • You can configure your editor so that type-checking happens automatically and appears inline, for example:
    image

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

Copy link
Member

@BrianJKoopman BrianJKoopman left a 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
Copy link
Member

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.

Copy link
Collaborator Author

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.

ocs/agents/registry/agent.py Show resolved Hide resolved
ocs/ocs_agent.py Show resolved Hide resolved
ocs/ocs_agent.py Show resolved Hide resolved
Copy link
Member

@BrianJKoopman BrianJKoopman Sep 18, 2024

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%

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.

2 participants