Create an instantly moving mock motor

DominicOram commented 1 year ago

It may be useful for tests to have a mock motor that instantly moves it's readback to it's setpoint. This mock should live near the Motor object. Maybe it should be a function that mocks out any Motor children of a given device?

This will be easier done when https://github.com/bluesky/ophyd-async/issues/49 is done.

callumforrester commented 7 months ago

@DominicOram is this part of https://github.com/bluesky/ophyd-async/issues/94?

DominicOram commented 7 months ago

@DominicOram is this part of https://github.com/bluesky/ophyd-async/issues/94?

Unrelated I think. This issue is to make it easier to spin up mock motors for unit tests. #94 is I think to make more generic soft signal backends.

coretl commented 7 months ago

I think there should be 2 sorts of "not real" motors:

A motor that looks realistic from the outside, i.e. takes time to move. There is ophyd_async.epics.demo.Mover that does this with a supporting IOC, and we should make ophyd_async.sim.demo.SimMotor that does the same thing and works with the PatternDetector created in #131
A motor that has no logic behind it, just a bunch of PVs that immediately accept the value that has been given to it. This will work for ophyd_async.epics.motion.Motor if connect(sim=True) is passed, with a slight issue, the readback will not update. This is what we want for tests, but I suspect not for this issue.

@DominicOram will 1. be sufficient to solve this? In which case I suggest we turn this ticket into "Make a slightly realistic SimMotor"

DominicOram commented 7 months ago

1 will work for what we want but I'm not sure I agree with the taking time to move etc. as it's more complicated than needed. All we need is the minimum to be able to perform unit tests nicely e.g. be able to do a mv successfully. The easiest way to do this is to a motor where setting the setpoint immediately sets the readback.

callumforrester commented 7 months ago

@coretl does 1. solve other use cases? If so @DominicOram, could you have a 1-style motor with the time to move set to 0?

gilesknap commented 7 months ago

If nobody objects I'll work on creating a 1 style motor with a default time to move of 0. In full sim mode it would use the velocity to work out how long the move would take and update it throughout the move (probably), Could also take acceleration into account if there is any application for that.

DominicOram commented 7 months ago

Yh, that's fine for me. Thank you!

coretl commented 7 months ago

@coretl does 1. solve other use cases? If so @DominicOram, could you have a 1-style motor with the time to move set to 0?

The primary use case is docs and tutorials, something you can poke in iPython to play around with bluesky plans

callumforrester commented 7 months ago

Does #224 provide everything this issue requires? If so we can close it

@DominicOram @coretl @gilesknap

coretl commented 7 months ago

I think it does

gilesknap commented 7 months ago

I've read through the previous comments and believe that we have implemented what the consensus is. Tom has approved this so I'll let Dom confirm if he is happy.

DominicOram commented 7 months ago

Sorry to jump in on this when it's already been merged. It looks good and I think does match my usecase but I have some additional quires:

The way this is written means it doesn't match the interface of the Motor e.g. there's no acceleration_time signal. This means if we want to replace all instances of Motor with SimMotor for our unit tests there may be places where things fail. Ideally we would have an inheritance structure that guarantees Motor and SimMotor match?
Probably a new issue but it would be nice to have an automated way of getting a SimMotor when you connect a device that contains a Motor using sim=True. A general framework for this would be even better

I do worry I'm starting to describe dummy mode though...

callumforrester commented 7 months ago

@DominicOram I'm on the fence...

The problem with dummy mode is not that exists or that it has complex dummy functionality, but that it has to be maintained separately and in addition to live mode. Each beamline has two sets of config that must be kept manually in-step and the dummy-ness/live-ness of each is arbitrary/defined by convention. A "dummy mode" which is built automatically behind the scenes when you feed a live config into it (i.e. what you're suggesting) seems like a good improvement.

However, it does seem useful to have a true mock (i.e. what we currently call sim=True) and a "sim" with complex behaviour as different things for different purposes

DominicOram commented 7 months ago

My requirement for this is that the complex behaviour should just be enough to get the device functional for most unit tests. The classic example is wire a setpoint to a readback. As with the non-zero velocity above, I don't want us to start going down the route of providing "realistic" simulations. If we want to do that we should do it in tickit

callumforrester commented 7 months ago

I agree it's a separate issue, I don't think this one can be considered "done" until the interfaces of Motor and SimMotor match at least though (your first bullet point). @gilesknap Are you able to quickly make an amending PR since you're already in that space?

gilesknap commented 7 months ago

Sure I can add acceleration.

My first try was derived from motor.@coretl insisted that this be independent of any epics classes, so this is the result.

DominicOram commented 7 months ago

Acceleration was just an example, i think we ideally need to match the whole interface. What about having this and the motor derive from a common class that holds that interface?

gilesknap commented 7 months ago

OK so if I understand you and your are talking about the ophyd-async device for the real epics motor then the missing properties are:

        self.max_velocity = epics_signal_r(float, prefix + ".VMAX")
        self.acceleration_time = epics_signal_rw(float, prefix + ".ACCL")
        self.precision = epics_signal_r(int, prefix + ".PREC")
        self.deadband = epics_signal_r(float, prefix + ".RDBD")
        self.motor_done_move = epics_signal_r(float, prefix + ".DMOV")
        self.low_limit_travel = epics_signal_rw(int, prefix + ".LLM")
        self.high_limit_travel = epics_signal_rw(int, prefix + ".HLM")

Are you saying that you need the actual functionality of these?

I now think the model that @coretl and @DominicOram have for what this is supposed to be may be very different. @coretl please can you comment on the above? Thanks.

DominicOram commented 7 months ago

Yep, those are the ones. No, I don't think I need them to have any sensible behavior for now. I guess my use-case is that I would like the sim motor to be a drop in replacement for a real motor in a test for a plan, as much is reasonably practical. I appreciate that the motor may not behave correctly and so I may have to do some more clever mocking if the plans are too complicated but I would like the general case to work. Here are some example plans that are semi-realistic to what we're doing in Hyperion:

def my_plan(my_motor: Motor | SimMotor):
     yield from bps.mv(my_motor)

Which now works when we pass in sim motor but doesn't under a normally motor that is connected with sim=True. However, I would have to make my own sim device to be able to run:

def my_plan(my_motor: Motor | SimMotor):
    position_to_move_to = 100
    low_limit = yield from bps.rd(my_motor.low_limit_travel)
    new_pos = min(position_to_move_to, low_limit)

    max_velo = yield from bps.rd(my_motor.max_velocity)
    accel =  yield from bps.rd(my_motor.acceleration_time)
    fastest_we_can_get_to_position = # some maths here using max_velo and accel
    yield from bps.abs_set(my_detector.exposure_time, fastest_we_can_get_to_position)

Re-reading the above comments I think this is my fault for not being clear that this is what I wanted from the start, I'm happy to spin this into a new issue.

gilesknap commented 7 months ago

@DominicOram let's see what @coretl thinks - my own feeling is that it would be OK to add in the other properties and even have them behave in a reasonably consistent fashion. However it then seems like maybe we just need to somehow tell Motor to move instantly when we specify sim mode rather than have a whole new thing.

callumforrester commented 7 months ago

From discussing with @coretl, the worry here is that injecting arbitrary functionality into something called e.g. EpicsMotor (emphasis on the Epics) is a slippery slope. It requires the user to see the sim=True and just know that means class X will now behave like Y. I think I agree with this, I don't think it's DeviceCollector's job to inject any hidden functionality.

I wonder if this is actually dodal's job, so you can define a dummy version of each device factory for a beamline. It would give us a version of dummy mode that couldn't diverge too far from live mode. What do you think @DominicOram?

DominicOram commented 7 months ago

I disagree but this is also going way outside the scope of this issue, which is my fault for derailing it. We're no where near actually implementing such a system so we can discuss in a follow on issue.

However, for this mock motor to be useful to me my requirement is that it needs to provide the same interface as EpicsMotor. I would suggest to keep that maintainable this and EpicsMotor should share a common parent class. If you reject my requirement then I would like to discuss it in person. If you reject my implementation suggestion then that's fine, implement how you would like, I'm not an ophyd-async maintainer.

Apologies for being terse, I'm just keen to use this in dodal/hyperion as it really cleans things up.

coretl commented 6 months ago

Yep, those are the ones. No, I don't think I need them to have any sensible behavior for now. I guess my use-case is that I would like the sim motor to be a drop in replacement for a real motor in a test for a plan, as much is reasonably practical. I appreciate that the motor may not behave correctly and so I may have to do some more clever mocking if the plans are too complicated but I would like the general case to work. Here are some example plans that are semi-realistic to what we're doing in Hyperion:
def my_plan(my_motor: Motor | SimMotor):
     yield from bps.mv(my_motor)
Which now works when we pass in sim motor but doesn't under a normally motor that is connected with sim=True. However, I would have to make my own sim device to be able to run:
def my_plan(my_motor: Motor | SimMotor):
    position_to_move_to = 100
    low_limit = yield from bps.rd(my_motor.low_limit_travel)
    new_pos = min(position_to_move_to, low_limit)

    max_velo = yield from bps.rd(my_motor.max_velocity)
    accel =  yield from bps.rd(my_motor.acceleration_time)
    fastest_we_can_get_to_position = # some maths here using max_velo and accel
    yield from bps.abs_set(my_detector.exposure_time, fastest_we_can_get_to_position)
Re-reading the above comments I think this is my fault for not being clear that this is what I wanted from the start, I'm happy to spin this into a new issue.

Having re-read this thread, let me check I have the right understanding:

SimMotor is the bare minimum needed to have something that looks a bit like a generic motor that can be used in a scan for tutorials and testing clients of bluesky
EpicsMotor with connect(sim=True) is to be used for testing plans that normally take an EpicsMotor

@DominicOram I propose we refocus this issue on making 2. work for your use case. Does this work for you? In order to do that I would like to see what you want to test as well as the plan. For instance after #251 I can imagine something like:

def my_plan(my_motor: EpicsMotor):
    position_to_move_to = 100
    low_limit = yield from bps.rd(my_motor.low_limit_travel)
    new_pos = min(position_to_move_to, low_limit)
    max_velo = yield from bps.rd(my_motor.max_velocity)
    accel =  yield from bps.rd(my_motor.acceleration_time)
    fastest_we_can_get_to_position = # some maths here using max_velo and accel
    yield from bps.abs_set(my_detector.exposure_time, fastest_we_can_get_to_position)

@fixture
def mock_motor():
    with DeviceConnector(mock=True):
        motor = EpicsMotor("BLxxI-...")
    yield motor

def test_my_plan(mock_motor, mock_detector):
    set_mock_value(mock_motor.low_limit_travel, -40)
    set_mock_value(mock_motor.max_velocity, 12)
    set_mock_value(mock_motor.acceleration_time, 0.5)
    RE(my_plan(mock_motor))
    assert_mock_put_called_with(mock_detector.exposure_time, 0.45323434)  # whatever we expect that maths to make

None of that has required any logic to be applied to the motor. However this plan would need some logic applying to set readback from motor:

def move_and_return(my_motor: EpicsMotor, value: float) -> float:
    yield from bps.abs_set(my_motor, value)
    readback = yield from bps.rd(my_motor)
    return readback

We could do this in the fixture:

@fixture
def not_quite_there_motor():
    with DeviceConnector(mock=True):
        motor = EpicsMotor("BLxxI-...")
    # Make the motor not quite get to the endpoint
    set_mock_callback(motor.setpoint, lambda v: set_sim_value(motor.readback, v + 0.1)) 
    yield motor

def test_my_plan(not_quite_there_motor):
    ret = RE(move_and_return(not_quite_there_motor, 15))
    assert ret.plan_result == 15.1

The idea here being that the logic you want is added in the fixture, so you can make the motor do exactly what you want for the test being written.

My discussion with @callumforrester was about "what happens if there is some common logic that you need in many unit tests?" and that is where we discussed making these common fixtures (like not_quite_there_motor) available somewhere like in dodal.

@DominicOram what do you think?

DominicOram commented 6 months ago

Our current solution looks like what I think you're suggesting https://github.com/DiamondLightSource/hyperion/blob/669f3ad6109020873fe9e3e56d9b7fcf077d4e22/tests/conftest.py#L252.

My original idea with this issue was to try and remove the need for the downstream boilerplate but if we want to just put it downstream that's fine. Happy to close this issue.

coretl commented 6 months ago

Please can we keep this open while you're converting those tests to ophyd-async, then revisit it to see if we can bring any of the boilerplate upstream.

I wasn't against removing downstream boilerplate, but I didn't want to make a single set of logic that lived next to the simulation mode of EpicsMotor that became the way you used motors in simulation. As long as we have utility functions or fixtures like instantly_moving_motor or jittery_motor or following_error_motor rather than sim_motor then I'm happy to have that logic in ophyd-async.

coretl commented 4 months ago

@DominicOram have you found any repeated patterns in mock motors that are worth bringing upstream?

DominicOram commented 4 months ago

We have patch_motor (https://github.com/DiamondLightSource/dodal/blob/e4c3d5a82b20bd83488464055f6af3be820c6cf8/tests/devices/unit_tests/conftest.py#L24) that we use quite a lot in dodal and hyperion. I still thin it would be nice to have in dodal (probably with a bit more reconfigurability and docs) but if you're strongly against it that's fine

coretl commented 4 months ago

I think something like patch_motor could be put into ophyd_async.sim.testing. Should it be a function that can be used in your own fixtures, or a fixture in its own right? Not sure how pytest fixtures work when they're in a different module...

DominicOram commented 4 months ago

We normally use it as a function. In fact, when we had the similar ophyd equivalent it was a context, which I think we should recreate, i.e:

with patch_motor(my_motor):
    do_test() # Motor patched
# Motor now unpatched

We also have the following for patching multiple motors:

with ExitStack() as motor_patch_stack:
        for motor in motors:
            motor_patch_stack.enter_context(patch_motor(motor))
        yield device

A further step would even be:

def find_all_motors(device) -> List[Motor]:
    # recursively find all motor children of the device

def patch_all_motors(device):
    with ExitStack() as motor_patch_stack:
        for motor in find_all_motors(device):
            motor_patch_stack.enter_context(patch_motor(motor))
        yield device

coretl commented 4 months ago

I learned something new today: ExitStack! This certainly neatens up the multiple context manager problem...

How about a pair of context managers in ophyd_async.epics.motor:

motor_mocked_to_instantly_move
all_motors_mocked_to_instantly_move

def motor_mocked_to_instantly_move(motor: Motor, initial_position: float = 0) -> contextlib.AbstractContextManager:
    set_mock_value(motor.user_setpoint, initial_position)
    set_mock_value(motor.user_readback, initial_position)
    return callback_on_mock_put(
        motor.user_setpoint,
        lambda pos, *args, **kwargs: set_mock_value(motor.user_readback, pos),
    )

def _find_all_motors(device: Device) -> Iterator[Motor]:
    if isinstance(device, Motor):
        yield device
    for child in device.children:
        yield from _find_all_motors(child)

def all_motors_mocked_to_instantly_move(device: Device) -> contextlib.AbstractContextManager:
    stack = ExitStack()
    for motor in _find_all_motors(device):
        stack.enter_context(motor_mocked_to_instantly_move(device))
    return stack

async def mock_dcm():
    async with DeviceCollector():
        dcm = ixx.dcm()
    with all_motors_mocked_to_instantly_move(dcm):
        yield dcm

Bike shedding welcome...

bluesky / ophyd-async

Create an instantly moving mock motor #57