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

Python distributions #1928

Merged
merged 11 commits into from Jun 21, 2019

Conversation

Projects
None yet
5 participants
@peterychang
Copy link
Collaborator

commented Jun 7, 2019

Rearranging the repo to make python distributions work
setup.py and all its supporting metadata files need to be at the common ancestor directory for all files that will be included in the source distribution

@peterychang

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 7, 2019

@gramhagen @Scott-Graham-Bose Does this change look sane to you? I was trying to find a way to keep most/all the python specific files in the python/ subdirectory, but it seemed like setup.py makes a lot of assumptions that can't be changed. Do you know if thats correct?

Show resolved Hide resolved setup.py Outdated
@gramhagen
Copy link
Contributor

left a comment

This looks great, the main trouble I often find is unless you have a very new version of cmake the FindBoost module won't actually work correctly for anaconda. You can however grab just the FindBoost.cmake module from a newer release and this works well. I think this is an improvement though. One thing to note is there is ongoing work to simplify this, so might be good to coordinate with @jackgerrits before pulling this in.

Show resolved Hide resolved setup.py Outdated

peterychang added some commits Jun 11, 2019

@peterychang

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 13, 2019

Confirmed this change will allow us to make source distributions and wheels for
linux: python2.7, 3.5, 3.6, 3.7
windows: python3.7 (limited to 3.7 due to vcpkg)

@gramhagen

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

that's excellent, I think there was some concern about statically linking boost when building wheels so they would be more resilient to different versions / installations. I don't know where that stands relative to this, @jackgerrits do you have comments on how that should work with these changes?

@peterychang

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 13, 2019

with a little bit of setup when building the weels, vcpkg will also let us build/release binary distributions for python3.6 on Windows (use vcpkg commit aa095559917a495b160986e9ad50556431509ace).

vcpkg has never supported python3.5, so that one's out
vcpkg has never supported boost-python for python2.7, so that one's also out
Switching to pybind11 would let us build against python2.7 on Windows. But with support for python2 ending in a few months, I don't think it's a high priority.

@peterychang

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 13, 2019

One bit of weirdness my vcpkg changes make though, is installing a VW wheel requires a different command from building/installing VW from the source distribution.

Via wheel: pip install vowpalwabbit
Via sdist: pip install --global-option="--vcpkg-root=path/to/vcpkg" vowpalwabbit

It looks like giving a global-option will cause pip to build from source even if a binary distribution is available

@jackgerrits

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

Do the wheels work when boost is not installed locally?

@peterychang

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 14, 2019

Yes. I'm pretty sure linux links against boost statically, so there's no problems there
For windows though, I couldn't get the static linking to work, so its actually a little gross. Installing the wheel will give you the following

python/lib/site-packages
    - boost_program_options.dll
    - boost_python.dll
    - pylibvw.pyd
    + vowpalwabbit
        - *.py

So the wheel will package the boost library files, but puts them directly into the site-packages subdirectory instead of the vw one

@jackgerrits

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

@peterychang

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 14, 2019

I think that would be ok, maybe with a follow-up task of getting boost statically linking on windows

@peterychang

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 17, 2019

Moving requirements to setup.py per packaging conventions
https://packaging.python.org/discussions/install-requires-vs-requirements/

Whereas install_requires defines the dependencies for a single project, Requirements Files are often used to define the requirements for a complete Python environment.

@peterychang

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 17, 2019

@jackgerrits Actually, I messed up when I was uninstalling boost while testing for requirements on linux.

Boost is dynamically linked in python, and the .so's aren't included. I'll check if we can enable static linking for the linux/python build.

A bigger problem is that pypi doesn't like linux_x86_64 platforms, since there are many flavors of linux. Instead, we'd have to figure out how to create manylinux1 platform wheels

peterychang
@peterychang

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 17, 2019

I wasn't aware only an optional part of the python wrapper required the other python packages. Those rightly belong in requirements.txt

@peterychang

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 18, 2019

It looks like we won't be able to release binary distributions for linux since we would need to build against the manylinux platform. That requires compilation against CENTOS 5, which only supported up to gcc 4.1 (before the inclusion of c++11).

We should be able to build and distribute the windows wheels, but linux users will still need to compile the source distribution

@gramhagen

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

@peterychang can we build for manylinux2010? That uses CENTOS 6.
more info here: https://github.com/pypa/manylinux

@peterychang

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 19, 2019

@gramhagen I've tried getting a build on manylinux2010 as well. The problem I ran into there is that the version of boost available on CENTOS6 is too old for what we need, and attempting to build a newer version is proving to be more problematic than I had anticipated.

We can always revisit this issue as a follow-up task if anything changes in the future

@gramhagen

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

got it, makes sense. perhaps leveraging conda forge will be more viable for linux

@peterychang

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 20, 2019

@lokitoth This change is ready for review

@JohnLangford

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

Let's merge as soon as tests pass.

@JohnLangford JohnLangford merged commit 2ee2bcf into VowpalWabbit:master Jun 21, 2019

8 checks passed

LGTM analysis: C# No code changes detected
Details
LGTM analysis: C/C++ No code changes detected
Details
LGTM analysis: Java No code changes detected
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No new or fixed alerts
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 73.418%
Details

peterychang added a commit that referenced this pull request Jul 9, 2019

Python distributions (#1928)
* Fix the ability to create python distributions

* Make python tox tests work

* clean up cmake arguments in setup.py

* fix build script

* remove erroneous chdir in build script

* Move python dependencies into setup.py, per convention

* Revert "Move python dependencies into setup.py, per convention"

This reverts commit 67e2d40.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.