Port to pyqt5 and python3.

[philipstarkey]

Original report (archived issue) by Ian B. Spielman (Bitbucket: Ian Spielman, GitHub: ispielma).

---

[philipstarkey]

Original comment by Jan Werkmann (Bitbucket: PhyNerd, GitHub: PhyNerd).

---

I started porting to python3 & PyQt5 and quickly stumbled upon qtutils not supporting pyqt5. @philipstarkey do you have a timeline on when qtutils will support PyQt5? I think it should mainly be the imports that need adjusting. Am I right in that regard? If this should be the case I might just do it myself and create a pull request over at the qtutils repo.

And more generally have there already been efforts to port? If so are there any more packages that cause(d) problems? @chrisjbillington does zprocess fully support python3?

[philipstarkey]

Original comment by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington).

---

This would be extremely welcome!

Phil can comment as well but I think qtutils should support PyQt5 without too much effort. The differences between PyQt4 and PyQt5 are relatively minor. There are some low level things in qtutils, but I don't think they pertain to differences between PyQt4 and PyQt5 though something might surprise us.

Zprocess was always the hardest part about porting to Python 3 because of the strings/bytes distinction, but yes, it now fully supports Python 3 - though of course it is less tested with Python 3 and so bugs may become apparent with more testing. I actually imagine that porting other components to Python 3 and PyQt5 should not be that difficult. A strategy I might suggest is:

1. First port GUI components to use the "version 2" APIs of Pyqt4, which lyse and runmanager already do, but BLACS and runviewer would need to be changed for. You can see it being enabled at the top of lyse and runmanager's __main__.py files

2. Then port to PyQt5 in a backward compatible way (if you're already using the version 2 API this should be mostly a matter of one or two if statements and module name aliasing in the imports, depending on which Python version).

3. Then port to Python 3. This should also be able to be done in a backward compatible way so that things work on Python 2 as well as Python 3 (using __future__ imports and modules like six). As of the more recent versions of Python 3, this is not so difficult. Zero effort should be made to support Python 3 versions less than 3.6 (the latest version and what anaconda currently distributes). I think it was 3.5 or so when they reintroduced the u'string' prefix to Python 3 to allow backward compatibility with Python 2 - you should go ahead and use these features to make it easier to support both Python versions.

This order of doing things minimises the complexity of porting since:

• PyQt5 only supports the "version 2" PyQt API regardless of Python version
• PyQt4 on Python 3 only supports the "version 2" API, but PyQt4 on Python 2 supports either API version

Keeping backward compatibility is nice to allow people to stay on Python 2 and still get updates until support is dropped in the future rather than right now. We should try to maintain the labscript suite working on both Python 2 and Python 3 for at least a short time ("short" here might still mean 2 years :p ). Imposing PyQt5 on people though is pretty harmless - it should be a matter of updating with conda.

Doing it in steps allows for regression testing in between. Porting to API version 2 and thoroughly testing before moving to PyQt5 I think is a good idea. Then moving to PyQt5 and testing before moving to Python 3 is similarly reassuring. That way if there is breakage it is easier to know what caused it rather than not knowing if it was the API change, other Qt4 vs Qt5 changes, or Python3 changes.

Splitting the porting into multiple steps like that also makes it more plausible that other features could be worked on simultaneously and still actually be successfully mergable. Or, at the least, that the task of manually merging would be simpler.

[philipstarkey]

Original comment by Philip Starkey (Bitbucket: pstarkey, GitHub: philipstarkey).

---

No I don't have a timeline for qtutils. You're welcome to make pull requests!

Python 3 support for qtutils shouldn't be too hard (it's already partially done; for instance invoke_in_main is python 3 compatible). PyQt5 could be harder. Will need testing thoroughly especially with widget promotion and signal blocking (probably creating tests would be a good idea!). We should probably consider moving some of the Qt toolkit (PyQt4/PySide/PyQt5) selection code into qtutils and out of the programs that currently support more than just PyQt4. We would then import qt from qtutils, and qtutils could do any toolkit abstraction necessary. I'm not sure which version we should try and abstract to. My preference would be to make everything (PySide/PyQt4) look like PyQt5 for future compatibility. That shouldn't break current qtutils compatibility with our software since we won't start using the qtutils abstracted "Qt" until we port the labscript suite to PyQt5 (so it will just lay dormant).

Other than that, I agree with @cbillington's suggested process for porting.

[philipstarkey]

Original comment by Jan Werkmann (Bitbucket: PhyNerd, GitHub: PhyNerd).

---

As for the abstraction we could just go ahead and use a lot of the abstraction code that pyqtgraph provides pyqtgraph.qt. This would however need adjustment if we say PyQt5 is the thing that we want everything to look like.

The porting process seems reasonable.

[philipstarkey]

Original comment by Philip Starkey (Bitbucket: pstarkey, GitHub: philipstarkey).

---

I'd rather not just use the pyqtgraph abstraction. Mainly because of the licensing mess we might get ourselves into (I think the MIT license is compatible with the BSD license, but borrowing other people's code always gets messy), but also because most of what is in that file is not really all that helpful for what we do. qtutils already has cross-toolkit ui loading (and done better than pyqtgraph does it) and the rest was either trivial or related to graphics scene transformations which we don't really need.

I think the cleaner approach would be to build up the abstraction from scratch based on matching the PyQt5 structure

[philipstarkey]

Original comment by Jan Werkmann (Bitbucket: PhyNerd, GitHub: PhyNerd).

---

Makes sense!

In runviewer and BLACS we still have some code to account for pyside I would remove that code in the act of switching to API 2 as that code is also missing in lyse and runmanager.

Or is support for PySide important in some way? It would comeback in step 2 anyway if we choose to also include it in the qtutils wrapper. I would however prefer to first write the wrapper for PyQt4 and PyQt5 only.

[philipstarkey]

Original comment by Philip Starkey (Bitbucket: pstarkey, GitHub: philipstarkey).

---

There is a lot of code in BLACS for PySide, I think it would be a pain to remove and then re-add. Although there is then the consideration of supporting PySide v2 (which I think supports Python 3 but is still in beta?) which potentially complicates things

qtutils should definitely keep PySide support since it is a standalone project to the labscript suite and may be used by other software (I use it in a few private projects, and it's on PyPi and I've spruiked it a few times on stackoverflow, so who knows who else might be using it).

I guess it's an open question as to whether we maintain PySide support (the license is nicer but it has more bugs)

I think, given this, that the best solution might be to ensure that the qtutils update can abstract away all of the PySide things we currently do in our software. That should come first. Then port the labscript suite to use the new qtutils, during which we remove all the toolkit specific hacks (since they should now live in qtutils).

---

UPDATE

I forgot to include the change to API v2 for PyQt in the suggested timeline. Since PyQt API v2 is closer to PySide than PyQt API v1, I think you'll actually find it easier to leave the PySide code in. Most of the hacks are actually probably working around the fact that we initially built for PySide, and then moved to PyQt API v1, so you'll probably find a lot of code ends up matching the PySide version and then you can remove the check in that part of the code that swaps between PySide/PyQt (hopefully that makes sense?)

[philipstarkey]

Original comment by Jan Werkmann (Bitbucket: PhyNerd, GitHub: PhyNerd).

---

I wasn't planing on doing a total revamp of qtutils I was thinking of adding PyQt5 in a appropriate way in all the existing files. As this is minimal effort and can later be replaced by the Qt wrapper.

But the new abstraction layer that you proposed would then just be PyQt4 & PyQt5 for the start as thats was I have running on my machines and what I can test against.

But you're most probably right that all of this should be done before even touching labscript code. As I would first introduce API2 lines just to remove them at a later time and replace them with the new wrapper.

As for removing PySide I was talking about the imports as they are currently not consistent in lyse, runmanager vs blacs, runviewer. We should decide on whats the way to go here as someone using PySide currently can only use half of the labscript suite. Either we drop support as a whole or reinstate it. Ok so here is my plan:

1. make existing qtutils files PyQt5 compatible.

2. Qt wrapper for PyQt4 and PyQt5 that behaves like PyQt5

3. Add all of that code into Labscript by replacing the PyQt imports with the import of the wrapper (It would be best to do this with lyse, runmanager, runviewer first as they have they have less Ui code)

4. Lots of testing and dissucssing

5. Do adjustment for Python 3

One last thing though should we maybe try to import the future package in labscripts files at some point prior to the python3 port to see if something breaks?

[philipstarkey]

Original comment by Philip Starkey (Bitbucket: pstarkey, GitHub: philipstarkey).

---

future imports will almost certainly break things, or worse introduce bugs that go undetected (especially with division - we already import future division in labscript, but I don't think we necessarily do in the others so we'll need to do a search for / and verify that we aren't relying on implicit conversion to integers in those programs).

[philipstarkey]

Original comment by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington).

---

importing __future__ things will be a necessary part of porting to Python 3 whilst supporting Python 2 still. Whilst I agree with Phil that it will break things, they are things that will break in a port to Python 3 anyway, so there's not much way around it. Turning on each __future__ import one by one and testing I think is a good idea as a first step toward Python 3 porting, but yes I definitely expect things to break. I suspect division to probably be alright, unicode_literals could possibly be trickier but I'm not sure. But considering them one by one could be a good way to get these aspects of the Python 3 port done in a regression-testing friendly way.

[philipstarkey]

Original comment by Jan Werkmann (Bitbucket: PhyNerd, GitHub: PhyNerd).

---

Ok so steps 1 and 2 are done. Step 3 is also done at least on my machine. When the qtutils pull request goes through there will be lots of pull requests. The best order in my opinion is:

1. labscript_utils

2. lyse

3. runmanager

4. BLACS

5. runviewer

This order arises from the dependencies. As lyse uses labscript_utils/qtwidgets/headerview_with_widgets.py this has to happen first. Then lyse can be ported. The next easies thing is runmanager so thats third. Next I would want to do runviewer but that depends on some BLACS code so I would do BLACS before.

Doing things in this order should allow for testing in between pull requests.

[philipstarkey]

Original comment by Jan Werkmann (Bitbucket: PhyNerd, GitHub: PhyNerd).

---

We should maybe also talk about namespacing. In the act of porting to PyQt5 we could replace all the "from QtGui import *" and add in QtGui.Class at the appropriate places instead. Where QtGui stands in for any Qt Module. This is a bunch of work (that I happened to have already done once) and has little benefit except for decluttering the global namespace. So should we do that or do we want to keep the convenience of import *?

[philipstarkey]

Original comment by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington).

---

My mind has gone back and forth about Qt namespacing. I originally was a sloppy coder importing * all over the place, then I tightened up and started importing explicitly. But now, I realise that because Qt comes from C++ where namespacing is less of a thing (is it even a thing at all?), the Qt names basically never clash with anything, they all start with 'Q', and I never remember which submodule they're from anyway, so importing * has saved a lot of having to look up which module things are in.

So I really don't mind if we're importing * or not - I think the only downside is that static analysis tools like linters are less able to detect errors in your code if you import *. This is a pretty good reason, and I'd like to use tools like that more often, so if someone made a pull request to get rid of the * imports I would vote to accept it. But it's otherwise not that big a deal for Qt specifically whereas I would consider it much worse form to be importing * from most other modules.

[philipstarkey]

Original comment by Jan Werkmann (Bitbucket: PhyNerd, GitHub: PhyNerd).

---

Ok then we should stick to the import as this is also a bit more futureproof. Considering that PyQt5 decided to split QtGui in QtWidgets, QtGui and QtCore and I've by now managed to get the Qt wrappers modules to do 'import \'

[philipstarkey]

Original comment by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington).

---

Sounds good!

[philipstarkey]

Original comment by Jan Werkmann (Bitbucket: PhyNerd, GitHub: PhyNerd).

---

I'm publishing my PyQt5 port branches one after the other. As I'm reconstructing them from a very messy local repo of mine this takes a bit of time.

They can be found at:

Labscript_utils

Lyse

BLACS

runmanager

labscript_devices

runviewer

There are most certainly still unfound bugs in the code. If someone wants to hunt them down that would be very much appreciated.

[philipstarkey]

Original comment by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington).

---

Be aware that since your lyse qt5 changes include pep8 formatting, it won't merge with your other pending lyse pull requests - might be worth avoiding doing too much work on it until the other pull requests are merged. I'll take a look at the other pull requests tomorrow to see where they're at and see if we can get them merged soon.

[philipstarkey]

Original comment by Jan Werkmann (Bitbucket: PhyNerd, GitHub: PhyNerd).

---

Yeah I know this is one of my sublime plugins that does all of this on it's own (if the file is not too long) usually this is really handy. I can create a new branch and just do the diffs without the whitespace anytime so there should be no problem there. Just creating the branches so that development happens out in the open and everyone can go bug hunting or comment. In the BLACS/main.ui I had to remove some lines that broke everything in Qt5 were they important?

[philipstarkey]

Original comment by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington).

---

I'm not sure - looks like some kind of automatic signal connections, which I didn't think we were using. Could you verify that both the window close button and the file/exit menu item still work? if so, then it was probably something redundant.

[philipstarkey]

Original comment by Jan Werkmann (Bitbucket: PhyNerd, GitHub: PhyNerd).

---

Close button works menu not so much. I'll do the connection in code and hope that that works.

[philipstarkey]

Original comment by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington).

---

Hm, that's unfortunate. Yes, I see now that it was telling the menu item to do the same as the window close event. Does Qt5 not support that? If not it's a shame - we use it in other places too I think (not in BLACS).

[philipstarkey]

Original comment by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington).

---

Qt5 does support connecting signals in Qt designer in this way. What exactly goes wrong when you leave the lines in? It looks like it should be ok, so it would be nice to work out how to do this correctly rather than just working around it, if possible.

[philipstarkey]

Original comment by Jan Werkmann (Bitbucket: PhyNerd, GitHub: PhyNerd).

---

At least it doesn't have QAction.activated

#!python

Traceback (most recent call last):
  File "/Users/janwerkmann/anaconda/lib/python2.7/runpy.py", line 174, in _run_module_as_main
    "__main__", fname, loader, pkg_name)
  File "/Users/janwerkmann/anaconda/lib/python2.7/runpy.py", line 72, in _run_code
    exec code in run_globals
  File "/Users/janwerkmann/labscript_suite/BLACS/__main__.py", line 728, in <module>
    app = BLACS(qapplication)
  File "/Users/janwerkmann/labscript_suite/BLACS/__main__.py", line 250, in __init__
    self.ui = loader.load(os.path.join(os.path.dirname(os.path.realpath(__file__)),'main.ui'), BLACSWindow())
  File "/Users/janwerkmann/anaconda/lib/python2.7/site-packages/qtutils/UiLoader.py", line 117, in load
    return uic.loadUi(*args, **kwargs)
  File "/Users/janwerkmann/anaconda/lib/python2.7/site-packages/PyQt5/uic/__init__.py", line 226, in loadUi
    return DynamicUILoader(package).loadUi(uifile, baseinstance, resource_suffix)
  File "/Users/janwerkmann/anaconda/lib/python2.7/site-packages/PyQt5/uic/Loader/loader.py", line 72, in loadUi
    return self.parse(filename, resource_suffix, basedir)
  File "/Users/janwerkmann/anaconda/lib/python2.7/site-packages/PyQt5/uic/uiparser.py", line 1000, in parse
    actor(elem)
  File "/Users/janwerkmann/anaconda/lib/python2.7/site-packages/PyQt5/uic/uiparser.py", line 906, in createConnections
    bound_signal = getattr(sender, signal_name)
AttributeError: 'QAction' object has no attribute 'activated'

[philipstarkey]

Original comment by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington).

---

Aha. It appears that in Qt5 it's called 'triggered'. So you could probably change the signal name in the ui file to 'triggered' instead, then things would work on pyqt5.

But they wouldn't work on Pyqt4 anymore. And in this case it's the same problem as a comment you made over in Qtutils about UILoader not getting monkey-patched objects. Even if qtutils' abstraction layer monkey patches QActioh.triggered to map to QAction.activated in PyQt4, the abstraction layer won't get the monkey patched version.

It's probably worth investigating if we can have the uiloader use monkey-patched widgets in some way or another. There might be a way to do it cleanly, otherwise the brute-force approach is to actually mess with the PyQt5 namespace and insert the monkey patched objects there so that the UI loader imports them.

[philipstarkey]

Original comment by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington).

---

I think I spoke wrong - it looks like 'triggered' exists in Qt4 and Qt5, and that 'activated' only exists in Qt4 for backward compatibility with Qt3. So you should be able to replace 'activated' with 'triggered' in the .ui file and otherwise leave it as-is.

(still probably worth solving the monkey-patching UI loader issue, but not for this reason anymore)

[philipstarkey]

Original comment by Jan Werkmann (Bitbucket: PhyNerd, GitHub: PhyNerd).

---

Triggered works under both PyQt4 and PyQt5 for me so I'll replace activated with triggered :) thanks

[philipstarkey]

Original comment by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington).

---

Just more comments about PEP8, there are active pull requests in labscript_devices as well as code in a fork of labscript_deivces yet to be merged that would be fairly severely inconvenienced by having to merge after PEP8 whitespace changes, so it might be a good idea to turn off the PEP8 autoformatting for at least labscript_utils.

We can slot in a PEP8 formatting of labscript_devices later when it has no unmerged code.

Also, whilst it's nice to have a one-off PEP8 reformatting for all the projects, unless we enforce and automate compliance with PEP8, there is going to be a lot of commit noise if say, person a) makes a non-pep8-compliant change and then say you make a change which makes their code PEP8 compliant. One-off autoformatting is fine because it shows up clearly in the commit history and you can just keep navigating back to see who wrote some line or whatever, but if it keeps happening it starts to be pretty detrimental to the mergability of unrelated pull requests and to inspecting history.

So once each project has had a round of PEP8 formatting, further auto PEP8 formatting might not be such a great idea, unless we start requiring all pull requests to respect PEP8. We might start doing that, but until we do, it might be a good idea to disable the sublime plugins's auto-pep8 formatting (I know it's an option to disable it).

What I would really like to see is a plugin that auto-pep8-formats only the lines you've modified. That would be great - wouldn't interfere with existing code but would ensure all new code was formatted correctly. This is basically sublime's default behaviour when it comes to whitespace at the end of lines, would be nice to automatically apply it to other automatic formatting.

Also, beware of creating diffs that ignore whitespace changes, in order to create commits that lack the auto-pep8 formatting. Since some Python whitespace changes are actually syntactically significant, modifying your commits via a whitespace-ignoring diff can result in wrong code! I've been very confused by this before.

Basically all this auto formatting stuff is a royal pain in the neck for collaborative projects, so even though I'm in favour of a unified code style and I use plugins like this myself in personal projects, I'd like to be pragmatic about it rather than too idealistic, at least until the idealism can be enforced automatically for everyone.

[philipstarkey]

Original comment by Jan Werkmann (Bitbucket: PhyNerd, GitHub: PhyNerd).

---

As allready mentioned I'll create a second branch when pull request time comes around and that one won't have PEP8 active. But for now this will have to do. I've done the same thing for the lyse and runviewer pull requests way back when I created them.

But I'll look into deactivating the auto formatting anyway.

[philipstarkey]

Original comment by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington).

---

Cool, sounds great - whatever works best for your wokflow!

[philipstarkey]

Original comment by Philip Starkey (Bitbucket: pstarkey, GitHub: philipstarkey).

---

So I just want to clarify here....

Are we aiming for everything to work with both PyQt4 and PyQt5 or are we porting to just PyQt5 and dropping PyQt4 support?

[philipstarkey]

Original comment by Jan Werkmann (Bitbucket: PhyNerd, GitHub: PhyNerd).

---

We want both to work (at least for some time) and thats what my code does.

[philipstarkey]

Original comment by Philip Starkey (Bitbucket: pstarkey, GitHub: philipstarkey).

---

That's my preference too! Just wanted to make sure no one was talking at cross purposes!

[philipstarkey]

Original comment by Jan Werkmann (Bitbucket: PhyNerd, GitHub: PhyNerd).

---

I undid all the PEP 8 in all places but lyse. Lyse will follow later today.

[philipstarkey]

Original comment by Jan Werkmann (Bitbucket: PhyNerd, GitHub: PhyNerd).

---

Ok so just to update this issue:

Experimental PyQt5 support is here and it's backwards compatible(Yeahy!!! :D). This is achieved through a abstraction layer in qtutils2 that makes the parts of PyQt4 we use look like PyQt5.

I'll start PyQt5 testing next week if there are no issues with the abstraction layer under PyQt4.

Once testing for PyQt5 has started we can start investing some (more) time into the python 3 port. Lyse should already be fully Python 3 compatible after merging @chrisjbillington s unicode Python 3 pull request.

[philipstarkey]

Original comment by Jan Werkmann (Bitbucket: PhyNerd, GitHub: PhyNerd).

---

Ok so I played with Python 3 compatibility a bit:

Runmanager works and compiles (my connection table) under python 3 now.

Labscript still needs to switch the numpy array types to store unicode instead of bytes but that caused problems for me so I stuck to byte strings for now. Also there might be more problems when actually compiling scripts. I need to finish up labscript_devices to be sure.

BLACS still needs a lot of work with all the unicode it doesn't even fully start up. Most of the unicode incompatibility is currently in connections.py but who knows what more there might be.

labscript_devices is still a mess with all the print '.....' and exec '......' in globals() but I might go in there later today and at least fix up all of those and add future imports.

Here are my branches: runmanager

labscript

labscript_utils

BLACS

labscript_devices

runviewer

I could really need some help with the BLACS (and maybe labscript) stuff as it's a real pain once one thing is fixed another breaks. I'd also be happy to grant anyone write access to these branches if they want to help make python 3 for labscript happen.

Also runviewer's resampler needs to be modified and recompiled for python3 since Py_InitModule3 doesn't exist in python3.

[philipstarkey]

Original comment by Jan Werkmann (Bitbucket: PhyNerd, GitHub: PhyNerd).

---

Ok so connections is such a pain because all numpy arrays have byte strings as type but we need unicode strings in the most places(module names etc.). As we want old files to still work and since hdf5 doesn't support numpys unicode dtype we will need to add lots of decoding in connections.py I guess

[philipstarkey]

Original comment by David Meyer (Bitbucket: dihm, GitHub: dihm).

---

Speaking of numpy Unicode problems, labscript_devices is going to have a bunch of issues since dtype field names must match the native python string type. Looks like there are two approachs to support both python2 and 3. First is to wrap all the field names in str(), which won't work for us if we are going to overload str() to Unicode() for python2. The second is to specify dtypes with field names using dictionaries instead of lists when specifying dtypes. For whatever reason the dictionary accepts Unicode or bytes for field names regardless of the python used, i.e.

#!python

dtype({'names':['label','time','state'],'formats':['a256',float,np.uint8]})

For whatever reason the type string can be Unicode or bytes using the dictionary or the list method.