Feature/pypi #196

EmDash00 · 2024-12-03T08:43:55Z

I added quite a few quality of life improvements for Python users in addition to making this buildable with pip. I had to go in and edit the C++ API a bit, but I tried to make all the new changes backwards compatible. There shouldn't be any breakage as far as I know. One final pain point I may address in a future commit before this gets merged is that getParams should return tuple in Python (to be more Pythonic). I might make some adapter and encourage the use of that instead of the current function.

Special thanks to @LimHyungTae for their initial contribution.

One bug that remains that I can't seem to figure out is that the pybind .cc file appears to be included in the wheel. I'm not sure what might be causing this as I'm not strong with cmake.

…lity

LimHyungTae · 2024-12-05T02:49:36Z

Oh ma gosh! That's incredible! Thanks for helping me to grant my wish in #78! I'll test the code ASAP and let you know (within 12 hours)

LimHyungTae · 2024-12-05T04:47:48Z

(Maybe I'll deal with in another issue or PR) In my Mac pc, the error occurs like this:


-- Configuring done (0.0s)
-- Generating done (0.0s)
-- Build files have been written to: /Users/fudxo/git/TEASER-plusplus/build/pmc-download
[ 11%] Performing update step for 'pmc'
-- Fetching latest from the remote origin
[ 22%] No patch step for 'pmc'
[ 33%] No configure step for 'pmc'
[ 44%] No build step for 'pmc'
[ 55%] No install step for 'pmc'
[ 66%] No test step for 'pmc'
[ 77%] Completed 'pmc'
[100%] Built target pmc
CMake Error at /opt/homebrew/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:233 (message):
  Could NOT find OpenMP_C (missing: OpenMP_C_FLAGS OpenMP_C_LIB_NAMES)
Call Stack (most recent call first):
  /opt/homebrew/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:603 (_FPHSA_FAILURE_MESSAGE)
  /opt/homebrew/share/cmake/Modules/FindOpenMP.cmake:616 (find_package_handle_standard_args)
  build/pmc-src/CMakeLists.txt:26 (find_package)

like this:

Oh my bad. I've found that it's not the problem of pip, but installation in Mac PC, which is just relevant to #33. Let me double-check it tomorrow morning and let you know.

LimHyungTae · 2024-12-05T14:24:49Z

But it works in both my local computer and the Docker system! (and also, example codes run successfully!)

LimHyungTae · 2024-12-05T15:07:32Z

CMakeLists.txt

@@ -4,6 +4,7 @@ project(teaserpp VERSION 1.0.0)
 set(CMAKE_CXX_STANDARD 14)

 set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake/modules/" ${CMAKE_MODULE_PATH})
+set(CMAKE_EXPORT_COMPILE_COMMANDS ON)


I'm just curious; is compile_commands.json needed to support pip installation?

No this is not. The compile flags just are convenient for development. The C++ linters and LSPs can use them to locate included libraries like pybind and Eigen so I don't get a billion errors everywhere and can get completion from the included libraries. I use clangd as my LSP so it's just helpful for me. I'm not sure if VSCode or other IDEs have more automatic detection. I'm just using a barebones setup with Neovim and their built-in LSP/linting support.

Thanks for your kind answer. I thought compile_commands.json was needed pip3. But isn't can be produced by -DCMAKE_EXPORT_COMPILE_COMMANDS=1?

I think it can yeah. I think when building with scikit-build it was easier just to put it in the file since I had to figure out a way to pass it through in the pyproject file. We could probably put a switch there where they aren't made during release builds.

LimHyungTae · 2024-12-05T15:07:56Z

python/CMakeLists.txt

@@ -1,4 +1,5 @@
 cmake_minimum_required(VERSION 3.10)
+set(CMAKE_EXPORT_COMPILE_COMMANDS ON)


(same question) I'm just curious; is compile_commands.json needed to support pip installation?

EmDash00 · 2024-12-06T03:35:48Z

(Maybe I'll deal with in another issue or PR) In my Mac pc, the error occurs like this:


-- Configuring done (0.0s)
-- Generating done (0.0s)
-- Build files have been written to: /Users/fudxo/git/TEASER-plusplus/build/pmc-download
[ 11%] Performing update step for 'pmc'
-- Fetching latest from the remote origin
[ 22%] No patch step for 'pmc'
[ 33%] No configure step for 'pmc'
[ 44%] No build step for 'pmc'
[ 55%] No install step for 'pmc'
[ 66%] No test step for 'pmc'
[ 77%] Completed 'pmc'
[100%] Built target pmc
CMake Error at /opt/homebrew/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:233 (message):
  Could NOT find OpenMP_C (missing: OpenMP_C_FLAGS OpenMP_C_LIB_NAMES)
Call Stack (most recent call first):
  /opt/homebrew/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:603 (_FPHSA_FAILURE_MESSAGE)
  /opt/homebrew/share/cmake/Modules/FindOpenMP.cmake:616 (find_package_handle_standard_args)
  build/pmc-src/CMakeLists.txt:26 (find_package)

like this:

Oh my bad. I've found that it's not the problem of pip, but installation in Mac PC, which is just relevant to #33. Let me double-check it tomorrow morning and let you know.

This just looks like you don't have OpenMP installed. From what I'm reading here the default clang compiler on Mac doesn't have it built-in. Though the llvm brew should have it. Says you can just brew install llvm and the modify your PATH so it's the default. Should be able to follow that article to see if you can get support for it.

Though ideally we should be able to build some wheels to make it so people don't have to actually build it every time they try to pip install (and thus have the build dependencies). One challenge I see with packaging it for PyPI is that this library technically has no version requirement aside from its dependency on NumPy. I ultimately think building the wheels in a consistent way probably needs to be another PR. I rather merge this in now so that people at least can install this from git.

LimHyungTae · 2024-12-06T10:10:26Z

Gotcha. One worry is that, this repository is highly linked to other repositories in our lab, so e7ba9bf might not be a good idea. Especially, as most parameters are fixed, so I rather feel it's messy. Could you give some feedback on that @jingnanshi?

EmDash00 · 2024-12-06T22:40:17Z

Gotcha. One worry is that, this repository is highly linked to other repositories in our lab, so e7ba9bf might not be a good idea. Especially, as most parameters are fixed, so I rather feel it's messy. Could you give some feedback on that @jingnanshi?

Just a note, the package name change in e7ba9bf was undone by d65b67e. I personally don't like when python packages have python in the name since it's implied when you install it through PyPI that it is a Python package; however, ultimately, I think backwards compatibility is always a plus with software.

In terms of the constructor the recommended way in Python to have fixed parameters is to use tuples since you can use tuple expansion in the function arguments (the "Pythonic" way so to speak). There's a lot of infrastructure around tuple manipulation already with Python's itertools and other packages. It feels odd as someone who uses Python so often that the Params aren't a tuple. I've always been for making sure that C++ bindings "feel" like Python when you use the Python packages.

See the new RobustRegistrationSolverParams which use Python's NamedTuple. You can construct it in the same way as the Params. To use it in the constructor, simply tuple expand it.

params = RobustRegistrationSolverParams(...)
solver = RobustRegistrationSolver(*params)

However, I think for backwards compatibility purposes, you may be right that we should support the old constructor. I think we can bind both in Pybind and simply deprecate the other one for removal in some potential v2.0 release if that ever happens.

jingnanshi · 2024-12-09T15:27:20Z

@EmDash00 thank you for your contribution and thanks @LimHyungTae for looking over the files. Overall, I'm okay w/ 1) removing python from the package name, and 2) for the python binding to not use the params class. After we merge this we should just tag the merge commit as v2.0. Nevertheless, for the C++ interface we need to keep the params struct interface. We can do so probably by creating a wrapper class that converts the parameters to a struct and pass it to the C++ class.

EmDash00 · 2024-12-10T00:01:24Z

@EmDash00 thank you for your contribution and thanks @LimHyungTae for looking over the files. Overall, I'm okay w/ 1) removing python from the package name, and 2) for the python binding to not use the params class. After we merge this we should just tag the merge commit as v2.0. Nevertheless, for the C++ interface we need to keep the params struct interface. We can do so probably by creating a wrapper class that converts the parameters to a struct and pass it to the C++ class.

Actually I already took care of this in 033c4a7. I didn't like the memory inefficiency involved with creating and destroying the struct so I just made a version of the constructor which takes in all the params of the struct.

I advocate for an incremental approach. Best to avoid breaking changes on existing code if we can. I can make multiple commits for the Python library and we can push a v1.1.0 version of the python library which is compatible with existing code and then a v2.0 the future. Let me work on these changes and I'll let you know when I'm done so you can review them @LimHyungTae.

In terms of packaging for PyPI, how critical do you see support for Windows being @jingnanshi?

jingnanshi · 2024-12-10T15:49:07Z

@EmDash00 thank you for your contribution and thanks @LimHyungTae for looking over the files. Overall, I'm okay w/ 1) removing python from the package name, and 2) for the python binding to not use the params class. After we merge this we should just tag the merge commit as v2.0. Nevertheless, for the C++ interface we need to keep the params struct interface. We can do so probably by creating a wrapper class that converts the parameters to a struct and pass it to the C++ class.

Actually I already took care of this in 033c4a7. I didn't like the memory inefficiency involved with creating and destroying the struct so I just made a version of the constructor which takes in all the params of the struct.

I advocate for an incremental approach. Best to avoid breaking changes on existing code if we can. I can make multiple commits for the Python library and we can push a v1.1.0 version of the python library which is compatible with existing code and then a v2.0 the future. Let me work on these changes and I'll let you know when I'm done so you can review them @LimHyungTae.

In terms of packaging for PyPI, how critical do you see support for Windows being @jingnanshi?

Thanks for the clarification. I don't see Windows support being important.

LimHyungTae · 2024-12-10T18:00:45Z

Regarding pip window issue, can it be handled by Github Action?

I've tested workflows to upload independent TEASER++ packaged to pypi, and I guess the below yml file works

name: Publish to PyPI.org

on:
  workflow_dispatch:
  release:
    types: [published]
  push:
    branches: ["master"]
  pull_request:
    branches: ["master"]

jobs:
  build_sdist:
    name: Build source distribution
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3

      - name: Build sdist
        run: pipx run build --sdist ${{github.workspace}}/python/
      - name: Move sdist to dist
        run: mkdir -p dist && mv ${{github.workspace}}/python/dist/*.tar.gz dist/

      - uses: actions/upload-artifact@v3
        with:
          path: dist/*.tar.gz

  cibuildwheel:
    name: Build wheels on ${{ matrix.os }}
    runs-on: ${{ matrix.os }}
    strategy:
      matrix:
        os: [ubuntu-22.04, windows-2022, macos-14]

    steps:
      - uses: actions/checkout@v3

      - name: Build test wheels (only PRs)
        if: github.event_name != 'release'
        uses: pypa/[email protected]
        env: # build 1 build per platform just to make sure we can do it later when releasing
          CIBW_BUILD: "cp310-*"
        with:
          package-dir: ${{github.workspace}}/python/

      - name: Build all wheels
        if: github.event_name == 'release'
        uses: pypa/[email protected]
        with:
          package-dir: ${{github.workspace}}/python/

      - uses: actions/upload-artifact@v3
        with:
          path: ./wheelhouse/*.whl

  pypi:
    if: github.event_name == 'release'
    needs: [cibuildwheel, build_sdist]
    runs-on: ubuntu-latest
    steps:
      - uses: actions/download-artifact@v3
        with:
          name: artifact
          path: dist

      - uses: pypa/gh-action-pypi-publish@release/v1
        with:
          password: ${{ secrets.PYPI_API_TOKEN }}

But, this is beyond the scope of this PR. I'll deal with it later after setting pip installation.

…EADS as Final

… compatible with v1.0.0

EmDash00 · 2024-12-11T06:51:57Z

@LimHyungTae I addressed your issues in 27a0311. The constructor should now be backwards compatible. Also added properties to make things more pythonic. We can get rid of the getter binds in a v2.0.

EmDash00 · 2024-12-11T06:56:06Z

Regarding pip window issue, can it be handled by Github Action?

I've tested workflows to upload independent TEASER++ packaged to pypi, and I guess the below yml file works

name: Publish to PyPI.org

on:
  workflow_dispatch:
  release:
    types: [published]
  push:
    branches: ["master"]
  pull_request:
    branches: ["master"]

jobs:
  build_sdist:
    name: Build source distribution
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3

      - name: Build sdist
        run: pipx run build --sdist ${{github.workspace}}/python/
      - name: Move sdist to dist
        run: mkdir -p dist && mv ${{github.workspace}}/python/dist/*.tar.gz dist/

      - uses: actions/upload-artifact@v3
        with:
          path: dist/*.tar.gz

  cibuildwheel:
    name: Build wheels on ${{ matrix.os }}
    runs-on: ${{ matrix.os }}
    strategy:
      matrix:
        os: [ubuntu-22.04, windows-2022, macos-14]

    steps:
      - uses: actions/checkout@v3

      - name: Build test wheels (only PRs)
        if: github.event_name != 'release'
        uses: pypa/[email protected]
        env: # build 1 build per platform just to make sure we can do it later when releasing
          CIBW_BUILD: "cp310-*"
        with:
          package-dir: ${{github.workspace}}/python/

      - name: Build all wheels
        if: github.event_name == 'release'
        uses: pypa/[email protected]
        with:
          package-dir: ${{github.workspace}}/python/

      - uses: actions/upload-artifact@v3
        with:
          path: ./wheelhouse/*.whl

  pypi:
    if: github.event_name == 'release'
    needs: [cibuildwheel, build_sdist]
    runs-on: ubuntu-latest
    steps:
      - uses: actions/download-artifact@v3
        with:
          name: artifact
          path: dist

      - uses: pypa/gh-action-pypi-publish@release/v1
        with:
          password: ${{ secrets.PYPI_API_TOKEN }}

But, this is beyond the scope of this PR. I'll deal with it later after setting pip installation.

No because of the way Windows links libraries. The library built links dependent libraries. It needs to be told where to find the dynamic link library dependencies. This is accomplished using INSTALL_RPATH and some relative directory stuff with respect to the $ORIGIN keyword in that variable representing the dynamic link library's current directory. You can check the CMakeLists.txt file of the bindings and the library itself to see how I did this.

I'm not aware of a way to fix this since I'm not super familiar with Windows builds. I'm not entirely sure my changes didn't brick the windows builds. They probably didn't since Windows doesn't use INSTALL_RPATH, but might be good to test that before merging...

EmDash00 · 2024-12-16T23:42:34Z

@LimHyungTae Wanted to bump this to see what the progress was on your end. I'm thinking if you're good with the current suite of changes, I can make a series of commits that can be tagged and targeted for specific python versions.

LimHyungTae · 2024-12-18T21:23:17Z

Sorry I'm late. Before the winter holiday season, we had another meeting.

Surprisingly, I'm not a maintainer of TEASER++, haha so @jingnanshi could you check the recent commits? I think you have to look into the name convention of _teaserpp.pyi in 43454c8

jingnanshi · 2024-12-20T12:27:06Z

@EmDash00 thanks for your commits! I think the latest changes are great. I just started a Github action run to go over the tests & we can merge if there are no test failures.

EmDash00 and others added 12 commits December 2, 2024 00:05

feat. buildable and installable wheels

3e847db

remove default(none) so clang v17+ doesn't complain

9a1f06e

Upgrade pybind11 to v2.13.6, gain support for numpy 2.0

15f2ea0

Add alternate rpaths for normal builds without sckit-build

6f11f10

Make to to install the python files to the wheel

d0c5610

Make the bindings for RobustRegistrationSolver more Pythonic

033c4a7

export compile commands

7567efa

Make python bindings overall more pythonic, add type interface

e7ba9bf

add python development dependencies

fb3c7fa

undo experimental package name change

d65b67e

Fix double declarations of enums while mantaining backwards compatabi…

9c0305c

…lity

Update example

c8a591e

LimHyungTae reviewed Dec 5, 2024

View reviewed changes

EmDash00 added 3 commits December 10, 2024 22:47

Refactor, fix typos, and format python bindings; annotate OMP_MAX_THR…

67d3bec

…EADS as Final

Make python binding of RobustRegistrationSolver constructor backwards…

27a0311

… compatible with v1.0.0

Add properties to make things more pythonic

43454c8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/pypi #196

Feature/pypi #196

EmDash00 commented Dec 3, 2024 •

edited

Loading

LimHyungTae commented Dec 5, 2024 •

edited

Loading

LimHyungTae commented Dec 5, 2024 •

edited

Loading

LimHyungTae commented Dec 5, 2024 •

edited

Loading

LimHyungTae Dec 5, 2024

EmDash00 Dec 6, 2024 •

edited

Loading

LimHyungTae Dec 6, 2024

EmDash00 Dec 6, 2024 •

edited

Loading

LimHyungTae Dec 5, 2024

EmDash00 commented Dec 6, 2024 •

edited

Loading

LimHyungTae commented Dec 6, 2024

EmDash00 commented Dec 6, 2024 •

edited

Loading

jingnanshi commented Dec 9, 2024 •

edited

Loading

EmDash00 commented Dec 10, 2024

jingnanshi commented Dec 10, 2024

LimHyungTae commented Dec 10, 2024

EmDash00 commented Dec 11, 2024

EmDash00 commented Dec 11, 2024 •

edited

Loading

EmDash00 commented Dec 16, 2024

LimHyungTae commented Dec 18, 2024

jingnanshi commented Dec 20, 2024 •

edited

Loading

		@@ -1,4 +1,5 @@
		cmake_minimum_required(VERSION 3.10)
		set(CMAKE_EXPORT_COMPILE_COMMANDS ON)

Feature/pypi #196

Are you sure you want to change the base?

Feature/pypi #196

Conversation

EmDash00 commented Dec 3, 2024 • edited Loading

LimHyungTae commented Dec 5, 2024 • edited Loading

LimHyungTae commented Dec 5, 2024 • edited Loading

LimHyungTae commented Dec 5, 2024 • edited Loading

LimHyungTae Dec 5, 2024

Choose a reason for hiding this comment

EmDash00 Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

LimHyungTae Dec 6, 2024

Choose a reason for hiding this comment

EmDash00 Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

LimHyungTae Dec 5, 2024

Choose a reason for hiding this comment

EmDash00 commented Dec 6, 2024 • edited Loading

LimHyungTae commented Dec 6, 2024

EmDash00 commented Dec 6, 2024 • edited Loading

jingnanshi commented Dec 9, 2024 • edited Loading

EmDash00 commented Dec 10, 2024

jingnanshi commented Dec 10, 2024

LimHyungTae commented Dec 10, 2024

EmDash00 commented Dec 11, 2024

EmDash00 commented Dec 11, 2024 • edited Loading

EmDash00 commented Dec 16, 2024

LimHyungTae commented Dec 18, 2024

jingnanshi commented Dec 20, 2024 • edited Loading

EmDash00 commented Dec 3, 2024 •

edited

Loading

LimHyungTae commented Dec 5, 2024 •

edited

Loading

LimHyungTae commented Dec 5, 2024 •

edited

Loading

LimHyungTae commented Dec 5, 2024 •

edited

Loading

EmDash00 Dec 6, 2024 •

edited

Loading

EmDash00 Dec 6, 2024 •

edited

Loading

EmDash00 commented Dec 6, 2024 •

edited

Loading

EmDash00 commented Dec 6, 2024 •

edited

Loading

jingnanshi commented Dec 9, 2024 •

edited

Loading

EmDash00 commented Dec 11, 2024 •

edited

Loading

jingnanshi commented Dec 20, 2024 •

edited

Loading