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

Histogram is not typed, and due to partial typing, this makes mypy --strict fail #760

Closed
tommyjcarpenter opened this issue Jan 31, 2022 · 9 comments · Fixed by #771
Closed

Comments

@tommyjcarpenter
Copy link

tommyjcarpenter commented Jan 31, 2022

Since prometheus_client provides some types, adding it to ignore_missing_imports doesnt work. The existence of this file: https://github.com/prometheus/client_python/blob/master/prometheus_client/py.typed
means that ignore_missing_imports is ignored with prometheus_client.

Counter inc is typed:
https://github.com/prometheus/client_python/blob/master/prometheus_client/metrics.py#L272-L280

However, Histogram isnt typed, including observe:
https://github.com/prometheus/client_python/blob/master/prometheus_client/metrics.py#L464-L476

This causes mypy --strict to fail when using Histogram. Typing all metrics will allow strict mypy users to stop adding # type: ignore anywhere this function is used.

@csmarchbanks
Copy link
Member

#759 should fix this issue for histograms. The goal with typing was to add types over time as contributions allowed. It sounds like that does not work well for users using mypy --strict though which is disappointing.

I believe to fully allow running --strict we will need to add types for everything documented in prometheus_client/__init__.py which can be a goal for 0.14.0.

@tommyjcarpenter
Copy link
Author

Thanks for the reply! Mypy will only complain about what users are using from the package, in my case, Counters and Histograms. So in my case, the fix for histograms Is enough to make - - strict happy - it doesn't introspect on types beyond what the user imports.

But outside of my own use case, yes - - strict means "complete typing" and any users using an unryped function will have strict fail.

@tkukushkin
Copy link

You can try to check type completeness by pyright tool. It would be nice to add this check to the CI.

@csmarchbanks
Copy link
Member

We have mypy in the pipeline today, and are making it more strict as more typing is contributed. For this to be closed I think we have to add disallow_untyped_defs = True to the pipeline. Would pyright give us better/different coverage than mypy?

FYI, #759 has been merged with types for histograms, no release yet as I am hoping to have the time soon to add more types and release 0.14.0.

@tkukushkin
Copy link

In my experience Pyright shows more type problems. And it does not enforce you to cover private API and other implementation details, you can cover public API at first and cover private API gradually later.

Just in case, I am not talking about pyright as type checker, but only as type completeness checker. You can check types by mypy with your config (strict or not) and check type completeness by pyright. It will give you the best coverage of public API.

Perhaps @erictraut can tell us more accurately what the difference is.

@erictraut
Copy link

Pyright has two different modes. The first is a regular type-checking mode similar to mypy. It validates static type rules within a code base. The second is the "--verifytypes" feature that analyzes an installed "py.typed" package, enumerates all of the symbols that comprise its "public interface" and reports on whether static type annotations have been provided for each of them. It also emits a "type completeness score", which is the percentage of public symbols with types. Many library authors have started to use this in their CI pipelines to achieve and maintain 100% type completeness for their "py.type" libraries. This makes library consumers happy because they can then use strict type checking in their own code. It also improves their programming experience because of completion suggestions become more accurate and complete if they're using a language server like Pylance.

@tommyjcarpenter
Copy link
Author

tommyjcarpenter commented Feb 8, 2022

I think, for this issue I raised, the entire notion of "slowy adding types over time" simply conflicts with mypy strict. I don't think the type checker here would matter. Since the module is partially typed and includes a py typed file, mypy says OK, go time, and in this case fails because that function isn't typed.

But because it includes the typed file, adding it to import ignores doesn't have any effect

So the only fix for mypy strict users is to type it.

Or, remove the typed file until completeness, but that seems backwards!

@csmarchbanks
Copy link
Member

csmarchbanks commented Feb 9, 2022

I have opened #771 which I believe covers most of the public surface area of this library (defined as things we reference in __init__.py). Hopefully that takes care of most users who use mypy --strict, and if possible I would appreciate a review/someone else testing it out!

I also tried using pyright some, and it is nice for verifying types as described. It does mean more than just what is defined in __init__.py will need to be typed, but hopefully we can add it to the CI sometime. #771 moves the completeness score from 39.4% to 69.6% though!

@tommyjcarpenter
Copy link
Author

@csmarchbanks I'm not a contributor here but I will take a look! Thank you :)

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 a pull request may close this issue.

4 participants