search

bluesky / ophyd-async

Hardware abstraction for bluesky written using asyncio
https://blueskyproject.io/ophyd-async
BSD 3-Clause "New" or "Revised" License
7 stars 21 forks source link

Add signal connection cache #368

Closed stan-dot closed 1 week ago

stan-dot commented 1 month ago

re: testing the signal. how should I instantiate the object? with epics_signal_rw or the constructor?

@coretl

stan-dot commented 4 weeks ago

@coretl I narrowed down where the knowledge gap is https://github.com/bluesky/ophyd-async/actions/runs/9397709655/job/25881480270?pr=368

stan-dot commented 3 weeks ago

the error string does not appear to be present anywhere but in the test. not sure where is it coming from . image

stan-dot commented 3 weeks ago 
async def test_signal_can_be_given_backend_on_connect():
    sim_signal = SignalR()
    backend = MockSignalBackend(int)
    assert sim_signal._backend is None
    await sim_signal.connect(mock=False, backend=backend)
    assert await sim_signal.get_value() == 0

what is the goal of this test? right now it clashes with the caching implementation. we can either stop supporting this use case and drop the test (easy) or somehow change it OR the implementation of the feature (hard)

that test was added 6 months ago by Rose

coretl commented 3 weeks ago

that test was added 6 months ago by Rose

Looks like it was added quite recently to support supplying backend on connect: image

We have to keep the test.

I suggest we always force a reconnect if backend is supplied, as this will only be used if the Signal is part of a Device, in which case the caching will happen there.

@evalott100 do you agree?

stan-dot commented 3 weeks ago

support supplying backend on connect:

I think I am missing some context. what does this do, when it is used and why do we support it?

coretl commented 3 weeks ago

support supplying backend on connect:

I think I am missing some context. what does this do, when it is used and why do we support it?

This is used when we know that a Signal will exist, like in https://github.com/bluesky/ophyd-async/blob/cbbb2950ed4c4b0bf4ca4c03729562b842b44b03/src/ophyd_async/panda/_common_blocks.py#L31-L37 but we don't know what PV it will connect to until connect(). What we will do is create the given SignalRW with backend=None, then on Device.connect() we actually create the backend and pass it to Signal.connect().

stan-dot commented 3 weeks ago

huh, I'd never have guessed that

stan-dot commented 3 weeks ago

should we have a flag for a case like that? it looks quite unusual. I'm thinking a flag next to 'force_reconnect' and 'mock'. then we can do logic flow around that.

coretl commented 3 weeks ago

should we have a flag for a case like that? it looks quite unusual. I'm thinking a flag next to 'force_reconnect' and 'mock'. then we can do logic flow around that.

The flag is a supplied backend. If backend is not None we should force a reconnect, otherwise do the existing behaviour

stan-dot commented 3 weeks ago

this is the error I get, but it looks like expected: 

File "/workspaces/ophyd-async/tests/core/test_signal.py", line 80, in test_signal_connect_fails_if_different_backend_but_same_by_value
    assert str(exc.value) == "Backend at connection different from initialised one."
AssertionError: assert 'Cannot make a MockSignalBackend for a MockSignalBackends' == 'Backend at connection different from initialised one.'

  - Backend at connection different from initialised one.
  + Cannot make a MockSignalBackend for a MockSignalBackends
stan-dot commented 3 weeks ago

image

these two are too similar, I'd delete the second one or change the expected message. because the value is different.

stan-dot commented 3 weeks ago

image

evalott100 commented 3 weeks ago

what is the goal of this test? right now it clashes with the caching implementation. we can either stop supporting this use case and drop the test (easy) or somehow change it OR the implementation of the feature (hard)

It tests that you can choose to pass the signal backend in at runtime - why would this be hard to support with your changes?

evalott100 commented 3 weeks ago

these two are too similar, I'd delete the second one or change the expected message. because the value is different.

I don't mind putting them in the same test but it's still nicer to check that equality by value and by ref

stan-dot commented 3 weeks ago

@evalott100 now the latest version has both tests, just the expected string is different for the second test

evalott100 commented 3 weeks ago

@evalott100 now the latest version has both tests, just the expected string is different for the second test

That makes sense, you got rid of the validation on the backends in the new connect logic

stan-dot commented 3 weeks ago

ok, trying to make this right now

stan-dot commented 3 weeks ago

and now we're back to the test string error: image

evalott100 commented 3 weeks ago

and now we're back to the test string error

This is because you're trying to mock a MockSignalBackend...

https://github.com/bluesky/ophyd-async/blob/b59d1e21c91a2697c36e519393f69a29b7601978/src/ophyd_async/core/mock_signal_backend.py#L19-L20

This is why we only mock if initial_backend isn't a MockSignalBackend (in which case it's already mocked).

https://github.com/bluesky/ophyd-async/pull/368#discussion_r1638133666

stan-dot commented 3 weeks ago

This is because you're trying to mock a MockSignalBackend...

I know that it is what happens in the test. I am not sure what is the expected behavior, as I do not fully understand the use case (https://github.com/bluesky/ophyd-async/pull/368#issuecomment-2161054867)

I know that the test or code should be changed

evalott100 commented 3 weeks ago

I know that it is what happens in the test. I am not sure what is the expected behavior, as I do not fully understand the use case (#368 (comment))

In those tests -

This should be what is failing

https://github.com/bluesky/ophyd-async/blob/b59d1e21c91a2697c36e519393f69a29b7601978/src/ophyd_async/core/signal.py#L76-L79

But in your change you're not checking to see if the backend passed in on connect is the same as the one passed into __init__.

stan-dot commented 3 weeks ago

is it all right now @coretl @evalott100 ?

evalott100 commented 3 weeks ago

For some reason github won't let me respond to your single comments.

https://github.com/bluesky/ophyd-async/pull/368#discussion_r1639598291

ok I still find this bizarre. What if there is no backend passed on init? then it's None.

We now allow for no backend passed in on init, then one will need to be provided on connect:

signal = SignalRW()
signal.connect(backend=any_backend_will_do) # passes
# or
signal.connect(backend=any_backend_will_do, mock=True) # passes

We don't allow for replacement of a signal passed in on init:

backend = PvaSignalBackend(int)
signal = SignalRW(backend)

signal.connect(backend=backend) # passes since the backend is the same

signal.connect(backend=PvaSignalBackend(int) # fails since the backend isn't the same by ref
# fails as to be accurate to production
signal.connect(backend=PvaSignalBackend(int), mock=True)

https://github.com/bluesky/ophyd-async/pull/368#discussion_r1639616142

I thought that the mode is redundant.

I believe the comment

    # If mock is false it will be handled like a normal signal, otherwise it will
    # initalize a new backend from the one in the line above

was from before we added the second condition to

https://github.com/bluesky/ophyd-async/blob/e4ab7d655df5e82d783fb735e100990a82986cb1/src/ophyd_async/core/signal.py#L81

with which, we don't replace the original MockSignalBackend.

I believe the test is still useful to show you can use a MockSignalBackend for a backend and have mock=True, whether you need it or not.

Edit

I could be convinced that we shouldn't allow the mock keyword if the backend is MockSignalBackend - since I've seen PRs assuming that mock=True is needed when the backend has been manually set as MockSignalBackend

mock_signal = SignalRW(MockSignalBackend(int))
non_mock_signal = SignalRW(PvaSignalBackend(int))

non_mock_signal.connect(mock=True) # passes
mock_signal.connect() # passes

mock_signal_connect(mock=True)
# raises ValueError("`mock=True` unnecessary, backend is already `MockSignalBackend`")

I think this is probably not required though? - We don't need to police behaviour in tests that much.

evalott100 commented 3 weeks ago

I suggest we always force a reconnect if backend is supplied, as this will only be used if the Signal is part of a Device, in which case the caching will happen there.

Missed this comment, but I agree

evalott100 commented 2 weeks ago

https://github.com/bluesky/ophyd-async/pull/368#discussion_r1646161096

what is the type of the self._previous_connect_was_mock boolean or an object?

Optional[boolean] - it should obviously be None if the signal hasn't been connected yet.

You're running backend.connect as a task whether backend is a MockSignalBackend or not.

stan-dot commented 2 weeks ago

@evalott100 that was passing finally, but I added some more tests and there have issues patching the debug method. I think I have a better grasp of what is going on here, now I'd just like to make those 2 tests pass