fzi-forschungszentrum-informatik / Lanelet2

Map handling framework for automated driving
BSD 3-Clause "New" or "Revised" License
799 stars 327 forks source link

Impossible to construct AllWayStop from Python #223

Closed mantkiew closed 1 year ago

mantkiew commented 2 years ago

The following code

ll_with_stop_line = LaneletWithStopLine(yielding_lanelet, stop_linestring)
all_way_stop = AllWayStop(getId(),
                              AttributeMap(),
                              [ll_with_stop_line])

gives the following error in Python 3.8:

RuntimeError: This class cannot be instantiated from Python

which refers to LaneletWithStopLine.

The problem is that LaneletWithStopLine is defined as

  class_<LaneletWithStopLine>("LaneletWithStopLine", "A lanelet with a stopline", no_init)
      .add_property("lanelet", &LaneletWithStopLine::lanelet)
      .add_property("stopLine", &LaneletWithStopLine::stopLine);

that is, it uses the no_init descriptor and it does not have any constructor or any other function to create it.

How should one instantiate LaneletWithStopLine from Python?

poggenhans commented 2 years ago

Ah, true, that is an oversight, the support for creating regulatory elements (and the things needed to create them, like LaneletWithStopLine) from python is still a bit incomplete. Feel free to open a PR for this to speed things up, otherwise we will fix it at some point.

mantkiew commented 2 years ago

@poggenhans I tried implementing this but I am simply lacking the knowledge. Normally, simple structs get a default constructor when you omit the no_init; however, here it does work. I keep getting

>>> from lanelet2.core import (LaneletWithStopLine)
>>> lwsl = LaneletWithStopLine() 
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
RuntimeError: This class cannot be instantiated from Python.

Here's what I tried:

class_<LaneletWithStopLine>("LaneletWithStopLine", "A lanelet with a stopline")
      .def_readwrite("lanelet", &LaneletWithStopLine::lanelet)
      .def_readwrite("stopLine", &LaneletWithStopLine::stopLine);

That is removed the no_init and made the properties read/write.

The possible designs are:

  1. Not have the stopLine Optional. Why is the property optional here?
  2. Change the LaneletWithStopLine into a class and follow the pattern like with RightOfWay with defining an explicit constructor.
  3. define a factory function makeLaneletWithStopLine with on optional argument stopLine to construct the instance properly.

I consulted this https://pretagteam.com/question/return-a-structure-to-python-from-c-using-boostpython Another useful example: https://stackoverflow.com/questions/36138533/boost-python-constructor-with-optional-arguments And, of course, the official docs: https://www.boost.org/doc/libs/1_68_0/libs/python/doc/html/tutorial/tutorial/exposing.html

mantkiew commented 2 years ago

I am on Ubuntu 20.04 and I am building with colcon. However, I'm seeing these errors

$ colcon build --packages-select lanelet2_python 
Starting >>> lanelet2_python
--- stderr: lanelet2_python                                             
/home/ma/.local/lib/python3.8/site-packages/setuptools/command/install.py:34: SetuptoolsDeprecationWarning: setup.py install is deprecated. Use build and pip and other standards-based tools.
  warnings.warn(
/home/ma/.local/lib/python3.8/site-packages/setuptools/command/easy_install.py:156: EasyInstallDeprecationWarning: easy_install command is deprecated. Use build and pip and other standards-based tools.
  warnings.warn(
/home/ma/.local/lib/python3.8/site-packages/pkg_resources/__init__.py:116: PkgResourcesDeprecationWarning: 0.1.36ubuntu1 is an invalid version and will not be supported in a future release
  warnings.warn(
/home/ma/.local/lib/python3.8/site-packages/pkg_resources/__init__.py:116: PkgResourcesDeprecationWarning: 0.23ubuntu1 is an invalid version and will not be supported in a future release
  warnings.warn(
zip_safe flag not set; analyzing archive contents...
lanelet2.__pycache__.__init__.cpython-38: module references __file__
---
Finished <<< lanelet2_python [2.73s]

Summary: 1 package finished [2.87s]
  1 package had stderr output: lanelet2_python

and when I source the workspace and try to use it in python3, the Python API is not exposed properly:

$ source install/local_setup.bash 
$ python3
Python 3.8.10 (default, Sep 28 2021, 16:10:42) 
[GCC 9.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from lanelet2.core import (LaneletWithStopLine)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ModuleNotFoundError: No module named 'lanelet2'
>>> 

So, the reason my changes were having no effect was because the binary installation was being used!

mantkiew commented 2 years ago

I have solved the issue. Here's the proof:

$ python3
Python 3.8.10 (default, Sep 28 2021, 16:10:42) 
[GCC 9.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from lanelet2.core import (LaneletWithStopLine, Lanelet, LineString3d)
>>> ls1 = LineString3d(1, [])
>>> ls2 = LineString3d(2, [])
>>> lanelet = Lanelet(1, ls1, ls2)
>>> stopLine = LineString3d(3, [])
>>> lwsl = LaneletWithStopLine(lanelet, stopLine)
>>> lwsl_default = LaneletWithStopLine()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
Boost.Python.ArgumentError: Python argument types in
    LaneletWithStopLine.__init__(LaneletWithStopLine)
did not match C++ signature:
    __init__(boost::python::api::object, lanelet::Lanelet lanelet, boost::optional<lanelet::LineString3d> stopLine=None)

I had to build with catkin and everything works. Not sure why building with colcon does not properly expose the Python API.

Here are the changes:

$ gitd
diff --git a/lanelet2_core/include/lanelet2_core/primitives/BasicRegulatoryElements.h b/lanelet2_core/include/lanelet2_core/primitives/BasicRegulatoryElements.h
index b93b321..bd46d54 100644
--- a/lanelet2_core/include/lanelet2_core/primitives/BasicRegulatoryElements.h
+++ b/lanelet2_core/include/lanelet2_core/primitives/BasicRegulatoryElements.h
@@ -144,10 +144,37 @@ class RightOfWay : public RegulatoryElement {
 };

 struct LaneletWithStopLine {
+  /**
+   * @brief Create a valid LaneletWithStopLine
+   * @param lanelet the lanelet that has a stop line.
+   * @param stopLine line where to stop. If there is none, stop at the end of
+   * the lanelet.
+   */
+  static std::shared_ptr<LaneletWithStopLine> make(const Lanelet& lanelet,
+                                const Optional<LineString3d>& stopLine = {}) {
+      std::shared_ptr<LaneletWithStopLine> lwsl{new LaneletWithStopLine()};
+      lwsl->lanelet = lanelet;
+      lwsl->stopLine = stopLine;
+    return lwsl;
+  }
   Lanelet lanelet;
   Optional<LineString3d> stopLine;
 };
+
 struct ConstLaneletWithStopLine {
+  /**
+   * @brief Create a valid LaneletWithStopLine
+   * @param lanelet the lanelet that has a stop line.
+   * @param stopLine line where to stop. If there is none, stop at the end of
+   * the lanelet.
+   */
+  static std::shared_ptr<ConstLaneletWithStopLine> make(const Lanelet& lanelet,
+                                const Optional<LineString3d>& stopLine = {}) {
+      std::shared_ptr<ConstLaneletWithStopLine> lwsl{new ConstLaneletWithStopLine()};
+      lwsl->lanelet = lanelet;
+      lwsl->stopLine = stopLine;
+    return lwsl;
+  }
   ConstLanelet lanelet;
   Optional<ConstLineString3d> stopLine;
 };
diff --git a/lanelet2_python/python_api/core.cpp b/lanelet2_python/python_api/core.cpp
index 86aaa31..231c987 100644
--- a/lanelet2_python/python_api/core.cpp
+++ b/lanelet2_python/python_api/core.cpp
@@ -754,10 +754,14 @@ BOOST_PYTHON_MODULE(PYTHON_API_MODULE_NAME) {  // NOLINT
       .def("__getitem__", wrappers::getItem<LaneletSequence>, return_internal_reference<>());

   class_<ConstLaneletWithStopLine>("ConstLaneletWithStopLine", "A lanelet with a stopline", no_init)
+      .def("__init__", make_constructor(&ConstLaneletWithStopLine::make, default_call_policies(),
+                                    (arg("lanelet"), arg("stopLine") = Optional<LineString3d>{})))
       .add_property("lanelet", &ConstLaneletWithStopLine::lanelet)
       .add_property("stopLine", &ConstLaneletWithStopLine::stopLine);

   class_<LaneletWithStopLine>("LaneletWithStopLine", "A lanelet with a stopline", no_init)
+      .def("__init__", make_constructor(&LaneletWithStopLine::make, default_call_policies(),
+                                (arg("lanelet"), arg("stopLine") = Optional<LineString3d>{})))
       .add_property("lanelet", &LaneletWithStopLine::lanelet)
       .add_property("stopLine", &LaneletWithStopLine::stopLine);

@poggenhans Would you like me to submit a PR with this?

mantkiew commented 2 years ago

There's still the weird problem with vector<->list conversion (I think):

>>> aws = AllWayStop(getId(), AttributeMap(), [lwsl])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
Boost.Python.ArgumentError: Python argument types in
    AllWayStop.__init__(AllWayStop, int, AttributeMap, list)
did not match C++ signature:
    __init__(boost::python::api::object, long id, lanelet::HybridMap<lanelet::Attribute, std::pair<char const*, lanelet::AttributeName const> const (&) [8], lanelet::AttributeNamesString::Map> attributes, std::vector<lanelet::LaneletWithStopLine, std::allocator<lanelet::LaneletWithStopLine> > lltsWithStop, std::vector<lanelet::LineStringOrPolygon3d, std::allocator<lanelet::LineStringOrPolygon3d> > signs=None)

All the required conversion pieces are present in the code, I don't know why it fails to match the signature.

mantkiew commented 2 years ago

The only way to use it is by providing a list for signs like this

>>> aws = AllWayStop(getId(), AttributeMap(), [lwsl], [])

despite it being declared as an optional list:

.def("__init__", make_constructor(&AllWayStop::make, default_call_policies(),
                                        (arg("id"), arg("attributes"), arg("lltsWithStop"),
                                         arg("signs") = Optional<LineStringsOrPolygons3d>{})))

I think we have to remove the Optional part like this:

.def("__init__", make_constructor(&AllWayStop::make, default_call_policies(),
                                        (arg("id"), arg("attributes"), arg("lltsWithStop"), arg("signs"))))

because the original C++ constructor is defined as

static Ptr make(Id id, const AttributeMap& attributes, const LaneletsWithStopLines& lltsWithStop,
                  const LineStringsOrPolygons3d& signs = {}) {
    return Ptr{new AllWayStop(id, attributes, lltsWithStop, signs)};
  }

In order to keep the Optional in Python API, we'd have to wrap signs in optional too:

const Optional<LineStringsOrPolygons3d&> signs = {}
poggenhans commented 2 years ago

I think you are lacking a conversion to the optional types such as Optional<LineStringsOrPolygons3d>. We have a registered a conversion from the optional types to python, and you can see that in the error output where signs=None. So your default argument for signs has correctly been converted to None. But in order to call the constructor, it must be converted back to the C++ type. And that conversion is missing.

If you also register that conversion by appending it here, it should work: https://github.com/fzi-forschungszentrum-informatik/Lanelet2/blob/21e911efcde20fc281e731b5abb0167e9f00e0c2/lanelet2_python/python_api/core.cpp#L538

poggenhans commented 2 years ago

Regarding your colcon error: Afaik we never tested with colcon, but it can be that lanelet2 installs its python code to a place that is different from the one colcon expects. You should probably check that the PYTHONPATH variable that local_setup.bash actually matches the location where lanelet2_python installed the python libraries.

mantkiew commented 2 years ago

Adding the converter has not helped:

ToOptionalConverter().fromPython<LineStringsOrPolygons3d>();

The parameter signs is a vector. Why use None as the default value and not simply use [] (an empty list)?

poggenhans commented 2 years ago

Ah, sorry I didn't look closely enough. You don't need the Optional<...> as default argument to construct an AllWayStop since that is not what the constructor expects.

You are probably assuming that the Optional is used to hint python that this value is optional. But actually it is a type used in C++ for variables that have no value (basically a replacement for the special value None in python).

Just change your initializer for AllWayStop like this:

.def("__init__", make_constructor(&AllWayStop::make, default_call_policies(),
                                        (arg("id"), arg("attributes"), arg("lltsWithStop"),
                                         arg("signs") = LineStringsOrPolygons3d{})))
poggenhans commented 2 years ago

Your changes on LaneletWithStopLane and friends look sensible to me, I would just suggest to use a simple lambda for make_constructor since the make funktion you added has no real value in the C++ world, only in python. We are doing a similar thing here: https://github.com/fzi-forschungszentrum-informatik/Lanelet2/blob/21e911efcde20fc281e731b5abb0167e9f00e0c2/lanelet2_python/python_api/core.cpp#L862-L865

So that the result looks more or less like this:

      .def("__init__", make_constructor(+[](Lanelet lanelet, Optional<LineString3d> stopLine){
          return std::make_shared(LaneletWithStopLine{std::move(lanelet), std::move(stopLine)};
      }, (arg("lanelet"), arg("stopLine") = Optional<LineString3d>{})))
mantkiew commented 2 years ago

@poggenhans All problems resolved in the #231 . Thanks for the help! The solution is neat and minimal. I've learned a lot!

github-actions[bot] commented 2 years ago

This issue is stale because it has been open for 90 days with no activity. Is this issue still work in progress? If yes, mark it as "WIP" or comment, otherwise it will be closed in 30 days.

github-actions[bot] commented 1 year ago

This issue was closed because it has been stalled for 30 days with no activity.

mantkiew commented 1 year ago

This issue has been fixed in PR #231