Cpp wrapper - Githubissues

kekeblom commented 4 years ago

I started writing the RobotWrapper class in c++ so that we could easily use this in c++ code. This could also enable to write the controllers in c++ to run them at a higher frequency.

I ran into the python 2 vs. 3 problem again. The issue is that ROS Melodic runs on python 2 whereas the code in this repository is in python 3. I would suggest we upgrade to a ros distribution which actually supports python 3 to sustainably bypass this issue.

Another option would be to write some cmake modules to find a python 3 interpreter and associated libraries, possibly from an anaconda environment. This would probably be quite brittle and I would rather stick to the standard ROS way of doing things.

I don't know how much of a footgun ROS 2 is but we could at least try ROS Noetic.

Don't merge as this is still work in progress and won't run any of the old python 3 code.

kekeblom commented 4 years ago

@grizzi What do you think about the python 2/3 issue? Should we try ROS noetic?

grizzi commented 4 years ago

Before answering I was reading about the topic ... All solutions are kind of hacks and they are not guaranteed to work all the time since anyway Melodic does not fully support python 3.

Noetic is LTS and should completelly backcompatible. I would rather spend time fixing bugs on Noetic which is something new than work on having python3 working with Melodic. What do you think?

kekeblom commented 4 years ago

Same. I would rather spend effort on fixing issues on future releases than coming up with solutions for how to run this on melodic, which would be on its way out.

Taking a pragmatic perspective, we might run into issues running this on our platform because the ridgeback and franka packages don't have ROS Noetic support currently.

grizzi commented 4 years ago

In any case we would use the cpp version on the robot. Otherwise which specific issues are you running in right now?

kekeblom commented 4 years ago

The immediate issue is, if we want to get rid of the python RobotWrapper class and just use the wrapped c++ version, we couldn't run the existing examples, unless they run under python 2. The larger idea being that most of the stuff would be written and accessible from c++ with thin wrappers for quick prototyping and easy use for non-performance constrained cases.

If you build this package and run it by importing using a python 3 interpreter, you get the error: “Symbol not found: __Py_ZeroStruct / _PyInstanceMethod_Type”. This is because the ros-melodic-pybind11-catkin package links against the ros/system python 2. Python 2 and 3 are not binary compatible when it comes to native modules.

Another way, with hardcoded paths just to demonstrate the principle, would be to use a cmake file in this direction:

cmake_minimum_required(VERSION 2.8.3)
project(robot_control)

find_package(catkin REQUIRED)
find_package(Eigen3 REQUIRED 3.3)
find_package(pinocchio REQUIRED)
catkin_package(LIBRARIES)

add_subdirectory(pybind11)

set(PYTHONLIBS_FOUND TRUE)
set(PYBIND11_PYTHON_VERSION 3.7.6)
set(PYTHON_PREFIX "/home/ken/miniconda3/envs/ctrl")
set(PYTHON_INTERPRETER "/home/ken/miniconda3/envs/ctrl/bin/python")
set(PYTHON_LIBRARIES "/home/ken/miniconda3/envs/ctrl/lib")
set(PYTHON_INCLUDE_DIRS "/home/ken/miniconda3/envs/ctrl/include")
set(PYTHON_SITE_PACKAGES "/home/ken/miniconda3/envs/ctrl/lib/python3.7/site-packages")
set(PYTHON_MODULE_EXTENSION ".so")

include_directories(include SYSTEM ${catkin_INCLUDE_DIRS} ${PYTHON_INCLUDE_DIRS}/python3.7m ${PINOCCHIO_INCLUDE_DIRS} ${EIGEN3_INCLUDE_DIRS})

message("PYTHONLIBS_FOUND: ${PYTHONLIBS_FOUND}")
message("PYTHON_PREFIX: ${PYTHON_PREFIX}")
message("PYTHON_LIBRARIES: ${PYTHON_LIBRARIES}")
message("PYTHON_INCLUDE_DIRS: ${PYTHON_INCLUDE_DIRS}")
message("PYTHON_MODULE_EXTENSION: ${PYTHON_MODULE_EXTENSION}")
message("PYTHON_MODULE_PREFIX: ${PYTHON_MODULE_PREFIX}")
message("PYTHON_SITE_PACKAGES: ${PYTHON_SITE_PACKAGES}")
message("PYTHON_IS_DEBUG: ${PYTHON_IS_DEBUG}")

pybind11_add_module(rc MODULE src/robot_control.cc src/robot_wrapper.cc)
target_link_libraries(rc PRIVATE ${PINOCCHIO_LIBRARIES} ${catkin_LIBRARIES} ${PYTHON_LIBRARIES})

install(DIRECTORY launch config
  DESTINATION ${CATKIN_PACKAGE_SHARE_DESTINATION}
)

Which builds the module and links againts python 3 libraries. However, now the module would not be able to load under the ros python 2. This might be fine though if we just use the anaconda setup you have been using.

grizzi commented 4 years ago

Thanks for the detailed explanation. As I have never written bindings I could not see the immediate issue.

On the other hand there is nothing that could not run in python 2.7. pinocchio is available for python 2.7 too and so also bullet. Reverting everything to python 2.7 might not be a big work and might require little changes.

The question is, should we do it or thrive to move to 3.x which of course is better? I do not have a precise answer. Probably for the sake of pragmacity we could have a melodic and noetic branch. In the first we would have a working python2.7 version and in the second we could nicely switch to noetic when it is the time.

I also think tough that we should plan to make the library somehow independent from ros and follow the recurrent pattern <package> and <package>_ros but I guess that this is something that we can address later.

kekeblom commented 4 years ago

I came up with one more solution. I'm not proud of it, but it works on my system.

The upside is that we can keep using the same anaconda setup. The downside is that the current setup only support python 3.7. Would that be possible for you to lock that for the anaconda environment? Check the changes in the README for how to build.

The hack copies the built library into the anaconda env by assuming the path to the site-packages. I'm not sure if this is a good assumption to make. Try it out and see if works on your system. This could probably very easily break from one system to the next.

A possibly better solution, would be to build and copy the library in the setup.py file, but I'm not an expert on python packaging, so I don't know off the top of my head how to do that and I leaned on my understanding of cmake. This would also nicely separate the build for ros and for anaconda.

Let me know what you think. From where I am sitting, all these options look bad in some respects.

grizzi commented 4 years ago

I came up with one more solution. I'm not proud of it, but it works on my system.

The upside is that we can keep using the same anaconda setup. The downside is that the current setup only support python 3.7. Would that be possible for you to lock that for the anaconda environment? Check the changes in the README for how to build.

The hack copies the built library into the anaconda env by assuming the path to the site-packages. I'm not sure if this is a good assumption to make. Try it out and see if works on your system. This could probably very easily break from one system to the next.

A possibly better solution, would be to build and copy the library in the setup.py file, but I'm not an expert on python packaging, so I don't know off the top of my head how to do that and I leaned on my understanding of cmake. This would also nicely separate the build for ros and for anaconda.

Let me know what you think. From where I am sitting, all these options look bad in some respects.

Probably doing everything through conda would be the best way. In this repo they show a python (no CMake) solution. This way one could use conda if he wants the python bindings and instead just catkin build the package for the actual cpp library

grizzi commented 4 years ago

I have spent quite some time thinking about this and I came to the conclusion that as you said there is no nice solution. I thought using the setup.py as suggested in the previous comment and link, but I wouldn't know on the top of my head how to make sure all libraries and flags are correctly imported. To me this seems quite a unnatural solution for a Cpp project.

Other solutions with CMake do not differ too much in complexity from what you have done here.

I will try this on my pc now and let you know.

kekeblom commented 4 years ago

I've now converged on a solution which checks all the boxes:

Supports the existing ros + anaconda workflow
Builds for python 2 and ros through catkin for other packages to access
Builds for python 3 through the setup.py
Doesn't require anaconda if not desired for python 3, if only the extensions want to be built without ros stuff

I can't speak for the robustness, but at least I didn't have to resort to any crazy hacks and all assumptions about paths etc. are handled by catkin and setuptool.

kekeblom commented 4 years ago

BTW the previous config copies rc.so to your anaconda library and breaks uninstallation. Be sure to delete it as you try this setup on your system.

grizzi commented 4 years ago

Nice!!! Honestly, given my entry-level knowledge of setup and CMake I am not 100% sure how all this work :sweat_smile: