Skip to content
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

Open
beniroquai opened this issue Oct 23, 2024 · 14 comments
Open

Comments

@beniroquai
Copy link

beniroquai commented Oct 23, 2024

  • 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.

@Czaki
Copy link
Contributor

Czaki commented Oct 23, 2024

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.

@beniroquai
Copy link
Author

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?

@Czaki
Copy link
Contributor

Czaki commented Oct 24, 2024

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.

@beniroquai
Copy link
Author

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

@tlambert03
Copy link
Member

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 fast: add emit_fast method for 10x faster emission (without safety checks) #331 (comment) for installing with no-binary
  2. Speed was a separate issue and was addressed in fast: add emit_fast method for 10x faster emission (without safety checks) #331

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

@Czaki
Copy link
Contributor

Czaki commented Nov 5, 2024

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

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

@tlambert03
Copy link
Member

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

@Czaki
Copy link
Contributor

Czaki commented Nov 5, 2024

I may join such zoom if it will be planned.

@jacopoabramo
Copy link

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.

@Czaki
Copy link
Contributor

Czaki commented Nov 5, 2024

Something like:

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

@beniroquai
Copy link
Author

Shall we find a date for zoom somehow by mail?

@jacopoabramo
Copy link

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?

@Czaki
Copy link
Contributor

Czaki commented Nov 8, 2024

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?

@tlambert03
Copy link
Member

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants