Ophyd v2 core #1078

coretl · 2022-10-21T14:49:51Z

Add:

Device baseclass
Signal baseclass
EpicsSignal with CA and Sim backends
Demo EPICS Device and IOC to drive it
Magics for some simple plan stubs
Docs and a tutorial
Reasonable test coverage

Add: - Device baseclass - Signal baseclass - EpicsSignal with CA and Sim backends - Demo EPICS Device and IOC to drive it - Magics for some simple plan stubs - Docs and a tutorial - Reasonable test coverage

coretl · 2022-10-21T15:20:05Z

What I'd like reviewed most is the docs. Downlowd and unzip https://github.com/bluesky/ophyd/suites/8898144421/artifacts/407113849 then point a browser at it and look at the User Guide v2 tutorials and how-tos.

coretl · 2022-10-21T15:21:56Z

For some reason GitHub won't let me ask @tacaswell @whs92 and @callumforrester for a review...

coretl · 2022-10-21T15:30:49Z

A couple of tests that worked on 3.9 but don't work on 3.8, will look at this next week...

prjemian · 2022-10-21T19:26:11Z

Since Channel Access PVs remain part of EPICS 7, change EPICS V3 PVs to EPICS Channel Access (V3) PVs

ZLLentz · 2022-10-21T20:27:29Z

I'll give the docs a readthrough, first impressions are very positive

ZLLentz · 2022-10-21T20:31:21Z

Since Channel Access PVs remain part of EPICS 7, change EPICS V3 PVs to EPICS Channel Access (V3) PVs

The top half of the main index that includes this text is actually the existing README of this repo- maybe the README needs a look-over. Not sure if that's in scope for v2 docs.

ZLLentz

The docs are clean and user-friendly to browse, I pointed out a few things I have questions about. I didn't keep reviewing past the docs. There's enough info here to implement and use my own v2 devices, though it feels really weird after doing v1 for so long since there are few similarities.

docs/user_v2/tutorials/installation.rst

ZLLentz · 2022-10-21T20:44:13Z

docs/user_v2/tutorials/using-existing-devices.rst

+
+.. ipython::
+
+    In [1]: mov samp.x 100


Is this the way we want our users to interact with devices directly? Remember that this may be many users' first interactions with a motor device.

This is very different from python syntax to the point of being it's own language.

Agreed. To our users, it's a third syntax to learn (magics, ophyd methods, bluesky plans). And it is context specific to ipython/jupyter command lines, not scriptable. Since I received this pushback from one instrument scientist, I've stopped showing the magics, in favor of the samp.x.move(100) syntax for command lines.

To clarify what the magics actually do, they are a shorthand for running the plan under the run engine. So mov samp.x 100 actually does RE(bps.mov(samp.x, 100)). This should be easily translatable to a plan:

def my_plan(): yield from bps.mov(samp.x, 100)

But I take your point that it is not scriptable and just another syntax to learn.

There's a reason for not implementing motor.move(): asyncio. There are good performance reasons for using it, but it means some changes. Things like signal.read() will now return a co-routine rather than the actual reading, this means you need to do reading = await signal.read() in the bluesky event loop in order to actually execute the code. This means there is no nice way to do the "dual duty" of read() and set() being both callable by the RE and on the commandline.

The options as I see it are:

Magics

from ophyd.v2 import magics magics.register() RE = ... ... In [1]: mov samp.x 100 # calls RE(bps.mov(samp.x, 100))

Helpers

from ophyd.v2.helpers import mov RE = ... ... mov(samp.x, 100) # calls RE(bps.mov(samp.x, 100))

Extra Methods

samp.x.move(100) # calls asyncio.Task(get_bluesky_event_loop()).wait()

Which one?

I'm not a big fan of the extra methods option, it muddies the Device level interface and it made things confusing for me when I started with Ophyd that you can put and set a Signal, and move and set a Motor. I'd prefer to keep the Ophyd Device interface pure Bluesky. It sounds like Magics will be contraversial, so maybe we're best off going with the helpers? It has the added bonus of providing a simple translation to a plan in case those scriptable commands need to be embedded in a bigger plan.

@prjemian, @ZLLentz: do you think mov(samp.x, 100) be be acceptable instead of samp.x.move(100)?

For users that interact with ophyd devices directly (which I understand is not recommended even in v1), the samp.x.move(100) syntax is important because it is easily discoverable for a non-expert. I know that I'm allowed to move a motor because it has a move method, and in IPython I can discover this by hitting tab after samp.x.

It's not a big deal for me if this isn't built into ophyd itself- I'll be adding the method to my motors for my users' preferences, and I accept that my users' preferences won't necessarily line up with the core community.

I do think having something like mov(samp.x, 100) is useful for keeping thing importable/pythonic regardless, but I don't think you should add it for my sake.

I've backed out the tutorial to use just plan syntax while we have a wider discussion on this. I had a go at making a motor.move() method, and this is what I came up with:

def move(self, new_position: float, timeout: float = None): from bluesky.run_engine import call_in_bluesky_event_loop, in_bluesky_event_loop async def do_move(): await self.set(new_position, timeout) assert not in_bluesky_event_loop(), "Will deadlock run engine if run in a plan, use mov(motor, pos) instead" call_in_bluesky_event_loop(do_move())

It will explicitly only work on the CLI as calling within a plan will block the asyncio event loop. The downside is that you don't get CTRL-C stopping the motor, as you're not actually running a plan. We don't have a handle on the RunEngine in this function, but if we did then we could run the plan using it. It feels a bit wrong coupling the Ophyd Devices back up to the run engine in this way though...

Agree that talking to the RE from Ophyd is non-ideal. Makes sense that this functionality should exist and crash if someone tries to use it in a plan. Seems like the best compromise.

ZLLentz · 2022-10-21T20:49:55Z

docs/user_v2/how-to/make-a-device.rst

+can use the `HasReadableSignals` mix-in class:
+
+.. literalinclude:: ../../../ophyd/v2/epicsdemo/__init__.py
+   :pyobject: Mover


I notice that for these v2 Devices, the PV structures are not known until runtime at __init__. Is there any way to inspect the device structures from the class as for a v1 Device? I realize that connection is no longer automatically done in __init__ so this might not matter as much as it does in v1.

There isn't at runtime, but mypy knows about it, so can provide static analysis of your code. It also knows about the types of signal.get_value() which means it can check complicated flyer code which might put and get many signals.

Is there a usecase for introspecting which signals a class would create before it is created?

ophyd v1 exposes some classmethods that I've found useful on occasion e.g.

ophyd/ophyd/device.py

Line 985 in 0e8acb3

def walk_components(cls):

but I can't immediately come up with specific reason that they'd need to be run before creation.

I think I instinctively want these things because instantiating an ophyd v1 device can be slow. So if that's no longer the case then we're in a good spot.

I think I instinctively want these things because instantiating an ophyd v1 device can be slow. So if that's no longer the case then we're in a good spot.

Yes, creating the Devices should be very quick, and they now connect in parallel.

There's enough info here to implement and use my own v2 devices, though it feels really weird after doing v1 for so long since there are few similarities.

What in particular makes it so weird? Is it the lack of Component? Having to write an __init__ method? Do you think it will be something that "grows on you", or is it a step backwards?

I'm keen to get as many of these questions ironed out now as possible before I start making Devices, although I will probably need to iterate after doing areaDetector anyway...

I think ophyd v1 was weird to me at first too, I don't think there's a specific part of the chain here that is lacking or a step backwards. I think this is just a familiarity thing.

ZLLentz · 2022-10-21T20:52:28Z

docs/user_v2/how-to/make-a-device.rst

+
+To make a Device using Ophyd.v2, you need to subclass from the `Device`
+class, create some `Signal` instances, implement :meth:`~Device.connect()` and
+implement some other suitable Bluesky `hardware_interface`.


How would I implement a non-EPICS device? By making special signal variants? I think there is enough interest in that to warrant a call-out in the docs somewhere.

Good call, I think we should make a How-To guide about this. Can you think of a simple usecase for this? @untzag is yaqc-bluesky built on top of Ophyd v1? If so, do you think this might be a usecase to take inspiration from?

My intuition says that this is a good source of inspiration, but also that a maximally simple example would suffice, e.g. a signal that reads from and writes to a file handle.

I started making a FileSignal and read() and set() were easy, but subscribe less so. I can abstract some helpers from EpicsSignal but don't want to abstract too early, so I'd prefer to defer this until we've done Tango support...

prjemian · 2022-10-21T21:31:57Z

I agree with the general flow of the documentation shown here. Very readable.

We're also being asked to comment on code?

I'm very concerned that the complexity shown in the example class Sensor(Configurable, Readable, Device): will add a barrier to further adoption of Bluesky at APS. How much of this can be hidden behind a much simpler interface, such as without type declarations?

coretl · 2022-10-24T11:11:59Z

Since Channel Access PVs remain part of EPICS 7, change EPICS V3 PVs to EPICS Channel Access (V3) PVs

The top half of the main index that includes this text is actually the existing README of this repo- maybe the README needs a look-over. Not sure if that's in scope for v2 docs.

We're planning PVA support too, so I'd prefer to exclude this README from the scope of the review and come back to it when that's done

coretl · 2022-10-24T11:19:28Z

We're also being asked to comment on code?

Yes please, I was aware that people had limited time, so I wanted people to focus on docs and the example user code that was being presented in the docs rather than the "behind-the-scenes" implementation of EpicsSignal for instance, but feel free to comment on anything in the PR.

I'm very concerned that the complexity shown in the example class Sensor(Configurable, Readable, Device): will add a barrier to further adoption of Bluesky at APS. How much of this can be hidden behind a much simpler interface, such as without type declarations?

I've made the Sensor demo Device use the same baseclass as the Mover device that gets rid of the boilerplate read, describe etc. methods. I didn't use it originally as I wanted to build up to it, but I can work the other way (start with the simple class using the helper mix-in, then show how you can implement those methods yourself in a more advanced How-To).

@prjemian please can you comment on the specific lines of ophyd/v2/epicsdemo/__init__.py that you'd like to hide away? Is it the connect() method? Or the explicit types in the EpicsSignal constructor? Or was it the Readable baseclass that has now gone?

coretl · 2022-10-24T11:27:52Z

Converting to Draft until I've actioned all the comments...

coretl · 2022-10-24T11:35:16Z

I'd like to discuss these changes in the community call on Thursday, can people make it to that?

coretl · 2022-10-26T10:52:57Z

@ZLLentz and @prjemian I've simplified the process of making a Device by introducing a SimpleDevice helper class that does a lot of the boilerplate code for you. I also discussed magics with @danielballan and @tacaswell and I think we need a wider discussion on this. Dan was going to try and find a good time. In the spirit of "not inventing a third thing for users to learn", I've converted the tutorial to only use plans through the RunEngine (with a few less characters courtesy of bluesky.utils.register_transform)

Please could you have another look at the tutorial and how-to on the docs and see if this is any better?
https://github.com/bluesky/ophyd/suites/8968599012/artifacts/412343865

- AsyncStatus should always make a Task - SimpleDevice -> StandardReadable - Add wait_for_value - Fix mypy issues - Promote wait to set argument - Add uncacheable read signals to StandardReadable

ZLLentz

Good to get this merged and tried out in a provisional sense

prjemian

Let's merge to test further.

coretl force-pushed the v2-dev branch from 9076150 to 8b3d2a3 Compare October 21, 2022 15:09

Ophyd v2 core

facb4ff

Add: - Device baseclass - Signal baseclass - EpicsSignal with CA and Sim backends - Demo EPICS Device and IOC to drive it - Magics for some simple plan stubs - Docs and a tutorial - Reasonable test coverage

coretl force-pushed the v2-dev branch from 8b3d2a3 to facb4ff Compare October 21, 2022 15:16

coretl requested review from ZLLentz, danielballan, dylanmcreynolds, klauer, mrakitin and prjemian October 21, 2022 15:20

ZLLentz reviewed Oct 21, 2022

View reviewed changes

coretl added 2 commits October 24, 2022 12:08

Fix CA test on py3.8

be3c4f9

Simplify Sensor demo

e2da036

Fix linting

a493086

coretl marked this pull request as draft October 24, 2022 11:26

coretl added 2 commits October 25, 2022 14:21

Fix numpy typing detection on py3.8

447dac9

Address review comments

013153b

coretl added 2 commits November 8, 2022 13:00

Add move() to Mover

28825d4

Replace some asserts

ece67dc

coretl force-pushed the v2-dev branch from 64a40af to ece67dc Compare November 9, 2022 13:13

coretl added 2 commits November 22, 2022 17:24

Support long strings better in CA

161bda8

Review comments

6619966

- AsyncStatus should always make a Task - SimpleDevice -> StandardReadable - Add wait_for_value - Fix mypy issues - Promote wait to set argument - Add uncacheable read signals to StandardReadable

ZLLentz approved these changes Nov 29, 2022

View reviewed changes

prjemian approved these changes Nov 29, 2022

View reviewed changes

coretl mentioned this pull request Dec 6, 2022

Possibility of supporting epicscorelibs for PyPI uploaded wheels epics-base/pvaPy#80

Open

Fixes from review

f440851

coretl marked this pull request as ready for review December 15, 2022 16:20

coretl merged commit a62f939 into master Dec 15, 2022

coretl deleted the v2-dev branch December 15, 2022 16:20

mrakitin mentioned this pull request Jun 22, 2023

DOC: release notes for v1.8.0 #1127

Merged

Ophyd v2 core #1078

Ophyd v2 core #1078

Uh oh!

Conversation

coretl commented Oct 21, 2022

Uh oh!

coretl commented Oct 21, 2022

Uh oh!

coretl commented Oct 21, 2022

Uh oh!

coretl commented Oct 21, 2022

Uh oh!

prjemian commented Oct 21, 2022

Uh oh!

ZLLentz commented Oct 21, 2022

Uh oh!

ZLLentz commented Oct 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZLLentz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prjemian Oct 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coretl Oct 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Magics

Helpers

Extra Methods

Which one?

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZLLentz Oct 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prjemian commented Oct 21, 2022

Uh oh!

coretl commented Oct 24, 2022

Uh oh!

coretl commented Oct 24, 2022

Uh oh!

coretl commented Oct 24, 2022

Uh oh!

coretl commented Oct 24, 2022

Uh oh!

coretl commented Oct 26, 2022

Uh oh!

ZLLentz left a comment

Choose a reason for hiding this comment

Uh oh!

prjemian left a comment

Choose a reason for hiding this comment

ZLLentz commented Oct 21, 2022 •

edited

Loading

prjemian Oct 21, 2022 •

edited

Loading

coretl Oct 24, 2022 •

edited

Loading

ZLLentz Oct 25, 2022 •

edited

Loading