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

Add some typing changes required by Mypyc #1658

Merged
merged 3 commits into from
Jan 5, 2025

Conversation

nickdrozd
Copy link
Contributor

I tried compiling Coverage with Mypyc, the compiler that comes with Mypy. Mypyc is pretty stringent about what it expects, and addressing some of its objections will require some design thought. But these changes are pretty straightforward.

@nickdrozd
Copy link
Contributor Author

#1659

@nickdrozd
Copy link
Contributor Author

@nedbat Any interest in this one? If not, I can close it.

@KRRT7
Copy link

KRRT7 commented Jan 5, 2025

Hi @nickdrozd,

It's been a while since you opened this PR, so I wanted to take a closer look.

Could you clarify the reasoning for integrating these changes directly into Coverage?

Are there tangible benefits to Coverage itself compiling its pure Python source code to C via mypyc?
Have you observed or measured any performance improvements (benchmarks are welcome) that would show that transpiling coverage's pure python code base into C via mypyc would help?

@nickdrozd
Copy link
Contributor Author

Hi @KRRT7. This set of changes makes it possible to successfully run Mypyc as follows:

mypyc coverage           \
      --exclude cmdline  \
      --exclude control  \
      --exclude env      \
      --exclude files    \
      --exclude parser   \
      --exclude plugin   \
      --exclude sysmon   \
      --exclude tracer

The changes are extremely minor, and IMO would be worth doing anyway.

I haven't done any measurements of any kind. Maybe Mypyc won't make much of a difference. On the other hand, Python is really really slow, so maybe it will make a difference. Coverage isn't exactly a performance-critical tool, but it would be nice if it were faster.

Two open questions:

  1. How much benefit is there?
  2. How much work would it take?

If the benefit is high or the work is low, I think it would be worth pursuing further.

@nedbat
Copy link
Owner

nedbat commented Jan 5, 2025

I don't yet understand how mypyc might fit into the coverage.py world, but these typing changes are good, so I'll merge them.

optimize_if_not_debug = 1
else:
optimize_if_not_debug = 2
optimize_if_not_debug = 1 if pep626 else 2
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

coverage/env.py:56: error: Unsupported statement in class body

In general Mypyc doesn't things being defined in multiple places. Using if/else means the class var is defined in two places, while using conditional statement means it is defined in just one place.

A more dramatic example is in files.py, where the function actual_path is defined conditionally:

if env.WINDOWS:

    _ACTUAL_PATH_CACHE: dict[str, str] = {}
    _ACTUAL_PATH_LIST_CACHE: dict[str, list[str]] = {}

    def actual_path(path: str) -> str:
        """Get the actual path of `path`, including the correct case."""
	...

else:
    def actual_path(path: str) -> str:
        """The actual path for non-Windows platforms."""
        return path

@@ -148,7 +145,7 @@ class PYBEHAVIOR:
soft_keywords = (PYVERSION >= (3, 10))

# PEP669 Low Impact Monitoring: https://peps.python.org/pep-0669/
pep669 = bool(getattr(sys, "monitoring", None))
pep669: Final[bool] = bool(getattr(sys, "monitoring", None))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conceptually, all of PYBEHAVIOR should be Final. How come only this attribute needed Final?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good question. I think it has to do with the file exclusions I mentioned above. Probably they would all require Final if everything were included.

@nedbat nedbat merged commit 57ea8a1 into nedbat:master Jan 5, 2025
32 checks passed
@nedbat
Copy link
Owner

nedbat commented Jan 5, 2025

I've also gone ahead and enabled strict mypy checking in commit ea8ae17.

@nickdrozd nickdrozd deleted the mypyc-changes branch January 5, 2025 18:35
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.

3 participants