-
Notifications
You must be signed in to change notification settings - Fork 31
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
Use dmypy instead of mypy #58
Conversation
Unfortunately I can't find the time to work on this right now, I'll try to do something the next weekend. |
No worries, don't expect you to finish this PR. I'm thinking the best approach is to completely switch the plugin to use dmypy since that makes it simpler for the users and also makes the most sense in pretty much every single way. However, I did do some more research on the follow imports stuff and realized that follow imports error is not the way to go. Ideally we still want Nonetheless, feel free to provide your thoughts when you find the time. I might test the above out next week. |
This issue has been automatically marked as stale because it has not had activity in the last 60 days. |
Update: by now |
P.S. However I'd still be very keen on getting this dmypy option since for regular use cases the performance gain is immense. In one project I'm working on mypy runtimes decreased from 70+ seconds to 200ms for incremental changes. |
@Bunkerbewohner thanks for the update. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @fgblomqvist ! I'd definitely would like to see this feature, too!
Thanks for the update @Bunkerbewohner. I'm atm not currently actively developing in Python, but I am willing to try to put some more effort into this to get it merged. I made the changes you suggested, happy that they added the "normal" option. However, I'm in a new environment and I can't get this plugin to build anymore. This is the error I'm getting:
I'm using Java 9 which I'm not sure is right (I'm no java dev). Maybe @leinardi can shed some light on this. Once I (or someone else) can get this to build I can verify that the changes I made work. |
I rebased on top of the latest master. |
For me your branch builds fine, but I'm using Java 8:
However, when I use the plugin built from this branch, it seems to be completely broken for me and I keep getting this error inside the Event log:
This does not happen using the latest master so it seems to be related to this branch. To reproduce the issue you can checkout the tag |
@@ -272,19 +268,15 @@ public static String detectSystemMypyPath() { | |||
if (filesToScan.isEmpty()) { | |||
return Collections.emptyList(); | |||
} | |||
boolean daemon = false; | |||
|
|||
GeneralCommandLine cmd = new GeneralCommandLine(mypyPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The daemon has its own executable called "dmypy" which is next to "mypy" in the same folder. If useDaemon is true we would need to change that here.
@fgblomqvist thanks a lot! @leinardi @fgblomqvist the problem is that dmypy has its own executable. The equivalent of "mypy --config-file mypy.ini file.py" using the daemon looks like this: "dmypy run -- --config-file mypy.ini file.py". I commented where it would need to be adjusted. I wonder if it's ok to simply take the path of mypy and prepend the "d" to the filename. If that file exists daemon mode is supported and should work, otherwise not. |
@Bunkerbewohner ah that's new (they didn't use to have a separate executable, I'm pretty sure). Easy to fix. And yeah I think that sounds good enough. @leinardi Hmm okay let me try with Java 8 then. |
I was able to compile with Java 8 🎉 I pushed changes that does a very simple (and stupid) path-replacement to use Regardless, the behavior can probably be improved a bit. |
I tried the latest branch and I found the following issues:
To reproduce you can run the plugin on this module: https://gitlab.com/leinardi/gwe/-/blob/master/gwe/__main__.py |
Fixed the persistence problem. I sometimes now get around 50 errors in that project and sometimes no problems at all. Can't figure out how to get debug logging in IDEA to work so don't really know how to debug this further. Regardless, the problem you're seeing with 200+ errors is not something I can replicate. |
Thanks @fgblomqvist! I have tested the current version on a project of my own and still have some issues. I get the following error when starting "Check Current File" using the plugin:
However, if I manually start the daemon on the command line the plugin works as expected and errors are checked super quickly. Also I don't get any unexpected mypy errors listed in my project which is quite sizeable (a clean mypy run takes more than 70s). The "Test" button successfully finds the mypy executable. |
|
||
public MypyConfigService() { | ||
customMypyPath = ""; | ||
mypyArguments = ""; | ||
mypyConfigFilePath = ""; | ||
useDaemon = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dmypy seems to be still somewhat experimental. In productive use we definitely encountered some unexpected errors here and there, as well as apparant infinite loops where dmypy would never finish. Therefore I would strongly suggest to disable this option by default.
P.S. I changed the error logging a little and now the error I see is this:
So the way that dmypy is invoked is correct including the path to the config file. No idea why launching the process doesn't work though. I mean in the earlier error it said "java.io.IOException: error=13, Permission denied" but why... |
I had similar odd java errors when using adopt. Switched to standard openjdk which worked a lot better. So yeah beyond the weird java error which I don't think has to do with the plug-in, it seems like the only thing left to do is to disable the daemon by default. I can do that this coming week. Thanks for reporting back and lmk if you solve the java issue! |
Got around to changing it, lmk if anything else should be done. When you have time @leinardi, please give it another go to see if you still see weird behavior. |
Hi @fgblomqvist, sorry for the late reply, it was a busy week at work. I tested your branch again and the daemon checkbox works correctly. But now, when I enable it, mypy doesn't find any violation at all. The Event log shows no errors. I made a short video to show the issue (unfortunately to be able to attach it I had to zip it): 2020-08-08 10-58-04.zip |
I see. Yeah I had similar results sometimes. Idk if it's because the daemon is finicky or if there is something wrong with how I run it (but I don't think so) 🤔 . |
i believe i've fixed some of these issues https://github.com/fgblomqvist/mypy-pycharm/pull/1 |
…me reason it reports no problems when the file is located outside of the project
fix dmypy issues
some further outstanding issues i've identified
looking at the mypy vscode plugin which uses the daemon and seems to be much more reliable, by default it only ever runs on the whole project and not individual files: |
Any updates on this PR? Would be a great feature. |
I updated the code to scan the whole project as per the above comment on my own branch, rebased, and tried running it on a project but am now running into this mypy bug: python/mypy#10709 |
Hey @fgblomqvist the issue linked by @nateschickler-puzzle seems to have been resolved. Any chance of getting this going again? |
Hi @isuro, unfortunately I don't really have the time or motivation to continue this at the moment (haven't worked with Python in the last couple of years at least), but I welcome any attempts to pick it up! 🙂 I would start with @nateschickler-puzzle's work here: https://github.com/nateschickler-puzzle/mypy-pycharm |
First time contributor checklist
Contributor checklist
using the
Fixes #1234
syntaxDescription
This PR intends to either switch this plugin over to use dmypy completely, or at least provide an option to do so. Using dmypy is much faster and efficient than using mypy, it will allow sub-second scans compared to the 5-15 seconds it could take for mypy to do the same task.
There is still some work left to do. E.g. deciding whether we should just always use dmypy or whether it should be configurable (and how that should work). A fix/solution is also needed to compensate for the fact that dmypy does not support
--follow-imports silent
. I'm proposing that the function that parses the mypy output can filter out any errors produced by--follow-imports error
and that we default to that option instead.Lastly, it probably just needs a tad more testing (e.g. in PyCharm, and perhaps some other projects).
Nonetheless, the code in this PR serves as a proof-of-concept for now, and I intend to follow through with getting it into a merge-able state as long as some decisions on the above are made.
Type of Changes
Related Issue
Closes #57
Closes #43