Artemis 262 move arming out of stage #43

olliesilvester · 2023-04-28T16:25:56Z

Fixes Artemis issue #262

codecov · 2023-04-28T16:38:54Z

Codecov Report

Merging #43 (4497b1d) into main (374e513) will increase coverage by 0.30%.
The diff coverage is 98.92%.

@@            Coverage Diff             @@
##             main      #43      +/-   ##
==========================================
+ Coverage   87.71%   88.02%   +0.30%     
==========================================
  Files          54       54              
  Lines        1767     1796      +29     
==========================================
+ Hits         1550     1581      +31     
+ Misses        217      215       -2

Impacted Files	Coverage Δ
src/dodal/devices/eiger.py	`99.30% <98.41%> (-0.70%)`	⬇️
src/dodal/devices/status.py	`100.00% <100.00%> (ø)`
src/dodal/devices/utils.py	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

DominicOram

Thanks! With the requested changes it looks like a good start that we could at least test on the beamline

DominicOram · 2023-05-02T08:47:53Z

src/dodal/devices/eiger.py

+    class ArmingSignal(Signal):
+        def set(self, value, *, timeout=None, settle_time=None, **kwargs):
+            return self.parent.stage()


Must: This creates the class but you don't actually have an instance of the signal

DominicOram · 2023-05-02T08:48:58Z

src/dodal/devices/eiger.py

@@ -85,7 +91,7 @@ def stage(self):
        LOGGER.info("Waiting on parameter callbacks")
        status.wait(self.STALE_PARAMS_TIMEOUT)

-        self.arm_detector()
+        return self.arm_detector()


Must: With this now being asynchronous it no longer conforms to the interface for stage so we should rename it.

DominicOram · 2023-05-02T08:52:40Z

src/dodal/devices/eiger.py


    def forward_bit_depth_to_filewriter(self):
        bit_depth = self.bit_depth.get()
        self.odin.file_writer.data_type.put(f"UInt{bit_depth}")

-    def arm_detector(self):
+    def arm_detector(self) -> Status:
+        statuses = [Status(1), Status(2), Status(3), Status(4)]


Should: Each function that we pass these to is overwriting the entry so it's a bit confusing having these here. Might be better to start with an empty list then each function can just append to the list.

DominicOram · 2023-05-02T14:34:34Z

src/dodal/devices/eiger.py

        LOGGER.info("Waiting on stale parameters to go low")
-        self.wait_for_stale_parameters()
+        self.wait_for_stale_parameters(statuses)  # Starts the chain of arming functions

        self.forward_bit_depth_to_filewriter()


Must: This needs to be async too

DominicOram · 2023-05-02T14:34:53Z

src/dodal/devices/eiger.py

-
-        LOGGER.info("Setting aquire")
-        self.cam.acquire.set(1).wait(timeout=10)
-
        self.filewriters_finished = self.odin.create_finished_status()


Must: This also needs to be async

DominicOram · 2023-05-02T14:36:31Z

src/dodal/devices/eiger.py

@@ -24,6 +25,11 @@ class EigerTriggerNumber(str, Enum):


 class EigerDetector(Device):
+    class ArmingSignal(Signal):
+        def set(self, value, *, timeout=None, settle_time=None, **kwargs):
+            return self.parent.stage()


Must: The function has been renamed so we need t change it here

DominicOram · 2023-05-22T09:53:02Z

@callumforrester and @coretl, you might have some opinions on this. Callum, I know you weren't keen on this approach but I think it makes the plan side quite elegant (https://github.com/DiamondLightSource/python-artemis/pull/631/files). I'm particularly interested in:

It feels like there might be a cleaner way of writing wrap_and_do_funcs?
Do we think wrap_and_do_funcs is worth pushing into core ophyd? Seems generically useful

(I am currently reviewing it for style etc. so if you do have comments around that they may be similar to mine in a minute...)

callumforrester

@DominicOram I can see the elegance of the plan side, but you've actually done two things:

Move arming out of stage
Remove automatic staging/unstaging from the plan and do it manually

Can we not achieve the same just by doing 2.?

i.e.

yield from bps.stage(eiger)
...
def do_fgs():
    try:
        yield from bps.wait()  # Wait for all moves to complete
        yield from bps.kickoff(fgs_motors)
        yield from bps.complete(fgs_motors, wait=True)
    finally:
        yield from bps.unstage(fgs_composite.eiger)

Regardless of that, wrap_and_do_funcs does look like a useful addition, see my other comment for musings on implementation.

src/dodal/devices/utils.py

callumforrester · 2023-05-22T10:34:47Z

src/dodal/devices/utils.py

@@ -12,3 +18,71 @@ def epics_signal_put_wait(pv_name: str, wait: float = 1.0) -> EpicsSignal:
        EpicsSignal: An EpicsSignal that will wait for a callback.
    """
    return Component(EpicsSignal, pv_name, put_complete=True, write_timeout=wait)
+
+
+def wrap_and_do_funcs(unwrapped_funcs, timeout=60):


@DominicOram Have been thinking about a cleaner implementation, how about something like this?
It's not completely right because some of the tests don't like it, but I think recursion may be the way to go:

def wrap_and_do_funcs( unwrapped_funcs: list[Callable[[], Status]], timeout: float = 60.0, previous_status: Optional[Status] = None, ) -> Status: func, other_funcs = unwrapped_funcs[-1], unwrapped_funcs[1:-1] def wrapper(previous_status: Optional[Status] = None) -> Status: if previous_status is not None and previous_status.exception() is not None: # If there is an exception, return the status immediately, ends recursion # so no further statuses are evaluated LOGGER.error( f"{previous_status} has failed with error {previous_status.exception()}" ) return previous_status LOGGER.info(f"Doing {func.__name__}") next_status = func() if len(other_funcs) > 0: # Queue up the next function to run when this status completes. # Do not queue up more functions if there aren't any, ends recursion next_status.add_callback(lambda: wrap_and_do_funcs(other_funcs, timeout)) return next_status return wrapper(previous_status)

Yes, I feel like we can do it nicer with recursion. I think that might require a bit more thought so let's do a beamline test first @olliesilvester then we'll try and refactor to use recursion

DominicOram

This is really good, should speed things up a lot. A few comments in code but most of these probably came from us chopping and changing the solution a few times. Additionally:

Can you pull the 10 timeout out into a constant
I think we could do with some separate unit tests on wrap_and_do_funcs

DominicOram · 2023-05-22T09:27:03Z

src/dodal/devices/eiger.py

+        dummy_status = Status()
+        dummy_status.set_finished()
+        return dummy_status


Should: I think we can use Status(done=True, success=True)

DominicOram · 2023-05-22T09:27:19Z

src/dodal/devices/status.py


 from ophyd.status import SubscriptionStatus

 T = TypeVar("T")


-def await_value(subscribable: Any, expected_value: T) -> SubscriptionStatus:
+def await_value(


Nice, thanks

DominicOram · 2023-05-22T09:28:54Z

src/dodal/devices/utils.py

+    This function can be used to convert a series of blocking status functions to being asynchronous
+    by making use of callbacks. It also checks for exceptions on each returned status
+    For example, instead of blocking on status.wait(), the code will continue running until the status
+    is marked as finished


Should: I think you need to mention something about them run in sequence, at the moment this could read like all of the functions are run in parallel. I also think the bit after For example doesn't add much to the explanation

DominicOram · 2023-05-22T09:30:46Z

src/dodal/devices/utils.py

+
+    # The returned status - marked as finished at the end of the callback chain. If any
+    # intermediate statuses have an exception, the full_status will timeout.
+    full_status = Status(timeout=timeout)


Could: Might be nice to set the full_status to have the exception thrown from individual statuses

DominicOram · 2023-05-22T09:32:08Z

src/dodal/devices/utils.py

+    # Once the series of functions have finished with successful statuses, the final action is to mark
+    # the full_status as finished


Nit: Rather than this comment I would create a closing_func=lambda: full_status.set_finished() then use that. Makes the code a bit more self documenting

DominicOram · 2023-05-22T10:05:55Z

src/dodal/devices/eiger.py

+                    energy=detector_params.current_energy
+                ),
+                self.set_cam_pvs,
+                self.set_odin_pvs_after_file_writer_set,


Must: We're missing the set_odin_pvs step. I also think we could rename those to be a bit nicer. Probably set_number_of_frame_chunks and set_odin_pvs

Thanks I missed this - the function is back in and renamed now

DominicOram · 2023-05-22T10:28:59Z

src/dodal/devices/eiger.py

+            ]
+        )
+
+        if not self.armed:


Should: As discussed it would be better to just check this before doing anything, probably in the stage itself

DominicOram · 2023-05-22T10:29:47Z

src/dodal/devices/eiger.py

+        if not self.armed:
+            unwrapped_funcs.extend(
+                [
+                    lambda: await_value(self.stale_params, 0, 60),


Should: we can use STALE_PARAMS_TIMEOUT here

DominicOram · 2023-05-22T10:33:45Z

src/dodal/devices/eiger.py


-        await_value(self.odin.fan.ready, 1).wait(10)
+        return self.arming_status


Should: I don't think this is the correct status to return, if we want it to return a status we should change the put to a set. We should actually also do a wait on it before moving on with the arming (even though that wasn't in there previously)

DominicOram · 2023-05-22T10:36:40Z

src/dodal/devices/eiger.py

+    def _wait_for_cam_acquire(self) -> Status:
        LOGGER.info("Setting aquire")
-        self.cam.acquire.set(1).wait(timeout=10)
+        this_status = self.cam.acquire.set(1, timeout=10)
+        return this_status


Should: This can probably just be a lambda now

DominicOram · 2023-05-22T10:43:08Z

@DominicOram I can see the elegance of the plan side, but you've actually done two things:

1. Move arming out of stage

2. Remove automatic staging/unstaging from the plan and do it manually
   Can we not achieve the same just by doing 2.?

i.e.

yield from bps.stage(eiger)
...
def do_fgs():
    try:
        yield from bps.wait()  # Wait for all moves to complete
        yield from bps.kickoff(fgs_motors)
        yield from bps.complete(fgs_motors, wait=True)
    finally:
        yield from bps.unstage(fgs_composite.eiger)

Regardless of that, wrap_and_do_funcs does look like a useful addition, see my other comment for musings on implementation.

Can we? I'm not sure if we can if stage doesn't return a status? We need something to wait on to make sure the stage has completed?

callumforrester · 2023-05-22T10:55:50Z

In this case, don't we just make stage block on the status?

callumforrester · 2023-05-22T12:13:19Z

Ah, didn't see that you were waiting for a group, nevermind

DominicOram · 2023-05-22T12:35:42Z

As discussed offline with @callumforrester, we should tidy up the recursion then put in a PR to ophyd

…is_262_move_arming_out_of_stage

…ng_out_of_stage

This reverts commit e5d53c9.

DominicOram

Thanks! I haven't fully reviewed the tests but have some comments on the rest of the changes

DominicOram · 2023-06-06T16:20:48Z

src/dodal/devices/eiger.py

+    # this is no longer used TODO clean up
    def enable_roi_mode(self):
        self.change_roi_mode(True)

    def disable_roi_mode(self):
        self.change_roi_mode(False)


Should: Agree, lets remove them in this PR (first do a quick search that they're not used anywhere)

DominicOram · 2023-06-06T16:32:34Z

src/dodal/devices/eiger.py

+        status = self.cam.roi_mode.set(
+            1 if enable else 0, timeout=self.GENERAL_STATUS_TIMEOUT
+        )
+        status &= self.odin.file_writer.image_height.set(
+            detector_dimensions.height, timeout=self.GENERAL_STATUS_TIMEOUT
+        )
+        status &= self.odin.file_writer.image_width.set(
+            detector_dimensions.width, timeout=self.GENERAL_STATUS_TIMEOUT
+        )
+        status &= self.odin.file_writer.num_row_chunks.set(
+            detector_dimensions.height, timeout=self.GENERAL_STATUS_TIMEOUT
+        )
+        status &= self.odin.file_writer.num_col_chunks.set(
+            detector_dimensions.width, timeout=self.GENERAL_STATUS_TIMEOUT
+        )


Could: You're right that these should all have a timeout but it's getting messy with them set so many times here. What about something like:

status = Status(timeout=self.GENERAL_STATUS_TIMEOUT) # Overall status to track timeout for the whole operation status &= self.cam.roi_mode.set(1 if enable else 0) ...

And we could repeat the pattern elsewhere

I think if we did this then the first part of the status would never automatically be set as finished. Would attaching a timeout to just one of these statuses give the same effect?

As discussed we'll leave this as it is for now

DominicOram · 2023-06-06T16:35:24Z

src/dodal/devices/eiger.py


    def disarm_detector(self):
        self.cam.acquire.put(0)
+        self.armed = False
+
+    def make_chained_functions(self) -> Status:


Should: I think we could do with a better name here. This is not juts making the chain, it's kicking it off. The name is also too generic, we're doing the arming, not just any chain.

DominicOram · 2023-06-06T16:37:16Z

src/dodal/devices/eiger.py

+        this_status = await_value(self.odin.fan.ready, 1, self.GENERAL_STATUS_TIMEOUT)
+        return this_status


Nit: Can do this in one line

DominicOram · 2023-06-06T16:37:50Z

src/dodal/devices/eiger.py


-        await_value(self.odin.fan.ready, 1).wait(10)
+        unwrapped_funcs.extend(


Should: We can just extend it once

DominicOram · 2023-06-06T16:46:20Z

tests/devices/unit_tests/test_eiger.py

+@pytest.fixture
+def finished_status():
+    status = Status()
+    status.set_finished()
+    return status


Nit: I wouldn't make this a fixture, it can just be a function you call to get a status when you need one

DominicOram · 2023-06-06T16:49:51Z

src/dodal/devices/utils.py

+
+
+def wrap_and_do_funcs(
+    unwrapped_funcs: list[Callable[[], StatusBase]],


Nit: functions_to_chain rather than unwrapped_funcs maybe?

DominicOram · 2023-06-06T16:51:19Z

src/dodal/devices/eiger.py


-        self.filewriters_finished = self.odin.create_finished_status()
+    def make_chained_functions(self) -> Status:
+        unwrapped_funcs = list()


Should: unwrapped_funcs is too generic, something like functions_to_do_arm

DominicOram · 2023-06-06T16:52:27Z

tests/devices/unit_tests/test_eiger.py

+        ("fake_eiger._finish_arm"),
+    ],
+)
+def test_check_callback_error(fake_eiger: EigerDetector, func):


Should: I think a more useful test here would be to keep the same list as defined in the eiger but change which one throws an error

DominicOram · 2023-06-06T16:52:50Z

tests/devices/unit_tests/test_eiger.py


    fake_eiger.odin.file_writer.capture.sim_put(1)
    fake_eiger.odin.file_writer.num_captured.sim_put(2000)
    fake_eiger.odin.check_odin_state = MagicMock(return_value=True)

    fake_eiger.detector_params.trigger_mode = TriggerMode.FREE_RUN
+    fake_eiger.armed = True


Should: No longer used, remove

Co-authored-by: Dominic Oram <dominic.oram@diamond.ac.uk>

…ng_out_of_stage

…tage' into artemis_262_move_arming_out_of_stage

DominicOram

Much nicer naming, thank you!

olliesilvester added 3 commits April 28, 2023 09:30

use test dummy signals

03bc3ec

first try to make arming asynchronous

4e871c3

Remove test .py files

d343f64

olliesilvester mentioned this pull request Apr 28, 2023

262 Move arming out of stage DiamondLightSource/hyperion#631

Merged

changed name back to stage

02a4582

DominicOram requested changes May 2, 2023

View reviewed changes

olliesilvester added 2 commits May 2, 2023 10:59

eiger instantiates ArmingSignal, changed stage name

f8e91ed

minor fix

081d4d0

DominicOram requested changes May 2, 2023

View reviewed changes

olliesilvester and others added 11 commits May 2, 2023 17:28

Moved logic to async functions

6554ffe

(DiamondLightSource/hyperion#262) Fixes from testing on beamline

98e24c3

Added async unit tests

29cdcb1

Removed deprecated methods, only runs arming when disarmed

02abdcb

fixed and added tests for only arming when unarmed and vice versa

fda8f1a

make rest of staging asynchronous and added unit tests

6096f0f

Fix unit tests and added optional timeout to await_value

aa687d4

Merge branch 'main' into artemis_262_move_arming_out_of_stage

c8c97f0

fixed unit tests

b7ec48a

Added decorator to wrap the chained functions

0bf8052

Further unit tests, fixes and comments

07db0e4

callumforrester reviewed May 22, 2023

View reviewed changes

DominicOram requested changes May 22, 2023

View reviewed changes

DominicOram mentioned this pull request May 22, 2023

Create an async disarm for the eiger #67

Open

Added missed function

9c4a59d

olliesilvester added 4 commits May 23, 2023 14:47

Merge branch 'main' of github.com:DiamondLightSource/dodal into artem…

1c8bb31

…is_262_move_arming_out_of_stage

async_stage now returns status

95a8cb4

Added wrap_and_do_funcs tests and cleanup

81a3a38

Removed eiger.armed variable

b1adf05

DominicOram mentioned this pull request May 30, 2023

Make stage synchronous if arming not yet started #76

Closed

olliesilvester mentioned this pull request May 31, 2023

76 make stage synchronous if arming not started #83

Closed

olliesilvester marked this pull request as ready for review June 5, 2023 14:07

olliesilvester added 5 commits June 6, 2023 14:44

Merge remote-tracking branch 'origin/main' into artemis_262_move_armi…

6e2ef62

…ng_out_of_stage

fix linting

86172db

Minor change to eiger unit test

c103141

attempted eiger unit test fix

dc227cd

recombine test

e5d53c9

olliesilvester requested a review from DominicOram June 6, 2023 14:15

olliesilvester added 4 commits June 6, 2023 15:21

Revert "recombine test"

79e335a

This reverts commit e5d53c9.

Fixed unreliable unit tests

b6ed9d9

fix unit tests

08e90c6

Actually fixes unit tests

da0ab6b

DominicOram requested changes Jun 6, 2023

View reviewed changes

olliesilvester and others added 6 commits June 9, 2023 09:40

Improve comments in src/dodal/devices/utils.py

23ad4fa

Co-authored-by: Dominic Oram <dominic.oram@diamond.ac.uk>

Requested changes from review

12f705d

Merge remote-tracking branch 'origin/main' into artemis_262_move_armi…

486845a

…ng_out_of_stage

Merge remote-tracking branch 'origin/artemis_262_move_arming_out_of_s…

a6b9bfb

…tage' into artemis_262_move_arming_out_of_stage

fix unit tests

27c92a8

fix linting

4497b1d

olliesilvester requested a review from DominicOram June 9, 2023 09:24

DominicOram approved these changes Jun 12, 2023

View reviewed changes

DominicOram merged commit b7f5342 into main Jun 12, 2023
16 checks passed

DominicOram deleted the artemis_262_move_arming_out_of_stage branch June 12, 2023 10:50

DominicOram restored the artemis_262_move_arming_out_of_stage branch June 12, 2023 16:12

olliesilvester mentioned this pull request Jun 14, 2023

Make stage synchronous if arming not started #92

Merged

		# Once the series of functions have finished with successful statuses, the final action is to mark
		# the full_status as finished


		await_value(self.odin.fan.ready, 1).wait(10)
		return self.arming_status

		this_status = await_value(self.odin.fan.ready, 1, self.GENERAL_STATUS_TIMEOUT)
		return this_status


		await_value(self.odin.fan.ready, 1).wait(10)
		unwrapped_funcs.extend(



		def wrap_and_do_funcs(
		unwrapped_funcs: list[Callable[[], StatusBase]],

Artemis 262 move arming out of stage #43

Artemis 262 move arming out of stage #43

Conversation

olliesilvester commented Apr 28, 2023 • edited Loading

codecov bot commented Apr 28, 2023 • edited Loading

Codecov Report

DominicOram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DominicOram commented May 22, 2023 • edited Loading

callumforrester left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DominicOram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DominicOram commented May 22, 2023

callumforrester commented May 22, 2023

callumforrester commented May 22, 2023

DominicOram commented May 22, 2023

DominicOram left a comment

Choose a reason for hiding this comment

DominicOram Jun 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DominicOram Jun 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DominicOram left a comment

Choose a reason for hiding this comment

olliesilvester commented Apr 28, 2023 •

edited

Loading

codecov bot commented Apr 28, 2023 •

edited

Loading

DominicOram commented May 22, 2023 •

edited

Loading

callumforrester left a comment •

edited

Loading

DominicOram Jun 6, 2023 •

edited

Loading

DominicOram Jun 6, 2023 •

edited

Loading