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

Python 3 compatilibility work #663

Merged
merged 44 commits into from Sep 9, 2011

Conversation

Projects
None yet
4 participants
@takluyver
Member

takluyver commented Aug 2, 2011

The eventual aim is to make one codebase that can be installed on Python 3 using 2to3. This doesn't complete that, but it does a substantial chunk of the work, and I think it's a good time to stop and go over it.

Most of the compatibility layer is in a new IPython.utils.py3compat module.

Stuff I know isn't yet done:

  • doctests (might not be needed - 2to3 installation with distribute should translate them automatically, but will it catch IPython doctests?)
  • Script names (e.g. ipython --> ipython3)
  • What do we do about external libraries? In particular, I've made changes to pexpect and simplegeneric for Python 3. I'm happy to push changes upstream, but that could take a while, and for pexpect would probably involve dropping some backwards comatibility (the pexpect website still mentions Python 1.5!)

Full view of the manual changes I've made for Python 3:
jupyter-attic/ipython-py3k@raw-2to3...master

Also, I found we had some more-or-less duplicate code for breaking down user input, in inputsplitter and prefilter. I've made them use a common version.

One test is failing at present: IPython.core.tests.test_inputsplitter.test_split_user_input, which attempts to break down the line u"Pérez Fernando". My gut feeling is that it's OK to change the test itself, because there's nothing intrinsically 'correct' about the result it gave before; but perhaps I'm missing something.

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Aug 2, 2011

Member

One other problem - the coloured display of the source you get with func?? doesn't appear properly (you just see the first character). It does work with %psource func, though. I'll look into this.

Member

takluyver commented Aug 2, 2011

One other problem - the coloured display of the source you get with func?? doesn't appear properly (you just see the first character). It does work with %psource func, though. I'll look into this.

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Aug 14, 2011

Member

OK, it now catches errors loading the default config file, and resets exec_lines to an empty list. But when I do ipython3 profile create (or ipython3 profile create python3), it creates the full default config file in .ipython/profile_python3. Doing ipython profile create python3 copies the file across as I'd expect, so I guess it's because python3 is the default profile for ipython3.

Member

takluyver commented Aug 14, 2011

OK, it now catches errors loading the default config file, and resets exec_lines to an empty list. But when I do ipython3 profile create (or ipython3 profile create python3), it creates the full default config file in .ipython/profile_python3. Doing ipython profile create python3 copies the file across as I'd expect, so I guess it's because python3 is the default profile for ipython3.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Aug 16, 2011

Member

Hey Thomas, sorry we let this one go stale, but right now it doesn't merge anymore. Could you rebase it against master and push again? I'll be happy to review it then so we can flush it quickly.

Member

fperez commented Aug 16, 2011

Hey Thomas, sorry we let this one go stale, but right now it doesn't merge anymore. Could you rebase it against master and push again? I'll be happy to review it then so we can flush it quickly.

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Aug 16, 2011

Member

Done. There were just a couple of small collisions. This can now be installed and run on Python 3, although I haven't tested it extensively. With Python 3, it now installs setuptools-style entry points as ipython3 etc. I've patched simplegeneric and sent the diff upstream, but not heard anything yet.

Before merging, we should consider the failing test for splitting user input containing unicode (I think it's alright to change the test, but I'm not sure why it was written).

Other questions I haven't yet addressed, but needn't block merging this: I need to work out a way to convert doctests, which currently cause a lot of failures. Pexpect will need some patches to work in Python 3. We should also give a bit of thought to how the python3 profile should fit in.

Member

takluyver commented Aug 16, 2011

Done. There were just a couple of small collisions. This can now be installed and run on Python 3, although I haven't tested it extensively. With Python 3, it now installs setuptools-style entry points as ipython3 etc. I've patched simplegeneric and sent the diff upstream, but not heard anything yet.

Before merging, we should consider the failing test for splitting user input containing unicode (I think it's alright to change the test, but I'm not sure why it was written).

Other questions I haven't yet addressed, but needn't block merging this: I need to work out a way to convert doctests, which currently cause a lot of failures. Pexpect will need some patches to work in Python 3. We should also give a bit of thought to how the python3 profile should fit in.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Aug 17, 2011

Member

no, that failing test is indeed a problem. I'm not sure what's going on, but it's really a breakage. In master, we get, correctly:

In [5]: from IPython.core import inputsplitter as isp

In [6]: isp.split_user_input(u'Pérez Fernando')
Out[6]: (u'', '', u'', u'P\xe9rez Fernando')

but with these changes, we get:

In [1]: from IPython.core import inputsplitter as isp

In [2]: isp.split_user_input(u'Pérez Fernando')
Out[2]: (u'', '', u'P', u'\xe9rez Fernando')

So the input is definitely being incorrectly split, because the third output value should be an empty string and we're getting a 'P' in there.

I'm too tired to dig into this one, but we do need to fix this one before merging, not changing the test. If you make any progress let me know...

Member

fperez commented Aug 17, 2011

no, that failing test is indeed a problem. I'm not sure what's going on, but it's really a breakage. In master, we get, correctly:

In [5]: from IPython.core import inputsplitter as isp

In [6]: isp.split_user_input(u'Pérez Fernando')
Out[6]: (u'', '', u'', u'P\xe9rez Fernando')

but with these changes, we get:

In [1]: from IPython.core import inputsplitter as isp

In [2]: isp.split_user_input(u'Pérez Fernando')
Out[2]: (u'', '', u'P', u'\xe9rez Fernando')

So the input is definitely being incorrectly split, because the third output value should be an empty string and we're getting a 'P' in there.

I'm too tired to dig into this one, but we do need to fix this one before merging, not changing the test. If you make any progress let me know...

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Aug 17, 2011

Member

I know it changes what happens in that case, but I'm not sure why the behaviour in master is more correct. Non-ascii characters outside a string are invalid in Python 2 anyway, so does it matter how we split up the invalid input?

Member

takluyver commented Aug 17, 2011

I know it changes what happens in that case, but I'm not sure why the behaviour in master is more correct. Non-ascii characters outside a string are invalid in Python 2 anyway, so does it matter how we split up the invalid input?

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Aug 17, 2011

Member

More generally, split_user_input looked like it was designed to break out the first valid Python identifier (from a comment: "fpart has to be a valid python identifier"), optionally with IPython escape characters. But it didn't do this very well, so x =1 would be split, but x=1 wouldn't. I tweaked the regex a bit so that it behaved more consistently. So when presented with u'Pérez Fernando', it breaks out P as the first valid Python identifier. Is there some better definition of how it should behave?

Member

takluyver commented Aug 17, 2011

More generally, split_user_input looked like it was designed to break out the first valid Python identifier (from a comment: "fpart has to be a valid python identifier"), optionally with IPython escape characters. But it didn't do this very well, so x =1 would be split, but x=1 wouldn't. I tweaked the regex a bit so that it behaved more consistently. So when presented with u'Pérez Fernando', it breaks out P as the first valid Python identifier. Is there some better definition of how it should behave?

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Aug 18, 2011

Member

You're actually right, now that I looked at it more carefully. Go ahead and update the PR so that the test passes.

I've looked it over and I don't see any problems, but since it's pretty big, let me run using this as my default branch for a day and see if I notice any problems. If all looks OK we'll go ahead and merge it.

@mirnk, @ellisonbg: if you have any objection to this one, let us know. It's a fairly large set of changes, but it will make py3 support much easier in the future, and I think Thomas did a great job of isolating the compatibility hacks to very well-defined locations. So unless I find any problems in actual use (I've reviewed it and ran the test suite), I'll merge it in a day or so.

Member

fperez commented Aug 18, 2011

You're actually right, now that I looked at it more carefully. Go ahead and update the PR so that the test passes.

I've looked it over and I don't see any problems, but since it's pretty big, let me run using this as my default branch for a day and see if I notice any problems. If all looks OK we'll go ahead and merge it.

@mirnk, @ellisonbg: if you have any objection to this one, let us know. It's a fairly large set of changes, but it will make py3 support much easier in the future, and I think Thomas did a great job of isolating the compatibility hacks to very well-defined locations. So unless I find any problems in actual use (I've reviewed it and ran the test suite), I'll merge it in a day or so.

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Aug 18, 2011

Member

Thanks, Fernando. I've updated it, and the there are no more test failures on Python 2.

(Tagging @minrk because a typo missed him above. Let us know of any objections to these changes.)

Member

takluyver commented Aug 18, 2011

Thanks, Fernando. I've updated it, and the there are no more test failures on Python 2.

(Tagging @minrk because a typo missed him above. Let us know of any objections to these changes.)

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Aug 18, 2011

Member

Thanks for catching the typo. If @ellisonbg and @minrk have no objections, I'll merge this one tonight (unless I find a glitch in usage today). I've been using it as my normal ipython and haven't seen any problems so far...

Member

fperez commented Aug 18, 2011

Thanks for catching the typo. If @ellisonbg and @minrk have no objections, I'll merge this one tonight (unless I find a glitch in usage today). I've been using it as my normal ipython and haven't seen any problems so far...

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Aug 19, 2011

Member

I just did a PR against @takluyver's branch to localise the Session as bytes/unicode, so we don't have to call encode('ascii') all over the place.

Member

minrk commented Aug 19, 2011

I just did a PR against @takluyver's branch to localise the Session as bytes/unicode, so we don't have to call encode('ascii') all over the place.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Aug 19, 2011

Member

Awesome, thanks! And @takluyver: given that this branch does have a few conflicts with the notebook stuff, which we really need for Euroscipy (Stefan and I are planning on using it), let's delay this merge until the nb branch lands. There will be a few small conflicts to fix (just stuff in setupbase, I think) and then we can merge it afterwards.

Member

fperez commented Aug 19, 2011

Awesome, thanks! And @takluyver: given that this branch does have a few conflicts with the notebook stuff, which we really need for Euroscipy (Stefan and I are planning on using it), let's delay this merge until the nb branch lands. There will be a few small conflicts to fix (just stuff in setupbase, I think) and then we can merge it afterwards.

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Aug 19, 2011

Member

OK, great. I'll get Min's bsession changes merged into this branch in the meantime, and possibly investigate converting the doctests.

Member

takluyver commented Aug 19, 2011

OK, great. I'll get Min's bsession changes merged into this branch in the meantime, and possibly investigate converting the doctests.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Aug 19, 2011

Member

Fantastic. The nb branch is a lot of work and will still take us a day or two to mege, so we may just get to actually merging this when we're in Paris, which isn't the end of the world. Between now and Tuesday I've basically run out of available time for just about anything.

Member

fperez commented Aug 19, 2011

Fantastic. The nb branch is a lot of work and will still take us a day or two to mege, so we may just get to actually merging this when we're in Paris, which isn't the end of the world. Between now and Tuesday I've basically run out of available time for just about anything.

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Aug 27, 2011

Member

Rebased after notebook branch merged.

Member

takluyver commented Aug 27, 2011

Rebased after notebook branch merged.

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Sep 3, 2011

Member

Rebased again.

Member

takluyver commented Sep 3, 2011

Rebased again.

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Sep 7, 2011

Member

Rebased again.

Member

takluyver commented Sep 7, 2011

Rebased again.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Sep 8, 2011

Member

What still needs to be done here?

If all tests pass on 2.6, it would seem that this is ready for merge, unless there are specific tasks that need doing.

Member

minrk commented Sep 8, 2011

What still needs to be done here?

If all tests pass on 2.6, it would seem that this is ready for merge, unless there are specific tasks that need doing.

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Sep 8, 2011

Member

I'd like to get it merged in. There's some more things to do (doctests, pexpect), but the changes that are already here keep ending up in conflict with changes in master. I should test on 2.6, though - so far I've only been testing on 2.7.

Member

takluyver commented Sep 8, 2011

I'd like to get it merged in. There's some more things to do (doctests, pexpect), but the changes that are already here keep ending up in conflict with changes in master. I should test on 2.6, though - so far I've only been testing on 2.7.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Sep 9, 2011

Member

OK, I've just run the tests on 2.6, and all looks good. Since we've had this in the wings for so long, I'm merging it now.

Thomas, thanks a lot for the great work! I hope having this now in trunk will make your life significantly easier re. py3 support. Please do yell at us if you see us doing things that make py3 support harder in master: since we don't use it regularly, we tend to slip up easily :)

Member

fperez commented Sep 9, 2011

OK, I've just run the tests on 2.6, and all looks good. Since we've had this in the wings for so long, I'm merging it now.

Thomas, thanks a lot for the great work! I hope having this now in trunk will make your life significantly easier re. py3 support. Please do yell at us if you see us doing things that make py3 support harder in master: since we don't use it regularly, we tend to slip up easily :)

fperez added a commit that referenced this pull request Sep 9, 2011

Merge pull request #663 from takluyver/py3compat
Python 3 compatibility work.  This doesn't fully get us to a single codebase supporting py2/3 at install time, but it does make significant progress in that direction.

@fperez fperez merged commit 37dd091 into ipython:master Sep 9, 2011

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Sep 9, 2011

Member

Thanks, Fernando, that will definitely make my life easier. I'll try to keep checking it.

When you've got a moment, can you also modify the description line at https://github.com/ipython/ipython-py3k to say it's superseded by work in the main repo. Leave it around for the moment, though, because the diff might still be useful.

Member

takluyver commented Sep 9, 2011

Thanks, Fernando, that will definitely make my life easier. I'll try to keep checking it.

When you've got a moment, can you also modify the description line at https://github.com/ipython/ipython-py3k to say it's superseded by work in the main repo. Leave it around for the moment, though, because the diff might still be useful.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Sep 9, 2011

Member

On Fri, Sep 9, 2011 at 1:32 AM, Thomas
reply@reply.github.com
wrote:

When you've got a moment, can you also modify the description line at https://github.com/ipython/ipython-py3k to say it's superseded by work in the main repo. Leave it around for the moment, though, because the diff might still be useful.

Done, let me know if you need any other changes...

Member

fperez commented Sep 9, 2011

On Fri, Sep 9, 2011 at 1:32 AM, Thomas
reply@reply.github.com
wrote:

When you've got a moment, can you also modify the description line at https://github.com/ipython/ipython-py3k to say it's superseded by work in the main repo. Leave it around for the moment, though, because the diff might still be useful.

Done, let me know if you need any other changes...

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Sep 9, 2011

Member

There's now a big problem. We can no longer run any files that are not utf8 encoded. Python itself respects the encoding header, but since we now hardcode the encoding in py3compat.open, all other codings are simply ignored. Fixed in PR #778. I'll merge right away if I get an okay, as it's a very serious issue.

Member

minrk commented Sep 9, 2011

There's now a big problem. We can no longer run any files that are not utf8 encoded. Python itself respects the encoding header, but since we now hardcode the encoding in py3compat.open, all other codings are simply ignored. Fixed in PR #778. I'll merge right away if I get an okay, as it's a very serious issue.

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Merge pull request #663 from takluyver/py3compat
Python 3 compatibility work.  This doesn't fully get us to a single codebase supporting py2/3 at install time, but it does make significant progress in that direction.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment