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

12061 make twisted.internet.test.test_inlinecb.py compatible with Python 3.13 #12092

Open
wants to merge 27 commits into
base: trunk
Choose a base branch
from

Conversation

eevelweezel
Copy link
Contributor

@eevelweezel eevelweezel commented Jan 25, 2024

Scope and purpose

Fixes #12061 - make twisted.internet.test.test_inlinecb.py compatible with Python 3.13
Fixes #12062

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.

@eevelweezel eevelweezel changed the title 12061 make twisted.inernet.test.test_inlinecb.py compatible with Python 3.13 12061 make twisted.internet.test.test_inlinecb.py compatible with Python 3.13 Jan 25, 2024
@eevelweezel
Copy link
Contributor Author

please review

@adiroiban
Copy link
Member

Thansk for the PR.

Do you have time to help reviewing this PR first #12059 ?

In that PR, python 3.13 automated tests are introduced, so that in future PR we can not only apply changes, but make sure they work on 3.13.

Thanks!

@eevelweezel
Copy link
Contributor Author

eevelweezel commented Jan 30, 2024 via email

@adiroiban
Copy link
Member

Thanks Heather for the PR.

I will give it try.

As part of this PT, we will also need to update the set of tests we run on Python 3.13

I see there is a FIXME for this Issue to enable more tests

It looks like with this change, we should be able to run all the internet tests.

            # FIXME:https://github.com/twisted/twisted/issues/12061
            # Add full twisted.internet

https://github.com/twisted/twisted/blob/trunk/.github/workflows/test.yaml#L146-L150

@eevelweezel
Copy link
Contributor Author

eevelweezel commented Feb 8, 2024

Please call me Weezel. :)
I added this, but there's some environmental difference between my local machine (3.13.a.3 venv on Debian) and 3.13-alpha.3-nodeps-nocov-3.13. Specifically, I see that .github/workflows/test.yaml has coverage disabled, but the output of os.environ includes a variable for COVERAGE.
twisted.trial.unittest.FailTest: b"[('[763 chars] ('CONDA', '/usr/share/miniconda'), ('COVERAGE[6422 chars]4')]" != b"[('[763 chars] ('COLUMNS', '80'), ('CONDA', '/usr/share/mini[6458 chars]4')]"

@adiroiban
Copy link
Member

Thanks Weezel for the update and your help with Python 3.13 support.

Which test is failing on your local dev system?

Do you use tox or you run trial directly?

For the GitHub Actions job, we use tox, as a way of "standardised" run environment.

It's important to have the test pass on local dev environments, but some test are harder to write to account for all the local variations.

self.assertIn("in calling2", tb)
self.assertIn("in calling3", tb)
self.assertNotIn("throwExceptionIntoGenerator", tb)
self.assertIn("calling2", tb)
Copy link
Member

Choose a reason for hiding this comment

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

I didn't had time to check what is going on here... but at first view, this test change doesn't look ok.

It just removes the check...and is similar to not having a test at all.


If it's easier to implement, we can have 2 separate tests.
One for 3.13 and newer and another for 3.12 and older.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've split these out into Python 3.13 and Python 3.12 and below. Still looking into why this check is saying my tests aren't covered by tests.

Copy link
Member

Choose a reason for hiding this comment

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

Still looking into why this check is saying my tests aren't covered by tests.

Thanks for the tests.

On Python 3.13 we don't have test coverage reporting.

Last time I tried, coverage.py was not yet supported on Python 3.13 nedbat/coveragepy#1682

Copy link
Member

Choose a reason for hiding this comment

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

also... slipcover doesn't yet support Python 3.13 ... so for 3.13 we are left uncovered :)

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 changes.
It looks much better.

Can you please check what happended with the trace for calling3 on Python 3.13.

I don't see any assertion for it.

If this is somehow a "feature", please add a comment in the test to explain why calling3 is not in the stacktrace...also, add an explicit assertion to check that calling3 is missing.... if calling3 is expected to be missing.

Thanks again.

src/twisted/internet/test/test_inlinecb.py Outdated Show resolved Hide resolved
src/twisted/internet/test/test_inlinecb.py Outdated Show resolved Hide resolved
src/twisted/internet/test/test_inlinecb.py Outdated Show resolved Hide resolved
src/twisted/internet/test/test_inlinecb.py Outdated Show resolved Hide resolved
src/twisted/internet/test/test_inlinecb.py Outdated Show resolved Hide resolved
src/twisted/internet/test/test_inlinecb.py Outdated Show resolved Hide resolved
f = self.failureResultOf(d)
tb = f.getTraceback()
self.assertIn("in calling", tb)
self.assertIn("yield calling2", tb)
Copy link
Member

Choose a reason for hiding this comment

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

what happens in Python 3.13, why we don't see the trace for calling3?

Copy link
Contributor Author

@eevelweezel eevelweezel Feb 15, 2024

Choose a reason for hiding this comment

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

I think this is just another change in traceback formatting (like the last 2 python releases), but not indicative of a change in behavior. When running it with a debugger, I see the entire callback chain, but only calling and calling2 (the start of the chain and its callback) included in the traceback text.

Screenshot_2024-02-14_20-51-06

Traceback text (apologies for the formatting):

Traceback (most recent call last):
 File "/home/eevel/twisted/src/twisted/python/util.py", line 955, in runWithWarningsSuppressed
 return f(*args, **kwargs)
 File "/home/eevel/twisted/src/twisted/internet/test/test_inlinecb.py", line 265, in test_forwardLotsOfTracebacks_313
 d = calling()
 File "/home/eevel/twisted/src/twisted/internet/defer.py", line 2260, in unwindGenerator
 return _cancellableInlineCallbacks(gen)
 File "/home/eevel/twisted/src/twisted/internet/defer.py", line 2172, in _cancellableInlineCallbacks
 _inlineCallbacks(None, gen, status, _copy_context())
 --- <exception caught here> ---
 File "/home/eevel/twisted/src/twisted/internet/defer.py", line 1999, in _inlineCallbacks
 result = context.run(
 File "/home/eevel/twisted/src/twisted/python/failure.py", line 519, in throwExceptionIntoGenerator
 return g.throw(self.value.with_traceback(self.tb))
 File "/home/eevel/twisted/src/twisted/internet/test/test_inlinecb.py", line 263, in calling
 yield calling2()
builtins.Exception: Error Marker

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added an assertion that calling3 is missing, and updated the docstring to reflect this.

Copy link
Member

Choose a reason for hiding this comment

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

I have reformated your example.
I am not sure what is going on here.
I see that the erroring call is also missing.

It looks like Traceback now only shows the top 2 stacks... I don't know.

I don't see anyting about this mentioned in the release notes https://docs.python.org/3.13/whatsnew/3.13.html

Maybe this is a bug in Python 3.13 ?

Copy link
Member

@adiroiban adiroiban Feb 15, 2024

Choose a reason for hiding this comment

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

If I put a traceback at File "/home/eevel/twisted/src/twisted/internet/defer.py", line 1999, in _inlineCallbacks

I can see the full stack... so this looks like a bug in Twisted.

(Pdb) print(result.getBriefTraceback())
Traceback: <class 'Exception'>: Error Marker
/home/adi/chevah/twisted/src/twisted/internet/defer.py:2004:_inlineCallbacks
/home/adi/chevah/twisted/src/twisted/internet/test/test_inlinecb.py:257:calling3
/home/adi/chevah/twisted/src/twisted/internet/defer.py:2261:unwindGenerator
/home/adi/chevah/twisted/src/twisted/internet/defer.py:2173:_cancellableInlineCallbacks
--- <exception caught here> ---
/home/adi/chevah/twisted/src/twisted/internet/defer.py:2004:_inlineCallbacks
/home/adi/chevah/twisted/src/twisted/internet/test/test_inlinecb.py:253:erroring

(Pdb) c
> /home/adi/chevah/twisted/src/twisted/internet/defer.py(2000)_inlineCallbacks()
-> result = context.run(
(Pdb) print(result.getBriefTraceback())
Traceback: <class 'Exception'>: Error Marker
/home/adi/chevah/twisted/src/twisted/internet/defer.py:2004:_inlineCallbacks
/home/adi/chevah/twisted/src/twisted/internet/test/test_inlinecb.py:261:calling2
/home/adi/chevah/twisted/src/twisted/internet/defer.py:2261:unwindGenerator
/home/adi/chevah/twisted/src/twisted/internet/defer.py:2173:_cancellableInlineCallbacks
--- <exception caught here> ---
/home/adi/chevah/twisted/src/twisted/internet/defer.py:2000:_inlineCallbacks
/home/adi/chevah/twisted/src/twisted/python/failure.py:519:throwExceptionIntoGenerator
/home/adi/chevah/twisted/src/twisted/internet/test/test_inlinecb.py:257:calling3

(Pdb) c
> /home/adi/chevah/twisted/src/twisted/internet/defer.py(2000)_inlineCallbacks()
-> result = context.run(
(Pdb) print(result.getBriefTraceback())
Traceback: <class 'Exception'>: Error Marker
/home/adi/chevah/twisted/src/twisted/internet/defer.py:2004:_inlineCallbacks
/home/adi/chevah/twisted/src/twisted/internet/test/test_inlinecb.py:265:calling1
/home/adi/chevah/twisted/src/twisted/internet/defer.py:2261:unwindGenerator
/home/adi/chevah/twisted/src/twisted/internet/defer.py:2173:_cancellableInlineCallbacks
--- <exception caught here> ---
/home/adi/chevah/twisted/src/twisted/internet/defer.py:2000:_inlineCallbacks
/home/adi/chevah/twisted/src/twisted/python/failure.py:519:throwExceptionIntoGenerator
/home/adi/chevah/twisted/src/twisted/internet/test/test_inlinecb.py:261:calling2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still investigating this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still digging, but I think this might be related to Traceback.exc_type vs Traceback.exc_type_str.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am ruling out TracebackException.exc_type vs exc_type_str as a cause. After digging around in pdb, the codepath appears to be identical, as are the results, until it starts retrieving lines from py3/lib/linecache.getline(). Here are some screenshots of the BdbQuit traceback in 3.12 and 3.13. The 3.13 tb is more sparse overall, and color-highlighted.

Python 3.12:
Screenshot_2024-02-28_22-16-50

Python 3.13:
Screenshot_2024-02-28_22-16-16

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the docstring for the 3.13 test to make the reason for this change more apparent, and changed the name of the first callback in chain for the test_forwardLotsOfTracebacks_* tests to "calling1."

@eevelweezel
Copy link
Contributor Author

needs-review

raise Exception("Error Marker")

@inlineCallbacks
def calling():
Copy link
Member

Choose a reason for hiding this comment

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

this needs to be called calling1 or something more specific.
Otherwise self.assertIn("in calling", tb) will match in callingANYTHIG

and better to use ``self.assertIn("in calling1\n", tb)` to make sure no extra text is added

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the single callback test.

@adiroiban
Copy link
Member

I am not sure what is going on here... Not sure if this is a Twisted bug for 3.13 ...or Python 3.13... or something else.

i can see the full stack for non-generators

11:32 $ cat demo.py 
def erroring():
    raise Exception("Error Marker")

def calling3():
    erroring()

def calling2():
    calling3()

def calling1():
    calling2()

calling1()

(venv-3.13) ✔  ~/chevah/twisted [12061-py3.13 L|✚ 1…2⚑ 3] 
11:33 $ python demo.py 
Traceback (most recent call last):
  File "/home/adi/chevah/twisted/demo.py", line 14, in <module>
    calling1()
  File "/home/adi/chevah/twisted/demo.py", line 12, in calling1
    calling2()
  File "/home/adi/chevah/twisted/demo.py", line 9, in calling2
    calling3()
  File "/home/adi/chevah/twisted/demo.py", line 6, in calling3
    erroring()
  File "/home/adi/chevah/twisted/demo.py", line 3, in erroring
    raise Exception("Error Marker")
Exception: Error Marker

@adiroiban
Copy link
Member

adiroiban commented Feb 15, 2024

There is also another issue. On 3.12, all tests are skipped

12:04 $ python --version
Python 3.12.1

12:04 $ trial twisted.internet.test.test_inlinecb.ForwardTraceBackTests
twisted.internet.test.test_inlinecb
  ForwardTraceBackTests
    test_forwardLotsOfTracebacks_312 ...                              [SKIPPED]
    test_forwardLotsOfTracebacks_313 ...                              [SKIPPED]
    test_forwardTracebacks_312 ...                                    [SKIPPED]
    test_forwardTracebacks_313 ...                                    [SKIPPED]

===============================================================================
[SKIPPED]
Needs Python 3.12 or older

twisted.internet.test.test_inlinecb.ForwardTraceBackTests.test_forwardLotsOfTracebacks_312
twisted.internet.test.test_inlinecb.ForwardTraceBackTests.test_forwardTracebacks_312
===============================================================================
[SKIPPED]
Needs Python 3.13 or newer

twisted.internet.test.test_inlinecb.ForwardTraceBackTests.test_forwardLotsOfTracebacks_313
twisted.internet.test.test_inlinecb.ForwardTraceBackTests.test_forwardTracebacks_313
-------------------------------------------------------------------------------
Ran 4 tests in 0.003s

PASSED (skips=4)

we need something like this ... so that we have the negation of the exact same condition.

diff --git a/src/twisted/internet/test/test_inlinecb.py b/src/twisted/internet/test/test_inlinecb.py
index e730e3daf7..36d4b620cf 100644
--- a/src/twisted/internet/test/test_inlinecb.py
+++ b/src/twisted/internet/test/test_inlinecb.py
@@ -127,7 +127,7 @@ class NonLocalExitTests(TestCase):
 
 
 class ForwardTraceBackTests(SynchronousTestCase):
-    @skipIf(sys.version_info > (3, 12), "Needs Python 3.12 or older")
+    @skipIf(not sys.version_info < (3, 13), "Needs Python 3.12 or older")
     def test_forwardTracebacks_312(self):
         """
         Chained inlineCallbacks are forwarding the traceback information
@@ -177,7 +177,7 @@ class ForwardTraceBackTests(SynchronousTestCase):
         self.assertIn("in calling", tb)
         self.assertIn("Error Marker", tb)
 
-    @skipIf(sys.version_info > (3, 12), "Needs Python 3.12 or older")
+    @skipIf(not sys.version_info < (3, 13), "Needs Python 3.12 or older")
     def test_forwardLotsOfTracebacks_312(self):

@adiroiban
Copy link
Member

Something even better

+
+HAVE_PY3_12_OR_OLDER = sys.version_info < (3, 13)
 class ForwardTraceBackTests(SynchronousTestCase):
-    @skipIf(sys.version_info > (3, 12), "Needs Python 3.12 or older")
+    @skipIf(not HAVE_PY3_12_OR_OLDER, "Needs Python 3.12 or older")
     def test_forwardTracebacks_312(self):
         """
         Chained inlineCallbacks are forwarding the traceback information
@@ -152,7 +154,7 @@ class ForwardTraceBackTests(SynchronousTestCase):
         self.assertIn("in calling", tb)
         self.assertIn("Error Marker", tb)
 
-    @skipIf(sys.version_info < (3, 13), "Needs Python 3.13 or newer")
+    @skipIf(HAVE_PY3_12_OR_OLDER, "Needs Python 3.13 or newer")
     def test_forwardTracebacks_313(self):
         """
         Chained inlineCallbacks are forwarding the traceback information
@@ -177,7 +179,7 @@ class ForwardTraceBackTests(SynchronousTestCase):
         self.assertIn("in calling", tb)
         self.assertIn("Error Marker", tb)
 
-    @skipIf(sys.version_info > (3, 12), "Needs Python 3.12 or older")
+    @skipIf(not HAVE_PY3_12_OR_OLDER, "Needs Python 3.12 or older")
     def test_forwardLotsOfTracebacks_312(self):
         """
         Several Chained inlineCallbacks gives information about all generators.
@@ -225,7 +227,7 @@ class ForwardTraceBackTests(SynchronousTestCase):
         self.assertIn("Error Marker", tb)
         self.assertIn("in erroring", f.getTraceback())
 
-    @skipIf(sys.version_info < (3, 13), "Needs Python 3.13 or newer")
+    @skipIf(HAVE_PY3_12_OR_OLDER, "Needs Python 3.13 or newer")

@itamarst
Copy link
Contributor

itamarst commented Mar 4, 2024

@eevelweezel Thanks for working on this! Is it ready for review again?

@eevelweezel
Copy link
Contributor Author

eevelweezel commented Mar 4, 2024 via email

@itamarst
Copy link
Contributor

itamarst commented Mar 4, 2024

trial twisted.internet segfaults when I run it locally with 3.13alpha4, specifically on twisted.internet.test.test_core.SystemEventTestsBuilder_AsyncioSelectorReactorTests.test_shutdownDisconnectsCleanly. 😢 So I'm going to upgrade the version in GitHub Actions and see what happens when tests run there. If it still segfaults I guess that (1) I will file an upstream bug and (2) trial twisted.internet can't be enabled yet in GHA.

@itamarst
Copy link
Contributor

itamarst commented Mar 5, 2024

Ugh, blocked by the Cython incompatibility. 3.13alpha5 will be out in a week and a bit, so we should update to that when it's released (it will fix the Cython issue I believe) and then we can see if it's still segfaulting.

@eevelweezel
Copy link
Contributor Author

eevelweezel commented Mar 7, 2024

I was looking at #12060 and #12099, and stumbled onto python/cpython#111459, which I think may be preventing failure.Failure._findFailure() from finding the expected information. At least, this seems to be the case for twisted.test.test_failure.ExtendedGeneratorTests.test_findFailureInGenerator, still trying to determine if this is also happening here.

My apologies for asking for review prematurely.

@eevelweezel
Copy link
Contributor Author

eevelweezel commented Mar 9, 2024

Alright, I have verified that this is where it's breaking, both here and in #12099.

On Python3.12, the last instruction executed is YIELD_VALUE:

/home/eevel/twisted/src/twisted/python/failure.py(560)_findFailure()
-> if (not lastFrame.f_code.co_code) or lastFrame.f_code.co_code[
(Pdb)
/home/eevel/twisted/src/twisted/python/failure.py(568)_findFailure()
-> if secondLastTb:
(Pdb)
/home/eevel/twisted/src/twisted/python/failure.py(578)_findFailure()
-> frame = tb.tb_frame.f_back
(Pdb) lastFrame.f_code.co_code[lastTb.tb_lasti]
150
(Pdb) cls._yieldOpcode
150

On Python3.13, the last instruction is RAISE_VARARGS:

/home/eevel/twisted/src/twisted/python/failure.py(560)_findFailure()
-> if (not lastFrame.f_code.co_code) or lastFrame.f_code.co_code[
(Pdb)
(Pdb)
(Pdb) n
/home/eevel/twisted/src/twisted/python/failure.py(561)_findFailure()
-> lastTb.tb_lasti
(Pdb)
/home/eevel/twisted/src/twisted/python/failure.py(560)_findFailure()
-> if (not lastFrame.f_code.co_code) or lastFrame.f_code.co_code[
(Pdb) cls._yieldOpcode
118
(Pdb) n
/home/eevel/twisted/src/twisted/python/failure.py(562)_findFailure()
-> ] != cls._yieldOpcode:
(Pdb) lastFrame.f_code.co_code[lastTb.tb_lasti]
101

I'm still investigating why.

@eevelweezel
Copy link
Contributor Author

I've rerun a bunch of the test scenarios locally with both trunk and this branch on 3.13a5. The segfault on twisted.internet is fixed, but now there's a new segfault in twisted.test. However, the last instruction issue is the same for both twisted.internet and twisted.test: lastFrame.f_code.co_code[lastTb.tb_lasti] != cls._yieldOpcode

@glyph
Copy link
Member

glyph commented Mar 14, 2024

The segfault on twisted.internet is fixed, but now there's a new segfault in twisted.test

It does seem like segfaults are entirely CPython's issue; did you report the new one upstream already?

@eevelweezel
Copy link
Contributor Author

eevelweezel commented Mar 14, 2024 via email

@glyph
Copy link
Member

glyph commented Mar 14, 2024

I haven't yet, but plan to once I have a chance to investigate a bit

Thanks, really appreciate your work here

@eevelweezel
Copy link
Contributor Author

eevelweezel commented Mar 20, 2024

This PR should address the segfault, cython/cython#6084. I'll try again once this merges. This looks relevant as well, python/cpython#108668.

@eevelweezel
Copy link
Contributor Author

Retested with 3.13.0a6, and still seeing the same problems. The segfault under twisted.test is still there, but that's to be expected, as the cython PR still hasn't merged.

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.

Fix twisted.internet.utils for Python 3.13 Fix inlineCallbacks tests on Python 3.13
6 participants