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

Initial support to Python packaging #733

Merged
merged 9 commits into from
Sep 10, 2020

Conversation

diegoferigo
Copy link
Member

@diegoferigo diegoferigo commented Sep 9, 2020

This PR finalizes the work started with #726 by allowing installing iDynTree using pip.

The versioning of the resulting packages is computed using pypa/setuptools_scm and it takes the most recent git tag. Note that, depending on the tag, it could not be compatible with PEP440.

Beyond the setup.py, I included also pyproject.toml to enable PEP517 and PEP518 compatibility.

This PR also contains a new GitHub workflow that builds and installs both sdist and bdist_wheel packages. The only test performed is the import of the SWIG bindings. On this regard, there is a big difference between the installation using CMake and pip:

  • With CMake, the Python package is installed inside CMAKE_INSTALL_PREFIX and requires exporting the PYTHONPATH environment variable. Then it can be imported with import iDynTree.
  • With pip, instead, the package is installed in the active Python's site-package. It can be imported with from idyntree import iDynTree.

The main reason is that with pip also the CMake project is installed in the Python's site-package. It enables importing the CMake project downstream also when installed with pip. However, this requires the creation of a <site-package>/idyntree that affects how the package is imported.

To prevent downstream projects that import the CMake targets from the pip install tree, iDynTree is built statically.

There are some limitations:

  • Only GNU/Linux is currently supported.
  • Editable installation is not supported.
  • Packages are not deployed to PyPI. This is a bit delicate because we should decide how to handle iDynTree dependencies. In fact, the shared libraries that iDyntree links against, are not included in the PyPI package. So we should either 1) ask the users to also install iDynTree dependencies before using pip, or 2) investigate pypa/auditwheel to vendor the depending shared libraries.

For the time being I would mark this feature as experimental and lift some limitations slowly in future PRs.

@traversaro
Copy link
Member

COOOOL.

@traversaro
Copy link
Member

To prevent downstream projects that import the CMake targets from the pip install tree, iDynTree is built statically.

I am not sure I get what you meant here.

On this regard, there is a big difference between the installation using CMake and pip:

I am not sure I follow why this difference is necessary, but I would trust you on this. Would it possible to make sure that also the CMake-installed version is imported with from idyntree import iDynTree, so that we have a unique way of importing iDynTree in both cases, even if we break backward compatibility with iDynTree 1.x ?

Copy link
Member

@traversaro traversaro left a comment

Choose a reason for hiding this comment

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

Some comments, but apart from that is good to go for me. As you write in the PR, we can sort out how to use this in the future, if necessary.

Comment on lines 33 to 36
sudo apt-get install -y \
git build-essential cmake cmake-extras libace-dev coinor-libipopt-dev libeigen3-dev swig \
qtbase5-dev qtdeclarative5-dev qtmultimedia5-dev libqt5charts5-dev \
libxml2-dev liboctave-dev python-dev python3-numpy valgrind libassimp-dev
Copy link
Member

Choose a reason for hiding this comment

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

Given that this should just build the Python bindings, I would limit the dependencies to the one strictly necessary.

Suggested change
sudo apt-get install -y \
git build-essential cmake cmake-extras libace-dev coinor-libipopt-dev libeigen3-dev swig \
qtbase5-dev qtdeclarative5-dev qtmultimedia5-dev libqt5charts5-dev \
libxml2-dev liboctave-dev python-dev python3-numpy valgrind libassimp-dev
sudo apt-get install -y \
git build-essential cmake cmake-extras libace-dev coinor-libipopt-dev libeigen3-dev swig \
libxml2-dev python3-numpy libassimp-dev

Copy link
Member Author

@diegoferigo diegoferigo Sep 9, 2020

Choose a reason for hiding this comment

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

Sure, I'll try to reduce them to the minimal ones.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think b3e4667 addresses the requirement on minimal dependencies. Unfortunately setuptools_scm is not compatible with PEP51{7,8), and it must be installed in the system otherwise the proper version is not computed from the git tags resulting in 0.0.0 which, as a consequence, creates a faulty sdist package.

# CMake configure arguments
configure_args = [
f"-GNinja",
f"-DCMAKE_INSTALL_PREFIX:PATH={cmake_install_prefix}",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
f"-DCMAKE_INSTALL_PREFIX:PATH={cmake_install_prefix}",
f"-DCMAKE_INSTALL_PREFIX:PATH={cmake_install_prefix}",
f"-DIDYNTREE_USES_IPOPT:BOOL=ON",
f"-DIDYNTREE_USES_ASSIMP:BOOL=ON",
f"-DIDYNTREE_USES_IRRLICHT:BOOL=OFF",
f"-DIDYNTREE_USES_QT5:BOOL=OFF",
f"-DIDYNTREE_USES_OSQPEIGEN:BOOL=OFF",
f"-DIDYNTREE_USES_ALGLIB:BOOL=OFF",
f"-DIDYNTREE_USES_WORHP :BOOL=OFF",
f"-DIDYNTREE_USES_YARP:BOOL=OFF",
f"-DIDYNTREE_USES_ICUB_MAIN:BOOL=OFF",

To ensure a reproducible package being generated regardless of the state of the dependency that are installled in the system, I would add here explicitly which dependency we build and which we don't . See https://github.com/robotology/idyntree/blob/devel/cmake/iDynTreeDependencies.cmake for more details.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! I update the PR accordingly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 7b89565. The location where the CMake options are passed is not there but below, where the extension is configured. The lines of your suggestion are meant to be project-independent and could be moved to a standalone library.

@diegoferigo
Copy link
Member Author

COOOOL.

The setup that enables all this Python packaging is basically the same used in gym-ignition and gazebo-scenario-plugins, and it is based on the existence of a CMake project. It is very easy to package in this way any CMake-based project, that luckily is the case of the majority of our projects :)

Since the CMakeExtension and BuildExtension class are duplicate, maybe on the long run it would be better making a standalone Python package with them and then just import it where needed. This centralization would for sure help the propagation of Windows and macOS support when we'll finalize it.

To prevent downstream projects that import the CMake targets from the pip install tree, iDynTree is built statically.

I am not sure I get what you meant here.

Think about a setup where iDynTree is installed with pip, and a downstream C++ project that links against it also publishes a component to PyPI. I'm not sure if we can exploit RPATH so good so that the shared libraries of the iDynTree package are found by those from the dowstream project (and using LD_LIBRARY_PATH is not an option). What complicates further is that with Python there could be multiple roots (e.g. system, user, virtualenv, ...) therefore I suspect that the RPATH way would not be robust. I'm not really 100% sure, but I have the feeling that compiling and publishing static libraries simplifies the life of downstream projects avoiding the usage of auditwheel workarounds. In any case, if a static compilation would not create troubles to the project, I think is a good starting point.

I am not sure I follow why this difference is necessary, but I would trust you on this. Would it possible to make sure that also the CMake-installed version is imported with from idyntree import iDynTree, so that we have a unique way of importing iDynTree in both cases, even if we break backward compatibility with iDynTree 1.x ?

Sure, I tried to avoid breaking backward compatibility but if you give me permission I'd be happy to do it, it's always a pleasure :)

@traversaro
Copy link
Member

To prevent downstream projects that import the CMake targets from the pip install tree, iDynTree is built statically.

I am not sure I get what you meant here.

Think about a setup where iDynTree is installed with pip, and a downstream C++ project that links against it also publishes a component to PyPI. I'm not sure if we can exploit RPATH so good so that the shared libraries of the iDynTree package are found by those from the dowstream project (and using LD_LIBRARY_PATH is not an option). What complicates further is that with Python there could be multiple roots (e.g. system, user, virtualenv, ...) therefore I suspect that the RPATH way would not be robust. I'm not really 100% sure, but I have the feeling that compiling and publishing static libraries simplifies the life of downstream projects avoiding the usage of auditwheel workarounds. In any case, if a static compilation would not create troubles to the project, I think is a good starting point.

If there is no need to re-use the iDynTree compiled library, could it make sense to not install at all the iDynTree static library, its headers and its CMake configuration file, and just install the iDynTree.so Python plugin that in any case already links statically the iDynTree library?

@diegoferigo
Copy link
Member Author

If there is no need to re-use the iDynTree compiled library, could it make sense to not install at all the iDynTree static library, its headers and its CMake configuration file, and just install the iDynTree.so Python plugin that in any case already links statically the iDynTree library?

This is what I was doing some while ago also with gym-ignition. Though, the CMake logic if you remember was a bit complicated. Installing the entire CMake project allows minimal changes to the existing CMakeLists and also allows importing the CMake project from a pip installation, which is nice since it comes for free.

@traversaro
Copy link
Member

If there is no need to re-use the iDynTree compiled library, could it make sense to not install at all the iDynTree static library, its headers and its CMake configuration file, and just install the iDynTree.so Python plugin that in any case already links statically the iDynTree library?

This is what I was doing some while ago also with gym-ignition. Though, the CMake logic if you remember was a bit complicated. Installing the entire CMake project allows minimal changes to the existing CMakeLists and also allows importing the CMake project from a pip installation, which is nice since it comes for free.

Ok, a last question: if we did not install all the C++ stuff, then we could just keep import iDynTree without from idyntree import iDynTree ?

@traversaro
Copy link
Member

Alternative trick, we can just prepend the examples and tests with:

import pkg_resources

try:
    pkg_resources.get_distribution('iDynTree')
except pkg_resources.DistributionNotFound:
    from idyntree import iDynTree
else:
    import iDynTree

to have them work in both ways, and be compatible also with iDynTree 1. From https://stackoverflow.com/a/20226931 .
By the way, I just realized that relying on camelcase difference to distinguish different libraries may be a problem in case insensitive file systems.

@diegoferigo
Copy link
Member Author

Ok, a last question: if we did not install all the C++ stuff, then we could just keep import iDynTree without from idyntree import iDynTree?

Yes we could. Though, I still think including the CMake project is a great feature that could be worth the price to pay. For many, a simple pip install idyntree and export CMAKE_INSTALL_PREFIX is way simpler and faster than cloning the repo and installing it. Beyon this, having a idyntree package is useful for future features. If we want to provide Python modules of any sort (e.g. a new idiomatic Python class that wraps kindyncomputation or anything comparable) we need a real python package. I prefer breaking things today that nobody (I know) is using iDynTree from python rather than doing it later.

In other words, I'd like to keep the possibility to have from idyntree import iDynTree for the raw SWIG objects and import idyntree.high_level and then using idyntree.high_level.MyHighLevelIdiomaticClass().

In these days I'm playing with osqp and its Python bindings, and prototyping things from python is way faster than C++. Even more, using high-level libraries like cvxpy enables playing with multiple solvers with the same code, which is quite powerful and maybe useful to few people in the lab. @GiulioRomualdi @S-Dafarra

By the way, I just realized that relying on camelcase difference to distinguish different libraries may be a problem in case insensitive file systems.

What we can do is improving the names, from idyntree import iDynTree is not the best but it's what, again, does not break backward compatibility of all the code that was using iDynTree.* classes. If we don't care about breaking changes, we can consider renaming the swig files (as we already do for ScenarI/O) to get something like from idyntree import bindings or whatever we feel more appropriate.

Alternative trick, we can just prepend the examples and tests with [...]

Yes this would work fine. Let's decide how many things we want to break before :) Do you know anyone that uses iDynTree from Python? Luckily module name changes are trivial to perform downstream.

@traversaro
Copy link
Member

Do you know anyone that uses iDynTree from Python? Luckily module name changes are trivial to perform downstream.

There are indeed a few users in scripts both in IIT and outside. Nothing that prevents us to change things as long as we document the changes, but anything that makes updating smoother is welcome. In any case, the from idyntree import iDynTree stuff is a nice compromise.

@diegoferigo
Copy link
Member Author

In any case, the from idyntree import iDynTree stuff is a nice compromise.

At this point I think we have to converge about the last thing: whether to break or not import iDynTree. From one hand, keeping the regular installation from CMake as it is preserves backward compatibility. From the other, it requires the double import attempt pretty much everywhere because from Python it's not possible to know in advance how the bindings have been installed.

I personally lean towards the breaking way, properly notifying users before the next release (that as far as I understood is going to be a major release).

@traversaro
Copy link
Member

I personally lean towards the breaking way, properly notifying users before the next release (that as far as I understood is going to be a major release).

Ok for me. We can still use:

import pkg_resources

try:
    pkg_resources.get_distribution('iDynTree')
except pkg_resources.DistributionNotFound:
    from idyntree import iDynTree
else:
    import iDynTree

for scripts that need to be used/be compatible with both iDynTree 1.x and 2.x .

@traversaro
Copy link
Member

By the way, while not directly related this PR can be of interest of users that use iDynTree with Python, even if in another form. @francesco-romano

@diegoferigo
Copy link
Member Author

Please do not merge yet, there are still some minor things to fix

@diegoferigo
Copy link
Member Author

I personally lean towards the breaking way, properly notifying users before the next release (that as far as I understood is going to be a major release).

Done in 6f4cb69 and import statements of the tests updated in c67a3b0.

@diegoferigo
Copy link
Member Author

I'm trying to import iDynTree in a downstream project from the pip install tree and the following error occurred:

CMake Error in cpp/scenario/controllers/CMakeLists.txt:
  Imported target "iDynTree::idyntree-core" includes non-existent path

    "/tmp/pip-req-build-71m8b5ga/build/lib.linux-x86_64-3.8/idyntree/include"

  in its INTERFACE_INCLUDE_DIRECTORIES.  Possible reasons include:

  * The path was deleted, renamed, or moved to another location.

  * An install or uninstall procedure did not complete successfully.

  * The installation package was faulty and references files it does not
  provide.



CMake Error in cpp/scenario/controllers/CMakeLists.txt:
  Imported target "iDynTree::idyntree-core" includes non-existent path

    "/tmp/pip-req-build-71m8b5ga/build/lib.linux-x86_64-3.8/idyntree/include"

  in its INTERFACE_INCLUDE_DIRECTORIES.  Possible reasons include:

  * The path was deleted, renamed, or moved to another location.

  * An install or uninstall procedure did not complete successfully.

  * The installation package was faulty and references files it does not
  provide.

I fixed it with fa336b2.

@diegoferigo
Copy link
Member Author

@traversaro ready to merge as soon as tests pass

@francesco-romano
Copy link
Collaborator

By the way, while not directly related this PR can be of interest of users that use iDynTree with Python, even if in another form. @francesco-romano

Actually I am afraid from idyntree import iDynTree will not work on "my" build system, but luckily this is confined to the tests. I do not see any other modification that can impact other build systems as they are all in the CMakeLists.txt.

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