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

FIX Python 3 compatibility #1361

Merged
merged 4 commits into from Nov 14, 2012
Merged

FIX Python 3 compatibility #1361

merged 4 commits into from Nov 14, 2012

Conversation

astaric
Copy link
Contributor

@astaric astaric commented Nov 13, 2012

Fixed old-style imports, a bug that prevented scikit-learn from importing in python 3.2 and some failing tests.

@amueller
Copy link
Member

Thanks for the pull request @astaric.
Does this code still work with Python 2.6?
Do you use the code using 2to3? I think another way is not possible at the moment, right?
Ping @ogrisel, our Python 3 expert.

@GaelVaroquaux
Copy link
Member

By the way, some of the modifications are in joblib. I am working on
making joblib more Python 3 friendly (I did not say fully working...
there's more effort there. Don't spend any efforts on joblib, as a
release will soon address this.

G

@astaric
Copy link
Contributor Author

astaric commented Nov 13, 2012

All tests still pass if run in python 2.6 or 2.7. (with nupy 1.6.2, scipy 0.11)
To install on python 3.2, I just used the existing functionality in setup.py that performs the 2to3 conversion and builds everything.

@amueller
Copy link
Member

Thanks, great. I just realized that our jenkins bot testing python 3 compatibility doesn't run at the moment, which explains why it is not working for you.
Unfortunately we are a bit behind on our python3 support.
Could you please remove the changes you did to joblib? as @GaelVaroquaux said, he manages this in another repository and we just include it from there.

@ogrisel could you please have a look at this then?

@GaelVaroquaux
Copy link
Member

On Tue, Nov 13, 2012 at 12:11:56PM -0800, Andreas Mueller wrote:

Could you please remove the changes you did to joblib?

Just don't worry about this. I've just committed a huge amount of fixes
for joblib to the main development repo:
https://github.com/joblib/joblib
and the tests now all pass under Python 3.2 (no skips :} )
https://travis-ci.org/joblib/joblib

I'll wait a bit, and then do a release and merge them in scikit-learn. In
the mean time, your fixes are useful, thanks!

I was just mentioning this to tell you not to spend too much time on
joblib :)

@ogrisel
Copy link
Member

ogrisel commented Nov 13, 2012

Yeah the jenkins build is disabled because we never found the time to finish the port so it was just wasting cpu cycles and failing over and over again.

BTW I would like to get rid of the 2to3 translation stuff and just have a single code base that supports both 2.6 / 2.7 and 3.3+. I had started a branch a while ago (during last pycon: https://github.com/ogrisel/scikit-learn/tree/py3k ) but it got stalled. All tests in scikit-learn were passing both in 2.6, 2.7 and 3.2 but there were remaining failures in joblib. Now that python 3.3 reintroduced the u"" notation for unicode string literals it will even be simpler to have single codebase support for both 2 and 3.

@astaric
Copy link
Contributor Author

astaric commented Nov 14, 2012

I removed the changes I made to joblib, it was a nice git exercise :).

A single code base sounds like a good idea, but in the mean time, I will still try to fix-up the current master version so I can use it in pyton 3.2.

@GaelVaroquaux
Copy link
Member

A single code base sounds like a good idea, but in the mean time, I will still
try to fix-up the current master version so I can use it in pyton 3.2.

Yes, this seems about right. I'll try to review your changes and merge
them in soon. Also, the joblib changes should be in in a few days, which
hopefully means that you'll have something that is usable under Python 3.

Thanks for pushing ahead with the Python 3 issues. This is useful!

@astaric
Copy link
Contributor Author

astaric commented Nov 14, 2012

I also managed to patch setup.py to make scikit-learn installation on python 3 play nicely with pip. Should I add the change to this pull request or open another one?

@ogrisel
Copy link
Member

ogrisel commented Nov 14, 2012

Thanks @astaric, can you please confirm that the tests still pass in 2.6 and 2.7 in this PR?

I also managed to patch setup.py to make scikit-learn installation on python 3 play nicely with pip. Should I add the change to this pull request or open another one?

Please open a new one so that we can already merge this one quickly independently.

@astaric
Copy link
Contributor Author

astaric commented Nov 14, 2012

I have rerun all the tests in 2.6 and 2.7 and they still pass.

@ogrisel
Copy link
Member

ogrisel commented Nov 14, 2012

Ok +1 for merging then.

@amueller
Copy link
Member

+1 for merge

ogrisel added a commit that referenced this pull request Nov 14, 2012
FIX Python 3 compatibility
@ogrisel ogrisel merged commit a345761 into scikit-learn:master Nov 14, 2012
@ogrisel
Copy link
Member

ogrisel commented Nov 14, 2012

Thanks!

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.

None yet

4 participants