Deprecate zero-copy keyword argument in @ez.subscriber#216
Deprecate zero-copy keyword argument in @ez.subscriber#216griffinmilsap wants to merge 2 commits intofeature/sync-lowlevel-apifrom
zero-copy keyword argument in @ez.subscriber#216Conversation
|
|
||
|
|
||
| def subscriber(stream: InputStream, zero_copy: bool = False): | ||
| def subscriber(stream: InputStream, zero_copy: Any = _ZERO_COPY_SENTINEL): |
There was a problem hiding this comment.
Pull request overview
This pull request deprecates the zero_copy keyword argument in the @ez.subscriber decorator, addressing issue #209 which identified that the parameter was being ignored. The PR takes the approach of deprecating the parameter entirely and always setting the internal ZERO_COPY_ATTR to True, while actual zero-copy behavior is now controlled by the InputStream's leaky property (non-leaky streams receive zero-copy messages, leaky streams receive deepcopied messages).
Changes:
- Deprecated the
zero_copyparameter on@subscriberdecorator with aDeprecationWarning - Removed all internal usages of
zero_copy=Truefrom utility modules - Removed the
ZERO_COPY_ATTRimport and associated deepcopy check frombackendprocess.py
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/ezmsg/core/unit.py |
Adds deprecation warning for zero_copy parameter, always sets ZERO_COPY_ATTR=True, updates documentation |
src/ezmsg/core/backendprocess.py |
Removes ZERO_COPY_ATTR import and removes deepcopy check for pass-through messages |
src/ezmsg/util/perf/impl.py |
Removes zero_copy=True from three @ez.subscriber decorators |
src/ezmsg/util/debuglog.py |
Removes zero_copy=True from @ez.subscriber decorator |
src/ezmsg/util/messages/modify.py |
Removes zero_copy=True from @ez.subscriber decorator |
src/ezmsg/util/messages/key.py |
Removes zero_copy=True from two @ez.subscriber decorators |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| The ``zero_copy`` argument is deprecated and ignored. Subscribers always receive | ||
| zero-copy messages, so callers can omit it. |
There was a problem hiding this comment.
The docstring states "Subscribers always receive zero-copy messages" which is misleading. The actual behavior depends on whether the InputStream is leaky: leaky subscribers receive deepcopied messages via recv(), while non-leaky subscribers receive zero-copy messages via recv_zero_copy() (see backendprocess.py lines 413-443). Consider clarifying that the parameter is deprecated because zero-copy behavior is now controlled by the InputStream's leaky property rather than an explicit parameter.
There was a problem hiding this comment.
This is accurate, though asserts tighter coupling between leaky behavior and deep-copy than was intended. The current implementation of leaky streams depends on copying the messages as more of a workaround. In the future, leaky streams may not deepcopy the messages while still providing the same leaky functionality.
| @@ -374,8 +374,6 @@ async def wrapped_task(msg: Any = None) -> None: | |||
| result = call_fn(msg) | |||
| if inspect.isasyncgen(result): | |||
| async for stream, obj in result: | |||
There was a problem hiding this comment.
This removes the safety check that prevented zero-copy message pass-through when obj is msg. Previously, if a subscriber yielded the exact same object it received, a deepcopy would be created to prevent unintended mutations. Now, the same object reference can be passed through to downstream subscribers. This is a significant behavior change that could lead to mutation bugs if multiple subscribers modify shared message objects. If this is intentional as part of the new "zero-copy by default" design, it should be clearly documented. Otherwise, consider restoring this safety check without the ZERO_COPY_ATTR condition.
| async for stream, obj in result: | |
| async for stream, obj in result: | |
| # Prevent unintended zero-copy pass-through when a subscriber | |
| # yields the exact same object it received. | |
| if obj is msg: | |
| obj = deepcopy(obj) |
There was a problem hiding this comment.
This is an astute observation - The change is documented appropriately in the ezmsg docs repo. We will not be restoring that deepcopy.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Closes #209
Summary
zero_copyargument on@subscriber(ignored for years) and emit a warning when it’s explicitly passed.zero_copy=True.Changes
src/ezmsg/core/unit.py: Treatzero_copyas deprecated, warn when provided, and always setZERO_COPY_ATTR=True.src/ezmsg/util/perf/impl.py: Removezero_copy=Truefrom@ez.subscriber.src/ezmsg/util/debuglog.py: Removezero_copy=Truefrom@ez.subscriber.src/ezmsg/util/messages/modify.py: Removezero_copy=Truefrom@ez.subscriber.src/ezmsg/util/messages/key.py: Removezero_copy=Truefrom@ez.subscriber.Deprecation
@subscriber(..., zero_copy=...)is now deprecated and ignored. ADeprecationWarningis emitted when used.Testing