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

Changes to support Python 3 #994

Closed
wants to merge 5 commits into from
Closed

Changes to support Python 3 #994

wants to merge 5 commits into from

Conversation

irustandi
Copy link

Tested on Anaconda Python running Python 2.7 and Python 3.5 running on Ubuntu. In python/Makefile, PYTHON_VERSION needs to be modified to the appropriate version. Learning_to_Search.ipynb not converted yet (when run on Python 2.7 and 3.5, it crashes the kernel).

@coveralls
Copy link

Coverage Status

Coverage remained the same at 69.022% when pulling 17755b7 on irustandi:master into 2a74ccb on JohnLangford:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 69.022% when pulling 17755b7 on irustandi:master into 2a74ccb on JohnLangford:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 69.028% when pulling b9eb91f on irustandi:master into 9f9ac62 on JohnLangford:master.

@gramhagen
Copy link
Contributor

gramhagen commented May 11, 2016

I get this error trying to build using python and boost installed via homebrew, any thoughts on how to make sure this works for anaconda and brew installs on os x?

/usr/bin/g++ -shared  pylibvw.o -L /usr/local/lib -L/usr/local/Cellar/python3/3.5.1/Frameworks/Python.framework/Versions/3.5/lib/python3.5/config-3.5m -lpython3.5m -ldl -framework CoreFoundation ../vowpalwabbit/libvw.a ../vowpalwabbit/liballreduce.a -L /usr/local/lib -lboost_program_options-mt -lboost_serialization-mt -l pthread -l z -l boost_python -o pylibvw.so
Undefined symbols for architecture x86_64:
  "boost::python::detail::init_module(PyModuleDef&, void (*)())", referenced from:
      _PyInit_pylibvw in pylibvw.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[1]: *** [pylibvw.so] Error 1
make: *** [python] Error 2

@irustandi
Copy link
Author

Check and see if you're linking against libboost-python.so built for Python 3.5 or for other versions of Python. Very likely you're linking against libboost-python.so built for Python 2.7.

@@ -8,7 +8,8 @@
# $(error Please run 'make' at the top level only)
#endif

PYTHON_VERSION = 2.7
# change this to 2.7 for Python 2.7
PYTHON_VERSION = 3
PYTHON_INCLUDE = $(shell python$(PYTHON_VERSION)-config --includes)
PYTHON_LDFLAGS = $(shell python$(PYTHON_VERSION)-config --ldflags)

Copy link
Contributor

Choose a reason for hiding this comment

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

I had to change this line to get it to build appropriately for 3 or 2.7

ifeq "$(PYTHON_VERSION)" "3"
    PYTHON_LIBS = -l boost_python3
else
    PYTHON_LIBS = -l boost_python
endif

Copy link
Author

Choose a reason for hiding this comment

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

The "-l boost_python3" part cannot be counted on to work in all systems. In my Ubuntu system using Boost from the APT package libboost-all-dev, there's no libboost_python3.so. libboost_python.so is just a symbolic link that originally points to Boost Python library for Python 2.7, I manually changed this symbolic link to point to Boost Python library for Python 3.5.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, that's unfortunate. I wonder if there's another way to find the correct library automatically?

@gramhagen
Copy link
Contributor

yes that was it, thanks

@gramhagen
Copy link
Contributor

also for posterity, if we're going to switch to python3 as the default in the Makefile when people run into this error:

ImportError: dynamic module does not define module export function (PyInit_pylibvw)

it means you are trying to run the pylibvw compiled for python3 with python2

@JohnLangford
Copy link
Member

@hal3 Can you take a close look at this?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 69.203% when pulling ed84beb on irustandi:master into 404cd80 on JohnLangford:master.

@JohnLangford
Copy link
Member

I want to have a broader discussion on the list about this one. My
understanding is that there are presently two drawbacks:

  1. Python 2.7 interface will no longer compile by default, although you can
    change a flag here:
    https://github.com/JohnLangford/vowpal_wabbit/pull/994/files#diff-0c67e320859c05045e7211eb3d0e3cf6R12
    to make it work.
  2. This patch disables the learning to search stuff.

W.r.t. 1, can the right version of python be autodetected rather than
defined?

What is needed to make problems with 2 go away? I realize that a python
callback to a C++ interface from python is a bit hairy, but it's also
pretty cool and something that I'd like to see not break.

More generally, there is a question about which version of python we should
be targeting. I don't know what the prevalent usage pattern is so advice
is desirable.

-John

On Tue, May 24, 2016 at 12:56 PM, Coveralls notifications@github.com
wrote:

[image: Coverage Status] https://coveralls.io/builds/6312587

Coverage remained the same at 69.203% when pulling ed84beb
ed84beb
on irustandi:master
into 404cd80
404cd80
on JohnLangford:master
.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#994 (comment)

@JohnLangford
Copy link
Member

My impression is that most people still use 2.7 and not 3. It would be
great to support 3, but I think 2.7 is the priority. (I could be wrong.)

For me, at least, it's very important not to lose l2s.

On 05/25/2016 02:29 PM, John Langford wrote:

I want to have a broader discussion on the list about this one. My
understanding is that there are presently two drawbacks:

  1. Python 2.7 interface will no longer compile by default, although you
    can change a flag here:
    https://github.com/JohnLangford/vowpal_wabbit/pull/994/files#diff-0c67e320859c05045e7211eb3d0e3cf6R12
    to make it work.
  2. This patch disables the learning to search stuff.

W.r.t. 1, can the right version of python be autodetected rather than
defined?

What is needed to make problems with 2 go away? I realize that a python
callback to a C++ interface from python is a bit hairy, but it's also
pretty cool and something that I'd like to see not break.

More generally, there is a question about which version of python we
should be targeting. I don't know what the prevalent usage pattern is
so advice is desirable.

-John

On Tue, May 24, 2016 at 12:56 PM, Coveralls <notifications@github.com
mailto:notifications@github.com> wrote:

Coverage Status <https://coveralls.io/builds/6312587>

Coverage remained the same at 69.203% when pulling *ed84beb
<https://github.com/JohnLangford/vowpal_wabbit/commit/ed84beb3d78ba7b82275e721c97eeb5292fe87ca>
on irustandi:master* into *404cd80
<https://github.com/JohnLangford/vowpal_wabbit/commit/404cd803f4bf496a1c7b9f76689ececba072e736>
on JohnLangford:master*.

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
<https://github.com/JohnLangford/vowpal_wabbit/pull/994#issuecomment-221335449>

@gramhagen
Copy link
Contributor

wrt 1)
I think the python community is approaching the tipping point for migration to python3. Many greenfield projects are starting with python3, but there is still a lot of institutional code on 2.7. Official support for python 2.7 will expire in 2020. IMO to avoid breaking changes with current users it would be safer to default to 2.7 but offer instructions on how to compile for 3.
You can certainly detect which version of python is the default in the system from the Makefile, but the problem lies in resolving the appropriate boost dependencies across different system types / installations.
It might more effective to be able to install a python vowpal_wabbit package using pip or conda, which could manage the versions and dependencies more appropriately. I have not had much time to purse setting either of those up though.

@irustandi
Copy link
Author

irustandi commented May 26, 2016

Compared to say a couple of years ago, support for Python 3 (3.4 onwards) in the popular numerical packages (e.g. numpy, scipy, sklearn) has definitely matured that there are no major obstacles that I'm aware of using those packages in Python 3. This page also adds some context on the recent (as of August 2015) status of Python 2 vs 3:

http://ncoghlan-devs-python-notes.readthedocs.io/en/latest/python3/questions_and_answers.html

The ideal case is to support both Python 2.7 and 3.x equally so that there are no discernible differences when using either Python version, I think this is now the case in numpy, scipy, and sklearn. I also think it is a good idea to manage the installation of the Python package of VW through setuptools, which is typically used in most Python packages these days. Time permitting, I can look into this.

@JohnLangford
Copy link
Member

Please do look into Python 2.7+3 support. That sounds quite good if we can get it.

@gramhagen
Copy link
Contributor

I just added some improvements to get the wrapper to work with setuptools and ultimately be installable using pip via PyPI. #1012

Will need to merge changes made here into that if it is pulled in.

Also, regardless of the change to the Makefile we should merge in the python3 updates since they are still compatible with Python 2. At least we can move forward there, and then we can work through how to resolve building pylibvw.so appropriately for Python 2-3 as a separate issue.

gramhagen pushed a commit to gramhagen/vowpal_wabbit that referenced this pull request Jul 24, 2016
@gramhagen gramhagen mentioned this pull request Jul 24, 2016
@JohnLangford
Copy link
Member

Closing as this one has been incorporated in #1052 .

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