ezmsg-org / ezmsg

Pure-Python DAG-based high-performance SHM-backed pub-sub and multi-processing pattern
https://ezmsg.readthedocs.io/en/latest/
MIT License
15 stars 6 forks source link

Enhancement: task_wrapper should inspect tasks to determine how they're called rather than make assumptions based on attributes #120

Closed griffinmilsap closed 6 months ago

griffinmilsap commented 6 months ago

The BackendProcess module defines how ezmsg runs tasks. Every task (a coroutine in an ez.Unit decorated with @ez.subscriber, @ez.publisher, or @ez.task) has custom attributes assigned to it based on whether it subscribes, publishes, or is just a task. These attributes are useful for identifying tasks that ezmsg needs to kick-off, and which streams the task is interested in.

Currently, ezmsg uses these attributes to define how these tasks are called, assuming that all tasks with SUBSCRIBES_ATTR require a message in the call signature, and all tasks with PUBLISHES_ATTR return an AsyncGenerator that we need to iterate through for publication outputs.

These assumptions can result in hostile behavior to developers. A few examples:

  1. When a developer creates an initial implementation of an @ez.publisher coroutine that doesn't have anything to publish yet, so it's not yielding anything; hence isn't returning an AsyncGenerator and causes an unhelpful and confusing runtime exception when ezmsg tries to async-iterate through the resulting coroutine.
  2. When a novice developer copy-pastes a method that was previously an @ez.subscriber, but changes it to be an ez.publisher and forgets to take the input msg out of the call signature; ezmsg will attempt to call the task with less parameters than it needs resulting in another system crash with an unhelpful error message.
  3. Complicates implementation for decorators that wrap ezmsg tasks with further functionality that modifies the call signature of the task in ways that violate the current assumptions (for example ros_subscriber and ros_publisher in ezmsg-ros2

This can be addressed by using Python's inspect module instead of all of the hasattr checks in task_wrapper. Seeing as task_wrapper is only called once to wrap the task and these checks aren't in the message loop; changing the task_wrapper implementation to inspect task signatures, calling them with the appropriate number of parameters followed by an isinstance check or inspect.isasyncgen on the output to treat the output appropriately could allievate a lot of these concerns.

griffinmilsap commented 6 months ago

Merged this change into dev. All tests pass, but we might consider introducing a few more tests that try the various combinations of call signatures and ensure that exceptions are raised appropriately.