DiamondLightSource / mx-bluesky

Bluesky plans, plan stubs, and utilities for MX beamlines
https://diamondlightsource.github.io/mx-bluesky/
Apache License 2.0
0 stars 2 forks source link

Discuss: Directory Provider for panda #248

Open DominicOram opened 5 months ago

DominicOram commented 5 months ago

Currently Hyperion makes the panda folder and updates the panda directory provider. This makes a lot of assumptions that hyperion has permissions etc. We should come up with a plan for what this looks like long term, likely in discussion with @DiamondJoseph, @callumforrester etc.

callumforrester commented 5 months ago

Is the idea that the provider should be able to verify those assumptions for itself?

DiamondJoseph commented 5 months ago

Not sure how much of this is already covered by the upcoming changes to the DirectoryProvider in ophyd-async that plays nicely with the AreaDetector FilePlugin create-dir-depth https://github.com/bluesky/ophyd-async/blob/6155efe8b2b02c246c2b4e712e77b36eae3a499f/src/ophyd_async/core/_providers.py#L30

DiamondJoseph commented 5 months ago

Re: Files though: AreaDetectors with the file plugin/the PandA IOC will (if they are creating the folder themself) need to be running as a user that can create said folders: which is fine for the detectors, as they are running as a user that can create files in the visit directories and using the file plugin. We don't want blueapi running as a privileged user at all, so if the DirectoryProvider implementation doesn't ever call os.createdir() that's ideal.

callumforrester commented 5 months ago

implementation doesn't ever call os.createdir() that's ideal.

More than ideal, it's critical

DominicOram commented 5 months ago

I think we need another broad discussion on how we want hyperion to handle file/folder writing. There are a number of factors:

callumforrester commented 5 months ago

There may be an argument that hyperion is itself a locked down service, no user should be able to inject code into it, therefore does it matter that it has access across all visits?

I think that's fine, as long as you can't inject code. It's not ideal, the ideal is to make the smallest possible code base run with write permissions. Could have a tiny service/python IOC to actually do it, but I'm happy enough with the final bullet point.

DominicOram commented 5 months ago

It's not ideal, the ideal is to make the smallest possible code base run with write permissions.

I agree with this as a goal but I think we need to think about it practically and how much complexity it might add.

DiamondJoseph commented 5 months ago

PandA output directory creation looks to be being resolved, what's the folder structure intended to look like?

https://github.com/PandABlocks/PandABlocks-ioc/pull/102

DominicOram commented 5 months ago

what's the folder structure intended to look like?

Undetermined, probably something like:

Where data_dir is user defined

GDYendell commented 5 months ago

Historically controls have not been keen for odin to make folders (though this may have changed @GDYendell?)

We just copied areaDetector at the time, which now does support creating directories. It is more complicated in Odin given the multiple writers on different nodes, but I expect it can be done.

I do think it is not ideal that, if hyperion runs a scan with an areaDetector, PandA and Odin detector writing to a directory, they are all going to have to individually check and create the directory with their own implementation of the same logic.

callumforrester commented 5 months ago

@GDYendell if supergraph goes ahead, this logic could live there, what do you think?

GDYendell commented 5 months ago

Yep could do. I think we probably need to go with everything being able to create directories for now?

DiamondJoseph commented 2 months ago

After https://github.com/DiamondLightSource/dodal/pull/764, the PathProvider that is passed to the Panda HDFWriter (and any StandardDetector(s)) takes a path and a number of levels of directory that it may create in order to ensure that path exists. Currently that's set at -1, or create as many as is required.