-
Notifications
You must be signed in to change notification settings - Fork 70
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
Conversation
COOOOL. |
I am not sure I get what you meant here.
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 |
There was a problem hiding this 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.
.github/workflows/python.yml
Outdated
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 |
There was a problem hiding this comment.
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.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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
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
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 :) |
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 |
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 |
Alternative trick, we can just prepend the examples and tests with:
to have them work in both ways, and be compatible also with iDynTree 1. From https://stackoverflow.com/a/20226931 . |
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 In other words, I'd like to keep the possibility to have 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
What we can do is improving the names,
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. |
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 |
ccf3f65
to
dba8387
Compare
At this point I think we have to converge about the last thing: whether to break or not 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). |
dba8387
to
7b89565
Compare
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 . |
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 |
Please do not merge yet, there are still some minor things to fix |
Co-authored-by: Silvio Traversaro <silvio.traversaro@iit.it>
both in the build and install trees
7b89565
to
c67a3b0
Compare
I'm trying to import iDynTree in a downstream project from the pip install tree and the following error occurred:
I fixed it with fa336b2. |
@traversaro ready to merge as soon as tests pass |
Actually I am afraid |
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 alsopyproject.toml
to enable PEP517 and PEP518 compatibility.This PR also contains a new GitHub workflow that builds and installs both
sdist
andbdist_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:CMAKE_INSTALL_PREFIX
and requires exporting thePYTHONPATH
environment variable. Then it can be imported withimport iDynTree
.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:
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.