Implement a machinery to easily access model locations on C++ and Python #130

GiulioRomualdi · 2022-02-15T23:12:03Z

With this PR I want to propose a solution for the #91. In 3c869f1, I implemented a C++ library that will allow us to easily access the location of a model installed in by icub-models repo. The location of the files is automatically generated by a cmake machine that is in charge to set the path accordingly to the installation folder.

I also implemented the python bindings (with pybind11) 8d7efc7. In theory, it would be possible to use swig to support more language. However, since I've never used it I've preferred to start with something simpler.

This is an python script where I used the module. I replicated the API suggested by @traversaro in #91 (comment)

>>> import icub_models
>>> print(f"Models have been installed in: {icub_models.get_models_path()}")
Models have been installed in: /home/gromualdi/robot-code/robotology-superbuild/build/install/share/iCub/robots
>>> print(f"Available robots: {icub_models.get_robot_names()}")
Available robots: {'iCubErzelli02', 'iCubGazeboV3', 'iCubGenova09', 'iCubNancy01', 'iCubParis01', 'iCubLisboa01', 'iCubParis02', 'iCubGazeboV2_5_visuomanip', 'iCubGenova04_plus', 'iCubGazeboV2_5_plus', 'iCubGazeboV2_5', 'iCubGenova02', 'left_wrist_mk2', 'iCubDarmstadt01', 'iCubGenova04', 'iCubGazeboV2_6', 'iCubGenova03', 'iCubGenova01', 'iCubGazeboV2_7', 'left_hand_mk3'}
>>> for robot_name in icub_models.get_robot_names():
...     print(f"{robot_name}: {icub_models.get_model_file(robot_name)}")
... 
iCubErzelli02: /home/gromualdi/robot-code/robotology-superbuild/build/install/share/iCub/robots/iCubErzelli02/model.urdf
iCubGazeboV3: /home/gromualdi/robot-code/robotology-superbuild/build/install/share/iCub/robots/iCubGazeboV3/model.urdf
iCubGenova09: /home/gromualdi/robot-code/robotology-superbuild/build/install/share/iCub/robots/iCubGenova09/model.urdf
iCubNancy01: /home/gromualdi/robot-code/robotology-superbuild/build/install/share/iCub/robots/iCubNancy01/model.urdf
iCubParis01: /home/gromualdi/robot-code/robotology-superbuild/build/install/share/iCub/robots/iCubParis01/model.urdf
iCubLisboa01: /home/gromualdi/robot-code/robotology-superbuild/build/install/share/iCub/robots/iCubLisboa01/model.urdf
iCubParis02: /home/gromualdi/robot-code/robotology-superbuild/build/install/share/iCub/robots/iCubParis02/model.urdf
iCubGazeboV2_5_visuomanip: /home/gromualdi/robot-code/robotology-superbuild/build/install/share/iCub/robots/iCubGazeboV2_5_visuomanip/model.urdf
iCubGenova04_plus: /home/gromualdi/robot-code/robotology-superbuild/build/install/share/iCub/robots/iCubGenova04_plus/model.urdf
iCubGazeboV2_5_plus: /home/gromualdi/robot-code/robotology-superbuild/build/install/share/iCub/robots/iCubGazeboV2_5_plus/model.urdf
iCubGazeboV2_5: /home/gromualdi/robot-code/robotology-superbuild/build/install/share/iCub/robots/iCubGazeboV2_5/model.urdf
iCubGenova02: /home/gromualdi/robot-code/robotology-superbuild/build/install/share/iCub/robots/iCubGenova02/model.urdf
left_wrist_mk2: /home/gromualdi/robot-code/robotology-superbuild/build/install/share/iCub/robots/left_wrist_mk2/model.urdf
iCubDarmstadt01: /home/gromualdi/robot-code/robotology-superbuild/build/install/share/iCub/robots/iCubDarmstadt01/model.urdf
iCubGenova04: /home/gromualdi/robot-code/robotology-superbuild/build/install/share/iCub/robots/iCubGenova04/model.urdf
iCubGazeboV2_6: /home/gromualdi/robot-code/robotology-superbuild/build/install/share/iCub/robots/iCubGazeboV2_6/model.urdf
iCubGenova03: /home/gromualdi/robot-code/robotology-superbuild/build/install/share/iCub/robots/iCubGenova03/model.urdf
iCubGenova01: /home/gromualdi/robot-code/robotology-superbuild/build/install/share/iCub/robots/iCubGenova01/model.urdf
iCubGazeboV2_7: /home/gromualdi/robot-code/robotology-superbuild/build/install/share/iCub/robots/iCubGazeboV2_7/model.urdf
left_hand_mk3: /home/gromualdi/robot-code/robotology-superbuild/build/install/share/iCub/robots/left_hand_mk3/model.urdf

GiulioRomualdi · 2022-02-15T23:12:40Z

cc @traversaro @Nicogene

traversaro · 2022-02-15T23:18:58Z

bindings/CMakeLists.txt

+      endif()
+      file(WRITE ${CMAKE_CURRENT_BINARY_DIR}/METADATA "")
+      file(APPEND ${CMAKE_CURRENT_BINARY_DIR}/METADATA "Metadata-Version: 2.1${NEW_LINE}")
+      file(APPEND ${CMAKE_CURRENT_BINARY_DIR}/METADATA "Name: icub_models${NEW_LINE}")


I think that for pep-0008 the use of underscore in package names is discouraged, see https://www.python.org/dev/peps/pep-0008/#package-and-module-names, while the underscore is actuall suggested for the module name.

Double checking with @diegoferigo .

If I interpret the PEP correctly, it states that the usage of underscores in module names is discouraged. Practically, if you cannot find a single name descriptive enough, this suggestion from a very old PEP is not really followed and you can find pretty often _.

This being said, the module name is different from the package name, and this section of CMake code is about the resulting package name. In this case, it does not matter much, because underscores in the metadata are parsed as dashes by PyPI and pip. What differs is just the sdist / wheel package name. And I mean, a package with name icub-models containing a module stored in folder icub_models so that it can be imported as import icub_models, when packaged, produces icub-models-....{zip|wheel}. This bother mostly CI since you have to take care of two different names. Since packages named icub_models can be installed with pip as pip install icub-models regardless, I'd suggest to keep all naming consistent, using always _.

bindings/CMakeLists.txt

traversaro · 2022-02-15T23:20:56Z

bindings/CMakeLists.txt

+      file(WRITE ${CMAKE_CURRENT_BINARY_DIR}/INSTALLER "${ICUB_MODELS_PYTHON_PIP_METADATA_INSTALLER}${NEW_LINE}")
+      install(
+        FILES "${CMAKE_CURRENT_BINARY_DIR}/METADATA" "${CMAKE_CURRENT_BINARY_DIR}/INSTALLER"
+        DESTINATION ${PYTHON_METADATA_PARENT_DIR}/icub_models-${PROJECT_VERSION}.dist-info)


Note that even if we call python package name icub-models, the directory name here should use underscore, see conda-forge/opencv-feedstock#300 (comment) .

traversaro

Thanks a lot @GiulioRomualdi ! My only doubt for what regards this implementation is that by itself is not relocatable. This is not a problem as long as we just distribute binaries via apt packages (as in that case the CMAKE_INSTALL_PREFIX used to install the binaries is always the same, i.e. /usr or /opt/something) or conda (as conda handles automatically strings in the binary packages to make them relocatable).

However, if we ever want to ship this via PyPI, this may be a problem. However, I do not think this should block the PR as in any case this is a great step forward, and if necessary I think it would be trivial to replace the pybind11 Python library with a native Python library for which handling the relocatability is trivial.

cpp/src/iCubModels.cpp.in

GiulioRomualdi · 2022-02-16T08:13:54Z

Hi @traversaro, I applied the suggestions you proposed. Before merging the PR I would like to highlight:

I set the project version in the CMake file. The version is the same of the latest release
I opened the PR on devel. I don't know if it is fine or I should change the base branch to master
I should write something in the readme

traversaro · 2022-02-16T08:25:59Z

I opened the PR on devel. I don't know if it is fine or I should change the base branch to master

Devel is fine.

I should write something in the readme

Yes, that would great otherwise people will never know of the feature! The README probably would need a revamp in general (see #90), but I think that for this feature I think it is ok to just add two sections after https://github.com/robotology/icub-models#use-the-models-with-gazebo, something like:

Use the models from the C++ helper library
Use the models from the Python helper library
Where you just provide the minimal snippets of code.

diegoferigo · 2022-02-16T08:26:20Z

bindings/python/CMakeLists.txt

+
+# Output package is:
+#
+# bipedal_locomotion


Suggested change

# bipedal_locomotion

# icub_models

bindings/python/CMakeLists.txt

xEnVrE · 2022-02-16T09:12:01Z

bindings/CMakeLists.txt

+    install(FILES "${ICUB_MODELS_PYTHON_PACKAGE}/__init__.py"
+            DESTINATION ${PYTHON_INSTDIR})
+
+    # Install pip metadata files to ensure that blf installed via CMake is listed by pip list


Similarly to #130 (comment), the reference to blf seems to be a leftover as well.

Co-authored-by: Silvio Traversaro <silvio@traversaro.it>

…aries to load the models

GiulioRomualdi · 2022-02-16T10:01:43Z

@traversaro I updated the README in 6da40e2

traversaro · 2022-02-16T11:19:47Z

For me it is ok! Can you add a note in the CHANGELOG.md ? Thanks!

traversaro · 2022-02-16T11:39:58Z

Thanks!

GiulioRomualdi mentioned this pull request Feb 15, 2022

Add machinery to easily access model locations on Python #91

Closed

traversaro reviewed Feb 15, 2022

View reviewed changes

bindings/CMakeLists.txt Outdated Show resolved Hide resolved

traversaro reviewed Feb 15, 2022

View reviewed changes

traversaro approved these changes Feb 15, 2022

View reviewed changes

traversaro reviewed Feb 15, 2022

View reviewed changes

cpp/src/iCubModels.cpp.in Outdated Show resolved Hide resolved

diegoferigo approved these changes Feb 16, 2022

View reviewed changes

traversaro reviewed Feb 16, 2022

View reviewed changes

bindings/python/CMakeLists.txt Show resolved Hide resolved

xEnVrE reviewed Feb 16, 2022

View reviewed changes

GiulioRomualdi force-pushed the icub-models-library branch from 5cf0b9a to 258ee6a Compare February 16, 2022 09:17

GiulioRomualdi and others added 3 commits February 16, 2022 10:55

Implement the iCubModels c++ library

d47d7bb

Implement the python bindings for the iCubModels library

b69a438

Apply suggestions from code review

a9ad867

Co-authored-by: Silvio Traversaro <silvio@traversaro.it>

GiulioRomualdi force-pushed the icub-models-library branch from 258ee6a to 0744f7f Compare February 16, 2022 09:57

Update the README with the instruction to use the C++ and python libr…

6da40e2

…aries to load the models

GiulioRomualdi force-pushed the icub-models-library branch from 0744f7f to 6da40e2 Compare February 16, 2022 09:58

traversaro mentioned this pull request Feb 16, 2022

Add support for Python helper library of icub-models robotology/robotology-superbuild#1037

Merged

Update the CHANGELOG

86e30f0

traversaro merged commit f286488 into robotology:devel Feb 16, 2022

GiulioRomualdi deleted the icub-models-library branch February 16, 2022 12:40

traversaro mentioned this pull request Feb 18, 2022

Add C++ CI workflow to GitHub Actions and fix build with Xcode's 12.4 C++ compiler #131

Merged

GiulioRomualdi mentioned this pull request Feb 28, 2022

Add the possibility to load the urdf model at runtime ami-iit/robot-log-visualizer#25

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement a machinery to easily access model locations on C++ and Python #130

Implement a machinery to easily access model locations on C++ and Python #130

GiulioRomualdi commented Feb 15, 2022

GiulioRomualdi commented Feb 15, 2022

traversaro Feb 15, 2022

traversaro Feb 15, 2022

diegoferigo Feb 16, 2022

traversaro Feb 15, 2022

traversaro left a comment

GiulioRomualdi commented Feb 16, 2022 •

edited

Loading

traversaro commented Feb 16, 2022

diegoferigo Feb 16, 2022

xEnVrE Feb 16, 2022 •

edited

Loading

GiulioRomualdi Feb 16, 2022

GiulioRomualdi commented Feb 16, 2022

traversaro commented Feb 16, 2022

traversaro commented Feb 16, 2022

Implement a machinery to easily access model locations on C++ and Python #130

Implement a machinery to easily access model locations on C++ and Python #130

Conversation

GiulioRomualdi commented Feb 15, 2022

GiulioRomualdi commented Feb 15, 2022

traversaro Feb 15, 2022

Choose a reason for hiding this comment

traversaro Feb 15, 2022

Choose a reason for hiding this comment

diegoferigo Feb 16, 2022

Choose a reason for hiding this comment

traversaro Feb 15, 2022

Choose a reason for hiding this comment

traversaro left a comment

Choose a reason for hiding this comment

GiulioRomualdi commented Feb 16, 2022 • edited Loading

traversaro commented Feb 16, 2022

diegoferigo Feb 16, 2022

Choose a reason for hiding this comment

xEnVrE Feb 16, 2022 • edited Loading

Choose a reason for hiding this comment

GiulioRomualdi Feb 16, 2022

Choose a reason for hiding this comment

GiulioRomualdi commented Feb 16, 2022

traversaro commented Feb 16, 2022

traversaro commented Feb 16, 2022

GiulioRomualdi commented Feb 16, 2022 •

edited

Loading

xEnVrE Feb 16, 2022 •

edited

Loading