Make our custom pylint plugin use the AST #5332

cognifloyd · 2021-08-17T20:45:19Z

pylint plugins are supposed to inspect code without importing it to avoid side effects like importing eventlet.
In some of my manual pylint runs, I ran into a variety of really odd errors and hangs that were ultimately caused by importing the st2 modules from within the pylint plugin.

So, this PR modifies the plugin to use the astroid AST instead of using __import__.

~~This is a draft while I compare run times with master branch. I'm curious if this will speed things up at all.~~

Fix pylint plugin to rely on ast instead of __import__()
finish pylint api_models plugin so it uses AST only
drop unused import

cognifloyd · 2021-08-17T20:54:35Z

If this change offers any speedups, the speedups are negligible.

CI master - https://github.com/StackStorm/st2/actions/runs/1140506567
py3.6 - make time 3m 26s
py3.8 - make time 2m 55s

CI unrelated PR - https://github.com/StackStorm/st2/actions/runs/1140762169
py3.6 - make time 3m 14s
py3.8 - make time 3m 35s

CI with pylint AST changes (this PR) - https://github.com/StackStorm/st2/actions/runs/1140785614
py3.6 - make time 3m 10s
py3.8 - make time 3m 0s

Kami

I guess LGTM if it works although I preferred older much simpler and shorter solution :)

But yeah in general I also agree on the import time side affects not being good (although in this case it was bit less of an issue since pylint always ran as part of a separate isolated process).

Could you please just gist an example of the error when is displayed if some of the code violates this pylint rule?

cognifloyd · 2021-08-27T17:23:02Z

I have been experimenting with running all of our linting tools via pants instead of via the Makefile. In doing so, I ran pylint over a lot more st2 code than the Makefile does. #5325 is a direct result of my running pylint on more code. I'm trying to isolate the changes from that pants experiment that can be upstreamed whether or not I/we continue with that experiment. Thus the original PR description did not mention pants as I was trying to justify this PR without reference to pants.

Here are the conversations from when I was debugging the pylint issues I was hitting.

where I started asking for help debugging my pylint erors (14 June 2021): https://pantsbuild.slack.com/archives/C046T6T9U/p1623727751367600
modified the plugin to use ast instead of import (11 July 2021): https://pantsbuild.slack.com/archives/C046T6T9U/p1625970571218000

Here's a gist of the pants + pylint output with lots of weird import errors. https://gist.github.com/cognifloyd/f428a423a07e1e859d4e2625c0cd66f1#file-pants-lint-st2api
Ultimately, pants looks at the imports, and only includes direct dependencies in the snapshot of files that it bundles up and then runs pylint on. For pylint, pants does not include any transitive dependencies, so attempting to __import__ those files results in these huge ugly errors.

A shorter/trimmed version of that error:

************* Module st2api.controllers.v1.actionalias
st2api/st2api/controllers/v1/actionalias.py:1:0: F0002: <class 'ImportError'>: cannot import name 'date' (astroid-error)
************* Module st2api.controllers.v1.actionexecutions
st2api/st2api/controllers/v1/actionexecutions.py:1:0: F0002: <class 'ImportError'>: cannot import name 'date' (astroid-error)
************* Module st2api.controllers.v1.actions
st2api/st2api/controllers/v1/actions.py:1:0: F0002: <class 'ImportError'>: cannot import name 'mongoescape' (astroid-error)
************* Module st2api.controllers.v1.auth
st2api/st2api/controllers/v1/auth.py:1:0: F0002: <class 'ImportError'>: cannot import name 'date' (astroid-error)
************* Module st2api.controllers.v1.keyvalue
st2api/st2api/controllers/v1/keyvalue.py:1:0: F0002: <class 'ModuleNotFoundError'>: No module named 'st2common.logging' (astroid-error)

Traceback (most recent call last):
  File "/home/cognifloyd/.cache/pants/named_caches/pex_root/venvs/short/fa0fe65b/lib64/python3.6/site-packages/pylint/lint/pylinter.py", line 1044, in get_ast
    return MANAGER.ast_from_file(filepath, modname, source=True)
  File "/home/cognifloyd/.cache/pants/named_caches/pex_root/venvs/short/fa0fe65b/lib64/python3.6/site-packages/astroid/manager.py", line 98, in ast_from_file
    return AstroidBuilder(self).file_build(filepath, modname)
...
  File "/home/cognifloyd/.cache/pants/named_caches/pex_root/venvs/short/fa0fe65b/lib64/python3.6/site-packages/astroid/transforms.py", line 90, in <listcomp>
    module.body = [self._visit(child) for child in module.body]
  File "/home/cognifloyd/.cache/pants/named_caches/pex_root/venvs/short/fa0fe65b/lib64/python3.6/site-packages/astroid/transforms.py", line 58, in _visit
    return self._transform(node)
  File "/home/cognifloyd/.cache/pants/named_caches/pex_root/venvs/short/fa0fe65b/lib64/python3.6/site-packages/astroid/transforms.py", line 41, in _transform
    ret = transform_func(node)
  File "__plugins/api_models.py", line 49, in transform
    module = __import__(module_name, fromlist=[class_name])

Some of the errors in that gist were caused by: pylint-dev/pylint#2095 (where in pylint's cache st2api/bin/st2api shadowed st2api/st2api). I ended up excluding bins like that (not running pylint on them) to work around the issue. But I still ran into the issues with __import__.

So, this PR makes our testing infrastructure more flexible, by allowing people to run pylint on more of our code and in more contexts.

edit: I don't have any tracebacks/gists from the times I ran into an eventlet triggered hang, and I'm not sure if I encountered it while running pylint or something else. I debugged all of this a couple months ago.

cognifloyd · 2021-09-13T21:55:05Z

Rebased on master. @Kami Is anything left before we can merge this? Could you review (and hopefully approve)?

m4dcoder · 2021-10-01T21:56:05Z

@cognifloyd How do you run pylint manually? I have not run into issue or have seen the errors you posted above. The changes here looks more complex than before.

cognifloyd · 2021-10-02T01:13:04Z

Importing code that we are linting is not nice. It breaks the promise of pylint because it is no longer static code analysis, it is reflection.

I have been experimenting running pylint via another program called pants, because it promises to have aggressive caching of results to speed things up. It understands python imports, and so it can track and see that if you change a file b and file a imports file b, then it needs to rerun lint and test steps on both file a and file b, not just the one file you changed. That could really speed up PR CI. Plus, I had one company suggest they would give StackStorm an open source plan to use their remote execution services which would allow us to share cached results between CI and local, so that the tests are that much faster both in CI and locally.

However, because our plugin is using __import__ there is no way to know statically that linting one file will require so many other transient dependencies, not because it uses them, but because of all of the import side effects.

cognifloyd · 2021-10-02T01:15:07Z

I guess I failed in my goal of justifying this without referencing pants. I guess I'll close this until I'm ready to demonstrate pants.

cognifloyd · 2021-10-02T08:37:51Z

OK. I'm adding a bunch of comments to this code to explain how it works and what it is doing.

winem

This looks good to me and I think it's a great improvement. But I'd love to see more reviews by other maintainers.

cognifloyd · 2021-10-08T16:31:50Z

I added tests for the pylint plugin.

cognifloyd · 2021-10-08T17:03:21Z

pylint_plugins/api_models.py

+        # Create a "property = node" assign node
+        assign_node = nodes.Assign(parent=cls)
+        assign_name_node = nodes.AssignName(property_name, parent=assign_node)
+        assign_node.postinit(targets=[assign_name_node], value=node)
+
+        # Finally, add the property node as an attribute on the class.
+        cls.locals[property_name] = [assign_name_node]


In writing tests, I realized that we were modifying the AST incorrectly in the original plugin. This is the correct way to do it.

pull-request-size bot added the size/L PR that changes 100-499 lines. Requires some effort to review. label Aug 17, 2021

cognifloyd added maintenance refactor RFR labels Aug 17, 2021

cognifloyd added this to the 3.6.0 milestone Aug 17, 2021

cognifloyd marked this pull request as ready for review August 17, 2021 20:55

cognifloyd requested a review from Kami August 17, 2021 20:58

Kami reviewed Aug 27, 2021

View reviewed changes

cognifloyd requested a review from Kami August 27, 2021 20:44

cognifloyd added 3 commits September 13, 2021 16:53

Fix pylint plugin to rely on ast instead of __import__()

c2a0d12

finish pylint api_models plugin so it uses AST only

75de9f5

drop unused import

9f0fdc7

cognifloyd force-pushed the pylint-plugins-use-ast branch from 537578d to 9f0fdc7 Compare September 13, 2021 21:53

Merge branch 'master' into pylint-plugins-use-ast

a07029d

arm4b requested a review from m4dcoder October 1, 2021 19:57

cognifloyd closed this Oct 2, 2021

cognifloyd removed this from the 3.6.0 milestone Oct 2, 2021

cognifloyd removed the RFR label Oct 2, 2021

cognifloyd added 4 commits October 2, 2021 02:54

pylint plugin: add overview of plugin documentation

e79b443

pylint plugin: reduce indentation

5ecab18

pylint plugin: Add comments explaining our use of AST

1891fb7

pylint_plugins: reformat with black

071e160

cognifloyd reopened this Oct 2, 2021

cognifloyd added 2 commits October 2, 2021 03:38

Merge branch 'master' into pylint-plugins-use-ast

595d3d6

pylint plugin: drop comment header

538d51d

winem approved these changes Oct 2, 2021

View reviewed changes

cognifloyd added 4 commits October 7, 2021 20:55

use the var name SKIPLIST

d4f0440

pylint plugin: better use of astroid API

455f20a

pylint plugin: more accurate use of cls.locals

70973e0

pylint plugin: Add tests for api_models plugin

5ede946

pull-request-size bot added size/XL PR that changes 500-999 lines. Consider splitting work into several ones that easier to review. and removed size/L PR that changes 100-499 lines. Requires some effort to review. labels Oct 8, 2021

cognifloyd added 3 commits October 8, 2021 11:24

pylint plugin: Add license block to new file

6553164

makefile fix

34fa4a1

Merge branch 'master' into pylint-plugins-use-ast

3c98617

makefile fix

ae2783e

cognifloyd commented Oct 8, 2021

View reviewed changes

arm4b removed the tooling label Oct 11, 2021

cognifloyd added 2 commits October 14, 2021 18:25

Add pylint test for api_models pylint plugin

2d5d3b6

reformat with black

75e18a0

m4dcoder approved these changes Oct 19, 2021

View reviewed changes

Merge branch 'master' into pylint-plugins-use-ast

d67c7a6

cognifloyd merged commit c917635 into StackStorm:master Oct 19, 2021

cognifloyd added the pantsbuild label Aug 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make our custom pylint plugin use the AST #5332

Make our custom pylint plugin use the AST #5332

cognifloyd commented Aug 17, 2021 •

edited

Loading

cognifloyd commented Aug 17, 2021

Kami left a comment

cognifloyd commented Aug 27, 2021 •

edited

Loading

cognifloyd commented Sep 13, 2021

m4dcoder commented Oct 1, 2021

cognifloyd commented Oct 2, 2021

cognifloyd commented Oct 2, 2021

cognifloyd commented Oct 2, 2021

winem left a comment

cognifloyd commented Oct 8, 2021

cognifloyd Oct 8, 2021

Make our custom pylint plugin use the AST #5332

Make our custom pylint plugin use the AST #5332

Conversation

cognifloyd commented Aug 17, 2021 • edited Loading

cognifloyd commented Aug 17, 2021

Kami left a comment

Choose a reason for hiding this comment

cognifloyd commented Aug 27, 2021 • edited Loading

cognifloyd commented Sep 13, 2021

m4dcoder commented Oct 1, 2021

cognifloyd commented Oct 2, 2021

cognifloyd commented Oct 2, 2021

cognifloyd commented Oct 2, 2021

winem left a comment

Choose a reason for hiding this comment

cognifloyd commented Oct 8, 2021

cognifloyd Oct 8, 2021

Choose a reason for hiding this comment

cognifloyd commented Aug 17, 2021 •

edited

Loading

cognifloyd commented Aug 27, 2021 •

edited

Loading