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

Support compilation with mypyc #1009

Merged
merged 3 commits into from Oct 30, 2019
Merged

Support compilation with mypyc #1009

merged 3 commits into from Oct 30, 2019

Conversation

msullivan
Copy link
Contributor

@msullivan msullivan commented Sep 7, 2019

Initial take on supporting compilation of black with mypyc. Most of the work here was done by @SanjitKal.

In my testing it sped things up by about a factor of 2 (with --fast).

Most of my testing was done by running CC=clang python3 setup.py --use-mypyc build_ext --inplace and then running black. clang needs to be used as the C compiler currently because gcc (at least the version I tried) barfed on some unicode identifiers (mypyc/mypyc#699).

The bulk of the diff is adding types to blib2to3 so it can be compiled, which is critical to the performance gains here: both because it speeds up access to the data structures from black itself and because the parser is a major hotspot.

More detail in the individual commit messages.

Work on #366.

(This requires python/mypy#7481 to land pickling support in mypyc before it works right)

black.py Outdated
@@ -188,12 +188,23 @@ class Feature(Enum):
}


@dataclass
class FileMode:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is dataclass support planned for mypyc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a bug open for it (mypyc/mypyc#671) but I'm not sure when I'll have a chance to do it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'm going to have dataclass support without optimized __init__ in the next few days, so we'll be able to keep using dataclasses though I'll probably keep the __init__ implementation on Line and anything else that gets created a lot.

msullivan added a commit to msullivan/black that referenced this pull request Sep 9, 2019
This is in preparation for adding type annotations to blib2to3 in
order to compiling it with mypyc (psf#1009, which I can rebase on top of
this).

To enforce that it stays blackened, I just cargo-culted the existing
test code used for validating formatting. It feels pretty clunky now,
though, so I can abstract the common logic out into a helper if that
seems better. (But error messages might be less clear then?)
msullivan added a commit to msullivan/black that referenced this pull request Oct 1, 2019
This is in preparation for adding type annotations to blib2to3 in
order to compiling it with mypyc (psf#1009, which I can rebase on top of
this).

To enforce that it stays blackened, I just cargo-culted the existing
test code used for validating formatting. It feels pretty clunky now,
though, so I can abstract the common logic out into a helper if that
seems better. (But error messages might be less clear then?)
msullivan added a commit to msullivan/black that referenced this pull request Oct 2, 2019
This is in preparation for adding type annotations to blib2to3 in
order to compiling it with mypyc (psf#1009, which I can rebase on top of
this).

To enforce that it stays blackened, I just cargo-culted the existing
test code used for validating formatting. It feels pretty clunky now,
though, so I can abstract the common logic out into a helper if that
seems better. (But error messages might be less clear then?)
ambv pushed a commit that referenced this pull request Oct 20, 2019
* Blacken .py files in blib2to3

This is in preparation for adding type annotations to blib2to3 in
order to compiling it with mypyc (#1009, which I can rebase on top of
this).

To enforce that it stays blackened, I just cargo-culted the existing
test code used for validating formatting. It feels pretty clunky now,
though, so I can abstract the common logic out into a helper if that
seems better. (But error messages might be less clear then?)

* Tidy up the tests
@ambv
Copy link
Collaborator

ambv commented Oct 20, 2019

Sorry, I haven't noticed that merging blackening of blib2to3 will conflict this PR. If you could blacken so we can merge, we will.

@msullivan
Copy link
Contributor Author

I've rebased and updated this. I dropped the commit that removed the attrs in favor of putting up a separate PR to switch attrs to dataclasses (#1116) in concert with a mypyc PR to add efficient dataclass support (python/mypy#7817)

Copy link
Collaborator

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

I have a couple of nitpicky comments but I'm fine with addressing those in later PRs myself instead.

More importantly though there is a merge conflict.

blib2to3/pgen2/driver.py Show resolved Hide resolved
from blib2to3.pytree import _Convert, NL
from blib2to3.pgen2.grammar import Grammar

Path = Union[Text, "os.PathLike"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing a type arg to os.PathLike. We should probably turn on the --disallow-any-generics option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed that and set the flag

Iterable,
List,
Optional,
Text,
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can just use str since this is Python 3-only code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll leave this to a follow-up, since the lib2to3 stubs used Text pretty heavily

@@ -66,9 +93,13 @@ def __eq__(self, other):
return NotImplemented
return self._eq(other)

__hash__ = None # For Py3 compatibility.
# __hash__ = None # For Py3 compatibility.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does mypyc choke on __hash__ = None? I'd be fine with just removing the line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mypy complains about it with a type error. I can fix it with a # type: Any though.

assert not isinstance(content, str), repr(content)
content = list(content)
for i, item in enumerate(content):
# assert not isinstance(content, str), repr(content)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's wrong with this assertion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It caused mypy to barf because it then couldn't figure out what type content (and thus newcontent) was. The actual issue is that the type of content was wrong, though. I'll fix that, though all this code is dead in black.

msullivan and others added 3 commits October 29, 2019 23:28
This used a combination of retype and pytype's merge-pyi to do the
initial merges of the stubs, which then required manual tweaking to
make actually typecheck and work with mypyc.

Co-authored-by: Sanjit Kalapatapu <sanjitkal@gmail.com>
Co-authored-by: Michael J. Sullivan <sully@msully.net>
The changes made fall into a couple categories:
 * Fixing actual type mistakes that slip through the cracks
 * Working around a couple mypy bugs (the most annoying of which being
   that we need to add type annotations in a number of places where
   variables are initialized to None

Co-authored-by: Sanjit Kalapatapu <sanjitkal@gmail.com>
Co-authored-by: Michael J. Sullivan <sully@msully.net>
@JelleZijlstra
Copy link
Collaborator

Thanks so much for getting this done!

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.

None yet

3 participants