Improve performance, convert real-time scanning to ExternalAnnotator API #77

intgr · 2021-12-02T13:59:11Z

Using the ExternalAnnotator API (instead of LocalInspectionTool) behaves a lot better with slow scanners like mypy and fixes reported performance problems with the plugin (fixes #43).

Following multiple successive changes to a file, LocalInspectionTool can invoke the checker for each modification from multiple threads in parallel, which can bog down the system. ExternalAnnotator cancels the previous running check (if any) before running the next one.

Modeled after com.jetbrains.python.validation.Pep8ExternalAnnotator

A few notes:

~~Inspections from ExternalAnnotator now look different.~~
~~Now using different HighlightSeverity levels instead of always a single one.~~
In the "Problems" window, mypy inspections can no longer be grouped by inspection type -- they always appear at the root level.
Intention actions ("suppress for statement" etc) no longer appear.
# noinspection Mypy comments no longer suppress the annotations.

Contributor checklist

I am using the provided codeStyleConfig.xml
I have tested my contribution on these versions of PyCharm/IDEA:

PyCharm Community 2021.2.3

My contribution is fully baked and ready to be merged as is
I ensure that all the open issues my contribution fixes are mentioned in the commit message of my first commit
using the Fixes #1234 syntax

Description

Type of Changes

	Type
✓	🐛 Bug fix
✓	🔨 Refactoring

Related Issue

Closes #43

leinardi · 2021-12-02T18:22:23Z

Oh wow this seems really interesting! Today I can't check it but I'll try to do it tomorrow, thanks a lot!

leinardi · 2021-12-03T15:00:31Z

Hi @intgr, I tested your branch and it seems to work fine!

I have some questions:

Inspections from ExternalAnnotator now look different

What does it mean that they look different? Because of the different severity levels?

In the "Problems" window, mypy inspections can no longer be grouped by inspection type -- they always appear at the root level.

I also don't get this: form what I saw the "Problems" window before and after your changes looks the same, beside the different severity level (at least with the project I'm testing):

Where can I find this "grooping by inspection type" that you say is not available anymore?

Finally, I saw that this is not checked:

is something else missing?

intgr · 2021-12-04T12:35:35Z

Some screenshots to illustrate the differences:

In the "Problems" window, mypy inspections can no longer be grouped by inspection type -- they always appear at the root level.

Before	After

Inspections from ExternalAnnotator now look different
What does it mean that they look different? Because of the different severity levels?

Yeah, the styles associated with the new severity levels are different.

Before	After

Finally, I saw that this is not checked: [...] is something else missing?

Not that I know of. I just want to do some additional testing and review myself before I'm confident in this.

intgr · 2021-12-04T12:44:08Z

Oh one more thing, the intention actions to suppress Mypy errors, disable inspection etc, is no longer available, and PyCharm suppression syntax is ignored.

Before: (this option is no longer available)

After: (noinspection comment ignored)

I haven't checked how difficult it would be to add back support for this. I guess there are some users relying on this feature. What do you think?

Although it would be better to generate the mypy-native # type: ignore comments instead.

src/main/java/com/leinardi/pycharm/mypy/MypyAnnotator.java

intgr · 2021-12-04T13:09:04Z

There's also this from comment from JetBrains developers: https://intellij-support.jetbrains.com/hc/en-us/community/posts/115000691050/comments/115000579464

we think of inspections as something which can be disabled, annotations - something which can't, something like compiler errors.

Considering that, the LocalInspectionTool API sounds like a better fit in theory. But fixing the performance issues I think outweighs any argument from purism. Even JetBrains keeps breaking this rule with PEP 8 and TypeScript inspections. I think it's just bad design to have so different APIs for this similar functionality. I have lots of other gripes about the API design as well, but there's nothing we can do about it really. :)

leinardi · 2021-12-04T13:40:50Z

I haven't checked how difficult it would be to add back support for this. I guess there are some users relying on this feature. What do you think?

Although it would be better to generate the mypy-native # type: ignore comments instead.

Yeah this is not be best but, as you said, would be better to use the mypy ignore comments.

src/main/java/com/leinardi/pycharm/mypy/checker/Problem.java

intgr · 2021-12-04T14:01:47Z

src/main/java/com/leinardi/pycharm/mypy/checker/Problem.java

+    public HighlightSeverity getHighlightSeverity() {
+        switch (severityLevel) {
+            case ERROR:
+                return HighlightSeverity.ERROR;


I didn't realize this before, but I could switch all inspections to HighlightSeverity.WARNING to retain the previous style (yellow background).

But I've grown to like the red squiggly underline look. It's also familiar to users of IDEA TypeScript integration. I'd keep it HighlightSeverity.ERROR personally.

Ah I totally forgot that severities are configurable in the inspection profile. I suppose using that is preferable. We can revisit this logic in another PR.

Using the `ExternalAnnotator` API (instead of `LocalInspectionTool`) behaves a lot better with slow scanners like mypy and fixes reported performance problems with the plugin (fixes leinardi#43). Following multiple successive changes to a file, `LocalInspectionTool` can invoke the checker for each modification from multiple threads in parallel, which can bog down the system. `ExternalAnnotator` cancels the previous running check (if any) before running the next one. Modeled after `com.jetbrains.python.validation.Pep8ExternalAnnotator`

intgr · 2021-12-04T16:37:17Z

Other than the shortcomings I pointed out in comments and bikeshedding about severity levels, this PR is ready.

In the "Problems" window, mypy inspections can no longer be grouped by inspection type -- they always appear at the root level.
Intention actions ("suppress for statement" etc) no longer appear.
# noinspection Mypy comments no longer suppress the annotations.

leinardi · 2021-12-04T16:55:34Z

Nice! Would it be OK for you if I add you to the Acknowledge section and to the changelog for your contribution to the project?

intgr · 2021-12-04T16:57:02Z

Yes of course. Thanks :)

leinardi · 2021-12-04T17:01:17Z

And do you think a new release can already be made or do you have other changes you want to submit before the next release?

intgr · 2021-12-04T17:04:13Z

I'm looking into how difficult it would be to create an intention for generating # type: ignore comments. If it's not done by tomorrow, it's probably worth releasing without it.

leinardi · 2021-12-06T20:02:30Z

Hi @intgr, unfortunately the review process failed due to this error:

This AnnotationBuilder class seems to not be available on any version prior 2020.1.4:

Shall we just drop those version and change the sinceBuild to 201.8743 or do you have any other suggestion?

intgr · 2021-12-06T20:13:43Z

Hi! There was an alternative API AnnotationHolder.createAnnotation (instead of newAnnotation()). But that one was marked deprecated.

But I don't think it's common for people to use IDEs that are more than a year old. Personally I wouldn't bother with compatibility.

leinardi · 2021-12-06T20:17:53Z

Yeah, CheckstyleIDEA is also supporting only recent IDE versions: https://github.com/jshiell/checkstyle-idea/blob/main/src/main/resources/META-INF/plugin.xml#L19

I'll drop everything pre 2020 👍

leinardi · 2021-12-07T09:29:21Z

Version 0.12.1 is out 🎉
https://plugins.jetbrains.com/plugin/11086-mypy/versions/stable/148936

intgr · 2021-12-07T09:48:39Z

Cool, thanks!

Unfortunately because the 0.12.0 version is hidden, the original release notes aren't visible there. :)

leinardi · 2021-12-07T10:03:04Z

Oh that's a shame, I never had a plugin rejection before so I was not aware of this, sorry :(

This was briefly discussed in #77 (comment) but reverted from that MR. I have found that separating mypy inspections from PyCharm's own type checker is helpful, because PyCharm is more prone to false positives. It's also familiar to users of IDEA TypeScript integration.

This was briefly discussed in leinardi#77 (comment) but reverted from that MR. I have found that separating mypy inspections from PyCharm's own type checker is helpful, because PyCharm is more prone to false positives. It's also familiar to users of IDEA TypeScript integration.

This was briefly discussed in leinardi/mypy-pycharm#77 (comment) but reverted from that MR. I have found that separating mypy inspections from PyCharm's own type checker is helpful, because PyCharm is more prone to false positives. It's also familiar to users of IDEA TypeScript integration.

intgr commented Dec 4, 2021

View reviewed changes

src/main/java/com/leinardi/pycharm/mypy/MypyAnnotator.java Show resolved Hide resolved

intgr commented Dec 4, 2021

View reviewed changes

src/main/java/com/leinardi/pycharm/mypy/checker/Problem.java Outdated Show resolved Hide resolved

intgr commented Dec 4, 2021

View reviewed changes

intgr force-pushed the convert-to-ExternalAnnotator-api branch 2 times, most recently from 1b72306 to ce8e226 Compare December 4, 2021 14:10

intgr added 2 commits December 4, 2021 18:03

Use severity level from inspection profile

27ec03c

intgr force-pushed the convert-to-ExternalAnnotator-api branch from 0df2c86 to 27ec03c Compare December 4, 2021 16:03

intgr marked this pull request as ready for review December 4, 2021 16:37

leinardi merged commit 6062726 into leinardi:master Dec 4, 2021

leinardi mentioned this pull request Dec 4, 2021

Stopping old instances of PyLint when requesting new ones leinardi/pylint-pycharm#11

Closed

intgr mentioned this pull request Dec 5, 2021

Create intention to suppress Mypy errors #81

Merged

4 tasks

leinardi mentioned this pull request Jan 22, 2022

Cancel future when process is canceled #86

Merged

7 tasks

intgr mentioned this pull request Feb 19, 2022

Change default inspection level to ERROR #91

Merged

5 tasks

intgr mentioned this pull request Feb 28, 2022

Update deprecated notifications API usage #93

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve performance, convert real-time scanning to ExternalAnnotator API #77

Improve performance, convert real-time scanning to ExternalAnnotator API #77

intgr commented Dec 2, 2021 •

edited

Loading

leinardi commented Dec 2, 2021

leinardi commented Dec 3, 2021 •

edited

Loading

intgr commented Dec 4, 2021

intgr commented Dec 4, 2021 •

edited

Loading

intgr commented Dec 4, 2021

leinardi commented Dec 4, 2021

intgr Dec 4, 2021

intgr Dec 4, 2021

intgr commented Dec 4, 2021

leinardi commented Dec 4, 2021

intgr commented Dec 4, 2021

leinardi commented Dec 4, 2021

intgr commented Dec 4, 2021

leinardi commented Dec 6, 2021

intgr commented Dec 6, 2021

leinardi commented Dec 6, 2021

leinardi commented Dec 7, 2021

intgr commented Dec 7, 2021

leinardi commented Dec 7, 2021

Improve performance, convert real-time scanning to ExternalAnnotator API #77

Improve performance, convert real-time scanning to ExternalAnnotator API #77

Conversation

intgr commented Dec 2, 2021 • edited Loading

Contributor checklist

Description

Type of Changes

Related Issue

leinardi commented Dec 2, 2021

leinardi commented Dec 3, 2021 • edited Loading

intgr commented Dec 4, 2021

intgr commented Dec 4, 2021 • edited Loading

intgr commented Dec 4, 2021

leinardi commented Dec 4, 2021

intgr Dec 4, 2021

Choose a reason for hiding this comment

intgr Dec 4, 2021

Choose a reason for hiding this comment

intgr commented Dec 4, 2021

leinardi commented Dec 4, 2021

intgr commented Dec 4, 2021

leinardi commented Dec 4, 2021

intgr commented Dec 4, 2021

leinardi commented Dec 6, 2021

intgr commented Dec 6, 2021

leinardi commented Dec 6, 2021

leinardi commented Dec 7, 2021

intgr commented Dec 7, 2021

leinardi commented Dec 7, 2021

intgr commented Dec 2, 2021 •

edited

Loading

leinardi commented Dec 3, 2021 •

edited

Loading

intgr commented Dec 4, 2021 •

edited

Loading