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

Automated wheel creation with cmake and more #604

Merged
merged 4 commits into from Oct 17, 2018

Conversation

yxlao
Copy link
Collaborator

@yxlao yxlao commented Oct 9, 2018

Usage

make                   # our regular make, creates build/lib/Python/open3d.so
make install           # does not install any python module
make python-package    # creates python package in build/lib/python_package
make pip-wheel         # creates wheel in build/lib/python_package/dist
make install-pip-wheel # installs wheel in virtualenv
make all-pip-wheels    # creates all wheels in PyPI: "open3d-python, py3d, ..."

Motivation

  1. Later we'll have a mix of pure python code (e.g. python visulization) and
    compiled python module. The current installation mechanism only work with one
    compiled python module. Using approach in this PR, all python package files
    will first be grouped in to a wheel file, then installed.
  2. Currently, make install installs the python module in the global user site.
    Therefore, it doesn't work with pip nor conda virtualenv.
    (see this discord discussion)
  3. Currently creating pip wheels and especially releaseing pip wheels under
    different names is a painful manual process.
  4. Currently we're including all python versions and all platforms inside a
    single wheel package. This is a simpler approach when we are doing the pip
    packaging manually, however the wheel size is large. With the automated
    process, creating wheels containing single compiled module is simple. Now
    the wheel size is about 12X smaller.
  5. There are also some user feedback (e.g. Please allow to build just a python module #566) to separate python build and
    installation process.

This change is Reviewable

Copy link
Contributor

@syncle syncle left a comment

Choose a reason for hiding this comment

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

Reviewed 27 of 27 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @yxlao, @takanokage, and @qianyizh)


src/Python/CMakeLists.txt, line 25 at r1 (raw file):

# FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
# IN THE SOFTWARE.
# ----------------------------------------------------------------------------

I think we are not putting the license header in CMakeLists.txt


src/Python/CMakeLists.txt, line 34 at r1 (raw file):

    find_program(PYTHON_IN_PATH "python")
    set(PYTHON_EXECUTABLE ${PYTHON_IN_PATH})
endif()

This is great. This will resolve many python version conflicts (compiled with system Python but the output module is called with virtualenv Python) we had encountered. Just quick question. Why this lines are inside if (NOT PYTHON_EXECUTABLE) ? Can you explain bit more?


src/Python/install_pip_wheel.cmake, line 26 at r1 (raw file):

# IN THE SOFTWARE.
# ----------------------------------------------------------------------------

Dropping license header?


src/Python/Package/setup.py.in, line 30 at r1 (raw file):


setup(
    author='IntelVCL',

Open3D team?

Copy link
Collaborator Author

@yxlao yxlao left a comment

Choose a reason for hiding this comment

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

Reviewable status: 22 of 27 files reviewed, 5 unresolved discussions (waiting on @syncle, @yxlao, @takanokage, and @qianyizh)


src/Python/CMakeLists.txt, line 22 at r1 (raw file):

PYTHON_EXECUTABLE

src/Python/CMakeLists.txt, line 25 at r1 (raw file):

Previously, syncle (Jaesik Park) wrote…

I think we are not putting the license header in CMakeLists.txt

Done.


src/Python/CMakeLists.txt, line 34 at r1 (raw file):

if (NOT PYTHON_EXECUTABLE)
if (NOT PYTHON_EXECUTABLE) is when the user didn't specify the -DPYTHON_EXECUTABLE. In this case, we use the python executable in the current path.
Added comments in the code as well.


src/Python/install_pip_wheel.cmake, line 26 at r1 (raw file):

Previously, syncle (Jaesik Park) wrote…

Dropping license header?

Done.


src/Python/Package/setup.py.in, line 30 at r1 (raw file):

Previously, syncle (Jaesik Park) wrote…

Open3D team?

Done.

@qianyizh qianyizh merged commit 1281563 into isl-org:master Oct 17, 2018
This was referenced Oct 17, 2018
@yxlao yxlao deleted the python-install branch October 18, 2018 00:08
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

3 participants