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

Set higher recusion limit (2**12) for PyPy #1984

Merged
merged 2 commits into from
Jan 29, 2023

Conversation

cdce8p
Copy link
Member

@cdce8p cdce8p commented Jan 29, 2023

Description

#1982 increases the number of recursions to properly determine all call variables. With the default recursion limit of 1000, this can sometimes lead to RecursionErrors, especially on PyPy which seems to need more steps / function calls compared to CPython.

This PR increases the recursion limit to 2**12 (4096). For comparison, mypy uses 2**14 (16.384).

Failing CI run: https://github.com/PyCQA/astroid/actions/runs/4035944885/jobs/6938193475
Successful rerun: https://github.com/PyCQA/astroid/actions/runs/4035944885/jobs/6938237623

The test causing the error is tests/unittest_inference.py::test_recursion_on_inference_tip added in #1392 to test infinite recursion. AFAICT this doesn't happen here, otherwise the test would always fail. The recursion limit is just a bit low.

@cdce8p
Copy link
Member Author

cdce8p commented Jan 29, 2023

Interesting side note
To debug this issue I tested cdce8p#31
The CI results are interesting, although I don't know why exactly they are different.

@DanielNoord
Copy link
Collaborator

Can we set the recursion limit for that single test? I think it would still be nice to make sure we handle the errors correctly.

I didn't know mypy set the limit higher. If I knew that I would have done that much earlier for astroid as well 😄

@cdce8p
Copy link
Member Author

cdce8p commented Jan 29, 2023

Can we set the recursion limit for that single test? I think it would still be nice to make sure we handle the errors correctly.

We could using a fixture and sys.getrecursionlimit / sys.setrecursionlimit. That doesn't solve the issue though, only hides it in our tests.

I didn't know mypy set the limit higher. If I knew that I would have done that much earlier for astroid as well 😄

It was a good idea. However, Windows doesn't seem to like it. Will try only increasing it for PyPy.

@cdce8p cdce8p changed the title Set higher recusion limit (2**14) Set higher recusion limit (2**14) for PyPy Jan 29, 2023
@codecov
Copy link

codecov bot commented Jan 29, 2023

Codecov Report

Merging #1984 (c8c61f1) into main (a0d219c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1984   +/-   ##
=======================================
  Coverage   92.64%   92.64%           
=======================================
  Files          94       94           
  Lines       10891    10894    +3     
=======================================
+ Hits        10090    10093    +3     
  Misses        801      801           
Flag Coverage Δ
linux 92.43% <75.00%> (-0.01%) ⬇️
pypy 80.09% <100.00%> (+<0.01%) ⬆️
windows 92.35% <75.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
astroid/__init__.py 100.00% <100.00%> (ø)

@cdce8p
Copy link
Member Author

cdce8p commented Jan 29, 2023

Let's merge this and see it that resolves the issue.

@cdce8p cdce8p changed the title Set higher recusion limit (2**14) for PyPy Set higher recusion limit (2**12) for PyPy Jan 29, 2023
@cdce8p cdce8p merged commit 68bf7d5 into pylint-dev:main Jan 29, 2023
@cdce8p cdce8p deleted the fix-recursion branch January 29, 2023 15:43
github-actions bot pushed a commit that referenced this pull request Jan 29, 2023
@cdce8p
Copy link
Member Author

cdce8p commented Jan 29, 2023

Let's merge this and see it that resolves the issue.

😞 Doesn't look like it for #1985 and #1977
https://github.com/PyCQA/astroid/actions/runs/4037512022/jobs/6940852383
https://github.com/PyCQA/astroid/actions/runs/4037529842/jobs/6940882729

A rerun still solves it. So it doesn't look like a systematic issue with astroid. Maybe a pytest + PyPy issue? Could be that stack frames don't get released or something. Tbh I don't know enough about it though.

Should we revert this change? An alternative approach could be to just skip the specific test on PyPy. What do you think @DanielNoord?

@DanielNoord
Copy link
Collaborator

I also don't know enough about it. Might also be related to pytest running with coverage? One of the runs seems to fail in coverage.

Anyway, I think reverting and then skipping the test might indeed be best here.

@cdce8p
Copy link
Member Author

cdce8p commented Jan 29, 2023

I also don't know enough about it. Might also be related to pytest running with coverage? One of the runs seems to fail in coverage.

Good guess! I was able to reproduce it locally after adding --cov.

pytest tests/unittest_inference.py --cov -k test_recursion_on_inference_tip

Anyway, I think reverting and then skipping the test might indeed be best here.

I'll prepare the PRs for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants