Fix "dictionary changed size during iteration" #6612

nyabkun · 2022-05-14T03:32:31Z

Type of Changes

	Type
✓	🐛 Bug fix

Description

pyreverse -k -m y -o puml -p manim.camera manim.camera

This command produced the following error.

File "C:\ScoopG\apps\python\current\lib\site-packages\pylint\pyreverse\inspector.py", line 177, in visit_classdef
   for assignattrs in node.instance_attrs.values():
RuntimeError: dictionary changed size during iteration

ManimCE
https://github.com/ManimCommunity/manim

manim 0.15.2
pylint 2.13.8

I'm new here and I don't fully understand the structure of the library.
But at any rate, with this change, I could fix the problem.

coveralls · 2022-05-14T05:53:11Z

Pull Request Test Coverage Report for Build 2340977036

1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 95.353%

Totals
Change from base Build 2338435851:	0.0%
Covered Lines:	16066
Relevant Lines:	16849

💛 - Coveralls

DudeNr33 · 2022-05-14T14:02:43Z

Hi @nyabkun,
thank you for submitting a PR!

In order to review the change I first want to reproduce the crash, as I want to understand why this happens in the first place.
However, I was unable to do so.

The versions I tried this with are:

pylint 2.13.9
astroid 2.11.5
Python 3.9.6 (default, Jul 28 2021, 21:15:06) 
[Clang 12.0.5 (clang-1205.0.22.9)]

And

pylint 2.14.0-b1
astroid 2.11.5
Python 3.10.2 (main, Apr  3 2022, 14:46:07) [Clang 12.0.5 (clang-1205.0.22.9)]

Can you give me the output of pylint --version for your environment and tell me what OS you are running?
Thank you!

Pierre-Sassoulas

To add to what @DudeNr33 said, we would need a regression test before merging.

nyabkun · 2022-05-14T18:34:16Z

@DudeNr33 san, @Pierre-Sassoulas san, Thank you for the response😍

I'm running this command on Windows 10 Home - Powershell 7.2.

C:\Ws\+python\manim-tutorials [main +10 ~0 -0 !]> pylint --version
pylint 2.13.8
astroid 2.11.5
Python 3.10.4 (tags/v3.10.4:9d38120, Mar 23 2022, 23:13:41) [MSC v.1929 64 bit (AMD64)]

C:\Ws\+python\manim-tutorials [main +10 ~0 -0 !]> $PSVersionTable 

Name                           Value
----                           -----
PSVersion                      7.2.0
PSEdition                      Core
GitCommitId                    7.2.0
OS                             Microsoft Windows 10.0.19044
Platform                       Win32NT
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0…}
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0


C:\Ws\+python\manim-tutorials [main +10 ~0 -0 !]> pip list
Package             Version
------------------- ---------
astroid             2.11.5
black               22.3.0
certifi             2021.10.8
charset-normalizer  2.0.12
click               8.1.3
click-default-group 1.2.2
cloup               0.13.1
colorama            0.4.4
colour              0.1.5
commonmark          0.9.1
cycler              0.11.0
decorator           5.1.1
dill                0.3.4
docutils            0.18.1
flake8              4.0.1
fonttools           4.33.3
glcontext           2.3.6
idna                3.3
isort               5.10.1
isosurfaces         0.1.0
kiwisolver          1.4.2
lazy-object-proxy   1.7.1
logilab-common      1.9.5
manim               0.15.2
ManimPango          0.4.1
mapbox-earcut       0.12.11
matplotlib          3.5.2
mccabe              0.6.1
moderngl            5.6.4
moderngl-window     2.4.1
multipledispatch    0.6.0
mypy-extensions     0.4.3
networkx            2.8
numpy               1.22.3
packaging           21.3
pathspec            0.9.0
Pillow              9.1.0
pip                 22.1
plantweb            1.2.1
platformdirs        2.5.2
pycairo             1.21.0
pycodestyle         2.8.0
pydub               0.25.1
pyflakes            2.4.0
pyglet              1.5.23
Pygments            2.12.0
pylint              2.13.8
pyparsing           3.0.9
pyrr                0.10.3
python-dateutil     2.8.2
requests            2.27.1
rich                12.4.1
scipy               1.8.0
screeninfo          0.8
setuptools          58.1.0
six                 1.16.0
skia-pathops        0.7.2
srt                 3.5.2
tomli               2.0.1
tqdm                4.64.0
typing_extensions   4.2.0
urllib3             1.26.9
watchdog            2.1.7
wrapt               1.14.1

C:\Ws\+python\manim-tutorials [main +10 ~0 -0 !]> pyreverse -k -m y -o puml -p manim.camera manim.camera
parsing C:\ScoopG\apps\python\current\lib\site-packages\manim\camera\__init__.py...
parsing C:\ScoopG\apps\python\current\lib\site-packages\manim\camera\camera.py...
parsing C:\ScoopG\apps\python\current\lib\site-packages\manim\camera\mapping_camera.py...
parsing C:\ScoopG\apps\python\current\lib\site-packages\manim\camera\moving_camera.py...
parsing C:\ScoopG\apps\python\current\lib\site-packages\manim\camera\multi_camera.py...
parsing C:\ScoopG\apps\python\current\lib\site-packages\manim\camera\three_d_camera.py...
parsing C:\ScoopG\apps\python\current\lib\site-packages\manim\camera\__init__.py...
Traceback (most recent call last):
  File "C:\ScoopG\apps\python\current\lib\runpy.py", line 196, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "C:\ScoopG\apps\python\current\lib\runpy.py", line 86, in _run_code
    exec(code, run_globals)
  File "C:\ScoopG\apps\python\current\Scripts\pyreverse.exe\__main__.py", line 7, in <module>
  File "C:\ScoopG\apps\python\current\lib\site-packages\pylint\__init__.py", line 44, in run_pyreverse
    PyreverseRun(argv or sys.argv[1:])
  File "C:\ScoopG\apps\python\current\lib\site-packages\pylint\pyreverse\main.py", line 213, in __init__
    sys.exit(self.run(args))
  File "C:\ScoopG\apps\python\current\lib\site-packages\pylint\pyreverse\main.py", line 228, in run
    diadefs = handler.get_diadefs(project, linker)
  File "C:\ScoopG\apps\python\current\lib\site-packages\pylint\pyreverse\diadefslib.py", line 219, in get_diadefs     
    diagrams = DefaultDiadefGenerator(linker, self).visit(project)
  File "C:\ScoopG\apps\python\current\lib\site-packages\pylint\pyreverse\utils.py", line 205, in visit
    self.visit(local_node)
  File "C:\ScoopG\apps\python\current\lib\site-packages\pylint\pyreverse\utils.py", line 202, in visit
    methods[0](node)
  File "C:\ScoopG\apps\python\current\lib\site-packages\pylint\pyreverse\diadefslib.py", line 151, in visit_module    
    self.linker.visit(node)
  File "C:\ScoopG\apps\python\current\lib\site-packages\pylint\pyreverse\utils.py", line 205, in visit
    self.visit(local_node)
  File "C:\ScoopG\apps\python\current\lib\site-packages\pylint\pyreverse\utils.py", line 202, in visit
    methods[0](node)
  File "C:\ScoopG\apps\python\current\lib\site-packages\pylint\pyreverse\inspector.py", line 177, in visit_classdef   
    for assignattrs in node.instance_attrs.values():
RuntimeError: dictionary changed size during iteration

nyabkun · 2022-05-14T23:51:59Z

I did a little debugging and it seems that while looping through attributes, one more attribute was added to the manim.camera.camera.Camera class. Added attribute name is model_matrix.

I haven't looked that closely into the manim library, so I don't know how it's being added at this point.

DudeNr33 · 2022-05-15T07:59:30Z

Thank you for the detailed response!

I could reproduce this issue on my Windows laptop with

pylint 2.6.0
astroid 2.4.2
Python 3.6.5 (v3.6.5:f59c0932b4, Mar 28 2018, 16:07:46) [MSC v.1900 32 bit (Intel)]

However, with Python 3.10.4 and pylint 2.13.8 I was not able to.

pylint 2.13.8
astroid 2.11.5
Python 3.10.4 (tags/v3.10.4:9d38120, Mar 23 2022, 23:13:41) [MSC v.1929 64 bit (AMD64)]

I have to dig a little deeper to see what is going on.

DudeNr33 · 2022-05-15T08:34:32Z

Nevermind, could also reproduce it with the current pylint version when installing manim with pip instead cloning their git repository. But only on Windows, not macOS.

DudeNr33 · 2022-05-15T09:12:46Z

I will use this answer to note down what I could find out during debugging - will update it when I find out more.
This should hopefully help to understand what's really going on and to come up with a suitable regression test.

The instance_attrs dictionary of the ClassDef node changes size when the AssignAttr node for self.pixel_height is handled in handle_assignattr_type, more precisely when calling utils.infer_node(node).
Even more precisely, this happens after calling node.infer() inside pylint.pyreverse.utils.get_annotation(node)
The bug can be reproduced on macOS if the -A flag (all-ancestors) is used. If it is omitted, the model_matrix attribute is present in instance_attrs from the beginning, and the crash does not occur.
Replacing the type of instance_attrs with a UserDict and placing a breakpoint to stop at the exact moment when the model_matrix attribute is added to Camera we can see, that this happens roughly through the following chain of events:
4.1. In order to determine the type of pixel_height, we need to infer its value. This value comes from the config instance, which is imported through a relative import in camera.py (from .. import config, logger).
4.2. For this, the AST for the top level manim package is built. The __init__.py here imports a lot of stuff from various subpackages, including from .scene.moving_camera_scene import *.
4.3. The wildcard import again triggers an import of said module in AstroidBuilder.add_from_names_to_locals.
4.4. Somewhere when determining the MRO, the base class manim.scene.scene.Scene is analyzed
4.5. Finally, Scene.interactive_embed monkey patches self.camera.model_matrix, which triggers AstroidBuilder.delayed_assattr(node).

In conclusion: ~~I need a break~~ it looks like we need a scenario where inferring the type for one instance attribute of our class we want to draw triggers an import of another, not yet analysed module that monkey patches an attribute of our class.

I haven't succeeded to come up with a minimal example yet, though.

DudeNr33 · 2022-05-15T13:06:17Z

Ok, I have a minimal example to reproduce the crash:

Given two files in the same directory:

# File tree.py
from monkey import Monkey


class Tree:
    def __init__(self):
        self.number_of_bananas = 5
        self.inhabitant = Monkey("Steve")  # This will trigger the evaluation of `monkey.py`

# File monkey.py
class Monkey:
    def __init__(self, name):
        from tree import Tree

        self.name = name
        self.tree = Tree()
        self.tree.has_tasty_bananas = True  # This monkey patching will increase the number of items in instance_attrs for `Tree`

When I run pyreverse tree, I get:

% pyreverse tree
parsing /Users/andreas/programming/forks/pylint/.notes/new/tree.py...
Traceback (most recent call last):
  File "/Users/andreas/.pyenv/versions/tmp396/bin/pyreverse", line 33, in <module>
    sys.exit(load_entry_point('pylint', 'console_scripts', 'pyreverse')())
  File "/Users/andreas/programming/forks/pylint/pylint/__init__.py", line 47, in run_pyreverse
    PyreverseRun(argv or sys.argv[1:])
  File "/Users/andreas/programming/forks/pylint/pylint/pyreverse/main.py", line 229, in __init__
    sys.exit(self.run(args))
  File "/Users/andreas/programming/forks/pylint/pylint/pyreverse/main.py", line 244, in run
    diadefs = handler.get_diadefs(project, linker)
  File "/Users/andreas/programming/forks/pylint/pylint/pyreverse/diadefslib.py", line 227, in get_diadefs
    diagrams = DefaultDiadefGenerator(linker, self).visit(project)
  File "/Users/andreas/programming/forks/pylint/pylint/pyreverse/utils.py", line 201, in visit
    self.visit(local_node)
  File "/Users/andreas/programming/forks/pylint/pylint/pyreverse/utils.py", line 201, in visit
    self.visit(local_node)
  File "/Users/andreas/programming/forks/pylint/pylint/pyreverse/utils.py", line 198, in visit
    methods[0](node)
  File "/Users/andreas/programming/forks/pylint/pylint/pyreverse/diadefslib.py", line 171, in visit_classdef
    self.extract_classes(node, anc_level, association_level)
  File "/Users/andreas/programming/forks/pylint/pylint/pyreverse/diadefslib.py", line 113, in extract_classes
    self.add_class(klass_node)
  File "/Users/andreas/programming/forks/pylint/pylint/pyreverse/diadefslib.py", line 77, in add_class
    self.linker.visit(node)
  File "/Users/andreas/programming/forks/pylint/pylint/pyreverse/utils.py", line 198, in visit
    methods[0](node)
  File "/Users/andreas/programming/forks/pylint/pylint/pyreverse/inspector.py", line 193, in visit_classdef
    for assignattrs in node.instance_attrs.values():
  File "/Users/andreas/.pyenv/versions/3.9.6/lib/python3.9/_collections_abc.py", line 868, in __iter__
    for key in self._mapping:
RuntimeError: dictionary changed size during iteration

After applying your fix, it works as intended.

In order to use this as a functional test for pyreverse, I have to update tests/pyreverse/test_pyreverse_functional.py so that it adds the directory of the test file to sys.path.
This is currently not possible, but I will prepare a PR later today.

nyabkun · 2022-05-15T13:17:10Z

Huge thanks for the detailed review, and the research 😊
I would not have been able to find so much detail on my own 🐕

DudeNr33 · 2022-05-15T14:18:45Z

You're welcome!
The analysis actually revealed another subtle bug which also explains why I could only reproduce this issues when installing manim through pip and not when checking out the repository: not the entire analysis of the source code was done with a patched sys.path.

Once #6617 is merged, you can rebase and add a functional test for your issue:

create a new Python file with the contents of the example tree.py under tests/pyreverse/functional/class_diagrams/attributes. Preferably with a name that better matches the underlying problem, like delayed_external_monkey_patching or something like that
add the contents of monkey.py beside it, but make sure to prepend this file with a leading underscore, so our testutils know that this is not a standalone test file itself
add the expected output in mmd format beside the source code. You can simply run pyreverse manually once, check if the contents look good, and then rename the result pyreverse produced.

It would also be great if you can add a ChangeLog and whatsnew entry (doc/whatsnew/2.14.rst)!

nyabkun · 2022-05-15T23:02:40Z

Huge thanks again, @DudeNr33 san. I truly appreciate your kindness!

I have followed the instructions for creating a Functional Test and a ChangeLog Entry.

I did push -f in the process, but if there are any problems, please point them out and correct them.

Pierre-Sassoulas

Just reviewing a little early so @DudeNr33 can merge when he approves, this looks great already. I'll add more monkey / tree / tasty bananas in the doc for #5953 😄

DudeNr33 · 2022-05-16T15:19:57Z

Also looks good from my side!
The failing tests in the CI are because #6617 is not merged yet, and the test suite also tries to treat _monkey.py as a separate functionl test file - once the PR is merged they should pass as well.

DudeNr33 · 2022-05-17T15:47:16Z

@nyabkun #6617 has been merged, you can merge the current main now which should fix the tests. 😊

`pyreverse -k -m y -o puml -p manim.camera manim.camera` This command produced the following error. > File "C:\ScoopG\apps\python\current\lib\site-packages\pylint\pyreverse\inspector.py", line 177, in visit_classdef > for assignattrs in node.instance_attrs.values(): > RuntimeError: dictionary changed size during iteration --- ManimCE https://github.com/ManimCommunity/manim --- I'm new here and I don't fully understand the structure of the library. But at any rate, with this change, I could fix the problem.

nyabkun · 2022-05-17T18:26:18Z

@DudeNr33 san, Thank you.
I've updated the branch by rebase 😀

DudeNr33

👍 Thank you for the contribution!

nyabkun · 2022-05-18T18:06:31Z

Thank you for examining the source code and presenting the test code!
It was a good learning experience 😍😍😍

Pierre-Sassoulas added pyreverse Related to pyreverse component Crash 💥 A bug that makes pylint crash labels May 14, 2022

Pierre-Sassoulas added this to the 2.15.0 milestone May 14, 2022

DanielNoord requested a review from DudeNr33 May 14, 2022 13:35

Pierre-Sassoulas requested changes May 14, 2022

View reviewed changes

DudeNr33 mentioned this pull request May 15, 2022

Run linking and diadef handling with patched sys.path #6617

Merged

2 tasks

nyabkun added a commit to nyabkun/pylint that referenced this pull request May 15, 2022

Update ChangeLog for pylint-dev#6612

cdd11a4

nyabkun added a commit to nyabkun/pylint that referenced this pull request May 15, 2022

Add the functional test for pylint-dev#6612

b9be2e7

nyabkun force-pushed the patch-1 branch from dc42acc to b9be2e7 Compare May 15, 2022 21:52

Pierre-Sassoulas approved these changes May 16, 2022

View reviewed changes

nyabkun and others added 2 commits May 18, 2022 03:13

Update ChangeLog for pylint-dev#6612

0c2a2e1

nyabkun added a commit to nyabkun/pylint that referenced this pull request May 17, 2022

Add the functional test for pylint-dev#6612

b417f72

nyabkun force-pushed the patch-1 branch from 7bbe44c to dcb9597 Compare May 17, 2022 18:13

Add the functional test for pylint-dev#6612

b22f7ed

nyabkun force-pushed the patch-1 branch from dcb9597 to b22f7ed Compare May 17, 2022 18:21

nyabkun added 2 commits May 18, 2022 03:49

disable import-outside-toplevel for test code

d359ce5

Modify .mmd file (Test output)

6c4df63

DudeNr33 modified the milestones: 2.15.0, 2.14.0 May 18, 2022

DudeNr33 approved these changes May 18, 2022

View reviewed changes

DudeNr33 merged commit a6ae75a into pylint-dev:main May 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix "dictionary changed size during iteration" #6612

Fix "dictionary changed size during iteration" #6612

nyabkun commented May 14, 2022

coveralls commented May 14, 2022 •

edited

DudeNr33 commented May 14, 2022

Pierre-Sassoulas left a comment

nyabkun commented May 14, 2022 •

edited

nyabkun commented May 14, 2022 •

edited

DudeNr33 commented May 15, 2022

DudeNr33 commented May 15, 2022

DudeNr33 commented May 15, 2022 •

edited

DudeNr33 commented May 15, 2022

nyabkun commented May 15, 2022

DudeNr33 commented May 15, 2022

nyabkun commented May 15, 2022 •

edited

Pierre-Sassoulas left a comment

DudeNr33 commented May 16, 2022

DudeNr33 commented May 17, 2022

nyabkun commented May 17, 2022

DudeNr33 left a comment

nyabkun commented May 18, 2022

Fix "dictionary changed size during iteration" #6612

Fix "dictionary changed size during iteration" #6612

Conversation

nyabkun commented May 14, 2022

Type of Changes

Description

coveralls commented May 14, 2022 • edited

Pull Request Test Coverage Report for Build 2340977036

💛 - Coveralls

DudeNr33 commented May 14, 2022

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

nyabkun commented May 14, 2022 • edited

nyabkun commented May 14, 2022 • edited

DudeNr33 commented May 15, 2022

DudeNr33 commented May 15, 2022

DudeNr33 commented May 15, 2022 • edited

DudeNr33 commented May 15, 2022

nyabkun commented May 15, 2022

DudeNr33 commented May 15, 2022

nyabkun commented May 15, 2022 • edited

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

DudeNr33 commented May 16, 2022

DudeNr33 commented May 17, 2022

nyabkun commented May 17, 2022

DudeNr33 left a comment

Choose a reason for hiding this comment

nyabkun commented May 18, 2022

coveralls commented May 14, 2022 •

edited

nyabkun commented May 14, 2022 •

edited

nyabkun commented May 14, 2022 •

edited

DudeNr33 commented May 15, 2022 •

edited

nyabkun commented May 15, 2022 •

edited