psygnal icon indicating copy to clipboard operation
psygnal copied to clipboard

Platform-specific Subclassing Issue with psygnal Integration in ImSwitch

Open beniroquai opened this issue 1 year ago • 14 comments

  • psygnal version: latest 0.11.1
  • Python version: 3.10
  • Operating System: Mac OS (ARM64), Raspberry Pi OS (ARM), Ubuntu 24.10 (x86); all in docker

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:

Starting Imswitch with the following parameters:
--headless --no-ssl --http-port 8001 --config-folder None --config-file example_virtual_microscope.json --ext-data-folder None
ERROR:root:Traceback (most recent call last):
File "/tmp/ImSwitch/imswitch/__main__.py", line 79, in main
from imswitch.imcommon import prepareApp, launchApp
File "/tmp/ImSwitch/imswitch/imcommon/__init__.py", line 1, in <module>
from .applaunch import prepareApp, launchApp
File "/tmp/ImSwitch/imswitch/imcommon/applaunch.py", line 6, in <module>
from .model import dirtools, pythontools, initLogger
File "/tmp/ImSwitch/imswitch/imcommon/model/__init__.py", line 1, in <module>
from .SharedAttributes import SharedAttributes
File "/tmp/ImSwitch/imswitch/imcommon/model/SharedAttributes.py", line 3, in <module>
from imswitch.imcommon.framework import Signal, SignalInterface
File "/tmp/ImSwitch/imswitch/imcommon/framework/__init__.py", line 4, in <module>
from .noqt import *
File "/tmp/ImSwitch/imswitch/imcommon/framework/noqt.py", line 151, in <module>
class Thread(abstract.Thread):
File "/tmp/ImSwitch/imswitch/imcommon/framework/noqt.py", line 153, in Thread
_started = Signal()
TypeError: interpreted classes cannot inherit from compiled

Steps to Reproduce:

  1. Run the code on an x86 platform (e.g., a Kubernetes Cluster).
  2. Use the socket integration in the ImSwitch project where subclassing a psygnal object is involved.
  3. Observe the above error on x86.

Links:

Additional Context:

  • This issue does not appear on ARM64 platforms, likely because the psygnal compiled binaries are not installed there, allowing subclassing to work.
  • A workaround may involve using pip install --no-binary psygnal, but this has not been tested fully yet.

Possible Solution:

  • It might be necessary to add additional wheels to psygnal to support x86/arm specifically, or ensure the decompiled version is used where subclassing is needed. For this I was trying to add the zip-Archive to the setup.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.

beniroquai avatar Oct 23 '24 05:10 beniroquai

There are macOS arm wheels on PyPI, but they provide a minimum version of 10.16, not 11.0 psygnal-0.11.1-cp310-cp310-macosx_10_16_arm64.whl. Something wrong happens here. Need to check which version is installed (if wheel or sdist).

@tlambert03 It looks like needs to be fixed on psygnal side using @mypyc_attr(allow_interpreted_subclasses=True) decorator.

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.

Czaki avatar Oct 23 '24 08:10 Czaki

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?

beniroquai avatar Oct 24 '24 20:10 beniroquai

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.

Czaki avatar Oct 24 '24 20:10 Czaki

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 :)

beniroquai avatar Oct 24 '24 20:10 beniroquai

ok, to summarize some stuff here:

  1. indeed, it is not possible to subclass SignalInstance in the compiled version (sorry that I forgot that when advising you to try that strategy 🫠). As @Czaki points out, it is possible to make them subclassable with @mypyc_attr(allow_interpreted_subclasses=True) but that comes at the expense of performance and is not something I want to generally do. Possible solutions include:
    • psygnal provides both compiled and uncompiled versions next to each other in different namespaces (I'm ok with that solution, looking into possible implementations that aren't too ugly to maintain)
    • we re-evaluate your usage patterns to see whether it's possible for you to avoid subclassing. That would give you the best performance, but might come at the expense of slightly uglier code.
    • you install with --no-binary (I'm confident that will work, but it comes at a slight performance hit for you... but you would have that anyway if you need to subclass). Use tips mentioned in https://github.com/pyapp-kit/psygnal/pull/331#issuecomment-2455192644 for installing with no-binary
  2. Speed was a separate issue and was addressed in #331

side-note: the link to the Version with socket integration no longer works

tlambert03 avatar Nov 05 '24 12:11 tlambert03

Instead of subclass, we may investigate composition instead of inheritance?

https://en.wikipedia.org/wiki/Composition_over_inheritance

Czaki avatar Nov 05 '24 13:11 Czaki

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

tlambert03 avatar Nov 05 '24 13:11 tlambert03

I may join such zoom if it will be planned.

Czaki avatar Nov 05 '24 13:11 Czaki

I'm guessing that for the specific use case of ImSwitch, this pattern would in the end look like a small wrapper around psygnal.SignalInstance, i.e.

class Signal:
    def __init__(self, *args):
         self._signal = SignalInstance(*args)

And all the methods would simply call the methods of self._signal. If the overhead isn't a lot I think that this would be a satisfying solution over the forced drop performance of @mypyc_attr(allow_interpreted_subclasses=True). Or rather it would work for the base case, I don't know what would happen with the socketed case.

jacopoabramo avatar Nov 05 '24 13:11 jacopoabramo

Something like:

class Signal:
    def __init__(self, *args):
         self._signal = SignalInstance(*args)
    
    def __getattr__(self, name):
        return getattr(self._signal, name)

Czaki avatar Nov 05 '24 13:11 Czaki

Shall we find a date for zoom somehow by mail?

beniroquai avatar Nov 07 '24 20:11 beniroquai

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, connect or disconnect method to check it's documentation?

jacopoabramo avatar Nov 08 '24 11:11 jacopoabramo

The type check will not work, unfortunately. This approach reduces the maintenance cost of following changes in psygnal itself. But by the cost of docstring and type annotation.

Do we still try to solve the problem of autoconecting of signals to remote listener?

Czaki avatar Nov 08 '24 11:11 Czaki

with a proxy object like that, I generally just lie to the type checker:

if TYPE_CHECKING:
    from psygnal import Signal
else:
    class Signal:
        ...

tlambert03 avatar Nov 09 '24 13:11 tlambert03