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

Removing Qt from the core #44

Open
hgrecco opened this issue Feb 8, 2015 · 13 comments
Open

Removing Qt from the core #44

hgrecco opened this issue Feb 8, 2015 · 13 comments

Comments

@hgrecco
Copy link
Contributor

hgrecco commented Feb 8, 2015

Qt is used in the core for two things.

Threading to provide async methods:

With the recent appearance of asyncio, I do not longer think that this is the way to go. I think we should move to concurrent calls to provide the async method integrating with asyncio as much as possible. Therefore removing Qt is not a problem for this purpose.

Signals and Threading to move drivers

Qt Signals are used to implement the observer pattern. This is used to communicate the core with the gui, not within the core.
Related to Signals. Qt has the concept of thread affinity: each object belongs to a thread. When a signal is emitted, connected slots are executed in the thread of the receiver.

Notice that while the discussion now is for Qt, this is a requirement to make responsive applications with any GUI toolkit. So we need to solve it.

I see three options:
A Add the concept of thread affinity to Python objects and use a pure Python implementation of signals such as this. Then dispatch the calls to the thread of the object owning the registered method.

B Try to import Qt and fallback to a pure Python implementation. GUI apps are only viable when Qt is available.

We tried this in Lantz 0.2. The problem is that Qt Signals are class members that belong to an object (not to the class). This is done using a special metaclass. In addition, Qt Signals requires objects with a thread affinity. In summary: is not just deriving changing the Signal object but also the base class for driver.

C In Qt GUI apps drivers should be wrapped. The wrapper is basically a QtObject that provide a Signal for each Feat and then redirects everything to the wrapped object.

This could be done in several ways:

After driver instantiation:

osci = QtWrapper(MyDriver())

where QtWrapper is provided by the GUI layer to be used.

During driver instantiation in the __new__ method:

osci = MyDriver(gui='qt')

where the gui parameter is standard for all drivers and accepts a string that tells which GUI layer and therefore which Wrapper to use. This could be also done automatically for all drivers by calling at the beginning of your application:

lantz.set_gui('qt')

(when we switch to enaml, we just need that the lantz_enaml package exports its own wrapper)

My view:
A seems to be cleaner, but we will never reach the performance of GUI toolkits. Moreover, we might find problems interacting with them.
B is a mess. We tried and it leads to applications that are difficult to test because their base class changes depending on whether you use Qt or not.
Then we are left with C (in any of its implementations). This allows better testing but forbids the use of signals in applications without a GUI toolkit (I am fine with that)

opinions?

@MatthieuDartiailh
Copy link

Yeah actually I also reached C as kind of the better alternative (mostly because I don't like to build the signals into the drivers). Also one nice thing, doing it that way, is that people can choose whether or not they want notifications. But we will have to state in the docs clearly how this works.
Furthermore, it will be easy to generalize (I have in the back of my mind, a notion of central server for all instruments allowing even multiple process to exchange infos and with a clear permission system allowing to make sure only one person works at a time (might an application to pyzco)).
It would also make sense to be sure that a single model is created per driver.
We will probably discuss this in detail somewhere else but I don't think we will need to override new as Eapii requires a lot of things to happen at the metaclass level it is better to simply override the call of the metaclass, that way people can still write standard init.

@hgrecco
Copy link
Contributor Author

hgrecco commented Feb 8, 2015

Ok. Lets go for C. The question is which one.

Regarding the init. What is the difference between Eapii init and Lantz initialize.?

@MatthieuDartiailh
Copy link

I wondered if did not answered a bit fast here. I started to think about how to do it in Atom and I think it will actually be a mess and in a more general fashion that such a system will be very easy to break. And against my first idea, I wonder if A would not be cleaner. If the signaling model does not exactly fit the GUI we can always have an intermediate layer (that we most of the time need anyway). However I don't think we need the thread-affinity stuff, Qt does this to avoid anybody modifying the GUI object from anything but the main thread. We can always make our GUI binding redirect the call to the main thread if it does not come from there. My main worries are about possible abuses of such an observer pattern but if we discourage it uses outside the GUI binding it should be fine (I would rather avoid people building whole traits-like application on it).
Actually all those issues are I think fairly tough to discuss without code to look at. If it is fine with you I will create the lantz-core repo fork it and start working on the merge in my repo and perhaps start and write one driver or two to let other comment on them.

@hgrecco
Copy link
Contributor Author

hgrecco commented Feb 9, 2015

I think is good to leave this discussion here as is a Meta discussion (it is related to all lantz-subprojects).

I am not so sure how easy will be to do A properly. I like your idea about getting some code to see how it work. I will create a dummy example for C.

@hgrecco
Copy link
Contributor Author

hgrecco commented Feb 9, 2015

I implemented type C. I think is doable and scalable. The code added to the core is really simple, which is a good thing. Take a look at https://gist.github.com/hgrecco/0765d7c51ccc08d7d9fe

@MatthieuDartiailh
Copy link

My point is that now you need to hook your qt signals into the Features to get the notification and we will have to do it for every backend. I think it would be better to do this once and have a common way to connect and disconnect signals.

@hgrecco
Copy link
Contributor Author

hgrecco commented Feb 9, 2015

In Lantz ~0.2 all drivers had signals (just pure Python signals). But was not very useful.

My experience is that most people write (single threaded) scripts or GUI applications. If this is really True, what would be the use case of this not-thread-aware signals? Are they just a way to connect a thin layer to a GUI enabled signal?

@MatthieuDartiailh
Copy link

My main worry is centered here about code duplication and inhomogeneities in the notification system. I would prefer to do things only once.

@hgrecco
Copy link
Contributor Author

hgrecco commented Feb 9, 2015

Let's try to get some code and see. I think that when you put an adapter layer there is always some amount of duplication. The question is how to reduce it by removing the functionality that is not needed in a particular layer.

@MatthieuDartiailh
Copy link

I guess you are probably right. If the hooks in the Feature are well designed this might be actually not so bad.

@hgrecco
Copy link
Contributor Author

hgrecco commented Feb 9, 2015

Additionally, the signal in core + adapter leads to the following call stack: setter/getter -> core_signal -> gui_signal -> callback. This might not look as bad until you realize that even if there is no callback connected, the stack is: setter/getter -> core_signal -> gui_signal. This can be mitigated but not in a nice way.

Basically my idea is that if only the GUI cares about signals, we only need to worry in the GUI layer. The core needs to provide a hooking place (which could be much simpler than a full signal) and we only need to agree about the signature of this hook. I propose:

  • new_value: the new value of the Feature
  • old_value: the old value of the Feature
  • info: a dict any other information that might be useful (i.e. channel number)

So, I updated the gist.

In the core we have a dictionary mapping feature name to callables and this is changed ONLY during wrapping (this write once behavior makes things easier) (See here)

Then is used by Feature after the value has changed in the following way:

if feat_name in self._on_changed_dict:
    self._on_changed_dict[feat_name](new_value, old_value, info_dict)

In other words if we have the right key in _on_changed_dict we emit the signal. The cool thing is that is up to the GUI toolkit to put the callable in the _on_changed_dict and therefore all the GUI specific code remains on the GUI layer.

Pros:

  • The code added to the core is small, very easy to understand and maintain.
  • When signals are not used, the overhead is really small (just a dictionary lookup).
  • When signals are used but no callback is connected, only a single function is called.
  • Any GUI toolkit should work, the specific code is the GUI layer

Cons:

  • Signals are NOT available when a GUI layer is not used
  • Is wrapping the object completely safe?
  • isinstance and issubclass will be broken. Could this be fixed with __instancecheck__ and __subclasscheck__?
  • others?

@MatthieuDartiailh
Copy link

To me what you have just described is the most basic implementation of an observer pattern save that it is a static one. Doing this or having a real observer pattern on which the GUI layer hook might not be so different, but might be easier. I say we keep that in mind, for the time being I will focus on the core (features and subsystem hooks) as I have some ideas how to rework Eapii metclasses to perhaps make alex happy and once we have made progress there we come back to the GUI part. How does that sound ?

@hgrecco
Copy link
Contributor Author

hgrecco commented Feb 9, 2015

Sounds good.

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

No branches or pull requests

2 participants