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

Add ICAROS Python Bindings #48

Merged
merged 38 commits into from
Apr 1, 2021
Merged

Add ICAROS Python Bindings #48

merged 38 commits into from
Apr 1, 2021

Conversation

sniyaz
Copy link

@sniyaz sniyaz commented Feb 28, 2021

This PR adds in the Python bindings from https://github.com/icaros-usc/libada. The ultimate goal is to replace their stack with ours, while still conforming to the same Python API. Since most of ICAROS's work goes through the Python bindings (as far as I know) it shouldn't matter that PRL's C++ guts are much more up-to-date.

This PR essentially 1) adds in the binding source files and then 2) fixes everything to build (switching over methods that we've deprecated in AIKIDO, finding pybind11 in a more-general way, adding methods to the Ada.hpp interface that ICAROS added, etc). I also think I've fixed a few bugs in the original bindings (or at least what I perceived as bugs 😅).

There are also two large outstanding issues:

1. The version of pybind11 used in these bindings (and it looks like the version used in our aikidopy bindings as well) is 2.2.0. Most of PRL currently uses Ubuntu 18.04, which packages pybind11 2.0.1. Downgrading the pybind11 version is messy and doesn't seem to make sense: my gut is to 1) make building the bindings optional and 2) provide clear documentation on how to install a proper version of pybind11 from source (it's actually incredibly easy).

2. More critical: right now it looks like ICAROS is using ROS1, whereas PRL one day wants to move to ROS2. Unfortunately, because ROS1 doesn't support Python 3 I've had to set the Python version at 2.7. There are other barriers to moving to Python 3: installing python3-rospkg seems to uninstall all of the ROS melodic packages, which is definitely a big deal. I know @egordon was really set on setting Python 3, so this is something we need to debate. It seems like 2.7 is ugly, while 3.6 is potentially a show-stopper for ICAROS.

Testing Plan

I ran simple_trajectories.py, and it works great! :)


Before creating a pull request

  • Document new methods and classes
  • Format code with make format

Before merging a pull request

  • Add unit test(s) for this change

@sniyaz sniyaz marked this pull request as draft February 28, 2021 03:43
@sniyaz sniyaz changed the title [DRAFT] Add ICAROS Python Bindings Add ICAROS Python Bindings Mar 2, 2021
@sniyaz sniyaz requested a review from Stefanos19 March 2, 2021 03:25
@sniyaz sniyaz marked this pull request as ready for review March 2, 2021 04:35
@sniyaz sniyaz requested a review from hejia-zhang March 4, 2021 00:27
Copy link
Member

@hejia-zhang hejia-zhang left a comment

Choose a reason for hiding this comment

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

Looks good!

@hejia-zhang hejia-zhang self-requested a review March 4, 2021 20:51
src/adapy/ada_py.cpp Outdated Show resolved Hide resolved
src/libada/Ada.cpp Outdated Show resolved Hide resolved
Copy link
Member

@hejia-zhang hejia-zhang left a comment

Choose a reason for hiding this comment

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

  1. We could probably rename ComputeJointSpacePath as ComputeRetimedJointSpacePath? 2. We could change the compute_retime_path function to only accept aikido::trajectory::Interpolated typed trajectory?
    I have tested this branch with another lab assignment code and it works great!

Copy link
Contributor

@brianhou brianhou left a comment

Choose a reason for hiding this comment

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

I mostly have some quick high-level questions:

  • Would it make sense to move src/adapy to a top-level adapy directory or python/adapy? Otherwise, the layout of src and include no longer match our other repos. (python/adapy would match python/aikidopy; it might also then make sense to move the Python scripts into, say, python/tests.)
  • Should we organize this PR into two? One with actual libada changes, one with bindings.
  • Do bindings need to be defined as C++ lambda expressions? I feel like this would be way more readable if, say, we explicitly named the create_ik function and then did all the module m.def stuff with those named functions.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@egordon egordon requested a review from madan96 March 15, 2021 22:13
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
python/adapy/aikido_py.cpp Outdated Show resolved Hide resolved
python/adapy/aikido_py.cpp Outdated Show resolved Hide resolved
python/adapy/pr_tsr_py.cpp Outdated Show resolved Hide resolved
python/adapy/pr_tsr_py.cpp Outdated Show resolved Hide resolved
python/adapy/pr_tsr_py.cpp Outdated Show resolved Hide resolved
python/tests/simple_trajectories.py Show resolved Hide resolved
python/tests/simple_trajectories.py Outdated Show resolved Hide resolved
@egordon egordon requested a review from brianhou March 16, 2021 00:17
Copy link
Contributor

@brianhou brianhou left a comment

Choose a reason for hiding this comment

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

Do bindings need to be defined as C++ lambda expressions? I feel like this would be way more readable if, say, we explicitly named the create_ik function and then did all the module m.def stuff with those named functions.

I'm willing to accept the changes as is for now, but I still feel like this would be better.

@sniyaz
Copy link
Author

sniyaz commented Mar 19, 2021

An update: I've (finally) managed to get my dev box set up with 20.04 + Noetic. I can confirm that from a fresh, standard machine install everything works fine. The pybind11 version is more than sufficient and everything works great in sim :)

I'm going to work on some more of this feedback from @brianhou now that I have the new env set up and working 😃

@sniyaz
Copy link
Author

sniyaz commented Mar 28, 2021

@brianhou and others, I've responded to the lambda feedback and cleaned up a few other things. I think we're ready for a final review?

I wasn't sure whether to use camel case for the named methods since that seemed a little confusing, but happy to change it if we want :)

@egordon egordon requested a review from brianhou March 29, 2021 19:30
Copy link
Member

@hejia-zhang hejia-zhang left a comment

Choose a reason for hiding this comment

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

It looks good!

@egordon
Copy link
Collaborator

egordon commented Apr 1, 2021

Everything still runs perfectly on the real bot. Merging as admin past the Travis checks. Good job all!

@egordon egordon merged commit d6928b9 into master Apr 1, 2021
@egordon egordon deleted the icaros_merge branch April 1, 2021 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants