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

Document why we choose to do Device load/save in the plan rather than in the Device #737

Open
coretl opened this issue Jan 20, 2025 · 7 comments
Labels
documentation Improvements or additions to documentation

Comments

@coretl
Copy link
Collaborator

coretl commented Jan 20, 2025

From @canismarko

What is the reason for setting the NDAttributes XML in a plan (ophyd_async.plan_stubs.setup_ndattributes) rather than in the area detector device itself?

Answer:
There are a few reasons we went for setup in plans rather than in the Device, all of them are judgement calls and not particularly clear cut:

  • Automated beamlines typically take the form of "get beamline into known state`, then "run samples as quickly as possible". If you put the setup in a plan, then the devices themselves don't need to do much in stage, which makes things faster and less likely to error
  • We are trying the pattern of "save the baseline settings of a known good setup", then "load a known good with some changes applied". These setup plans may cross devices and are beamline specific. For this reason they are more flexible in a plan.
  • There are multiple bits of logic that can control a single Device. If we embed the setup logic in the Device then we need to have multiple Devices that control the same physical piece of hardware. This is possible, but I think it would get messier then having a single Detector that could be setup in a plan specific way
  • Plans are our way of doing logic that sets multiple signals in a particular order, and load/save requires ordering logic. You can also mutate the messages if you need to adapt a plan's effect without changing the internals. Setup of a Device seems to be an area where this might be useful

The downside of putting it in the plan is working out when to call it. Is this something we could push to queueserver? When it goes from user-controlled to consuming from the queue it runs a "get my beamline into a good state" plan?

@prjemian may have some thoughts on this too

Acceptance Criteria

  • This text appears in an ADR
@canismarko
Copy link

Thanks, @coretl

I can see the value in that. The first point doesn't apply to us, but doesn't hurt either.

The downside of putting it in the plan is working out when to call it.

I think this is the challenge I'm facing. Seems like doing this in the queueserver means that I couldn't use area detectors without a queueserver, or else we have to "just know" that this plan needs to be run when operating directly in an ipython terminal.

I asked some of my group members to work through these steps for usability testing, and it was awkward enough that I want to automate executing the setup_ndattributes() plan somehow.

Maybe I want a wrapper that looks for devices (in the stage message?) that have a nd_attributes() method, then injects the setup_ndattributes() plan with those attributes? Not thrilled about how tightly the decorator and devices are coupled, though. Plus this runs afoul of your 2nd and 3rd points above.

Maybe there are reasons to set ndattributes both in the device class, and as a plan, depending on the situation.

@coretl
Copy link
Collaborator Author

coretl commented Jan 20, 2025

Maybe there are reasons to set ndattributes both in the device class, and as a plan, depending on the situation.

I realize that setup_ndattributes hasn't been converted to produce a Settings object. I think we want the setup plan to look more like #16 (comment):

det_hdf_and_stats = settings_from_yaml(det, "/path/to/det_hdf_and_stats.yaml")

def setup_my_det_plan():
    settings = (
        det_hdf_and_stats
        | ndstats_sum_settings(det.stats)
    )
    yield from apply_settings_if_different(settings, apply_areadetector_settings)

However, I realize that getting people to remember to run this when they start using bluesky is a bit of an ask, especially if you haven't got queueserver on top.

I wonder whether we can have 2 options:

  • If you take the plan based approach you get to load from file, add in some settings that are fixed, and then only apply those that need to change, based on some partitioning that is device specific
  • If you want to embed this in a Device then you just blanket apply a few signal values

I'll try and think of a neat way to do the second while still using the Settings objects created from the plans...

@prjemian
Copy link
Contributor

prjemian commented Jan 20, 2025

Can't you have it both ways? Write as a plan stub and also call it as the session starts (RE(session_setup())). Here's an example.

We're starting to adopt a philosophy of everything is a plan (or plan stub) so users see one, clear way to write their code. (Previously, when we also showed how to create ophyd-style code for command-line use, this proved confusing to all but our expert users.) This philosophy enables easier use of queueserver as an option.

@prjemian
Copy link
Contributor

The example above shows device creation. It could be extended to include setup plan stubs.

@canismarko
Copy link

I think the core issue I'm facing is that I don't want to lock-in the names of data keys that end up in the catalog based on a device name at startup, because starting a second instance of python that runs this startup code but with a different device name will clobber the PVs set by the first instance. APS 25-ID has 3 hutches that can run simultaneously, and detectors that may move between them making this a real possibility.

But maybe my use case is not incompatible with doing this all at startup.

ADHDFWriter.open() creates an HDFDataset for each attribute, and it currently uses the XML attribute name for both the data_key and dataset attributes. To avoid naming collisions in case there are two detectors in one scan and to ensure the dataset names in tiled are clear, I'm setting these NDAttribute names to be f"{detector.name}-element0-dead_time_percent" or similar.

Instead, if I could create HDFDataset objects in .open() for the nd attributes where data_key and dataset are different, then I can set the NDAttributes XML at startup using generic names (e.g. "element0-dead_time_percent") and create the HDFDataset as:

HDFDataset(
    data_key=f"{name}-{datakey}",
    dataset=f"/entry/instrument/NDAttributes/{datakey}",
    ...
)

I think this would give equivalent results in the case of a single python session, and would produce the expected catalog columns in the case of two sessions with the same detector but named differently between the them. One difference would be that the HDF5 datasets produced by the detector don't contain the detector name, but I don't see that being an issue as long as tiled knows where they are.

The way it's written now, I would need to vendor the entire open() method into my device to change that one line.

If it's not reasonable to change this as the default behavior, could the creation of the HDFDatasets for individual attributes be split out into a method, so that I could subclass the HDFWriter for my xspress3 device and make this tweak?

Or maybe there's a reason this won't work that I haven't thought of. I'll test this and see.

@coretl
Copy link
Collaborator Author

coretl commented Jan 21, 2025

We need to support the ability to not have {name} prepended, because we might choose to put beam current in an NDAttribute to be captured on each frame, and that definitely shouldn't have a datakey that starts with the detector name. Maybe we can make it opt-in though? If we change the line to data_key=datakey.format(name=name), then if the datakey is beam-current it stays as is, but if it is {name}-dead-time-percent then it will have the detector name prepended.

Would that work?

The only thing that I'm not sure about is whether areaDetector allows attribute names with { in them, and whether HDF files allow datasets with { in them...

@canismarko
Copy link

Yes, I think this would work!

I tested this on our Xspress IOC. I added a bunch of params like:

NDAttributeParam(name='{name}-element0-deadtime_factor', param='XSP3_CHAN_DTFACTOR', datatype=<NDAttributeDataType.DOUBLE: 'DOUBLE'>, addr=0, description='Chan 0 DTC Factor')

This produces valid XML, and if I set the NDAttributesFile PV and trigger the detector, I get datasets like "primary/external/{name}-element0-deadtime_factor", as expected.

If I then make the change to suggested to the ADHDFWriter.open() and measure again, then the datasets are "primary/external/ge_8element-element0-deadtime_factor" (detector.name == "ge_8element").

@coretl coretl added the documentation Improvements or additions to documentation label Feb 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

3 participants