Remove outdated attach_data_session_metadata

[oliwenmandiamond]

Since the conversion to the DeviceManager, there is still some legacy code that exists in dodal which is no longer needed #1734 (comment). This was pointed out by @EmsArnold when looking at #2050.

I have removed attach_data_session_metadata as no longer needed.

Instructions to reviewer on how to test:

1. Check code removed is correct
2. Check tests pass

Checks for reviewer

• Would the PR title make sense to a scientist on a set of release notes
• If a new device has been added does it follow the standards
• If changing the API for a pre-existing device, ensure that any beamlines using this device have updated their Bluesky plans accordingly
• Have the connection tests for the relevant beamline(s) been run via dodal connect ${BEAMLINE}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.28%. Comparing base (7b8fff3) to head (989f13f).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2201      +/-   ##
==========================================
- Coverage   99.34%   99.28%   -0.06%     
==========================================
  Files         367      366       -1     
  Lines       14396    14367      -29     
==========================================
- Hits        14301    14264      -37     
- Misses         95      103       +8     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[DominicOram]

I'm happy that this can be removed from an SCM+X POV

[tpoliaw]

The path provider methods were left due to this comment. @rtuck99 is this still the case with hyperion?

[rtuck99]

Hi, please can we retain get_path_provider()?

There are some draft PRs for fastCS eiger which update it. Currently they are blocked behind a bunch of other things, but there is still the intention to work on them.

#1706
DiamondLightSource/mx-bluesky#1807

Although, the idea of a single path provider is pretty flawed, there are potentially multiple devices and they may wish to write to different paths (which was the reason for the change).

[oliwenmandiamond]

Hi, please can we retain get_path_provider()?

There are some draft PRs for fastCS eiger which update it. Currently they are blocked behind a bunch of other things, but there is still the intention to work on them.

#1706 DiamondLightSource/mx-bluesky#1807

Although, the idea of a single path provider is pretty flawed, there are potentially multiple devices and they may wish to write to different paths (which was the reason for the change).

Why do they need global path provider though? Is this a bluapi limitation again where you need access to it inside of plans and can't without global state?

[tpoliaw]

please can we retain get_path_provider()?

Is there any reason to keep the get if there is no way to set it? What is it expecting to get back?

[rtuck99]

Hi, please can we retain get_path_provider()?
There are some draft PRs for fastCS eiger which update it. Currently they are blocked behind a bunch of other things, but there is still the intention to work on them.
#1706 DiamondLightSource/mx-bluesky#1807
Although, the idea of a single path provider is pretty flawed, there are potentially multiple devices and they may wish to write to different paths (which was the reason for the change).

Why do they need global path provider though? Is this a bluapi limitation again where you need access to it inside of plans and can't without global state?

If the devices exposed the path provider as something we could access at run time then we wouldn't need to have a global function. The reason we didn't need it before is because we could set the paths on the device signals directly. If ophyd-async exposed path provider on the devices such that plans could update it directly then we wouldn't need a top-level function.

[rtuck99]

please can we retain get_path_provider()?

Is there any reason to keep the get if there is no way to set it? What is it expecting to get back?

The reason we have a getter is because device initialisation in the beamline module requires that we specify a path provider, but at run time the plan has no way to determine it. The devices in ophyd-async do not expose the path provider. Hyperion does not use tiled and writes files directly so needs to be able to specify the location.

[oliwenmandiamond]

please can we retain get_path_provider()?

Is there any reason to keep the get if there is no way to set it? What is it expecting to get back?

The reason we have a getter is because device initialisation in the beamline module requires that we specify a path provider, but at run time the plan has no way to determine it. The devices in ophyd-async do not expose the path provider. Hyperion does not use tiled and writes files directly so needs to be able to specify the location.

Okay, so we have 2 issue. First is that ophyd-async doesn't exposes path_provider directly as this is put behind DetectorDataLogic which is private in StandardDetector. The second issue is also that BlueAPI cannot expose things that are not devices for the beamline when injecting objects in plans (same reason we have global config_client).

@tpoliaw how far along is DiamondLightSource/blueapi#1462? (I don't think we need the obj to support HasName, but we use the function name as the name when defined as a fixture for a beamline in dodal which can be found using inject for plans)

[oliwenmandiamond]

Just a thought @rtuck99, but can you not make your detector in dodal have an explicit public reference to path_provider as a class attribute and then use inject for that device in your plans so you can access the path_provider as a work around for now?

[coretl]

StandardDetector also supports detectors that don't have a path provider, and different path providers for different logics, so I would suggest it's better for the thing that made the detector to keep track of the path provider it passed it

[oliwenmandiamond]

Okay @rtuck99 @tpoliaw @DominicOram, I've updated this to remove only attach_data_session_metadata. I will raise a separate issue and PR to remove the global path provider which can be done at a later date.

[rtuck99]

StandardDetector also supports detectors that don't have a path provider, and different path providers for different logics, so I would suggest it's better for the thing that made the detector to keep track of the path provider it passed it

At the moment blueapi does not offer good facilities for accessing such objects where they are used. Ideally I want to be able to access the path provider at the point where I am setting up the detector. This might be somewhere in a deeply nested subplan.

As I see it the options are:

• Get my path provider from the device (which the subplan has already)
• Register the path provider with the blueapi context and have it injected into the blueapi plan arguments using future functionality (e.g. Allow HasName object to be added to inject of plans blueapi#1462). This would require the path provider to be propagated through the call stack to the consuming function and require changes to all the plans using the detector. Whilst this would be useful for some cases, such as accessing the instrument session probably for this I think it is less useful.
• Register the path provider with a service at beamline creation and use the service to access the path provider in the plan. Unfortunately blueapi and python rest frameworks in general AFAIK don't seem to offer the same kind of injection facilities that typical Java frameworks like Spring offer so without a service registry of some kind you end up with a global function which is get_path_provider().

[oliwenmandiamond]

* Register the path provider with the blueapi context and have it `inject`ed into the blueapi plan arguments using future functionality (e.g. [Allow HasName object to be added to inject of plans blueapi#1462](https://github.com/DiamondLightSource/blueapi/issues/1462)). This would require the path provider to be propagated through the call stack to the consuming function and require changes to all the plans using the detector. Whilst this would be useful for some cases, such as accessing the instrument session probably for this I think it is less useful.

* Register the path provider with a service at beamline creation and use the service to access the path provider in the plan. Unfortunately blueapi and python rest frameworks in general AFAIK don't seem to offer the same kind of injection facilities that typical Java frameworks like Spring offer so without a service registry of some kind you end up with a global function which is `get_path_provider()`.

So my understanding of the these 2 options, this is a deliberate design decision taken by the core team taken long ago. In GDA, you can basically use Finder to find anything on the beamline deep into a function call so the args for the function were basically meaningless as you never actually know what the function is going to need reference to. So this wanted to be changed in new system.

The decision was made so that if you wanted a device, it must be made part of the functions signature, hence inject and you would have to propagate this into your nested plans. This makes it so functions are a lot easier to understand and there's nothing hidden on what it depends on. This also makes the last option unlikely to happen due to this unless it was also using inject. @tpoliaw correct me if wrong? This is also why we should we move away from global state path provider as it goes against this design choice and instead get blueapi to support fixtures with inject