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

Restructure the python bindings to support official iDynTree bindings #578

Merged
merged 14 commits into from
Dec 17, 2022

Conversation

GiulioRomualdi
Copy link
Member

@GiulioRomualdi GiulioRomualdi commented Dec 14, 2022

⚠️ ⚠️ THIS PR WILL BREAK ALREADY EXISTING PYTHON CODE THAT USES IDYNTREE BINDINGS IN THE INTERFACE

As you may know, the iDynTree python bindings are designed using swig while blf bindings use pybind11. This choice causes several problems so far, indeed it was not possible to have out-of-the-box interoperability between the bindings of the two libraries (See this issue if you are interested in more details pybind/pybind11#1706).

The only iDynTree object exposed in the blf interface is the iDynTree::KinDynComputations. As a consequence, we were forced to develop iDynTree::KinDynComputations bindings inside blf, for this reason, the user had to instantiate a blf binding of iDynTree::KinDynComputations and use it inside the code. This approach has some issues:

  1. API breaking in iDynTree::KinDynComputations were not automatically handled and the pybind11 glue code need to be manually modified
  2. As iDynTree user, I would like to exploit all the iDynTree framework along with blf and I don't want to use a subset of function only because the blf iDynTree bindings are not implemented

In light of this, I decided to restructure the python code to automatically handle swig objects.
The core of this PR is given by a9e3a68. Here, following what was written here I implemented extract_swig_wrapped_pointer function that allows us to extract a swig pointer and use it in C++.
So what I implemented is the following conversion

graph TD
    A[Python Swig] -->|extract_swig_wrapped_pointer| B(C++)
Loading

This conversion is not only related to iDynTree but it can be used to exploit all the libraries that use swig bindings in the interface of python blf (for instance casadi @Giulero)
⚠️ The other direction (i.e. From C++ to Python swig compatible) is not implemented since we do not need it in blf. Moreover, it is not super clear to me how to design it.

Thanks to a9e3a68 I then simplified the python bindings by directly using the official iDynTree bindings and I removed C++ classes required only to design the blf iDynTree bindings.

To conclude thanks to this PR we can finally write the following

import idyntree.swig as idyn
import bipedal_locomotion_framework as blf
import icub_models

ml = idyn.ModelLoader()
ml.loadModelFromFile(str(icub_models.get_model_file("iCubGenova09")))
kindyn = idyn.KinDynComputations()
kindyn.loadRobotModel(ml.model())

task = blf.ik.SE3Task()
task.set_kin_dyn(kindyn)

As far as I know, only @paolo-viceconte and @RiccardoZuppetti will be affected by this breaking change. We can schedule a meeting to update the code in case you need it. Sorry for the inconvenience 😭

@DanielePucci
Copy link
Member

Important then for all @ami-iit/artificial-mechanical-intelligence

@GiulioRomualdi GiulioRomualdi changed the title Restructure the python bindings to support official iDynTree bindings Restructure the python bindings to support _official_ iDynTree bindings Dec 14, 2022
@GiulioRomualdi GiulioRomualdi changed the title Restructure the python bindings to support _official_ iDynTree bindings Restructure the python bindings to support official iDynTree bindings Dec 14, 2022
@GiulioRomualdi GiulioRomualdi force-pushed the bindings/idyntree_swig_compatibility branch from effae01 to e6cf70e Compare December 14, 2022 17:16
@GiulioRomualdi
Copy link
Member Author

To fix conda ci:

  1. wait for Force compilation to be compatible with iDynTree 8.0.0 conda-forge/libunicycle-footstep-planner-feedstock#2
  2. switch from robotology to conda-forge channel for the following packages
    • lie-group-controllers -> liblie-group-controllers
    • unicycle-footstep-planner -> libunicycle-footstep-planner
    • matio-cpp -> libmatio-cpp

@GiulioRomualdi GiulioRomualdi force-pushed the bindings/idyntree_swig_compatibility branch from e6cf70e to e4b3f4a Compare December 14, 2022 17:46
@traversaro
Copy link
Collaborator

To fix conda ci:

1. wait for [Force compilation to be compatible with iDynTree 8.0.0 conda-forge/libunicycle-footstep-planner-feedstock#2](https://github.com/conda-forge/libunicycle-footstep-planner-feedstock/pull/2)

2. switch from robotology to conda-forge channel for the following packages
   
   * `lie-group-controllers` -> `liblie-group-controllers`
   * `unicycle-footstep-planner` -> `libunicycle-footstep-planner`
   * `matio-cpp` -> `libmatio-cpp`

Sorry for that, I did not originally planned for those name changes, but they were required as part of the conda-forge review process, and then I forgot to advertise them properly.

@GiulioRomualdi
Copy link
Member Author

To fix conda ci:

1. wait for [Force compilation to be compatible with iDynTree 8.0.0 conda-forge/libunicycle-footstep-planner-feedstock#2](https://github.com/conda-forge/libunicycle-footstep-planner-feedstock/pull/2)

2. switch from robotology to conda-forge channel for the following packages
   
   * `lie-group-controllers` -> `liblie-group-controllers`
   * `unicycle-footstep-planner` -> `libunicycle-footstep-planner`
   * `matio-cpp` -> `libmatio-cpp`

Sorry for that, I did not originally planned for those name changes, but they were required as part of the conda-forge review process, and then I forgot to advertise them properly.

Don't worry :)

…ges in the conda CI

- `lie-group-controllers` -> `liblie-group-controllers`
- `unicycle-footstep-planner` -> `libunicycle-footstep-planner`
- `matio-cpp` -> `libmatio-cpp`
@GiulioRomualdi
Copy link
Member Author

GiulioRomualdi commented Dec 15, 2022

Conda ci fails because of

- package idyntree-8.0.0-py310h3b7b3b7_0 requires assimp >=5.2.5,<5.2.6.0a0, but none of the providers can be installed

while apt/brew fails with https://github.com/ami-iit/bipedal-locomotion-framework/actions/runs/3705058484/jobs/6278469743#step:22:445

@traversaro do you know anything about this?

@GiulioRomualdi
Copy link
Member Author

Conda ci on ubuntu now fails due to an incompatibility between manifpy and blf/numpy, perhaps it is similar to #514

@traversaro
Copy link
Collaborator

Conda ci on ubuntu now fails due to an incompatibility between manifpy and blf/numpy, perhaps it is similar to #514

That is due to an old manifpy version installed for some reason, that for some reason is is caused by the numpy fixed to 1.22.4, see:

traversaro@IITICUBLAP257:~$ mamba create -n test124 -c conda-forge manifpy  "numpy=1.22.4"

Looking for: ['manifpy', 'numpy=1.22.4']

conda-forge/linux-64                                        Using cache
conda-forge/noarch                                          Using cache
Transaction

  Prefix: /home/traversaro/mambaforge/envs/test124

  Updating specs:

   - manifpy
   - numpy=1.22.4


  Package               Version  Build                Channel                    Size
───────────────────────────────────────────────────────────────────────────────────────
  Install:
───────────────────────────────────────────────────────────────────────────────────────

  + _libgcc_mutex           0.1  conda_forge          conda-forge/linux-64     Cached
  + _openmp_mutex           4.5  2_gnu                conda-forge/linux-64     Cached
  + bzip2                 1.0.8  h7f98852_4           conda-forge/linux-64     Cached
  + ca-certificates   2022.12.7  ha878542_0           conda-forge/linux-64     Cached
  + ld_impl_linux-64       2.39  hcc3a1bd_1           conda-forge/linux-64     Cached
  + libblas               3.9.0  16_linux64_openblas  conda-forge/linux-64     Cached
  + libcblas              3.9.0  16_linux64_openblas  conda-forge/linux-64     Cached
  + libffi                3.4.2  h7f98852_5           conda-forge/linux-64     Cached
  + libgcc-ng            12.2.0  h65d4601_19          conda-forge/linux-64     Cached
  + libgfortran-ng       12.2.0  h69a702a_19          conda-forge/linux-64     Cached
  + libgfortran5         12.2.0  h337968e_19          conda-forge/linux-64     Cached
  + libgomp              12.2.0  h65d4601_19          conda-forge/linux-64     Cached
  + liblapack             3.9.0  16_linux64_openblas  conda-forge/linux-64     Cached
  + libnsl                2.0.0  h7f98852_0           conda-forge/linux-64     Cached
  + libopenblas          0.3.21  pthreads_h78a6416_3  conda-forge/linux-64     Cached
  + libsqlite            3.40.0  h753d276_0           conda-forge/linux-64     Cached
  + libstdcxx-ng         12.2.0  h46fd767_19          conda-forge/linux-64     Cached
  + libuuid              2.32.1  h7f98852_1000        conda-forge/linux-64     Cached
  + libzlib              1.2.13  h166bdaf_4           conda-forge/linux-64     Cached
  + manifpy               0.0.4  py310hbdd0f47_7      conda-forge/linux-64      561kB
  + ncurses                 6.3  h27087fc_1           conda-forge/linux-64     Cached
  + numpy                1.22.4  py310h4ef5377_0      conda-forge/linux-64        7MB
  + openssl               3.0.7  h0b41bf4_1           conda-forge/linux-64     Cached
  + pip                  22.3.1  pyhd8ed1ab_0         conda-forge/noarch       Cached
  + pybind11-abi              4  hd8ed1ab_3           conda-forge/noarch       Cached
  + python               3.10.8  h4a9ceb5_0_cpython   conda-forge/linux-64     Cached
  + python_abi             3.10  3_cp310              conda-forge/linux-64     Cached
  + readline              8.1.2  h0f457ee_0           conda-forge/linux-64     Cached
  + setuptools           65.5.1  pyhd8ed1ab_0         conda-forge/noarch       Cached
  + tk                   8.6.12  h27826a3_0           conda-forge/linux-64     Cached
  + tzdata                2022g  h191b570_0           conda-forge/noarch       Cached
  + wheel                0.38.4  pyhd8ed1ab_0         conda-forge/noarch       Cached
  + xz                    5.2.6  h166bdaf_0           conda-forge/linux-64     Cached

@GiulioRomualdi could we try to remove the numpy pinning?

@traversaro
Copy link
Collaborator

That is due to an old manifpy version installed for some reason, that for some reason is is caused by the numpy fixed to 1.22.4, see:

Actually, I know the reason. The matrix of Python Versions/Numpy Versions build by conda-forge is limited, and the one requested is not available, while it is available for the old _7 build.

@traversaro
Copy link
Collaborator

@GiulioRomualdi could we try to remove the numpy pinning?

This will not be necessary after conda-forge/manif-feedstock#28 is merged.

@traversaro
Copy link
Collaborator

traversaro commented Dec 16, 2022

while apt/brew fails with https://github.com/ami-iit/bipedal-locomotion-framework/actions/runs/3705058484/jobs/6278469743#step:22:445

@traversaro do you know anything about this?

I suspect this is related to robotology/idyntree#1040 . However, we can you compile iDynTree can you install assimp and enable the IDYNTREE_USES_ASSIMP option? Even if assimp is already installed, differently from the option dependency related option IDYNTREE_USES_ASSIMP needs to be enabled manually for historical reason.

@GiulioRomualdi GiulioRomualdi force-pushed the bindings/idyntree_swig_compatibility branch from 192742d to c1948c2 Compare December 16, 2022 13:51
@GiulioRomualdi
Copy link
Member Author

We're almost there: only ubuntu is not working

@GiulioRomualdi GiulioRomualdi merged commit 2ed4b90 into master Dec 17, 2022
@GiulioRomualdi GiulioRomualdi deleted the bindings/idyntree_swig_compatibility branch December 17, 2022 12:42
@traversaro
Copy link
Collaborator

Good catch with the LD_LIBRARY_PATH! It is a bit problematic as we eventually wanted to get rid of it in the superbuild setup.sh (see robotology/robotology-superbuild#1192) but that is another story.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants