Add bindings for AutomotiveSimulator #177

mikaelarguedas · 2017-12-22T15:29:24Z

This adds the bindings required to instantiate and configure the AutomotiveSimulator from Python.

This includes the commits from #176 pending it getting merged.
In the meantime the porting is done in:
1ef6b7c
And the missing bindings are documented in bbbffe1 with the classes the need to be bind before being able to use them

fixes #165

mikaelarguedas · 2017-12-22T15:43:55Z

+@EricCousineau-TRI for a more authoritative review.

FYI: This is slightly related to RobotLocomotion/drake#7660. ATM I ported only the small subset of AutomotiveSimulator methods we use and the SimpleCarState class. I'm not sure it's significant enough to deserve starting an automotive pydrake module, if not, feel free to grab whatever is here when the work of binding the AutomotiveSimulator starts on the Drake side

EricCousineau-TRI

I'm a little confused by this merge, as it seems to combine the unmerged #172 here.

Could you change the merge base in GitHub to be this PR, and then change it back to AUtomotiveSimulatorPort once it has merged?

EricCousineau-TRI · 2017-12-22T16:20:38Z

backend/CMakeLists.txt

@@ -1,5 +1,9 @@
 include (${project_cmake_dir}/Utils.cmake)
+include_directories(${PYTHON_INCLUDE_DIRS})


BTW Should this just rely solely on pybind11::module, and not need the include_directories?
Also, I think the Python include directories should be a transitive dependency of pybind11 - could it be removed here (if you're not using it directly)?

Yeah that's what I thought as well but noticed that it was currently not the case. If I remove this line:

In file included from /home/mikael/work/delphyne_demo/install/include/pybind11/pytypes.h:12:0, from /home/mikael/work/delphyne_demo/install/include/pybind11/cast.h:13, from /home/mikael/work/delphyne_demo/install/include/pybind11/attr.h:13, from /home/mikael/work/delphyne_demo/install/include/pybind11/pybind11.h:43, from /home/mikael/work/delphyne_demo/src/delphyne/backend/simulation_runner.cc:39: /home/mikael/work/delphyne_demo/install/include/pybind11/detail/common.h:111:20: fatal error: Python.h: No such file or directory

I agree that this should be provided by the pybind module and I think pybind doest the right thing as I don't need this line if I use pybind built with CMake. There must be something fishy happening when building it with bazel that doesnt walk the imported target properly.

I'm in favor of leaving it for now as it allows us to move forward, I can ticket it on the Drake repo and or add a todo in this repo.

EricCousineau-TRI · 2017-12-22T16:24:08Z

backend/simulation_runner.cc

@@ -94,22 +96,37 @@ SimulatorRunner::SimulatorRunner(
    ignerr << "Error advertising service [" << kRobotRequestServiceName
              << "]" << std::endl;
  }
+  // Initialize the python machinery so we can invoke a python callback
+  // function on each simulation step.
+  Py_Initialize();


I'm a tad confused.... Why does this need Py_Initialize if this can be created from Python?
Is this so you can effectively call Python functions directly from C++?

Could you isolate your call sites from pure C++ to Python (which I believe would be better) and use pybind11::scoped_interpreter? http://pybind11.readthedocs.io/en/stable/advanced/embedding.html#getting-started

If not, is there a way to check if Python has already been initialized?

mikaelarguedas · 2017-12-22T16:33:13Z

Yes sorry it's uneasy to review.
#172 has been merged, I was expecting #176 to land before this got reviewed.
The only actual change of this PR is: AutomotiveSimulatorPort_merge_master...more_bindings.

I tried to highlight in the PR description what commits were actually new in this PR. I'll change the base branch to facilitate review and switch it back after as you suggested.

mikaelarguedas · 2017-12-22T16:34:55Z

This is now using AutomotiveSimulatorPort_merge_master as a base

mikaelarguedas · 2017-12-22T17:11:06Z

#176 landed, switch base branch back to AutomotiveSimulatorPort

EricCousineau-TRI · 2017-12-22T19:42:16Z

Thanks!

I'm not sure it's significant enough to deserve starting an automotive pydrake module, if not, feel free to grab whatever is here when the work of binding the AutomotiveSimulator starts on the Drake side

My preference would be to introduce drake-scoped classes directly into pydrake always, and avoid implementing them in delphyne, regardless of whether or not they're introducing a small set of features (as an example of small, see some of the current primitives).
If the classes are implemented exposed via pybind11 multiple times (drake upstream update + older delphyne branch), pybind11 will not be happy and throw an error, breaking delphyne when drake introduces conflicting features.

That being said, I'd hate for this to slow y'all down for short-term PRs and such.
Do y'all have a workflow / procedure for co-developing drake and delphyne that could assist in prototyping both and submitting / managing both PRs simultaneously?

EricCousineau-TRI · 2017-12-22T19:42:43Z

backend/CMakeLists.txt

-find_package(Boost COMPONENTS python REQUIRED)
-include_directories(${Boost_INCLUDE_DIR})
+find_package(pybind11 REQUIRED)
+include_directories(${pybind11_INCLUDE_DIRS})


Is include_directories still needed if using pybind11::module?

You're right that's not needed now that we use the exported target, thanks!

EricCousineau-TRI · 2017-12-22T19:43:16Z

backend/python_bindings_example.py

+
+from pydrake.common import AddResourceSearchPath
+
+from simulation_runner_py import AutomotiveSimulator


Consider importing multiple symbols using a single import statement:

from simulation_runner_py import ( AutomotiveSimulator, SimpleCarState, SimulatorRunner, )

EricCousineau-TRI · 2017-12-22T19:44:35Z

backend/simulation_runner_py.cc

+    // .def("GetCurrentPoses", &AutomotiveSimulator<double>::GetCurrentPoses)
+  ;
+  // TODO(mikaelarguedas) check with @EricCousineau-TRI for moving this to pydrake
+  py::class_<SimpleCarState<double>>(m, "SimpleCarState")


Following up from my comment on the issue thread, and since Jon requested this, I'll go ahead and submit a PR with this in it. (Feel free to fork that PR if you need more things.)

Great, thanks!

basicNew · 2017-12-22T20:55:23Z

Thanks for the comments @EricCousineau-TRI ! Yes, we will submit our changes to Drake, we just needed this code to work on other fronts, so decided to push all the bindings in this PR. As soon as we return from the holidays I will split the code and send the required PRs.

remove unnecessary include_directories and find_package since SearchForStuff CMake refactor import multiple symbols in simgle import call

add todo for contributing some bindings upstream

mikaelarguedas · 2017-12-23T00:19:49Z

Thank you both for the feedback! I addressed some comments and responded to other. rebased and squashed to have cleaner commit history

EricCousineau-TRI · 2017-12-28T03:06:27Z

backend/python_bindings_example.py

+    AutomotiveSimulator,
+    SimpleCarState,
+    SimulatorRunner
+)


 def get_from_env_or_fail(var):


BTW Below, the stripping could be done by value.rstrip(' :').

EricCousineau-TRI · 2017-12-28T03:08:15Z

backend/simulation_runner_py.cc

      return std::make_unique<SimulatorRunner>(std::move(simulator), time_step);
    }))
    .def("Start", &SimulatorRunner::Start)
    .def("Stop", &SimulatorRunner::Stop)
    .def("AddStepCallback", &SimulatorRunner::AddStepCallback);
  ;
+  py::class_<AutomotiveSimulator<double>, std::unique_ptr<AutomotiveSimulator<double>>>(m, "AutomotiveSimulator")


BTW Specifying unique_ptr as the holder type is redundant; if you're going to always specify, I'd suggest making an extension of py::class_ to handle these things by default for you.

(At some point in the near future, I'll be introducing something like pydrake::class_ or something like that, for this and other facets.)

mikaelarguedas · 2018-01-02T16:52:23Z

Thanks @EricCousineau-TRI for the review, comments adressed in 57dea01

mikaelarguedas · 2018-01-02T17:35:37Z

Merging this.
What has not been done in this PR: contribute bindings (SimpleCar for example) to Drake directly rather than defining them here
@basicNew FYI

EricCousineau-TRI · 2018-01-02T21:32:41Z

backend/simulation_runner_py.cc

-
-      // Instantiate the simulator runner and pass the simulator.
-      const double time_step = 0.001;
+    .def(py::init([](std::unique_ptr<AutomotiveSimulator<double>> simulator, double time_step) {
      return std::make_unique<SimulatorRunner>(std::move(simulator), time_step);


Just a heads up (sorry for adding this post-merge): You should be able to use py::init<Args...>() as a shorthand for py::init([](Args&& args...) { return make_unique<...>(...); }.

Thanks @EricCousineau-TRI , done in f3232e0

EricCousineau-TRI suggested changes Dec 22, 2017

View reviewed changes

mikaelarguedas changed the base branch from AutomotiveSimulatorPort to AutomotiveSimulatorPort_merge_master December 22, 2017 16:33

mikaelarguedas changed the base branch from AutomotiveSimulatorPort_merge_master to AutomotiveSimulatorPort December 22, 2017 17:10

mikaelarguedas mentioned this pull request Dec 22, 2017

use latest Drakes' pybind11 version to get support for moving unique pointers osrf/drake#5

Merged

EricCousineau-TRI reviewed Dec 22, 2017

View reviewed changes

mikaelarguedas added 2 commits December 23, 2017 01:17

move to pybind and move simulator initialization to Python side

9045f35

remove unnecessary include_directories and find_package since SearchForStuff CMake refactor import multiple symbols in simgle import call

document missing bindings and their dependencies

06edbb0

add todo for contributing some bindings upstream

mikaelarguedas force-pushed the more_bindings branch from 1e1312f to 06edbb0 Compare December 23, 2017 00:18

EricCousineau-TRI reviewed Dec 28, 2017

View reviewed changes

EricCousineau-TRI approved these changes Dec 28, 2017

View reviewed changes

address review comments

57dea01

mikaelarguedas merged commit b508fa8 into AutomotiveSimulatorPort Jan 2, 2018

mikaelarguedas deleted the more_bindings branch January 2, 2018 17:35

EricCousineau-TRI reviewed Jan 2, 2018

View reviewed changes

basicNew mentioned this pull request Jan 3, 2018

Add backwards compatibility support to AutomotiveSimulator #181

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add bindings for AutomotiveSimulator #177

Add bindings for AutomotiveSimulator #177

mikaelarguedas commented Dec 22, 2017 •

edited

Loading

mikaelarguedas commented Dec 22, 2017

EricCousineau-TRI left a comment

EricCousineau-TRI Dec 22, 2017

mikaelarguedas Dec 23, 2017

EricCousineau-TRI Dec 22, 2017

mikaelarguedas commented Dec 22, 2017

mikaelarguedas commented Dec 22, 2017

mikaelarguedas commented Dec 22, 2017

EricCousineau-TRI commented Dec 22, 2017

EricCousineau-TRI Dec 22, 2017

mikaelarguedas Dec 23, 2017

EricCousineau-TRI Dec 22, 2017

EricCousineau-TRI Dec 22, 2017

basicNew Dec 22, 2017

basicNew commented Dec 22, 2017

mikaelarguedas commented Dec 23, 2017

EricCousineau-TRI Dec 28, 2017

EricCousineau-TRI Dec 28, 2017 •

edited

Loading

mikaelarguedas commented Jan 2, 2018

mikaelarguedas commented Jan 2, 2018

EricCousineau-TRI Jan 2, 2018 •

edited

Loading

basicNew Jan 3, 2018

		@@ -1,5 +1,9 @@
		include (${project_cmake_dir}/Utils.cmake)
		include_directories(${PYTHON_INCLUDE_DIRS})


		from pydrake.common import AddResourceSearchPath

		from simulation_runner_py import AutomotiveSimulator

Add bindings for AutomotiveSimulator #177

Add bindings for AutomotiveSimulator #177

Conversation

mikaelarguedas commented Dec 22, 2017 • edited Loading

mikaelarguedas commented Dec 22, 2017

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikaelarguedas commented Dec 22, 2017

mikaelarguedas commented Dec 22, 2017

mikaelarguedas commented Dec 22, 2017

EricCousineau-TRI commented Dec 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

basicNew commented Dec 22, 2017

mikaelarguedas commented Dec 23, 2017

Choose a reason for hiding this comment

EricCousineau-TRI Dec 28, 2017 • edited Loading

Choose a reason for hiding this comment

mikaelarguedas commented Jan 2, 2018

mikaelarguedas commented Jan 2, 2018

EricCousineau-TRI Jan 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikaelarguedas commented Dec 22, 2017 •

edited

Loading

EricCousineau-TRI Dec 28, 2017 •

edited

Loading

EricCousineau-TRI Jan 2, 2018 •

edited

Loading