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

12084 PyPy stack depth check change for 7.3.14 #12083

Merged
merged 11 commits into from
Jan 14, 2024
Merged

Conversation

mattip
Copy link
Contributor

@mattip mattip commented Jan 14, 2024

Scope and purpose

Fixes #12084

PyPy 7.3.14 made stack crawling more compatible with CPython, this broke tests in twisted. Since this is a testing-only fix, does it need a release note?

Contributor Checklist:

This process applies to all pull requests - no matter how small.
Have a look at our developer documentation before submitting your Pull Request.

Below is a non-exhaustive list (as a reminder):

  • The title of the PR should describe the changes and starts with the associated issue number, like “#9782 Remove twisted.news. #1234 Brief description”.
  • A release notes news fragment file was create in src/twisted/newsfragments/ (see: Release notes fragments docs.)
  • The automated tests were updated.
  • Once all checks are green, request a review by leaving a comment that contains exactly the string please review.
    Our bot will trigger the review process, by applying the pending review label
    and requesting a review from the Twisted dev team.

@mattip
Copy link
Contributor Author

mattip commented Jan 14, 2024

There was a timeout in the 3.8-macos-12-default-tests run, but this seems to be happening in other PRs.

@mattip
Copy link
Contributor Author

mattip commented Jan 14, 2024

please review

@adiroiban
Copy link
Member

Thanks Matti for the PR.

So we had #12077 in which we updated the code to work with pypy3.10-v7.3.13

I am not sure what is our policy for pypy support, but I guess that with our limited resourcers we should try to support the latest pypy version.

With that in mind, I think that we can consider this PR as something like Add support for PyPy 7.3.14 on Python 3.10

I will push a commit on this branch to run our tests with 7.3.14

Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks for the fix. Much appreciated.

I only have minor comments.

I left some inline suggestion. I hope @glyph can review and we can have this merged soon.

I can see PyPy test running with Successfully set up PyPy 7.3.14 with Python (3.10.13)
. So the changes in this PR ar tested.

src/twisted/newsfragments/12083.bugfix Outdated Show resolved Hide resolved
src/twisted/internet/defer.py Outdated Show resolved Hide resolved
src/twisted/internet/defer.py Outdated Show resolved Hide resolved
Co-authored-by: Adi Roiban <adiroiban@gmail.com>
@mattip
Copy link
Contributor Author

mattip commented Jan 14, 2024

I am not sure what is our policy for pypy support

Please feel free to ping me directly in this repo whenever PyPy problems crop up. I appreciate the efforts you all do to support PyPy, and will try to make it a priority to help out where I can.

@adiroiban
Copy link
Member

There was a timeout in the 3.8-macos-12-default-tests run, but this seems to be happening in other PRs.

MacOS tests are flaky.

We need to fix that soon as they can be a big annoyance and confusion for contributors.

Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good. Thanks, Matti.

We now run Twisted tests on CPython 3.10 with whatever GitHub Action thinks it's the latest PyPy release.

So we should be able to observe any future changes and we will get in touch.


We don't run tests with unreleased PyPy versions.

For reference, if someone would like to run Twisted tests with main PyPy dev version , there is subset of Twisted tests that can be executed and which should provide good enough coverage

https://github.com/twisted/twisted/actions/runs/7518115938/job/20465098025?pr=12083#step:10:689

On our GitHub Actions, the test run takes 1 minutes...but it mostly tox venv setup. The tests run is about 10 seconds.

@adiroiban
Copy link
Member

adiroiban commented Jan 14, 2024

@glyph looks like codecov.io is flaky again.

Note that we have this PR for review in which the diff coverage is generated using only GitHub Actions.


Please double check that we are ok with the coverage and feel free to merge. Thanks!

I have flagged PR so that we will not forget to merge this before the next release.

@glyph
Copy link
Member

glyph commented Jan 14, 2024

I am going to make a few minor tweaks here (getting the additional arithmetic / comparison out of a very hot loop, moving to a single constant, and re-pinning pypy to .14; we should figure out another way to get notified of pypy upgrades that doesn't break CI for unrelated issues)

Thanks a ton @mattip for figuring this out; great to work with you on it :).

@glyph
Copy link
Member

glyph commented Jan 14, 2024

@glyph looks like codecov.io is flaky again.

This is a real coverage issue. If we drop .13 from the matrix entirely, then we don't cover the old branch. I'm going to run .13 with coverage, and .14 without, since we already have 2 pypy jobs, and that should make codecov happy.

@glyph glyph changed the title pypy/pypy#4850 PyPy stack depth check change for 7.3.14 #12084 PyPy stack depth check change for 7.3.14 Jan 14, 2024
@glyph glyph changed the title #12084 PyPy stack depth check change for 7.3.14 12084 PyPy stack depth check change for 7.3.14 Jan 14, 2024
@glyph glyph merged commit d827ff4 into twisted:trunk Jan 14, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

twisted's test suite fails on pypy 7.3.14 due to stack depth changes
4 participants