Move WarningException and some oav plans to dodal #1423
Conversation
|
It seems like |
There was a problem hiding this comment.
Great, thanks, one comment but also DiamondLightSource/dodal#589 (comment). I think you're right about not moving more. Wanting to know where the beam is on an OAV image seems generically useful. Doing a centring algorithm on that seems potentially more specific, probably something more for mx_bluesky
| @@ -30,7 +28,7 @@ def _fake_grid_detection( | |||
| box_size_um: float = 0.0, | |||
| ): | |||
| oav = i03.oav(fake_with_ophyd_sim=True) | |||
| smargon = fake_smargon() | |||
| smargon = i03.smargon(fake_with_ophyd_sim=True) | |||
There was a problem hiding this comment.
Should: This isn't equivalent as fake_smargon patched the motors too. Can you use the smargon() in conftest here?
| def catch_exception_and_warn( | ||
| exception_to_catch: Type[Exception], | ||
| func: Callable[..., Generator[Msg, None, T]], | ||
| *args, | ||
| **kwargs, | ||
| ) -> Generator[Msg, None, T]: | ||
| """A plan wrapper to catch a specific exception and instead raise a WarningException, | ||
| so that UDC is not halted |
There was a problem hiding this comment.
Love this helper function, thank you!
tests/unit_tests/test_exceptions.py
Outdated
| def dummy_plan(): | ||
| raise TestException | ||
| yield from null |
There was a problem hiding this comment.
Nit: If you swap the yield from and the raise then you can remove the type: ignore later on
tests/unit_tests/test_exceptions.py
Outdated
| def dummy_plan(): | ||
| raise TestException | ||
| yield from null |
There was a problem hiding this comment.
Must: I think it needs to be yield from null()
| @@ -26,11 +28,10 @@ def _fake_grid_detection( | |||
| snapshot_dir: str, | |||
| grid_width_microns: float = 0, | |||
| box_size_um: float = 0.0, | |||
| smargon=None, | |||
There was a problem hiding this comment.
I think 2274ac6 is slightly cleaner, feel free to disagree though
src/hyperion/exceptions.py
Outdated
| def warn_if_exception_matches(exception: Exception): | ||
| if isinstance(exception, exception_to_catch): | ||
| raise WarningException(str(exception)) | ||
| else: | ||
| raise | ||
|
|
||
| return ( | ||
| yield from contingency_wrapper( | ||
| func(*args, **kwargs), | ||
| except_plan=warn_if_exception_matches, | ||
| auto_raise=True, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Should: There's a bit of redundancy here. I think we either need the else raise and then auto_raise=False or no else raise and then auto_raise=True. Both having the else cause and the auto_raise is redundant, which is fine but maybe a little misleading
src/hyperion/exceptions.py
Outdated
| @@ -42,6 +42,5 @@ def warn_if_exception_matches(exception: Exception): | |||
| yield from contingency_wrapper( | |||
| func(*args, **kwargs), | |||
| except_plan=warn_if_exception_matches, | |||
| auto_raise=True, | |||
There was a problem hiding this comment.
Nit: This defaults to True so you still have the redundancy that you had before. Though I'm not too bothered as this protects us against future changes in the default.
There was a problem hiding this comment.
I will take it out. I also just realised that warn_if_exception_matches is a plan so it needs a yield from null()
…/hyperion#1423) * Move WarningException and some oav plans to dodal * make wrapper function to raise WarningExceptions from plans * Simplify xrc tests slightly --------- Co-authored-by: Dominic Oram <[email protected]>
Fixes dodal #539
Link to dodal PR (if required): #XXX
(remember to update
setup.cfgwith the dodal commit tag if you need it for tests to pass!)To test: