-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add signal connection cache #368
Conversation
re: testing the signal. how should I instantiate the object? with |
@coretl I narrowed down where the knowledge gap is https://github.com/bluesky/ophyd-async/actions/runs/9397709655/job/25881480270?pr=368 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can also remove the parameterization from
ophyd-async/tests/core/test_mock_signal_backend.py
Lines 24 to 37 in 0b05b9c
@pytest.mark.parametrize("connect_mock_mode", [True, False]) | |
async def test_mock_signal_backend(connect_mock_mode): | |
mock_signal = SignalRW(MockSignalBackend(datatype=str)) | |
# 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 | |
await mock_signal.connect(mock=connect_mock_mode) | |
assert isinstance(mock_signal._backend, MockSignalBackend) | |
assert await mock_signal._backend.get_value() == "" | |
await mock_signal._backend.put("test") | |
assert await mock_signal._backend.get_value() == "test" | |
assert mock_signal._backend.put_mock.call_args_list == [ | |
call("test", wait=True, timeout=None), | |
] |
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 |
Looks like it was added quite recently to support supplying backend on connect: 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? |
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 ophyd-async/src/ophyd_async/panda/_common_blocks.py Lines 31 to 37 in cbbb295
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() .
|
huh, I'd never have guessed that |
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 |
this is the error I get, but it looks like expected:
|
It tests that you can choose to pass the signal backend in at runtime - why would this be hard to support with your changes? |
I don't mind putting them in the same test but it's still nicer to check that equality by value and by ref |
@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 |
ok, trying to make this right now |
tests/core/test_signal.py
Outdated
await mock_signal_rw.connect(mock=False) | ||
RE(ensure_connected(mock_signal_rw, mock=True)) | ||
assert ( | ||
mock_signal_rw._connect_task | ||
and mock_signal_rw._connect_task.done() | ||
and not mock_signal_rw._connect_task.exception() | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
await mock_signal_rw.connect(mock=False) | |
RE(ensure_connected(mock_signal_rw, mock=True)) | |
assert ( | |
mock_signal_rw._connect_task | |
and mock_signal_rw._connect_task.done() | |
and not mock_signal_rw._connect_task.exception() | |
) | |
await signal_rw.connect() | |
RE(ensure_connected(signal_rw)) | |
assert ( | |
mock_signal_rw._connect_task | |
and mock_signal_rw._connect_task.done() | |
and not mock_signal_rw._connect_task.exception() | |
) |
This test would have misleadingly failed I assume... The second connect wouldn't have succeeded since mock
changed value, however it wouldn't have updated the connect_task
so the assert here would have shown the first connect task was successful and passed.
I'm assuming you're choosing a soft
signal over a mock
signal because you want to mimick the performance of real signals within the connect? I'd like to see a version of this test where you use a real pva signal, passing mock=True
on both connects.
Edit
Because of the unset _previous_connect_was_mock
we don't get failure from changing mocks
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming you're choosing a soft signal over a mock signal because you want to mimick the performance of real signals within the connect? I'd like to see a version of this test where you use a real pva signal, passing mock=True on both connects.
so mock=True
creates a 'real pva signal'? I lost the plot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a commit causing Device
to fail on connect if mock
changes value. Realised it was probably nicer to fail outright for Devices
too.
A couple more changes left.
tests/core/test_signal.py
Outdated
async def test_signal_lazily_connects(RE): | ||
mock_signal_rw = soft_signal_rw(int, 0, name="mock_signal") | ||
await mock_signal_rw.connect(mock=False) | ||
RE(ensure_connected(mock_signal_rw, mock=True)) | ||
assert ( | ||
mock_signal_rw._connect_task | ||
and mock_signal_rw._connect_task.done() | ||
and not mock_signal_rw._connect_task.exception() | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please respond to this
https://github.com/bluesky/ophyd-async/pull/368/files#r1650538856
This test would have misleadingly failed I assume... The second connect wouldn't have succeeded since mock changed value, however it wouldn't have updated the connect_task so
the assert here would have shown the first connect task was successful and passed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above comment. This will always work because it's a soft_signal - but we want it to fail on the first connect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I see now - so here we test a scenario where the device rejects at first but later accepts.
I think the best way to do this is to have a mock device that has its own connect with this 'fail on first connect' functionality, to mimic a real life device that misbehaves when we try to connect to it. So a similar class to this one:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do it by swapping out a MockSignalBackend
's connect. That way you'll still be using the regular connect method/functionality of Device and Signal.
9d243e6
to
b374e7b
Compare
b374e7b
to
cd9b7bc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job! I don't know if we want to wait for @coretl to give this a once over before merging, but I'm happy with it.
cd9b7bc
to
264f16d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor comment, otherwise good to go
No description provided.