FIX: improve how we interact with the asyncio event loop by danielballan · Pull Request #1125 · bluesky/bluesky

danielballan · 2018-12-07T21:09:38Z

Do not merge. Needs work.

Closes #1062
Closes #1087

TODO:

rename run_matplotlib_qApp, it also handles the non Qt cases (semi-) gracefully
make stop, request_pause, abort, halt use the same pattern as request_suspend
make sure it actually works in a jupyter notebook and figures are responsive
discuss wait_for api changes
remove _delayed_partial which is a poorly implemented version of threading.Timer

danielballan · 2018-12-07T21:11:12Z

This is essentially bait left to nerd-snipe @tacaswell into finishing the Qt integration sketched in #1062.

danielballan · 2018-12-10T22:46:58Z

Status update:

Basic scanning and live-updating plots now works in both IPython and Jupyter (including with tornado > 5). Pause/resume is currently broken.

danielballan · 2018-12-10T23:58:07Z

Now that this PR has rendered install_qt_kicker() unnecessary, it would be nice to do the same for install_nb_kicker(). One way to do this would be to check the matplotlib backend and automatically install the kicker on the "bluesky event loop" if it's nbAgg.

danielballan · 2019-03-10T14:24:47Z

This is I think, now basically correct. Several tests that stopped/restarted the event loop need to be updated, and then we're done.

To surface some in-person comments by @tacaswell --- his work on https://github.com/NSLS-II/mily has surfaces several other ways that we could consider architecting this. The current approach may not be the best one in the long term, though it's unclear yet which approach is best.

This is detail-oriented work that requires focus that is in short supply at the moment, and I propose completing and merging the current approach. Our incompatibility with tornado>=5 is a known stumbling point for users who are evaluating the library.

stuartcampbell · 2019-03-10T16:01:36Z

If this PR makes it possible to work with tornado>5 and is a good 'first attempt' then I would be in favour of merging and then revisiting / refactoring when we are clearer what the best approach is.

tacaswell · 2019-05-01T02:00:36Z

I can locally sometimes get the test to run through without hanging or segfaulting.

tacaswell · 2019-05-01T10:58:42Z

force-pushed to fix ci typo.

tacaswell · 2019-05-11T17:45:46Z

rebased to account for conflicts in .travis.yml.

Will likely amend and force-push the last commit as well.

tacaswell · 2019-05-11T20:29:21Z

The justification for the API change to wait_for is in these two snippets:

Old way:

import asyncio
import time
start = time.monotonic()


async def f1():
    loop = asyncio.get_event_loop()
    ev = asyncio.Event()

    print('launch the task', time.monotonic() - start)
    tsk = loop.create_task(f2(ev.wait()))
    loop.call_later(2, ev.set)
    print('sleep for a bit (1s)', time.monotonic() - start)
    await asyncio.sleep(1)
    print('cancel the task', time.monotonic() - start)
    tsk.cancel()
    print('sleep for a bit', time.monotonic() - start)
    print('really cancelled?: ', tsk.cancelled(), time.monotonic() - start)
    ex = await tsk
    print('victory!, should be ~2s', time.monotonic() - start)
    if ex:
        raise ex


async def f2(fut):
    while True:
        try:
            print('  try to wait', time.monotonic() - start)
            (t,), _ = (await asyncio.wait([fut]))
            print('  and escape later', time.monotonic() - start)
            return t.exception()
        except Exception as ex:
            print('  Should fail once', time.monotonic() - start)
            print(repr(ex))




loop = asyncio.get_event_loop()
loop.run_until_complete(f1())

New way:

import asyncio
import time
start = time.monotonic()


async def f1():
    loop = asyncio.get_event_loop()
    ev = asyncio.Event()

    print('launch the task', time.monotonic() - start)
    tsk = loop.create_task(f2(ev.wait))
    loop.call_later(2, ev.set)
    print('sleep for a bit (1s)', time.monotonic() - start)
    await asyncio.sleep(1)
    print('cancel the task', time.monotonic() - start)
    tsk.cancel()
    print('sleep for a bit', time.monotonic() - start)
    print('really cancelled?: ', tsk.cancelled(), time.monotonic() - start)
    ex = await tsk
    print('victory!, should be ~2s', time.monotonic() - start)
    if ex:
        raise ex


async def f2(fut):
    while True:
        try:
            print('  try to wait', time.monotonic() - start)
            (t,), _ = (await asyncio.wait([fut()]))
            print('  and escape later', time.monotonic() - start)
            return t.exception()
        except Exception as ex:
            print('  Should fail once', time.monotonic() - start)
            print(repr(ex))




loop = asyncio.get_event_loop()
loop.run_until_complete(f1())

The old way "works" on py3.6+ in that the timing is correct, but the inner task still sets a RuntimeError due to trying to re-await a coroutine. In 3.5 this fails immediately (which is why the tests were failing) but it is no less wrong on 3.6+ and there is no garuntee that it won't re-break in the future. The new way works everywhere. I strongly suspect that there is no non-test use of 'wait_for' in the wild (it is under documented and any use of it would likely be in a plan that is less than ideal anyway as I don't see how to make use of 'wait_for' and still have a plan that you can pass through list with no side-effects).

tacaswell · 2019-05-11T20:56:58Z

However

import asyncio
import time
from asyncio.futures import Future
start = time.monotonic()


async def f1():
    loop = asyncio.get_event_loop()
    fut = Future()

    print('launch the task', time.monotonic() - start)
    tsk = loop.create_task(f2(fut))
    loop.call_later(2, fut.set_result, 3)
    print('sleep for a bit (1s)', time.monotonic() - start)
    await asyncio.sleep(1)
    print('cancel the task', time.monotonic() - start)
    tsk.cancel()
    print('sleep for a bit', time.monotonic() - start)
    print('really cancelled?: ', tsk.cancelled(), time.monotonic() - start)
    ex = await tsk
    print('victory!, should be ~2s', time.monotonic() - start)
    if ex:
        raise ex


async def f2(fut):
    while True:
        try:
            print('  try to wait', time.monotonic() - start)
            (t,), _ = (await asyncio.wait([fut]))
            print('  and escape later', time.monotonic() - start)
            return t.exception()
        except Exception as ex:
            print('  Should fail once', time.monotonic() - start)
            print(repr(ex))




loop = asyncio.get_event_loop()
loop.run_until_complete(f1())

Works correctly on all versions so the problem is passing on co-routines as futures. We should have a bit of a think about what to change this API to.

danielballan · 2019-05-16T01:05:44Z

  - pip install -r requirements.txt
  - pip install -r dev-requirements.txt
  - pip install -U ophyd --pre
+  - pip install pyqt5


Should this be added to dev-requirements.txt?

I agree with @danielballan's suggestion.

danielballan · 2019-05-16T01:14:20Z

        Expected signature: f(md)
        Expected return: updated scan_id value

+    during_task : callable, optional


👍 very clear

tacaswell · 2019-05-17T15:50:54Z

#1191 (comment) @prjemian

That comment reminded me that interrupters exist and that this PR may break them. Would you be able to manually test this locally?

We really need to move "interuptors" upstream.

tacaswell · 2019-05-21T22:32:16Z

force-pushed to hide a dumb mistake

codecov-io · 2019-05-21T22:46:39Z

Codecov Report

Merging #1125 into master will increase coverage by 0.08%.
The diff coverage is 97.49%.

@@            Coverage Diff             @@
##           master    #1125      +/-   ##
==========================================
+ Coverage   89.24%   89.33%   +0.08%     
==========================================
  Files          52       52              
  Lines        8600     8766     +166     
==========================================
+ Hits         7675     7831     +156     
- Misses        925      935      +10

Impacted Files	Coverage Δ
bluesky/tests/utils.py	`97.22% <100%> (+0.07%)`	⬆️
bluesky/suspenders.py	`89.15% <100%> (ø)`	⬆️
bluesky/tests/test_scans.py	`100% <100%> (ø)`	⬆️
bluesky/tests/test_new_examples.py	`99.39% <100%> (ø)`	⬆️
bluesky/plan_stubs.py	`96.19% <100%> (ø)`	⬆️
bluesky/tests/test_zmq.py	`80.43% <100%> (ø)`	⬆️
bluesky/tests/test_suspenders.py	`100% <100%> (ø)`	⬆️
bluesky/preprocessors.py	`94.73% <100%> (+0.01%)`	⬆️
bluesky/tests/test_examples.py	`97.89% <100%> (+0.03%)`	⬆️
bluesky/tests/conftest.py	`87.17% <50%> (-7.27%)`	⬇️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72588b7...f75d128. Read the comment docs.

klauer

This looks to be a great improvement, but also a very hard to review one...

Take these comments with a grain of salt, as I'm distractedly reviewing during our weekly meeting ;)

klauer · 2019-05-31T18:04:32Z

        futs, = msg.args
-        yield from asyncio.wait(futs, loop=self.loop, **msg.kwargs)
+        futs = [f() for f in futs]
+        await asyncio.wait(futs, loop=self.loop, **msg.kwargs)


https://docs.python.org/3/library/asyncio-task.html#asyncio.wait
The loop argument is deprecated and scheduled for removal in Python 3.10

Same comment as the other one, want to deal with this in a follow up PR as we need to sort out what the minimum version of python that correctly works without loop is.

klauer · 2019-05-31T18:11:14Z

-    will do nothing.
-
-    It is safe to call this function multiple times.
+    DEPRECATED: This functionality is now handled automatically by default and


klauer · 2019-05-31T18:12:31Z

+                wakeupsn.activated.connect(handleWakeup)
+
+            else:
+                # On Windows, non-blocking anonymous pipe or socket is


Has any interactive testing been done on Windows with this?

We do have at least one collection machine on the floor of NSLS-II, but we haven't tried. I don't know if we have a chance to test it from the PR branch before it gets merged.

this was adapted from tortioushg which does get heavy windows use so I am optimistic.

- make bad sleep longer - make timeout condition less stringent - add more time details to the debugging prints

When we moved the table logic below the plotting we no longer were over-writing the `columns` local which change which code path we went down in the plotting.

tacaswell · 2019-06-03T17:21:40Z

The mac test failures are very confusing as they are UnbouldLocalError s from inside of a list comprehension that works on linux. Going to restart the 3.7 mac on the last passing build.

mrakitin

Here are a few comments for the first part. run_engine.py and tests will go next.

mrakitin · 2019-05-30T16:01:20Z

  - pip install -r requirements.txt
  - pip install -r dev-requirements.txt
  - pip install -U ophyd --pre
+  - pip install pyqt5


I agree with @danielballan's suggestion.

mrakitin · 2019-05-31T17:29:48Z

+
+services:
+  - xvfb
+


I think this section is not needed for testing, as discovered in #1203.

Pushed a commit to see if it works (but am skeptical).

It seems to pass: https://travis-ci.org/bluesky/bluesky/builds/540895701?utm_source=github_status&utm_medium=notification, so it should be OK.

mrakitin · 2019-05-31T17:33:38Z

+                # plot everything, independent or dependent variables
+                self._table = LiveTable(list(self.all_dim_fields) + columns)
+                self._table('start', self._start_doc)
+                self._table('descriptor', doc)


👍
That particular change resolves the issue with arranging live table and live plot in a Jupyter notebook.

mrakitin · 2019-05-31T18:31:03Z

+                wakeupsn.activated.connect(handleWakeup)
+
+            else:
+                # On Windows, non-blocking anonymous pipe or socket is


We do have at least one collection machine on the floor of NSLS-II, but we haven't tried. I don't know if we have a chance to test it from the PR branch before it gets merged.

seems to be due to py 1.8.0 ?

tacaswell · 2019-06-03T18:58:04Z

The issue was due to pytest trying to un-roll assert all(...) which they are in the process of reverting matplotlib/matplotlib#14403 .

tacaswell · 2019-06-04T13:27:49Z

I think this is ready to merge!

tacaswell reviewed Dec 11, 2018

View reviewed changes

Comment thread bluesky/utils.py Outdated

danielballan force-pushed the run-eventloop-continuously branch from bf6c1e6 to 94dcb68 Compare December 14, 2018 22:31

tacaswell added this to the 1.6 milestone Dec 18, 2018

tacaswell mentioned this pull request Jan 28, 2019

Incorrect behaviour: plt.show(block=True) with qt5 and jupyter matplotlib/matplotlib#13302

Closed

danielballan mentioned this pull request Mar 6, 2019

Add rechunk_event_page, rechunk_datum_page. bluesky/event-model#49

Closed

tacaswell force-pushed the run-eventloop-continuously branch from cca48a1 to eb7aa90 Compare April 3, 2019 19:59

tacaswell force-pushed the run-eventloop-continuously branch from 78ee3c5 to c617c47 Compare May 1, 2019 10:58

tacaswell force-pushed the run-eventloop-continuously branch from bac7ce5 to 77d083c Compare May 3, 2019 14:21

danielballan mentioned this pull request May 8, 2019

zmq.asyncio event loop integration #1176

Open

tacaswell force-pushed the run-eventloop-continuously branch from 04f25e7 to 09550e2 Compare May 11, 2019 17:45

tacaswell force-pushed the run-eventloop-continuously branch from 09550e2 to e09e1e4 Compare May 11, 2019 20:54

tacaswell force-pushed the run-eventloop-continuously branch 2 times, most recently from 17a1feb to 93fc013 Compare May 15, 2019 22:14

danielballan commented May 16, 2019

View reviewed changes

tacaswell mentioned this pull request May 16, 2019

MNT: update setup.py for changes to codebase and python 3.7 #1063

Closed

tacaswell force-pushed the run-eventloop-continuously branch from e87fc03 to d6c0fce Compare May 21, 2019 22:31

tacaswell changed the title ~~WIP: improve how we interact with the asyncio event loop~~ FIX: improve how we interact with the asyncio event loop May 21, 2019

tacaswell force-pushed the run-eventloop-continuously branch from 3deaa7c to c30c157 Compare May 31, 2019 02:54

FIX: also support notebook backend

34bbb59

tacaswell force-pushed the run-eventloop-continuously branch from a98ad7d to 34bbb59 Compare May 31, 2019 16:09

ENH: move table stuff below figure stuff to be better with notebook

946b520

klauer reviewed May 31, 2019

View reviewed changes

tacaswell added 13 commits May 31, 2019 18:04

TST: tweak many-hit panic test

7086ce8

- make bad sleep longer - make timeout condition less stringent - add more time details to the debugging prints

CI: move pyqt5 dep to dev-requirements.txt

c937f42

CI: put nightly back

1bec10d

FIX: Don't use invalid input to Matplotlib

b399afd

MNT: move column filtering up in BEC

b4a0f78

When we moved the table logic below the plotting we no longer were over-writing the `columns` local which change which code path we went down in the plotting.

FIX: account for instance being None sometimes in descriptors

c1d099d

DOC: tweaks to RunEngine docstring around blocking_func

e6ab6ff

MNT: simplify request_pause

f00c530

MNT: __interpterer_helper -> __interrupter_helper

e022c19

DOC: clarify in-line comment

3ee2193

DOC: add quote about too-fast system signals

1394467

DOC: correct pep8 in API changes docs

002b45b

MNT: don't use py37+ features

86a9950

mrakitin reviewed Jun 3, 2019

View reviewed changes

tacaswell added 3 commits June 3, 2019 13:38

TST: work around a weird bug in pytest assertion re-writing

be7a4f5

seems to be due to py 1.8.0 ?

BLD: remove python 3.5 from setup.py

f4f3523

CI: try without xvfb service

4fbe224

tacaswell approved these changes Jun 4, 2019

View reviewed changes

danielballan merged commit 530cf02 into bluesky:master Jun 5, 2019

danielballan deleted the run-eventloop-continuously branch June 5, 2019 20:20

tacaswell mentioned this pull request Jun 24, 2019

BS throws error when APS dumps the ring APS-USAXS/usaxs-bluesky-ended-2023#266

Closed

prjemian mentioned this pull request Jun 24, 2019

RuntimeWarning: coroutine 'Event.wait' was never awaited bluesky/ophyd#741

Closed


		services:
		- xvfb

Conversation

danielballan commented Dec 7, 2018 • edited by tacaswell Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danielballan commented Dec 7, 2018

Uh oh!

danielballan commented Dec 10, 2018

Uh oh!

danielballan commented Dec 10, 2018

Uh oh!

Uh oh!

danielballan commented Mar 10, 2019

Uh oh!

stuartcampbell commented Mar 10, 2019

Uh oh!

tacaswell commented May 1, 2019

Uh oh!

tacaswell commented May 1, 2019

Uh oh!

tacaswell commented May 11, 2019

Uh oh!

tacaswell commented May 11, 2019

Uh oh!

tacaswell commented May 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tacaswell commented May 17, 2019

Uh oh!

tacaswell commented May 21, 2019

Uh oh!

codecov-io commented May 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

klauer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tacaswell commented Jun 3, 2019

Uh oh!

mrakitin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

danielballan commented Dec 7, 2018 •

edited by tacaswell

Loading

codecov-io commented May 21, 2019 •

edited

Loading