-
Notifications
You must be signed in to change notification settings - Fork 16
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
Platform-specific Subclassing Issue with psygnal Integration in ImSwitch #330
Comments
There are macOS arm wheels on PyPI, but they provide a minimum version of 10.16, not 11.0 @tlambert03 It looks like needs to be fixed on psygnal side using https://mypyc.readthedocs.io/en/latest/native_classes.html#inheritance As noted, it may impact code performance. Another option is to provide Signal twice, to provide compiled and non-compiled versions. |
Thanks @Czaki! The performance is indeed something that is dropping on my machine (at least it feels like that). In ImSwitch the internal image callback (e.g. for processing a frame by an internal controller) is organized by Signals (QT in the "non-headless" mode and psyngal in the headless mode). Is psygnal creating a copy of the argument in that case or is it pointer on the data? @jacopoabramo and I wer discussing this briefly. How exactly would I feel the performance impact? The reason why I discovered this issue was that I used imswitch+psygnal outside the docker container running on ARM. So it was not necessarily a MAC issue I guess, or? |
No. It is issue for Linux, Windows and MacOS. In the context of performance. The compiled version of psygnal should be 3-4 times faster than non compiled version. Psygnal should work using python references. I do not remember if we perform comparison with Qt signals. Did you see, a real difference? Maybe we do not have proper case in our benchmarks so we omit some bug. |
I have not measured it yet, but the frame-rate drops significantly in the psygnal case (might be another reason though). We will investigate it. Anyway, it's great to have this in-place replacement! with this ImSwitch can run in docker :) |
ok, to summarize some stuff here:
side-note: the link to the Version with socket integration no longer works |
Instead of subclass, we may investigate composition instead of inheritance? |
yeah, that would be one option to look into for the second option of "evaluating usage patterns in imswitch". might be easiest to do that over zoom with @beniroquai |
I may join such zoom if it will be planned. |
I'm guessing that for the specific use case of ImSwitch, this pattern would in the end look like a small wrapper around class Signal:
def __init__(self, *args):
self._signal = SignalInstance(*args) And all the methods would simply call the methods of |
Something like: class Signal:
def __init__(self, *args):
self._signal = SignalInstance(*args)
def __getattr__(self, name):
return getattr(self._signal, name) |
Shall we find a date for zoom somehow by mail? |
I've been thinking a bit about using the approach suggested by @Czaki and I have a doubt: what would happen to the type hinting mechanism? More specifically: class Signal:
def __init__(self, *args):
self._signal = SignalInstance(*args)
def __getattr__(self, name):
return getattr(self._signal, name) Mostly for my convenience and to make sure that other developers who want to check the code locally have better type hinting in their editors, wouldn't this cause some problems when trying to access the, for example, |
The type check will not work, unfortunately. Do we still try to solve the problem of autoconecting of signals to remote listener? |
with a proxy object like that, I generally just lie to the type checker: if TYPE_CHECKING:
from psygnal import Signal
else:
class Signal:
... |
Issue Description:
There seems to be a platform-specific behavior with psygnal on x86 vs ARM architectures. The problem occurs when attempting to subclass a psygnal object after integrating websockets in the ImSwitch project. The error only appears on x86 (such as on a Kubernetes Cluster) but works fine on ARM64 platforms like Raspberry Pi and MacOS, presumably because the compiled psygnal binary is installed differently between architectures.
Stack trace:
Steps to Reproduce:
Links:
Additional Context:
pip install --no-binary psygnal
, but this has not been tested fully yet.Possible Solution:
zip
-Archive to thesetup.py
so that it will compile it from scratch when installing imswitch.Thanks to @tlambert03 for the insights into the issue - and I hope I didn't for get anything. Not sure how I can explain the issue any better or give better code snippets.
The text was updated successfully, but these errors were encountered: