ENH: add Client.apply method, adjust TaskStatus to properly capture exceptions #53
Conversation
…uarantee TaskStatus objects are returned
| for pv, data in zip(pv_list, data_list): | ||
| logger.debug(f'Putting {pv} = {data}') | ||
| status: TaskStatus = self.cl.put(pv, data) | ||
| if status.exception(): |
There was a problem hiding this comment.
Is status guaranteed to be complete after self.cl.put returns it?
There was a problem hiding this comment.
It does currently, since we asyncio.run and block until the coroutine is finished. I'm not entirely convinced this is the best way to do things, but returning the status immediately would require us either
- be in an async context
- run the event loop in a separate thread (ophyd style).
I've been ok with this for now since we have the ability to run parallel puts (and wait on all of them), but it has been something I've been pondering changing
There was a problem hiding this comment.
I think this is totally ok, I wanted to make sure you hadn't skip a wait call here (since wait was added in this PR).
It's not so strange to require more nuanced asynchronous behavior to be done in an async context.
| return | ||
|
|
||
| if isinstance(entry, Setpoint): | ||
| return [self.cl.put(entry.pv_name, entry.data)] |
There was a problem hiding this comment.
Given the extra wrapping of list/not list here, I have to wonder if it would have been simpler for put to always return a list of statuses, even if we only put one PV
There was a problem hiding this comment.
I had this thought too. In the end I decided that if I, as a user of the Client, put to a single PV and had to index into a single-element list, I'd be annoyed.
But maybe the users and I can just deal with it 🤷
There was a problem hiding this comment.
Either way is probably ok, your explanation here makes sense to me
| @@ -57,6 +59,27 @@ def success(self) -> bool: | |||
| and self.task.exception() is None | |||
| ) | |||
|
|
|||
| def wait(self, timeout=None) -> None: | |||
There was a problem hiding this comment.
Small documentation thing: you've added this wait function here and not documented its inclusion in the release notes or in the PR text. It's also not used anywhere outside the test suite yet.
There was a problem hiding this comment.
I thought I'd need it, then I realized the full ramifications of the statuses I'd created 😆
Co-authored-by: Devan Agrawal <[email protected]>
|
Alright, merging and moving along |
Description
TaskStatusto actually capture exceptions, instead of propagating themMotivation and Context
closes #7
Assorted thoughts
TaskStatuswas built to hold exceptions raised in the coroutines they wrap, but were propagating them to the calling context. This was caused by our used ofasyncio.gather, which needed thereturn_exceptions=Truekwarg.asynciohas various ways of handling and propagating exceptions, we just needed to choose the right one for us.How Has This Been Tested?
Basic unit test added
Where Has This Been Documented?
This PR, docstrings etc
Pre-merge checklist
docs/pre-release-notes.shand created a pre-release documentation page