fix(api): call module functions across threads #5194
Conversation
Moving to the threadmanager broke the case of calling module functions through the threadmanager, since the module objects would be returned directly. They are now wrapped in CallBridger objects that have the __getattribute__ override the same as thread managers but don't do the actual thread managing. This is LRU cached so we don't build a bunch of them.
sfoster1
added
api
Codecov Report
@@ Coverage Diff @@
## edge #5194 +/- ##
==========================================
+ Coverage 60.29% 60.79% +0.50%
==========================================
Files 1025 1025
Lines 29029 30132 +1103
==========================================
+ Hits 17502 18320 +818
- Misses 11527 11812 +285
Continue to review full report at Codecov.
|
| @functools.lru_cache(8) | ||
| def wrap_module( | ||
| self, module: AbstractModule) -> CallBridger[AbstractModule]: | ||
| return CallBridger(module, self._loop) |
There was a problem hiding this comment.
For directness' sake, should this be object.__getattribute__(self, '_loop')?
| elif asyncio.iscoroutine(attr): | ||
| # Return awaitable coroutine properties run in managed thread/loop | ||
| fut = asyncio.run_coroutine_threadsafe(attr, loop) | ||
| wrapped = asyncio.wrap_future(fut, loop=asyncio.get_event_loop()) | ||
| wrapped = asyncio.wrap_future(fut) |
There was a problem hiding this comment.
Before, this was failing in some contexts because the main thread didn't have an event loop. Do you have reason to believe this isn't an issue anymore?
There was a problem hiding this comment.
Do you remember what those contexts were?
| self.wrapped_obj = wrapped_obj | ||
| self._loop = loop | ||
|
|
||
| def __getattribute__(self, attr_name: str) -> Any: |
There was a problem hiding this comment.
As this is really identical to the __getattribute__ defined in the ThreadManager, and we don't want them to diverge individually, is there some way that they can both call a shared abstracted function to enforce this similarity?
| WrappedObj = TypeVar('WrappedObj') | ||
|
|
||
|
|
||
| class CallBridger(Generic[WrappedObj]): |
There was a problem hiding this comment.
Is this another interface like ThreadManager that wraps an API object? What is aCallBridger?
There was a problem hiding this comment.
It is a threadmanager lite, I suppose. The ThreadManager is-a CallBridger, but it also is the actual owner of the thread. It could probably be changed to use the CallBridger
There was a problem hiding this comment.
Is there a future state where calling methods on a HardwareController or API or whatever it is does not happen through a ThreadManager or CallBridger any other wrapper that hides the interface?
It's probably beyond the scope of this PR, but It's worth stating that the developer experience of using these wrappers is not pleasant.
There was a problem hiding this comment.
There is; it would be when the Smoothie serial driver and the module serial drivers are async. Right now, the serial transactions block and therefore block the event loop.
| WrappedObj = TypeVar('WrappedObj') | ||
|
|
||
|
|
||
| class CallBridger(Generic[WrappedObj]): |
There was a problem hiding this comment.
Is there a future state where calling methods on a HardwareController or API or whatever it is does not happen through a ThreadManager or CallBridger any other wrapper that hides the interface?
It's probably beyond the scope of this PR, but It's worth stating that the developer experience of using these wrappers is not pleasant.
Moving to the threadmanager broke the case of calling module functions
through the threadmanager, since the module objects would be returned
directly.
They are now wrapped in CallBridger objects that have the
__getattribute__override the same as thread managers but don't do theactual thread managing. This is LRU cached so we don't build a bunch of
them.
Testing