ENH: Upstream helpful PV Positioners, add wait_for_value util #1046

ZLLentz · 2022-05-10T00:06:22Z

Part of the May 2022 EPICS codeathon effort
Relies on the InternalSignal from #1045, which would need to be merged first

Adds PVPositionerIsClose, a helpful PV positioner for when your IOC doesn't supply a done signal.
Adds PVPositionerDone, a dummy class for when the user wants their signal to "look like" a motor and cannot be dissuaded.
split out the "waiting" part of ophyd.utils.epics_pvs._set_and_wait into its own standalone utility, wait_for_value because I needed it to write a reasonable test

tacaswell · 2022-05-10T15:18:12Z

ophyd/pv_positioner.py

+        if None not in (self._last_readback, self._last_setpoint):
+            is_done = self.done_comparator(self._last_readback,
+                                           self._last_setpoint)
+            self.done.put(int(is_done), internal=True)


Is it worth only updating if it has changed? I am 95% sure that the down-stream callbacks are going to fire on over put, I'm not sure ("not done yet", "not done yet", ... ) or ("done", "done", ...) is interesting to most things that would subscribe to the done status?

Yes, it's worth modifying this so that it only updates when changed. It's easy to do, makes the situation more clear, and removes burden on downstream callbacks. I'll make this change.

tacaswell · 2022-05-10T15:22:17Z

The test failure is from the new tests, but looks like CI related timing issues....

ZLLentz · 2022-05-10T17:12:57Z

I'll modify the test a bit to make it less susceptible to timing errors. I will claim that these tests passed locally every time.

tacaswell · 2022-05-10T17:34:58Z

Would you mind rebasing this?

ZLLentz · 2022-05-10T17:36:07Z

Rebased on current master

ZLLentz · 2022-05-10T18:09:44Z

We're having a strange issue here where the 3.8 build is getting permanently stuck without any apparent text output. Yesterday this happened to a 3.9 build and I only noticed once the runtime had crept up to 2 hours with no activity. I'm not sure how to investigate this.

tacaswell · 2022-05-10T18:39:32Z

I manually cancled it and we got

coverage run --concurrency=thread --parallel-mode -m pytest -v -k "${TEST_CL}"
+ coverage run --concurrency=thread --parallel-mode -m pytest -v -k pyepics
============================= test session starts ==============================
platform linux -- Python 3.8.12, pytest-7.1.2, pluggy-1.0.0 -- /opt/hostedtoolcache/Python/3.8.12/x64/bin/python
cachedir: .pytest_cache
rootdir: /home/runner/work/ophyd/ophyd, configfile: pytest.ini
plugins: rerunfailures-10.2
collecting ... collected 3738 items / 1869 deselected / 1869 selected

ophyd/tests/test_areadetector.py::test_basic[pyepics] PASSED             [  0%]
ophyd/tests/test_areadetector.py::test_stubbing[pyepics] SKIPPED (co...) [  0%]
ophyd/tests/test_areadetector.py::test_detector[pyepics] PASSED          [  0%]
ophyd/tests/test_areadetector.py::test_tiff_plugin[pyepics] PASSED       [  0%]
ophyd/tests/test_areadetector.py::test_hdf5_plugin[pyepics] PASSED       [  0%]
ophyd/tests/test_areadetector.py::test_subclass[pyepics] PASSED          [  0%]
ophyd/tests/test_areadetector.py::test_getattr[pyepics] PASSED           [  0%]
ophyd/tests/test_areadetector.py::test_invalid_plugins[pyepics] PASSED   [  0%]
ophyd/tests/test_areadetector.py::test_validate_plugins_no_portname[pyepics] PASSED [  0%]
ophyd/tests/test_areadetector.py::test_get_plugin_by_asyn_port[pyepics] PASSED [  0%]
ophyd/tests/test_areadetector.py::test_visualize_asyn_digraph_smoke[pyepics] PASSED [  0%]
ophyd/tests/test_areadetector.py::test_read_configuration_smoke[pyepics] PASSED [  0%]
ophyd/tests/test_areadetector.py::test_str_smoke[pyepics] PASSED         [  0%]

which suggests the problem is the next test.

tacaswell · 2022-05-10T18:41:14Z

Which looking at a different run (and assuming they are deterministically orderded) gives us

ophyd/tests/test_areadetector.py::test_default_configuration_smoke[pyepics] PASSED [  0%]

and then a block of area detector tests...which suggests we need better timeouts on those?

ZLLentz · 2022-05-10T18:57:42Z

I think I agree, it must be area detector tests getting stalled out. I'm surprised that GH actions doesn't give us partial text output from these stages and requires a cancellation to see where we are stuck.

I don't think this is related to this PR- I see the same thing in the log statements PR.

ZLLentz · 2022-05-10T18:58:59Z

This actually stalls even earlier in the logs PR:

coverage run --concurrency=thread --parallel-mode -m pytest -v -k "${TEST_CL}"
+ coverage run --concurrency=thread --parallel-mode -m pytest -v -k pyepics
============================= test session starts ==============================
platform linux -- Python 3.9.[12](https://github.com/bluesky/ophyd/runs/6375385206?check_suite_focus=true#step:7:12), pytest-7.1.2, pluggy-1.0.0 -- /opt/hostedtoolcache/Python/3.9.12/x64/bin/python
cachedir: .pytest_cache
rootdir: /home/runner/work/ophyd/ophyd, configfile: pytest.ini
plugins: rerunfailures-10.2
collecting ... collected 3734 items / [18](https://github.com/bluesky/ophyd/runs/6375385206?check_suite_focus=true#step:7:18)67 deselected / 1867 selected

ophyd/tests/test_areadetector.py::test_basic[pyepics] PASSED             [  0%]
ophyd/tests/test_areadetector.py::test_stubbing[pyepics] SKIPPED (co...) [  0%]
ophyd/tests/test_areadetector.py::test_detector[pyepics] PASSED          [  0%]
ophyd/tests/test_areadetector.py::test_tiff_plugin[pyepics] PASSED       [  0%]

klauer

This LGTM overall 👍

ophyd/pv_positioner.py

klauer · 2022-05-12T16:44:10Z

ophyd/utils/epics_pvs.py

+    wait_for_value(signal, val, poll_time=poll_time, timeout=timeout, rtol=rtol, atol=atol)
+
+
+def wait_for_value(signal, val, poll_time=0.01, timeout=10, rtol=None,


set_and_wait is now internal API. Should this be the same?

Need to think on this for a moment. Unlike set_and_wait, wait_for_value should always have the desired behavior in any context: "wait for the value of this signal to be this value", whereas for set_and_wait it had weird interaction specifically with signal set methods, particularly when we need to customize _set_and_wait. I could see an argument for wait_for_value being an overridable method on Signal objects, I'm not sure I'm ready to commit to a change like that in this PR? If that's desirable in the future, it could be wise to preemptively internalize this.

I don't have a feel for how these might be used, if I'm being honest. The only thing I could say is that it's easier to go from private->public than it is public->private in a library.

In the interest of finishing this up for the codeathon, I'm going to make it private and someone else can choose to make it public later if needed.

Co-authored-by: Ken Lauer <klauer@users.noreply.github.com>

tacaswell reviewed May 10, 2022

View reviewed changes

ZLLentz added 12 commits May 10, 2022 10:35

DOC: non-epics internal signal should not have an epics example.

8e56371

ENH: add various pv positioners from pcdsdevices essentially as-is

1a0b79f

ENH: clean up implementation of new pv positioner utility classes

9136888

ENH: add new pv positioner classes to top-level import

0709d6a

DOC: extend docstring

fdd6e9d

ENH: split out the wait part of set_and_wait as a standalone utility.

ba37b7d

TST: add tests for new pv positioner utility classes.

4c95447

BUG: reorder initialization to make the check not error

32d9dbd

TST: fix testing errors

efa407e

ENH: only update done signal if it has changed

63437b5

DOC: adjust docstring slightly for fewer assumptions

40df4b3

TST: update positioner is close tests for consistency

9298ab9

ZLLentz force-pushed the pv-positioner-utils branch from befd6ee to 9298ab9 Compare May 10, 2022 17:35

tangkong mentioned this pull request May 10, 2022

CI: area detector tests need better timeout settings? #1049

Open

ZLLentz requested a review from klauer May 12, 2022 16:35

klauer approved these changes May 12, 2022

View reviewed changes

ZLLentz and others added 2 commits May 12, 2022 10:56

DOC: docstring typo

ab331d2

Co-authored-by: Ken Lauer <klauer@users.noreply.github.com>

DOC: add init parameter docstrings to the pv_positioner utility classes

e07ec91

klauer requested a review from tacaswell May 12, 2022 20:26

MAINT: make _wait_for_value private for now, adjust imports and usage

7870a7d

ZLLentz merged commit 0895f9f into bluesky:master May 13, 2022

ZLLentz deleted the pv-positioner-utils branch May 13, 2022 16:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Upstream helpful PV Positioners, add wait_for_value util #1046

ENH: Upstream helpful PV Positioners, add wait_for_value util #1046

ZLLentz commented May 10, 2022

tacaswell May 10, 2022

ZLLentz May 10, 2022

tacaswell commented May 10, 2022

ZLLentz commented May 10, 2022

tacaswell commented May 10, 2022

ZLLentz commented May 10, 2022

ZLLentz commented May 10, 2022

tacaswell commented May 10, 2022

tacaswell commented May 10, 2022

ZLLentz commented May 10, 2022

ZLLentz commented May 10, 2022

klauer left a comment

klauer May 12, 2022

ZLLentz May 12, 2022

klauer May 12, 2022

ZLLentz May 13, 2022

		wait_for_value(signal, val, poll_time=poll_time, timeout=timeout, rtol=rtol, atol=atol)


		def wait_for_value(signal, val, poll_time=0.01, timeout=10, rtol=None,

ENH: Upstream helpful PV Positioners, add wait_for_value util #1046

ENH: Upstream helpful PV Positioners, add wait_for_value util #1046

Conversation

ZLLentz commented May 10, 2022

tacaswell May 10, 2022

Choose a reason for hiding this comment

ZLLentz May 10, 2022

Choose a reason for hiding this comment

tacaswell commented May 10, 2022

ZLLentz commented May 10, 2022

tacaswell commented May 10, 2022

ZLLentz commented May 10, 2022

ZLLentz commented May 10, 2022

tacaswell commented May 10, 2022

tacaswell commented May 10, 2022

ZLLentz commented May 10, 2022

ZLLentz commented May 10, 2022

klauer left a comment

Choose a reason for hiding this comment

klauer May 12, 2022

Choose a reason for hiding this comment

ZLLentz May 12, 2022

Choose a reason for hiding this comment

klauer May 12, 2022

Choose a reason for hiding this comment

ZLLentz May 13, 2022

Choose a reason for hiding this comment