Update testing etc for Python 3.8 #75

altendky · 2020-01-18T18:35:31Z

No description provided.

cdunklau · 2020-02-17T23:36:04Z

appveyor.yml

@@ -26,6 +26,12 @@ environment:
    - TOXENV: py37-defaultreactor, win-py37-qt5reactor, py37-asyncioreactor
      PYTHON: "C:\\Python37-x64"

+    - TOXENV: py38-defaultreactor, win-py38-qt5reactor, py38-asyncioreactor
+      PYTHON: "C:\\Python37"


Shouldn't this be Python38?

sigh bonus, I forgot I even had this open.

Oh right... I started this, noticed the failure and said "wait just a minute here... Twisted doesn't even 'support' 3.8!" So I went and thought hey, that's an easy PR... until I realized you can't PR because of Azure web config. So I walked away and gave up.

cdunklau · 2020-02-18T00:40:55Z

The failure on windows ~~makes no sense to me~~ winds up being obvious after some digging (see below):

Traceback (most recent call last):
  File "c:\projects\pytest-twisted\.tox\py38-asyncioreactor\lib\site-packages\_pytest\main.py", line 192, in wrap_session
    config._do_configure()
  File "c:\projects\pytest-twisted\.tox\py38-asyncioreactor\lib\site-packages\_pytest\config\__init__.py", line 778, in _do_configure
    self.hook.pytest_configure.call_historic(kwargs=dict(config=self))
  File "c:\projects\pytest-twisted\.tox\py38-asyncioreactor\lib\site-packages\pluggy\hooks.py", line 308, in call_historic
    res = self._hookexec(self, self.get_hookimpls(), kwargs)
  File "c:\projects\pytest-twisted\.tox\py38-asyncioreactor\lib\site-packages\pluggy\manager.py", line 93, in _hookexec
    return self._inner_hookexec(hook, methods, kwargs)
  File "c:\projects\pytest-twisted\.tox\py38-asyncioreactor\lib\site-packages\pluggy\manager.py", line 84, in <lambda>
    self._inner_hookexec = lambda hook, methods, kwargs: hook.multicall(
  File "c:\projects\pytest-twisted\.tox\py38-asyncioreactor\lib\site-packages\pluggy\callers.py", line 208, in _multicall
    return outcome.get_result()
  File "c:\projects\pytest-twisted\.tox\py38-asyncioreactor\lib\site-packages\pluggy\callers.py", line 80, in get_result
    raise ex[1].with_traceback(ex[2])
  File "c:\projects\pytest-twisted\.tox\py38-asyncioreactor\lib\site-packages\pluggy\callers.py", line 187, in _multicall
    res = hook_impl.function(*args)
  File "c:\projects\pytest-twisted\.tox\py38-asyncioreactor\lib\site-packages\pytest_twisted.py", line 328, in pytest_configure
    reactor_installers[config.getoption("reactor")]()
  File "c:\projects\pytest-twisted\.tox\py38-asyncioreactor\lib\site-packages\pytest_twisted.py", line 277, in init_asyncio_reactor
    _install_reactor(
  File "c:\projects\pytest-twisted\.tox\py38-asyncioreactor\lib\site-packages\pytest_twisted.py", line 292, in _install_reactor
    reactor_installer()
  File "c:\projects\pytest-twisted\.tox\py38-asyncioreactor\lib\site-packages\twisted\internet\asyncioreactor.py", line 320, in install
    reactor = AsyncioSelectorReactor(eventloop)
  File "c:\projects\pytest-twisted\.tox\py38-asyncioreactor\lib\site-packages\twisted\internet\asyncioreactor.py", line 69, in __init__
    super().__init__()
  File "c:\projects\pytest-twisted\.tox\py38-asyncioreactor\lib\site-packages\twisted\internet\base.py", line 571, in __init__
    self.installWaker()
  File "c:\projects\pytest-twisted\.tox\py38-asyncioreactor\lib\site-packages\twisted\internet\posixbase.py", line 286, in installWaker
    self.addReader(self.waker)
  File "c:\projects\pytest-twisted\.tox\py38-asyncioreactor\lib\site-packages\twisted\internet\asyncioreactor.py", line 151, in addReader
    self._asyncioEventloop.add_reader(fd, callWithLogger, reader,
  File "C:\Python38-x64\Lib\asyncio\events.py", line 501, in add_reader
    raise NotImplementedError
NotImplementedError

That is quite strange. If I'm reading py3.8's code right, on windows asyncio.SelectorEventLoop should be asyncio.windows_events.SelectorEventLoop, which is an alias for asyncio.windows_events._WindowsSelectorEventLoop, which inherits from asyncio.selector_events.BaseSelectorEventLoop, which most definitely implements add_reader.

...hold up. 3.8 made ProactorEventLoop the default for windows, and that link also states:

ProactorEventLoop has the following limitations: The loop.add_reader() and loop.add_writer() methods are not supported.

...and twisted.internet.asyncioreactor.AsyncioSelectorReactor looks like it's only intended to work with a SelectorEventLoop.

Judging by a recent coverage report, AsyncioSelectorReactor should definitely get that add_reader interaction exercised... but it looks like the Twisted folks haven't set up their CI to run py3.8 yet. I did a cursory search through their tickets and PRs and couldn't find any report of this.

Edit: I filed a ticket on Twisted's tracker

cdunklau · 2020-02-18T00:50:44Z

@altendky So I wonder what the reasonable thing to do here is... one option would be to special-case py3.8 in the tests for asyncioreactor by setting the event loop policy to asyncio.WindowsSelectorEventLoopPolicy. It'd probably be a good idea to mention that in the docs somewhere too...

https://twistedmatrix.com/trac/ticket/9766

cdunklau · 2020-02-18T07:24:00Z

pytest_twisted.py

+            import asyncio
+
+            selector_policy = asyncio.WindowsSelectorEventLoopPolicy()
+            asyncio.set_event_loop_policy(selector_policy)


This is a bit extreme, don't you think? There's gotta be a nicer way of dealing with this... Come to think of it, this isn't really pytest_twisted's job at all, and it should probably not try to be clever.

Perhaps get_event_loop, check if it's a selector event loop, then issue a warning otherwise? I think a special case to make the tests pass would be reasonable, but not something in the lib like this.

Oh, I just realize the NotImplementedError happens when we install the reactor, so maybe just trap it and raise a contextual exception? On py3 it would just look like:

def init_asyncio_reactor(): from twisted.internet import asyncioreactor try: _install_reactor( reactor_installer=asyncioreactor.install, reactor_type=asyncioreactor.AsyncioSelectorReactor, ) except NotImplementedError as e: raise RuntimeError( "Failed to install AsyncioSelectorReactor. " "If you're on windows and python 3.8+, you might need to change " "the event loop policy to WindowsSelectorEventLoopPolicy." ) from e

...but that's invalid syntax in py2. On py2, NotImplementedError will still show up in the traceback, so it's not too bad, and to get syntax compat six.raise_from would work, or we could just borrow its MIT-licensed implementation as it would be kinda silly to involve six for one case. IANAL but the phrasing "all copies or substantial portions of the Software" does not seem like it would apply to 11 lines (we would still credit them accordingly anyway, of course).

As long as we support py2, a six dependency seems reasonable. It's not like we need to put a restrictive version requirement on it, I would assume. I'm not totally clear why doing a thing that works in a case where it otherwise won't is particularly bad. When either twisted or asyncio address this in a new version then a version check is added here to limit the application of this. I expected to add docs notes and perhaps a warning if this was actually run. And I suppose a check to see if it's needed.

But sure, just failing is simpler and simple is good. (with a good message on how to solve it) But, it just makes everyone else implement this code themselves. So, I dunno how to balance that.

It is my passionate and not-entirely-uninformed opinion that a library should not muck around with global state, except where it's either somehow scoped to the lib (say, logging.getLogger(__name__)) or really necessary.

This situation is clearly not the former, and IMO not really the latter... the failure case is both easily detectable by pytest-twisted, and has a clear, easy (enough) to describe resolution. The burden to the end user is a matter of two lines at module scope in conftest.py (I think? We should probably test that...), and the benefit is not needing to hunt through other probably-nonsensical errors caused by the global fiddling.

OTOH, my feeling is that the intersection between the userbases of asyncio, twisted, pytest-twisted, and windows is small as it is, and might just vanish entirely when you intersect in Python 3.8 and people who depend on ProactorEventLoop. I wish it would be feasible to get some actual data on that 😐. Then again, the number will probably only increase.

Personal ideology and guessing aside, I still prefer the except NotImplementedError approach over "look at versions" for a more practical reason: version detection sucks. We don't actually care what Python version we're running under for this check, only if the event loop policy will give Twisted an incompatible event loop... and that can happen on pre-3.8.

Maybe the best thing to do for this PR is to not attempt to handle it in the lib at all, and just special-case the tests. We can take another look, and with a working py38 CI, do some experiments in another PR to get a clearer picture of the various ways this issue could be triggered.

I think I'm convinced. In the meantime we see that 3.8 works otherwise. NotImplementedError isn't very specific. :[ I'll see about special casing the tests.

https://twistedmatrix.com/trac/ticket/9766

altendky · 2020-02-18T20:49:56Z

Could also make it a cli option. --reactor= any of asyncio, asyncio-selector, and asyncio-proactor.

cdunklau

LGTM, feel free to merge once appveyor gets up that hill...

...aww, took too long finding that gif, it fell off the track :(

altendky · 2020-02-18T21:11:36Z

time for my local vm...

altendky · 2020-02-18T22:02:49Z

I dunno... so even if I move conftest.py to the root so it gets processed and even when its pytest_configure() runs first everything still fails. If I set the policy in pytest_twisted.pytest_configure() then everything but test_blockon_in_hook_with_asyncio() passes. If I move it into init_asyncio_reactor() then all the tests pass. Somehow outside vs. inside init_asyncio_reactor() matters. :| Anyways, heading out in a few minutes to pick up the kids. I'll look more later I guess.

altendky · 2020-02-18T22:16:19Z

pytest-twisted/testing/test_basic.py

Lines 627 to 634 in 37616a6

    
           def test_blockon_in_hook_with_asyncio(testdir, cmd_opts, request): 
        
               skip_if_reactor_not(request, "asyncio") 
        
               conftest_file = """ 
        
               import pytest_twisted as pt 
        
               from twisted.internet import defer 
        
               def pytest_configure(config): 
        
                   pt.init_asyncio_reactor()

That explains that one I guess. Hooks competing for who gets to go first, or else conftest goes first. So maybe make a function and call it from both the real conftest and in the test. Private or a utility for users to call as well?

But still another layer to figure out...

altendky · 2020-02-19T18:10:02Z

pytest_twisted.py

+def use_asyncio_selector_if_required(config):
+    # https://twistedmatrix.com/trac/ticket/9766
+
+    if (
+        config.getoption("reactor", "default") == "asyncio"
+        and sys.platform == 'win32'
+        and sys.version_info >= (3, 8)
+    ):
+        import asyncio
+
+        selector_policy = asyncio.WindowsSelectorEventLoopPolicy()
+        asyncio.set_event_loop_policy(selector_policy)


I'm writing up some docs on the topic too (and fixed the missing trailing newline) but it feels like there's some thing to provide users with that isn't copy/paste. But, this isn't a pytest-twisted issue... and it could be run regardless of version. I can't decide how I feel about this.

Outdated and didn't have auto-dismiss enabled yet...

cdunklau

🆗

altendky added 3 commits January 6, 2020 12:44

Update testing etc for Python 3.8

6cd9667

Merge branch 'master' into py38

57003d6

Merge branch 'master' into py38

0a11d88

cdunklau reviewed Feb 17, 2020

View reviewed changes

Update appveyor.yml

33a1d1a

altendky added 4 commits February 17, 2020 20:42

Use asyncio.WindowsSelectorEventLoopPolicy for 3.8+

c6eff3d

https://twistedmatrix.com/trac/ticket/9766

Update pytest_twisted.py

adb46b6

Update pytest_twisted.py

d5f5bf8

Update pytest_twisted.py

84fd407

cdunklau reviewed Feb 18, 2020

View reviewed changes

altendky added 3 commits February 18, 2020 05:46

Merge branch 'master' into py38

54bfe45

remove asyncio.WindowsSelectorEventLoopPolicy attempt

08bc017

Use asyncio.WindowsSelectorEventLoopPolicy() for applicable tests

277a4e9

https://twistedmatrix.com/trac/ticket/9766

cdunklau previously approved these changes Feb 18, 2020

View reviewed changes

cdunklau mentioned this pull request Feb 18, 2020

Figure out how to handle asyncioreactor on windows: ProactorEventLoop, loop policies, py38 oh my! #80

Open

altendky and others added 2 commits February 18, 2020 18:56

Trying again for asyncio on windows on py38

c0c9a15

add missing trailing newline

5e84a28

altendky commented Feb 19, 2020

View reviewed changes

altendky added 5 commits February 19, 2020 16:54

Add notes about py3.8/asyncio/proactor to readme

0bdf552

Make the proactor helper _private for now

41ba6e2

Merge branch 'master' into py38

39f9fbb

Add CPython 3.8 to GitHub Actions

22fd7c5

remove whitespace from blank lines

44130c8

altendky requested a review from cdunklau February 19, 2020 22:38

cdunklau approved these changes Feb 19, 2020

View reviewed changes

altendky merged commit d869a7c into master Feb 19, 2020

altendky deleted the py38 branch February 19, 2020 22:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update testing etc for Python 3.8 #75

Update testing etc for Python 3.8 #75

altendky commented Jan 18, 2020

cdunklau Feb 17, 2020

altendky Feb 18, 2020

altendky Feb 19, 2020

cdunklau commented Feb 18, 2020 •

edited

Loading

cdunklau commented Feb 18, 2020

cdunklau Feb 18, 2020 •

edited

Loading

cdunklau Feb 18, 2020

altendky Feb 18, 2020

cdunklau Feb 18, 2020

cdunklau Feb 18, 2020

altendky Feb 18, 2020

altendky commented Feb 18, 2020

cdunklau left a comment •

edited

Loading

altendky commented Feb 18, 2020

altendky commented Feb 18, 2020

altendky commented Feb 18, 2020

altendky Feb 19, 2020

cdunklau left a comment

Update testing etc for Python 3.8 #75

Update testing etc for Python 3.8 #75

Conversation

altendky commented Jan 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdunklau commented Feb 18, 2020 • edited Loading

cdunklau commented Feb 18, 2020

cdunklau Feb 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

altendky commented Feb 18, 2020

cdunklau left a comment • edited Loading

Choose a reason for hiding this comment

altendky commented Feb 18, 2020

altendky commented Feb 18, 2020

altendky commented Feb 18, 2020

Choose a reason for hiding this comment

cdunklau left a comment

Choose a reason for hiding this comment

cdunklau commented Feb 18, 2020 •

edited

Loading

cdunklau Feb 18, 2020 •

edited

Loading

cdunklau left a comment •

edited

Loading