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

Enhancement: Compile using mypy #57

Open
Goldziher opened this issue Oct 7, 2022 · 9 comments
Open

Enhancement: Compile using mypy #57

Goldziher opened this issue Oct 7, 2022 · 9 comments
Assignees
Labels
enhancement New feature or request mypy An upstream issue with Mypy

Comments

@Goldziher
Copy link

Hi again,

So I've noticed that refurb is pretty slow compared to other tools. I would guess its because its using mypy and is not itself compiled. I would therefore suggest investing some time into setting up a compilation using mypyc - this shouldnt be hard to do given the fact that codebase is well typed. The annoying and complex part is going to be the github pipeline to test this in different environments and the release of the wheels. But you should see a x2-x4 performance increase out of the box according to mypyc.

@dosisod
Copy link
Owner

dosisod commented Oct 7, 2022

This is on my todo list, and something that I hope to play around with by the end of next week. I'll make sure to report back here on any progress I make!

@dosisod dosisod self-assigned this Oct 7, 2022
@dosisod dosisod added the enhancement New feature or request label Oct 7, 2022
@dosisod
Copy link
Owner

dosisod commented Oct 8, 2022

Looks like there is a long road ahead for mypyc:

$ mypyc refurb/checks/pathlib/with_suffix.py
refurb/checks/pathlib/with_suffix.py:42: error: Match statements are not yet supported

There is currently an open issue in the mypyc repo for this. I wouldn't mind taking a crack at implementing this, might be fun who knows!

At the very least we could mypycify all the non-match related code, though most of the meat of this project lies in the checks.

@Goldziher
Copy link
Author

I'd wait on your shows. Or change the local implementation

@Goldziher
Copy link
Author

Looks like there is a long road ahead for mypyc:

$ mypyc refurb/checks/pathlib/with_suffix.py
refurb/checks/pathlib/with_suffix.py:42: error: Match statements are not yet supported

There is currently an open issue in the mypyc repo for this. I wouldn't mind taking a crack at implementing this, might be fun who knows!

At the very least we could mypycify all the non-match related code, though most of the meat of this project lies in the checks.

So, the problem for me as a user is that refurb is currently extremely slow on large code bases - significantly more than mypy itself, because it's not compiled.

I would suggest improving two points- 1. Compilation and 2. Ignoring all imports in a better way, even those not missing.

@dosisod
Copy link
Owner

dosisod commented Oct 31, 2022

Progress is under way! I have an open PR for adding match support to Mypy (see python/mypy#13953), and I recently added a --disable-all flag which will disable all checks, thus not loading them. In that case, you would have to explicitly enable the checks you want.

Does that help answer your question? Thanks!

@Goldziher
Copy link
Author

Progress is under way! I have an open PR for adding match support to Mypy (see python/mypy#13953), and I recently added a --disable-all flag which will disable all checks, thus not loading them. In that case, you would have to explicitly enable the checks you want.

Does that help answer your question? Thanks!

Hi, the issue is not the checks, it's the amount of time it takes in traversing types. I would suggest to try and ensure it never enters library code at all, unlike mypy. I don't know if it's possible, but if it is I'm sure it will speed up things.

@dosisod dosisod added the mypy An upstream issue with Mypy label Nov 13, 2022
@dosisod
Copy link
Owner

dosisod commented Dec 11, 2022

Hi @Goldziher,

python/mypy#13953 has been merged, and progress is well underway to get Refurb compiling with Mypy! There are some additional roadblocks which will need to be resolved, but for the most part, we are much closer then we where before.

As for your comment about speeding things up by not checking library code, how were you bench marking that? I don't doubt your methods, I'm just curious how you came to that conclusion (as I have yet to look into this for myself). There are probably ways to prevent the library code from being traversed, though I wonder how much type information (if any) would be able to be gathered.

@Goldziher
Copy link
Author

Hi @Goldziher,

python/mypy#13953 has been merged, and progress is well underway to get Refurb compiling with Mypy! There are some additional roadblocks which will need to be resolved, but for the most part, we are much closer then we where before.

As for your comment about speeding things up by not checking library code, how were you bench marking that? I don't doubt your methods, I'm just curious how you came to that conclusion (as I have yet to look into this for myself). There are probably ways to prevent the library code from being traversed, though I wonder how much type information (if any) would be able to be gathered.

Hi, i haven't benchmarked this. I simply noticed that the moment I set the missing dependencies in the "additional_dependencies" list in the pre-commit config, checking became excruciatingly slow.

For now btw I removed refurb from our pipelines - it simply takes too long to run on our code base. I'll certainly give it another go once its faster.

@dosisod
Copy link
Owner

dosisod commented Dec 11, 2022

I completely understand removing Refurb for the time being, it is a bit slower then I would like. For the time being I will look into ways to speed things up without using compilation, and add benchmarks so that we can track the speed of Refurb over time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request mypy An upstream issue with Mypy
Projects
None yet
Development

No branches or pull requests

2 participants