Add attenuator motor squad to i19 access controlled devices #1654

CoePaul · 2025-10-22T13:33:20Z

Add attenuator motor squad class to represent a set of mutually cooperating motors which are not co-mounted ( therefore do not form a physical stage ); but rather act in concert
Add initimately related position demand class used as driving argument to command motor squad to alter the posture of a beamline's transmission system
Add tests for both new classes: AttenuatorMotorPositionDemands AttenuatorMotorSquad
Adds access controlled subdirectory in the i19 device tests (to match the src devices file system layout)
Moves established test class for access controlled I19 shutters into the access controlled subdirectory

#1384

Instructions to reviewer on how to test:

Continues PR 1652 in a new branch
Confirm all 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 · 2025-10-22T14:44:18Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.09%. Comparing base (2b7fa79) to head (7267283).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1654   +/-   ##
=======================================
  Coverage   99.09%   99.09%           
=======================================
  Files         275      276    +1     
  Lines       10129    10158   +29     
=======================================
+ Hits        10037    10066   +29     
  Misses         92       92

☔ View full report in Codecov by Sentry.
📢 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.

rtuck99 · 2025-10-23T14:33:28Z

src/dodal/devices/i19/access_controlled/attenuator_motor_squad.py

+    @model_validator(mode="after")
+    def no_keys_clash(self) -> Self:
+        common_key_filter = filter(
+            lambda k: k in self.continuous_demands, self.indexed_demands


I think this doesn't do what you expect, comma operator doesn't exist in python, so

"a" in {"a":1,"b":2,"c":3}, {"b":2} Out[17]: (True, {'b': 2})

Probably more concise to do

self.continuous_demands.keys() ^ self.indexed_demands.keys()

that's a surprise - I was sure I had tested this.

I wll check it and get back to You

I think it does do what it means to do, it's just confusingly formatted. lambda k: k in self.continuous_demands, self.indexed_demands are two arguments to the filter function, not a single lambda trying to look up a key in two dicts.

Either way it would be simpler to use & to get the common keys so the whole function becomes

if common := self.continuous_demands.keys() & self.indexed_demands: raise ValueError(f"duplicate key(s) found: {common}") return self

Sorry, yes you are right, I was suckered by the line break.

rtuck99 · 2025-10-23T14:41:04Z

src/dodal/devices/i19/access_controlled/attenuator_motor_squad.py

+        common_key_filter = filter(
+            lambda k: k in self.continuous_demands, self.indexed_demands
+        )
+        common_key_count = sum(1 for _ in common_key_filter)


if you had done a for-comprehension instead of using filter then you could just do
if filtered_list:

Alternatively, since you have an iterator, you could do
next(common_key_filter, False), that way you don't have to iterate through the whole set (although this is not going to make any practical performance difference in this case).

I consulted this
https://www.geeksforgeeks.org/python/python-find-the-common-keys-from-two-dictionaries/

rtuck99 · 2025-10-23T14:43:41Z

src/dodal/devices/i19/access_controlled/attenuator_motor_squad.py

+        if common_key_count < 1:
+            return self
+        else:
+            ks: str = "key" if common_key_count == 1 else "keys"


It may be more useful to return the set intersection in the error message than the number?

rtuck99 · 2025-10-23T14:59:23Z

src/dodal/devices/i19/access_controlled/blueapi_device.py

        # Value here vould be request params dictionary.
        request_params = json.dumps(value)

        async with ClientSession(base_url=self.url, raise_for_status=True) as session:


Hmm, why does dodal use both requests and aiohttp????

Might that be because this is not a real device - it's a rest call factory which is in turn calling (under guard) the real device elsewhere - or is that completely unrelated?

Unrelated I think. As far as I'm aware requests doesn't support async calls so something else was needed. There is also httpx that supports both that we looked at switching to at one point (DiamondLightSource/blueapi#1069)

rtuck99 · 2025-10-23T15:07:18Z

src/dodal/devices/i19/access_controlled/blueapi_device.py

        # Value here vould be request params dictionary.
        request_params = json.dumps(value)

        async with ClientSession(base_url=self.url, raise_for_status=True) as session:


should we really be creating a new session for every request?

I think You're reviewing code which is already merged here - because it was too late to review it before?

If it seems relevant, I will review it!

rtuck99 · 2025-10-23T15:16:04Z

tests/devices/i19/access_controlled/test_attenuator_motor_squad.py

+    position_demands: AttenuatorMotorPositionDemands = given_position_demands()
+    await motors.set(position_demands)  # when motor position demand is set
+
+    expected_hutch: str = invoking_hutch.value


Since all of these things are only used once I think it would have been clearer just to inline them all into a single expected_request

When is a do-it-all-in-big-heap ever clearer than break it down into individual baby steps?
That makes no sense whatsoever

rtuck99 · 2025-10-23T15:19:27Z

src/dodal/devices/i19/access_controlled/attenuator_motor_squad.py

+
+    For details see the architecture described in \
+    https://github.com/DiamondLightSource/i19-bluesky/issues/30.
+    """


what are the concurrency semantics of this device? Is it expected that on completion of this set, that all motors will have completed their move? Or is it just that the move will have been requested, in which case how do we monitor for completion?

Here for now the move is just fire and forget requested.
We have fast and slow motors and the overall move may take many seconds - we don't want to block.

Changes in motor position will fire listener events in the caller ( GDA for now ) and when the overall system
is interested - it wll check the system transmission and how far we are from the requested transmission.

Any time out for that event driven listening wait is independent of dodal for now.
( ie it's up to the caller to decide how patient it wants to be )

rtuck99 · 2025-10-23T15:28:23Z

src/dodal/devices/i19/access_controlled/blueapi_device.py

                        f"""Session PUT responded with {response.status}: {response.reason}.
                        Unable to run plan {value["name"]}."""  # type: ignore
                    )
                    return


If we fail to set the worker task as active, then this effectively returns and the Status will be successful

rtuck99 · 2025-10-23T15:34:20Z

tests/devices/i19/access_controlled/test_attenuator_motor_squad.py

+    )
+    with pytest.raises(ClientConnectionError):
+        restful_response: AsyncMock = given_an_unhappy_restful_response()
+        unsuccessful_post.return_value.__aenter__.return_value = restful_response


This test doesn't fail in the expected way because you haven't mocked the PUT following the POST.
aiohttp raises ClientConnectionError because it does actually attempt a connect, but the POST error status is never evaluated.

There probably needs to be a few additional test cases here to check the various combinations of ways in which the HTTP requests can fail

rtuck99 · 2025-10-23T15:35:33Z

src/dodal/devices/i19/access_controlled/blueapi_device.py

                )

                try:
                    data = await response.json()


This response HTTP status code is never checked

tpoliaw · 2025-10-23T10:32:38Z

src/dodal/devices/i19/access_controlled/attenuator_motor_squad.py

+    """ I19-specific proxy device which requests absorber position changes in the x-ray attenuator.
+
+    Sends REST call to blueapi controlling optics on the I19 cluster.
+     The hutch in use is compared against the hutch which sent the REST call.
+    Only the hutch in use will be permitted to execute a plan (requesting motor moves).
+    As the two hutches are located in series, checking the hutch in use is necessary to \
+    avoid accidentally operating optics devices from one hutch while the other has beam time.
+
+    The name of the hutch that wants to operate the optics device is passed to the \
+    access controlled device upon instantiation of the latter.
+
+    For details see the architecture described in \
+    https://github.com/DiamondLightSource/i19-bluesky/issues/30.
+    """


Suggested change

""" I19-specific proxy device which requests absorber position changes in the x-ray attenuator.

Sends REST call to blueapi controlling optics on the I19 cluster.

The hutch in use is compared against the hutch which sent the REST call.

Only the hutch in use will be permitted to execute a plan (requesting motor moves).

As the two hutches are located in series, checking the hutch in use is necessary to \

avoid accidentally operating optics devices from one hutch while the other has beam time.

The name of the hutch that wants to operate the optics device is passed to the \

access controlled device upon instantiation of the latter.

For details see the architecture described in \

https://github.com/DiamondLightSource/i19-bluesky/issues/30.

"""

"""

I19-specific proxy device which requests absorber position changes in the x-ray attenuator.

Sends REST call to blueapi controlling optics on the I19 cluster.

The hutch in use is compared against the hutch which sent the REST call.

Only the hutch in use will be permitted to execute a plan (requesting motor moves).

As the two hutches are located in series, checking the hutch in use is necessary to

avoid accidentally operating optics devices from one hutch while the other has beam time.

The name of the hutch that wants to operate the optics device is passed to the

access controlled device upon instantiation of the latter.

For details see the architecture described in

https://github.com/DiamondLightSource/i19-bluesky/issues/30.

"""

Trying to join lines with \ results in the docstring having weird whitespace issues

tpoliaw · 2025-10-23T10:33:42Z

src/dodal/devices/i19/access_controlled/attenuator_motor_squad.py

+            )
+            raise ValueError(error_msg)
+
+    def restful_format(self) -> dict[PermittedKeyStr, Any]:


restful_format is a bit meaningless given rest doesn't specify a format. Maybe combined or merged or all_demands

Suggested change

def restful_format(self) -> dict[PermittedKeyStr, Any]:

def all(self) -> dict[PermittedKeyStr, Any]:

Whatever it's called - I want it to be blatantly "Thing needed because REST needs something"
rather than "What the heck is this for?"
and no I don't want the reader to have to know REST is involved prior to reading that code - I want the variable name to hint at that fact if the reader is new to the whole system and doesn't yet know this piece of code is building a packet to be sent by REST

Just because REST doesn't use a "format" it obviously has things by other names which basically do what formats do ... as a physicist by training I accept the need in engineering and software for precise names - but also tend to name a thing by a similar name until someone points out the nuanced issue with my first guess for a label

( see Roses by other names - W. Shakespeare etc )

But naming it after what it does also explains what it does. restful_format explains what it's used for but you need to read the code to figure out what it does.

tpoliaw · 2025-10-23T10:38:21Z

tests/devices/i19/access_controlled/test_attenuator_motor_squad.py

+def given_position_demands() -> AttenuatorMotorPositionDemands:
+    position_demand = MagicMock()
+    restful_payload = {"x": 54.3, "y": 72.1, "w": 4}
+    position_demand.restful_format = MagicMock(return_value=restful_payload)


Why does this need to be a magic mock instead of setting the return value directly?

Suggested change

position_demand.restful_format = MagicMock(return_value=restful_payload)

position_demand.restful_format.return_value = restful_payload

The same "infrastructure" can be used by many tests and other tests might have their own purposes / needs

Once You open the can of MagicMock it should be turtles all the way down as far as I am concerned.

tpoliaw · 2025-10-23T10:42:02Z

src/dodal/devices/i19/access_controlled/blueapi_device.py

        self.headers = HEADERS
        super().__init__(name)

+    def _get_invoking_hutch(self) -> str:


Suggested change

def _get_invoking_hutch(self) -> str:

@property

def _invoking_hutch(self) -> str:

If I read / understand notes on property annotation / decoration of functions - it's primary purpose seems to be to guard public facing methods with the effect of invisible getter and setter methods ( and a free del while we're at it ).

Here the getter itself is not supposed to be public. Is it still obvious that property annotation covers that scenario?

tpoliaw · 2025-10-23T15:38:04Z

src/dodal/devices/i19/access_controlled/attenuator_motor_squad.py

+    @model_validator(mode="after")
+    def no_keys_clash(self) -> Self:
+        common_key_filter = filter(
+            lambda k: k in self.continuous_demands, self.indexed_demands


I think it does do what it means to do, it's just confusingly formatted. lambda k: k in self.continuous_demands, self.indexed_demands are two arguments to the filter function, not a single lambda trying to look up a key in two dicts.

Either way it would be simpler to use & to get the common keys so the whole function becomes

if common := self.continuous_demands.keys() & self.indexed_demands: raise ValueError(f"duplicate key(s) found: {common}") return self

tpoliaw · 2025-10-23T15:41:43Z

src/dodal/devices/i19/access_controlled/blueapi_device.py

        # Value here vould be request params dictionary.
        request_params = json.dumps(value)

        async with ClientSession(base_url=self.url, raise_for_status=True) as session:


Unrelated I think. As far as I'm aware requests doesn't support async calls so something else was needed. There is also httpx that supports both that we looked at switching to at one point (DiamondLightSource/blueapi#1069)

rtuck99 · 2025-10-23T15:42:29Z

tests/devices/i19/access_controlled/test_attenuator_motor_squad.py

+    )
+    position_demands: AttenuatorMotorPositionDemands = given_position_demands()
+
+    logger.error.assert_not_called()


The await should raise an exception instead of succeeding.

rtuck99 · 2025-10-23T15:47:22Z

src/dodal/devices/i19/access_controlled/blueapi_device.py


            while not plan_complete:
                async with session.get(f"/tasks/{task_id}") as res:
                    plan_result = await res.json()


This logic here needs test cases, for what happens if the plan execution fails on the remote server.
We ought to check response ok so that we can gracefully handle the case where we get an HTTP error status but no json payload.

rtuck99 · 2025-10-23T15:56:22Z

src/dodal/devices/i19/access_controlled/attenuator_motor_squad.py

+
+
+class AttenuatorMotorPositionDemands(BaseModel):
+    continuous_demands: dict[PermittedKeyStr, float] = {}


Interestingly, PyCharm gives me angry red underlining here although pyright doesn't warn about it.
Mutable default '{}' is not allowed. Use 'default_factory'

I'm inclined to agree.

Pydantic does some magic to handle mutable default values: https://docs.pydantic.dev/latest/concepts/fields/#mutable-default-values

rtuck99 · 2025-10-23T16:00:52Z

tests/devices/i19/access_controlled/test_attenuator_position_demand.py

+        continuous_demands=wedge_position_demands,
+        indexed_demands=wheel_position_demands,
+    )
+    assert position_demand is not None


What are we really asserting here? This will always either raise an exception, in which case the assert is never reached, or be true.

I think we could parametrize all the happy path tests together, they don't test anything particularly interesting individually.

Tests don't just test - that is a side function
the primary purpose of tests is documenting actual behaviour -
parameterising tends to obfuscate that function - it can be used when You're just hammering the same nail over and over ( but that strikes me as a waste of testing time in general )
Different scenarios should be indicated-as-having-been-thought-about-distinctly not by being folded and knotted up into the laundry basket of a scruffy parameterised bundle
but should instead be carefully laid out for the reader one well named test at a time

if You want to know what code does, reading the test function names should tell You everything
one bullet point at a time
it also makes clear what the developers haven't thought about

testing is for the CIs benefit and is a very important but far inferior aspect IMHO

* Add attenuator motor squad class to represent a set of mutually cooperating motors which are not co-mounted ( therefore do not form a physical stage ); but rather act in concert * Add initimately related position demand class used as driving argument to command motor squad to alter the posture of a beamline's transmission system * Add tests for both new classes: AttenuatorMotorPositionDemands AttenuatorMotorSquad * Adds access controlled subdirectory in the i19 device tests (to match the src devices file system layout) * Moves established test class for access controlled I19 shutters into the access controlled subdirectory dodal#1384

For #dodal #1384

rtuck99 · 2025-11-20T17:00:59Z

All good apart from the comment regarding the unit test that passes accidentally due to the PUT being checked and not the POST.

I think, given that as I understand from my discussions with @noemifrisina it we are deliberately not checking the POST, that we should update our tests so that we are explictly testing the desired behaviour (even if it goes contrary to normal expectations) with suitable commentary as to why we want this.

CoePaul requested a review from a team as a code owner October 22, 2025 13:33

CoePaul requested a review from noemifrisina October 22, 2025 13:33

CoePaul force-pushed the dodal_1384b branch from 614e9a5 to 9a8d0a4 Compare October 22, 2025 14:40

CoePaul added i19-1 i19-2 labels Oct 22, 2025

rtuck99 mentioned this pull request Oct 23, 2025

Add attenuator motor squad to i19 access controlled devices #1652

Closed

4 tasks

rtuck99 self-assigned this Oct 23, 2025

rtuck99 reviewed Oct 23, 2025

View reviewed changes

tpoliaw reviewed Oct 23, 2025

View reviewed changes

rtuck99 reviewed Oct 23, 2025

View reviewed changes

CoePaul requested review from rtuck99 and tpoliaw November 18, 2025 16:36

CoePaul force-pushed the dodal_1384b branch from 6c5ef22 to 624f475 Compare November 18, 2025 16:55

CoePaul unassigned rtuck99 Nov 19, 2025

CoePaul added 2 commits November 20, 2025 12:35

add requested clean up after code review

7267283

For #dodal #1384

CoePaul force-pushed the dodal_1384b branch from 624f475 to 7267283 Compare November 20, 2025 12:35

CoePaul self-assigned this Nov 20, 2025

	def restful_format(self) -> dict[PermittedKeyStr, Any]:
	def all(self) -> dict[PermittedKeyStr, Any]:

	position_demand.restful_format = MagicMock(return_value=restful_payload)
	position_demand.restful_format.return_value = restful_payload

	def _get_invoking_hutch(self) -> str:
	@property
	def _invoking_hutch(self) -> str:



		class AttenuatorMotorPositionDemands(BaseModel):
		continuous_demands: dict[PermittedKeyStr, float] = {}

Add attenuator motor squad to i19 access controlled devices #1654

Are you sure you want to change the base?

Add attenuator motor squad to i19 access controlled devices #1654

Uh oh!

Conversation

CoePaul commented Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Instructions to reviewer on how to test:

Checks for reviewer

Uh oh!

codecov bot commented Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CoePaul Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CoePaul commented Oct 22, 2025 •

edited

Loading

codecov bot commented Oct 22, 2025 •

edited

Loading

CoePaul Oct 23, 2025 •

edited

Loading