-
Notifications
You must be signed in to change notification settings - Fork 28
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 a Watchable AsyncStatus and extend the wrap decorator #176
Conversation
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 had more thoughts...
823a48f
to
3dc36c9
Compare
b1a9fbb
to
21560a0
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.
I have a general preference for timeout
rather than timeout_s
as it matches all the asyncio arguments, and also the ophyd convention
335a125
to
db312ac
Compare
db312ac
to
c0ab271
Compare
64452fc
to
493ba2a
Compare
return AsyncStatus(coro, watchers) | ||
@WatchableAsyncStatus.wrap | ||
async def set(self, new_position: float, timeout: float = 0.0): | ||
update, move_status = await self._move(new_position) |
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.
and inlining _move
update, move_status = await self._move(new_position) | ||
start = time.monotonic() | ||
async for current_position in observe_value( | ||
self.user_readback, done_status=move_status |
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.
But here the done_status=move_status
is correct as we are talking to a motor record
@@ -54,7 +54,7 @@ def set(self, new_position: float, timeout: Optional[float] = None) -> AsyncStat | |||
""" | |||
watchers: List[Callable] = [] | |||
coro = asyncio.wait_for(self._move(new_position, watchers), timeout=timeout) | |||
return AsyncStatus(coro, watchers) | |||
return AsyncStatus(coro) | |||
|
|||
async def _move(self, new_position: float, watchers: List[Callable] = []): |
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 can we use the same wrap
logic here? Something like
async def _move(self, new_position: float, watchers: List[Callable] = []): | |
@WatchableAsyncStatus.wrap | |
async def set(self, new_position: float) -> : |
then yielding WatcherUpdate
instead of calling watchers
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.
Just spotted this...
class ASTestDevice(StandardReadable, Movable): | ||
def __init__(self, name: str = "") -> None: | ||
self._staged: bool = False | ||
self.sig = SignalR(backend=SimSignalBackend(datatype=int)) |
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.
Could be soft_signal_r_and_backend
src/ophyd_async/core/detector.py
Outdated
@AsyncStatus.wrap | ||
async def kickoff(self) -> None: | ||
self._fly_status = AsyncStatus(self._fly(), self._watchers) | ||
def kickoff(self, timeout: Optional[float] = None): |
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.
The original code kicked off self._observe_writer_indices
and returned immediately, then returned this status in complete
. This code now actually awaits this Status, which is wrong.
However, I think a simplification of the code is the better fix:
@AsyncStatus.wrap
async def kickoff(self) -> None:
# Nothing to do in kickoff, we already armed in prepare
pass
@WatchableAsyncStatus.wrap
async def complete(self):
async for index in self.writer.observe_indices_written(self._frame_writing_timeout):
yield WatcherUpdate(
name=self.name,
current=index,
initial=self._initial_frame,
target=end_observation,
unit="",
precision=0,
time_elapsed=time.monotonic() - self._fly_start,
)
if index >= end_observation:
break
Then can delete _fly
and _observe_writer_indices
...
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.
Flyable
tells us that kickoff
and complete
must both return statuses.
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.
That's fine, they're both being wrapped by *AsyncStatus
so return a Status
. It's just that kickoff
returns one that is immediately done...
@WatchableAsyncStatus.wrap # uses the timeout argument from the function it wraps | ||
async def set(self, new_position: float, timeout: float | None = None): |
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.
Can't see timeout being used below...
@WatchableAsyncStatus.wrap # uses the timeout argument from the function it wraps | |
async def set(self, new_position: float, timeout: float | None = None): | |
@WatchableAsyncStatus.wrap | |
async def set(self, new_position: float): |
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.
No, the WatchableAsyncStatus
wrapper uses the timeout argument from functions that it wraps, as we discussed in detail earlier in this PR
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.
Yes, but doesn't it add that to the signature itself? So the function doesn't need to mention timeout at all...
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.
It pulls it from kwargs
, so the function still needs to either take timeout
or **kwargs
tests/epics/demo/test_demo.py
Outdated
@@ -76,8 +100,9 @@ async def wait_for_call(self, *args, **kwargs): | |||
|
|||
|
|||
async def test_mover_moving_well(sim_mover: demo.Mover) -> None: | |||
set_sim_put_proceeds(sim_mover.setpoint, False) |
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.
These shouldn't be needed
set_sim_put_proceeds(sim_mover.setpoint, False) |
# Trigger any callbacks | ||
await self.user_readback._backend.put(await self.user_readback.get_value()) |
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.
This shouldn't be needed now we added the done_status
argument above?
# Trigger any callbacks | |
await self.user_readback._backend.put(await self.user_readback.get_value()) |
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.
not needed for that specific status, but if you have any other callbacks here and call stop any other way it is
if time.monotonic() > timeout_time: | ||
raise TimeoutError | ||
|
||
|
||
async def test_motor_moving_well(sim_motor: motor.Motor) -> None: |
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 could you revert this test and check it still passes?
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.
It doesn't the way it is in main - it needs a sleep after the set_mock_value(readback ...)
to process the call to the watcher. This slight alteration does pass:
async def test_motor_moving_well_2(sim_motor: motor.Motor) -> None:
set_mock_put_proceeds(sim_motor.user_setpoint, False)
s = sim_motor.set(0.55)
watcher = Mock()
s.watch(watcher)
done = Mock()
s.add_callback(done)
await asyncio.sleep(A_BIT)
assert watcher.call_count == 1
assert watcher.call_args == call(
name="sim_motor",
current=0.0,
initial=0.0,
target=0.55,
unit="mm",
precision=3,
time_elapsed=pytest.approx(0.0, abs=0.05),
)
watcher.reset_mock()
assert 0.55 == await sim_motor.user_setpoint.get_value()
assert not s.done
await asyncio.sleep(0.1)
set_mock_value(sim_motor.user_readback, 0.1)
await asyncio.sleep(0.1)
assert watcher.call_count == 1
assert watcher.call_args == call(
name="sim_motor",
current=0.1,
initial=0.0,
target=0.55,
unit="mm",
precision=3,
time_elapsed=pytest.approx(0.11, abs=0.05),
)
set_mock_put_proceeds(sim_motor.user_setpoint, True)
await asyncio.sleep(A_BIT)
assert s.done
done.assert_called_once_with(s)
a61afc0
to
2a12be3
Compare
f373fd8
to
5691fcf
Compare
Fixes #117, fixes #45
As according to discussion in #117, adds a
WatchableAsyncStatus
, and improves the wrappers so that they can be applied to any function returning anAwaitable
or anAsyncIterator
Will break anything previously passing watchers to
AsyncStatus
which will need to update toWatchableAsyncStatus
(but there hopefully isn't much out there yet)