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

Integration feedback report: psf/black #886

Open
ichard26 opened this issue Aug 17, 2021 · 2 comments
Open

Integration feedback report: psf/black #886

ichard26 opened this issue Aug 17, 2021 · 2 comments
Labels

Comments

@ichard26
Copy link
Collaborator

ichard26 commented Aug 17, 2021

Hello! So I'm Richard from the psf/black maintainer team, and for the past few months I've been working on finishing up the work done by Sully in psf/black#1009 so mypyc can finally be deployed. As a token of appreciation (and to help out the project) I've written up a report containing my experience integrating mypyc with psf/black :)


First of all, here's the relevant links:

The good

  • The performance wins are excellent: ~2x average across multiple workloads!
  • As long as the hot code wasn't too spread out, optimizing for mypyc wasn't that hard (tightening up types, adding lots of Final, etc.) - additional performance wins of 5-15% were achieved :D
  • There weren't actually that many crashes or other incompatibilities (ignoring dataclasses) and they all had easy enough workaround
  • Wheel building was pretty easy and straightforward (once some changes were made to the setup.py logic) - I did steal lots of ideas from mypyc/mypy_mypyc-wheels :P

The bad

  • Dataclasses were responsible for the majority of crashes and other compatibility issues. The cases that led to issues include:
    • an dataclass that is also subclassing ast.NodeVisitor crashes with AttributeError: attribute 'dict' of 'type' objects is not writable (maybe not fixable?)
    • an dataclass inheriting from Generic (already reported in Dataclass + Generic fails during sleight of hand #827)
    • abc.ABC + dataclass crashes too
  • Unanalyzed or unreachable code continues to be problematic - a "fun" case involved this:
    >   if sys.platform == "win32":
    RuntimeError: Reached allegedly unreachable code!
    
    This was caused by mypy.ini hard-coding platform=linux. Other interesting case was one involving a module typed as Any causing an else branch to appears as it's never ever taken. The resulting unanalyzed code crashed when hit. More details in this commit: psf/black@acb77f7

And other various issues already reported on the mypyc issue tracker:

Suggestions

  • Add a note about platform=$OS being problematic
  • Add a note about docstrings being stripped - the click CLI toolkit uses docstrings to get command descriptions ... which ended breaking without a clear reason why (until I stumbled on some issue on mypyc issue tracker that explained why :p)
  • Prioritize Reduce mypy/c package size, using -g0 CFLAG #849 - while I'm OK with wheel sizes going up adding mypyc, the ~25-35x (120KB -> ~3.3MB) I observed wasn't the easiest thing to swallow. Adding CFLAGS=-g0 brought the wheels to a much more reasonable ~1MB each.
  • Prioritize [mypyc] Introduce subcommand to mypyc command line interface python/mypy#10395 - quick iteration and experimentation was slowed down by repeated cleaning up + python -c "import foo"
  • Perhaps add a way to skip invoking the C extension build - this would make analyzing differences at the IR or C level faster ... although I would understand if this is too low-level and shouldn't be made easier

Questions

  • Can anything be done about analysis + Python-to-C transcompilation being done twice during any modern wheel build? Unfortunately any evaluations of setup.py end up triggering the above and it turns out both pip wheel and python -m build call at-least two PEP 517 hooks that eventually end up evaluating setup.py. It's not a big deal, but it slows down wheel builds a little bit. Sounds hard to avoid tho (caching would be complicated).
  • What else can we do to help mypyc mature and overall get better?

What's next?

Currently the stability and performance data for Linux were just collected and look good so a PR was opened against psf/black. There's a lot of TODOs listed in that PR but it is progress.

The main thing is that I need to do is getting the wheels significantly more battle tested so they can eventually find their way to PyPI. First of all I need to run a mypy-primer equivalent on Windows and MacOS. Assuming that doesn't turn up anything scary, the PR will probably get merged once the reviews are over with. Then at that point, it's off to community testing where we'll be asking members of the community to use the experimental wheels and report issues. So expect updates on this issue as more stability data comes in ... but I'm sure this is already enough insights for a post ^^

Finally

As much as this is riddled with negativity and bugs, I appreciate all of the work y'all put into mypyc. It's awesome to see so much work into making Python code faster lately! 🖤 If you have any questions, feel free to ask away.

@ichard26
Copy link
Collaborator Author

ichard26 commented Feb 4, 2022

It's been a long while I've updated this post, I'll try to write an update soon but the TL;DR is that we are shipping mypyc compiled wheels in Release 22.1.0 of Black. Other than psf/black#2845 and unexpected unofficial API breakage it's been smooth sailing so far :)

I'm actually in the midst of writing a blog post describing my experience integrating mypyc into Black so that's currently eating my OSS time 🙂 If any mypyc core developers wants to review it once I finish it (no promises on when!) feel free to @ me and we can discuss. Obviously please don't feel like you must, it's just an invitation, I get we're all busy as OSS maintainers 🙂

Once again, thank you for the great product!

@ichard26
Copy link
Collaborator Author

So I'm going to assume that people have heard of my blog series since it kinda blew up 😅 but just in case, here's part one.

Anyway, there's not actually too much to comment on even after five months. With the help of Jelle, I was able to land fixes for #917 to allow us to use the latest mypy[c] to compile Black. Turns out that's not possible quite yet thanks to #941. psf/black#2845 is still a pain and not much progress has been made on it, but there doesn't seem to be any other crashes or issues using mypyc with Black as of writing 🎉

For what it's worth, I've been learning C this summer and slowly familiarizing myself with the mypyc codebase so I can contribute fixes and other improvements. My hope is to be able to eventually fix the issues impacting Black myself, ... but some of these issues do look very involved so I'm not too sure about that :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants