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

Convert to Python 3 #1407

Closed
wants to merge 12 commits into from
Closed

Convert to Python 3 #1407

wants to merge 12 commits into from

Conversation

cbrnr
Copy link
Contributor

@cbrnr cbrnr commented Apr 27, 2017

OK, here we go. I ran 2to3 and added some manual edits so that the following simple example from the documentation already works (on my Mac):

from psychopy import visual, core

win = visual.Window()
msg = visual.TextStim(win, text=u"\u00A1Hola mundo!")

msg.draw()
win.flip()
core.wait(1)
win.close()

Eventually, this will hopefully fix #1061. Right now, we should probably ignore the GUI parts since these will likely require a lot of work.

I hope that this is a first step towards getting PsychoPy to run with Python 3. Note that it will probably be inconvenient to write code that is compatible with both 2 and 3, so I suggest that once everything runs in Python 3, development will switch to that version (and PsychoPy for Python 2 will remain available but not receive any further updates).

Let me know if this approach works for you. Since this is a big one, I hope that others will contribute as well. I guess all devs already have write access to my branch, I hope GitHub makes coordinating this straightforward (I have never done that before).

@hoechenberger
Copy link
Contributor

hoechenberger commented Apr 27, 2017

Thanks for all this work, @cbrnr, this is awesome!

I hope that this is a first step towards getting PsychoPy to run with Python 3. Note that it will probably be inconvenient to write code that is compatible with both 2 and 3, so I suggest that once everything runs in Python 3, development will switch to that version (and PsychoPy for Python 2 will remain available but not receive any further updates).

There's many great Python-3-only features that cannot be used when supporting both Python 2 and 3, like keyword-only arguments or extended iterable unpacking. IPython, for example, has also made the switch to Python-3-only for that reason. Still, there will be a painful transition period, and I'm afraid it will last until Python 2 is EOL (2020). Looking very much forward to finally dumping my Python 2 installation, which I only have to keep around for PsychoPy these days!

Tests on Travis don't pass (actually don't really start running), could you look into this please?

@cbrnr
Copy link
Contributor Author

cbrnr commented Apr 27, 2017

Sure, I'd dump Python 2 support immediately if the Python 3 version worked. Just like you I only have a Python 2 env just for PsychoPy...

Regarding the Travis issue: I didn't expect that it passed because this is a very first premature step 😄. Turns out that it fails because it uses a Python 2.7 env, so we need to set up a Python 3 (preferably 3.6) environment.

@hoechenberger
Copy link
Contributor

I just realized you also got rid of all the futures etc., so the changes make the code Python3-only. I think this is not the way to go for now! I believe should aim for Python2/3 compatibility for the time being, and only transition to Python3-only at a later time.

@cbrnr
Copy link
Contributor Author

cbrnr commented Apr 27, 2017

Yes, apparently that's what the 2to3 tool does. However, like I said, it will be difficult to maintain compatibility to both versions mainly because of the string changes. I'm not super familiar with converting this stuff so that it runs on both versions, but the six package might be useful. However, this introduces yet another layer of complexity, and prevents us from using all the nice Python 3 features. Why don't we create a Python 3 only branch and try to get that to work?

@hoechenberger
Copy link
Contributor

hoechenberger commented Apr 27, 2017

Because this would essentially fork development. We'd have to freeze the Python 2 branch, otherwise we'd have to implement all new features in both branches. Yet we'd STILL have to provide bug fixes for the Python2 branch. I'm worried this might not be the best approach, given the extremely scarce development resources of this project.

@@ -2111,7 +2111,7 @@ def renameRoutine(self, name, event=None, returnName=True):
# namespace:
name = exp.namespace.makeValid(
name, prefix='routine')
if oldName in self.exp.routines.keys():
if oldName in list(self.exp.routines.keys()):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand these changes, could someone explain? Why the explicit conversion?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Python 2, d.keys() returns a list. Python 3 returns a dict_keys view. Therefore, if the old behavior was really intended, it is correct to explicitly convert the return value into a list. However, in this particular case I would simply write if oldName in self.exp.routines.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good point, thank you. I agree with you on the proposed simplification.

@hoechenberger
Copy link
Contributor

hoechenberger commented Apr 27, 2017

That said, the approach I proposed is also not very efficient. Like you already pointed out, even creating a 2/3-compatible code base is going to be difficult. It's going to introduce another layer of complexity -- the IPython project didn't remove it for nothing!

What I could realistically imagine is something like this:

  • we agree upon a time when to create the "final" Python 2 release of PsychoPy (sometime this summer???)
  • Python 2 development stops after that, entirely, and we move the entire master branch over to Python 3
  • the API remains stable throughout the process

@cbrnr
Copy link
Contributor Author

cbrnr commented Apr 27, 2017

Yep, that's what I meant 😄 - sounds like a good approach!

@mdcutone
Copy link
Member

Hello,

It would be helpful to plan and outline the 2 to 3 transition, it needs to be done someday, and attempts have been intermittent (rightfully so given the complexity).

I'm working on stereoscopic 3D libraries, they are quite substantial (new window classes, 3D object stimuli, etc.) and will likely have an unstable API for some time afterwards. If there is going to be a Python 2.x code freeze, I would like to postpone a release until there is a functioning Python 3 fork. It would be a better use of the resources I have to support only one version of PsychoPy.

@cbrnr
Copy link
Contributor Author

cbrnr commented Apr 28, 2017

It seems like this topic needs more discussion. I think we all agree that we should switch to Python 3 sooner than later, but we all have limited resources. However, we are already seeing popular packages getting developed for Python 3 only. The IPython project is a good example which PsychoPy could follow. That is, create one last Python 2 release which will be supported for some time in the future, but focus all efforts on a Python 3 only version for all future releases. In other words, I'd stop adding new features to the current version until everything works with Python 3. I also agree with @mdcutone that supporting only one Python version is a better use of our resources.

@cbrnr
Copy link
Contributor Author

cbrnr commented Apr 28, 2017

Note that monitors saved with Python 2 need to be recreated with Python 3 because they are stored in pickle objects (stored this information as JSON or something similar would be a better option IMO).

@cbrnr
Copy link
Contributor Author

cbrnr commented Apr 28, 2017

Status update: http://www.psychopy.org/coder/tutorial1.html is working.

@hoechenberger
Copy link
Contributor

Note that monitors saved with Python 2 need to be recreated with Python 3 because they are stored in pickle objects (stored this information as JSON or something similar would be a better option IMO).

Yeah I also brought this up recently for saving of experiments. A change like this would be very welcome.

@hoechenberger
Copy link
Contributor

Generally I think we should not compile such super-mega-ultra-huge commits, but "slowly" move from module to module. I guess nobody is able to review such a massive PR at once and understand its implications. Splitting this up into many small commits, one per module or so, would make things much easier.

@peircej
Copy link
Member

peircej commented Apr 28, 2017

Note that monitors saved with Python 2 need to be recreated with Python 3 because they are stored in pickle objects (stored this information as JSON or something similar would be a better option IMO).

Yeah I also brought this up recently for saving of experiments. A change like this would be very welcome.

For monitor calib files I agree with it - they are simple dict-type objects that would be easily represented in a json format (barely existed in 2003, when calibration code was first written). For ExperimentHandler it would be a big job to work out how to save such large, arbitrary objects in JSON

@peircej
Copy link
Member

peircej commented Apr 28, 2017

Clemens, thanks for testing this and seeing where the issues will crop up.
I guess it's useful to know:

  • what's achieved by the automated 2to3 and what has to be done afterwards
  • what parts of the change make things incompatible with python2 so we know what decision we're taking here. If it's doable to have code compatible with both then I'm in favour of it - forced switching is going to give headaches to many users that don't care which version of python they use). Just because another package chose to switch wholesale doesn't mean its the same decision for us if our users and code is different, so I'm keen to know what are the issues and workarounds if we were to be dual-compatible for a transition period
  • another option is whether we can create some sort of automated porting 2to3 or 3to2 that runs on a github hook. e.g. we make the master a py3 version but any pull request triggers 3to2 to run again and changes are automatically pushed back to the py2 branch

I'm guessing this is currently "kicking the tyres" for now rather than the final conversion. Until we can get wx4 working this is a non-starter for me

thanks again, Jon

@hoechenberger
Copy link
Contributor

Until we can get wx4 working this is a non-starter for me

@lindeloev had recently brought up the idea of splitting the GUI app development from the library (unfortunately I cannot seem to find his comment and @peircej's response anymore). Maybe it's time to reconsider this possibility?

@peircej
Copy link
Member

peircej commented Apr 29, 2017

Splitting the app from the lib is going to be tricky, and I don't think it's worth the effort. But it also doesn't alter the current situation. I would still need both the lib and the app to be compatible before either of them switch, whether or not they are in a single repos.

@mdcutone
Copy link
Member

I agree with @peircej, splitting the project will be very difficult to manage. In the case of Psychopy, GUI and libs will be using two different python versions which complicates things for deployment and development.

Creating an outline of required tasks and have developers assign themselves should better coordinate this transition process.

@cbrnr
Copy link
Contributor Author

cbrnr commented May 2, 2017

The staircase example from the coder tutorial works. Note that gui must be imported before any other modules, otherwise the script hangs (must be some kind of circular import or something).

@cbrnr
Copy link
Contributor Author

cbrnr commented May 2, 2017

Let's not implement any major changes before the Python 3 conversion (although I think splitting psychopy into a lib and a GUI part is a good idea).

So far, I managed to get some examples from the tutorial to work. Even wxPython Phoenix works (but the gui module needs to be imported first for whatever reason). Note that wxPython doesn't work with Anaconda Python on a Mac, so anyone wanting to try my changes please use either the official Python 3 installer or Homebrew for now (I'm trying to figure out how to get it to work with Anaconda in the meantime).

@cbrnr
Copy link
Contributor Author

cbrnr commented May 2, 2017

The GUI parts now start with Python 3 (builder and coder).

@peircej
Copy link
Member

peircej commented May 11, 2017

I've added your commits to branch python3 in upstream. They can't be pulled into master anytime soon.
In the longer term I can see the possibility of doing it but I still agree that we should aim for dual compatibility where it's going to be possible.

@cbrnr
Copy link
Contributor Author

cbrnr commented May 11, 2017

Thanks @peircej I'll make sure to track this branch.

@peircej peircej closed this May 11, 2017
@cbrnr
Copy link
Contributor Author

cbrnr commented May 11, 2017

Where do we discuss issues related to Python 3 now that you've closed this PR? I was planning on pushing more changes...

@peircej
Copy link
Member

peircej commented May 11, 2017

I closed it because there's no short-term intention of merging with the master branch. Can you submit a pull request to the python3 branch instead? We can refer back to this one from there. I could reopen this if you like tho

@cbrnr
Copy link
Contributor Author

cbrnr commented May 11, 2017

Maybe it is easiest if we just keep this PR open and use my python3 branch for future commits? You (and other project maintainers) can push to my branch anyway. I'd also make sure to keep this branch in sync with the current master.

@peircej
Copy link
Member

peircej commented May 11, 2017

ah, actually it looks like I can't do that. The 'reopen' button or is commented out and complains that the "branch was force-pushed recreated". Back to my prev plan then; submit a pull request from your branch to the psychopy/python3 branch and we can reconvene there

@mmagnuski
Copy link
Contributor

Sorry if I've not followed the discussion close enough - but is there no possibility to make psychopy py2&3 compatible? Many projects have six as their internal dependency (ie. they ship with six onboard) - maybe it would be worth trying with six here too? I would be very happy if only the non-gui part was py2&3 compatible - as I don't use the gui.

@jona-sassenhagen
Copy link

I'm in the same boat as @mmagnuski .

@peircej
Copy link
Member

peircej commented May 11, 2017

My feeling is that we should only decide to drop the py2 support in places where it's actually needed.

Many of the changes that @cbrnr has made will "just work" in both places e.g.:

  • iterating over keys in a dict

Some of the changes the 2to3 has made are quite annoying and break things unnecessarily e.g.:

  • removing from __future__ lines seems totally unnecessary
  • removing the u"" for a unicode is unnecessary if we're happy not to support py3.2 (it was reintroduced in python3.3 so it's no longer an error as far as I'm aware even though it's obviously unecessary)

A few further changes would really not be a big deal to have a line that says:

if PY2:
    doThis()
else:
    doThat()

BUT we might some pieces of code where writing the dual-compatible version will be hard. I don't know. i.e. I'm not ready to commit either way. If there is a place that requires break compatibility then we'll do it.

I do think we should proceed with changes that support both options where possible and keep an eye out for places where that's a problem.

@cbrnr
Copy link
Contributor Author

cbrnr commented May 12, 2017

I don't think there's a need to put that extra burden on us to make the code compatible with both 2 and 3. Of course, if there is an easy way that works in both versions without too much additional work we should go for it, but that's not going to be the case all the time. I'd rather not start with branches of code conditioned on the version of Python, this soon gets pretty ugly (instead, the six library could be used).

Consider the users of PsychoPy. Most people will likely want to use the GUI and just install the prebuilt installer packages, which come with their own Python version. These people probably won't care if this Python version is 2 or 3. Others who want to use PsychoPy in their scripts (like most of the people here in this discussion) will use Python 3, so they too shouldn't care about Python 2 compatibility. Do you think there is one single good reason why anyone would still need Python 2 support for PsychoPy? I doubt that a machine that only supports Python 2 (such as some arcane commercial Linux versions like Red Hat) is really suitable for presenting stimuli with PsychoPy in the first place...

So unless there are dependencies that don't work with Python 3 there is no reason to keep supporting Python 2.

Of course anyone is welcome to put in the extra effort of looking through all changes that 2to3 made to make it work with Python 2, but this tool did most of the changes. This cheatsheet is a pretty nice summary of how to write nice code that runs on both versions.

Finally, this website lists projects that will remove Python 2 support by 2020. I really like the idea of releasing one last LTS version for Python 2 and stick with Python 3 for all newer version.

@peircej
Copy link
Member

peircej commented May 12, 2017

if there is an easy way that works in both versions without too much additional work we should go for it, but that's not going to be the case all the time

This is asserted but not yet clear to me. When I wrote pyosf I wrote it to be cross-compatible. In 2400 lines of code I needed just 2 if PY3... statements. It really wasn't hard. It might be harder with PsychoPy but let's start with dual compatible code (which can be developed in master) and discuss it when we get to an actual problem.

I'd rather not start with branches of code conditioned on the version of Python

I agree. The aim of cross-compatible code is the only way I see to avoid this. If the codebase fully drops support for py2 then there must be a period both branches while we troubleshoot the py3 code.

Consider the users of PsychoPy.

I really really do. More than most.

Most people will likely want to use the GUI and just install the prebuilt installer packages, which come with their own Python version. These people probably won't care if this Python version is 2 or 3

A user that has no code components (e.g. no print statements) in their experiment will be unaffected. Any Builder user with a print statement will get an error and they'll be confused. Sure, they will have to deal with this eventually but we could make the transition easier for them.

Others who want to use PsychoPy in their scripts (like most of the people here in this discussion) will use Python 3

You're talking about coding enthusiasts. There is another massive sector of users that prefer to write code but it's a struggle and they don't relish changing it (ever). They do care about python2 and they are the majority. Note that of the 16,000 monthly PsychoPy users only 70 people have ever made a contribution to the project. The people involved with psychopy on github are not the "average" user and we shouldn't forget what that "average" user is actually like.

Again, we're agreed they'll have to switch one day but Im' hoping we don't need to choose for them when that day is.

@cbrnr
Copy link
Contributor Author

cbrnr commented May 12, 2017

This is asserted but not yet clear to me. When I wrote pyosf I wrote it to be cross-compatible. In 2400 lines of code I needed just 2 if PY3... statements. It really wasn't hard. It might be harder with PsychoPy but let's start with dual compatible code (which can be developed in master) and discuss it when we get to an actual problem.

It's easier if you start from scratch. It's way more difficult if you have such a large project as PsychoPy.

A user that has no code components (e.g. no print statements) in their experiment will be unaffected. Any Builder user with a print statement will get an error and they'll be confused. Sure, they will have to deal with this eventually but we could make the transition easier for them.

I disagree. They should have to deal with it right now.

You're talking about coding enthusiasts. There is another massive sector of users that prefer to write code but it's a struggle and they don't relish changing it (ever). They do care about python2 and they are the majority. Note that of the 16,000 monthly PsychoPy users only 70 people have ever made a contribution to the project. The people involved with psychopy on github are not the "average" user and we shouldn't forget what that "average" user is actually like.

Yes, I agree that people contributing here are certainly not the average user. Many people will have existing experiments that they won't want to convert. However, there will always be that last Python 2 LTS version they can use.

Again, we're agreed they'll have to switch one day but Im' hoping we don't need to choose for them when that day is.

I disagree. The average user is neither aware of the ongoing Python 2/3 transition, and nor should they have to care. The developers must make this decision, otherwise it will be difficult to maintain this code before long.

Anyway, I'm happy to contribute more, but it would be good to have a final roadmap/decision on how you would like to go about it. Postponing the decision won't help at the end of the day.

@peircej
Copy link
Member

peircej commented May 12, 2017

Anyway, I'm happy to contribute more, but it would be good to have a final roadmap/decision on how you would like to go about it. Postponing the decision won't help at the end of the day.

The decision is:

  • we keep compatibility where possible. If we find places where that isn't possible we debate it about those specific issues.
  • if future code gradually becomes incompatible that's fine (i.e some new features may be restricted to py3 installations only)
  • at some point (I'm not going to decide a date now) I will get tired of releasing Py2 standalone releases and I'll stop.

peircej added a commit to peircej/psychopy that referenced this pull request May 12, 2017
This is a superset of the changes in psychopy#1407
(thanks @cbrnr for revealing this cool stuff!)

 - py3 dict.keys() doesn't return a list so do one of:
   - list(myDict.keys())
   - list(myDict)  # easier but less explicit

Also sorted() func has existed since python2.4 so instead of:

  keys = myDict.keys()
  keys.sort()

we can do:

  keys = sorted(myDict)  # sweet!
@cbrnr
Copy link
Contributor Author

cbrnr commented May 12, 2017

And BTW, Python 2 support means 2.7 - or do you care for older versions (I hope not)?

@peircej
Copy link
Member

peircej commented May 12, 2017

Py2.7 is all I care about :-)

Looks like this will be a handy alternative to 2to3:
http://python-future.org/quickstart.html
It's basically doing the 2to3 conversion but then fixing back to allow py2 to work again (with things like __future__ imports)

@mdcutone
Copy link
Member

Thanks @peircej and @cbrnr for addressing this.

Jon touched upon an issue that we need to consider when working on this project. The reality is that programming, while seemingly necessary, is not a compulsory requirement for undergraduate (and some graduate) psychology students in many universities. If they pick up any programming experience in graduate studies, its usually only enough to re-purpose some existing experiments (either an RA or past-colleague wrote) without understanding much about the underlying hardware. The average user of any stimulus generating software needs something that "just works" in most cases.

I usually write compatible code out of habit. In fact, the stereoscopy libs should work with both versions (2.7 and 3.x).

@hoechenberger
Copy link
Contributor

hoechenberger commented Jun 10, 2017

@peircej, python-future looks great, this could be the solution of choice!

btw it's starting to become more and more difficult for me to use PsychoPy, as I sometimes require new features from other packages that are not available / not fully supported on Python 2 anymore. So for me, personally, the urgency of the conversion is increasing :)

@hoechenberger
Copy link
Contributor

(In fact, I'm probably going to use expyriment for my next study for this reason)

@peircej
Copy link
Member

peircej commented Jun 12, 2017

Ouch! ;-)

OK, I'll try and spend some time on it this morning. I've tried to pull in some of @cbrnr commits but they break too much existing code. I think I'll have to use python-future instead and then use his commits (above) as a reference for when I hit problems

@cbrnr
Copy link
Contributor Author

cbrnr commented Jun 12, 2017

Let me know if I can help. Most of the stuff I did after the script should also work after running python-future. One big issue will be the switch to wxPython 4 - I suggest that we don't try to maintain compatibility between both the old wxPython and Phoenix but focus on the latter.

@peircej
Copy link
Member

peircej commented Jun 12, 2017

I suggest that we don't try to maintain compatibility between both...

Sorry, but I'm going to disagree with you here. We should try and possibly find that we fail, in which case we give up. But generally this is going to be a matter of an import statement that has try...except at the top of the script. In my experience (supporting previous changes betwee wx versions) it isn't generally a big problem. Users don't like having to change their system, so we should try to be compatible with any version.

@cbrnr
Copy link
Contributor Author

cbrnr commented Jun 12, 2017

I don't have experience with wxPython, but maybe the changes in 4.0.0 aren't that big. I'd just be careful not to try to support too much old stuff, at some point this will become unmanageable. E.g. will there be support for Python 2 and 3, wxPython 4 and <4, possibly PyQt and PySide (not to think about Qt 5 vs. Qt 4), etc. At some point users will need to start updating their systems if they want to continue to use a piece of software. IMHO.

@peircej
Copy link
Member

peircej commented Jun 12, 2017

I'd just be careful not to try to support too much old stuff, at some point this will become unmanageable

I've done this for a while now and in my experience it really isn't unmanageable. Yes, it sometimes means very slightly uglier code but I expect the (few dozen) developers to cope with that in order to save the (tens of thousands of) users some pain.
For example, we do currently support both Qt4 and Qt5 and it isn't that big a deal:
https://github.com/psychopy/psychopy/blob/master/psychopy/gui/qtgui.py

@cbrnr
Copy link
Contributor Author

cbrnr commented Jun 12, 2017

I've done this for a while now and in my experience it really isn't unmanageable.

OK, that's good to hear.

Yes, it sometimes means very slightly uglier code but I expect the (few dozen) developers to cope with that in order to save the (tens of thousands of) users some pain.

Here I'm going to disagree with you on both points 😄 .

For example, we do currently support both Qt4 and Qt5 and it isn't that big a deal:
https://github.com/psychopy/psychopy/blob/master/psychopy/gui/qtgui.py

Maybe it's no big deal most of the time, but if you have dozens of no big deals this still adds up. BTW, there's a package for abstracting various Qt versions: https://pypi.python.org/pypi/QtPy

@peircej
Copy link
Member

peircej commented Jun 15, 2017

master branch is now pretty much compatible with all of py2/py3/wx3/wx4/qt4/qt5

Thanks for your help Clemens. Some of your changes I cherry-picked across but others were, at least, helpful in finding the issues.

I'm sure there are still bugs to find but most of the demos appear to work and most of the windows in the app appear to load (and for py2 all tests still pass). If there are any doubts about how to keep things dual-compatible then just ask.

@mihaic
Copy link

mihaic commented Jul 14, 2017

@peircej, considering your last comment, will you be making a Python 3 release on PyPI?

@peircej
Copy link
Member

peircej commented Jul 17, 2017

It will eventually be released on PyPI but not soon. For now I would not consider this safe for release until we've spent several months testing it

@mwaskom mwaskom mentioned this pull request Jan 22, 2018
8 tasks
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

Successfully merging this pull request may close these issues.

refactor to support python 3
7 participants