Add support for writing systems in Python #2081

azeey · 2023-08-17T23:22:21Z

🎉 New feature

Closes #790
Replaces #2045, which was automatically closed by Github when the branch it was targeting was deleted.

Summary

Creates a C++ system that is able to load a python module and pass along the System interface calls, such as Configure, PreUpdate, etc. The convention is pretty simple. The python module exposes a get_system function which returns an instance of a class that implements the desired System interface functions.

I'd appreciate feedback on the convention/general interface between PythonSystemLoader and python systems.

TODO:

Add tests

Test it

Run examples/worlds/python_system_loader.sdf. See the file for instructions on environmental variables that need to be set.

Checklist

Signed all commits for DCO
Added tests
Added example and/or tutorial
Updated documentation (as needed)
Updated migration guide (as needed)
Consider updating Python bindings (if the library has them)
codecheck passed (See contributing)
All tests passed (See test coverage)
While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

examples/scripts/python_api/systems/test_system.py

mjcarroll · 2023-08-17T23:38:40Z

src/systems/python_system_loader/PythonSystemLoader.cc

+    }
+    catch (const pybind11::error_already_set &_err)
+    {
+      gzerr << _err.what() << std::endl;


Will the error message contain the name of the method and python module here? It would certainly help with tracking down errors, otherwise they will all have the same name, correct?

It should, but I'll double check.

I threw an exception in the python file and this is what I got

[Err] [PythonSystemLoader.cc:174] NameError: name 'entity' is not defined At: ~/ws/harmonic/src/gz-sim/python/test/plugins/test_model_system.py(51): pre_update

I think that's sufficient.

mjcarroll · 2023-08-17T23:39:41Z

src/systems/python_system_loader/PythonSystemLoader.cc

+}
+
+//////////////////////////////////////////////////
+void PythonSystemLoader::PostUpdate(const UpdateInfo &_info,


Did we determine what happens when there are multiple python modules? Is that supported/unsupported?

Postupdate is the most interesting case because of parallel execution

So, we're currently creating an instance of pybind11::scoped_interpreter in ServerPrivate and it seems to work fine with multiple python modules, since there's only one Server instance. However, if we want to have multiple Servers in the same process, this will not work. So maybe we need a Singleton for this.

I think we can generally assume that multiple Servers per process isn't likely at the moment. We may want to add a note somewhere in the event that someone attempts it, though.

Some of the tests were failing because they initialized two Server instances. I fixed it by using pybind11::initialize_interpreter() instead of pybind11::scoped_interpreter so we can skip initialization of the interpreter is already initialized.

mjcarroll · 2023-08-21T17:37:24Z

Hmm, approval is more of approval-of-approach. I think we still need a few tests here (which you have already noted)

caguero

It works for me, this is very cool. I just left very minor comments.

I think the PythonSystemLoader is a good option. I was thinking if it would make sense to promote this upstream somehow but I didn't find a better approach than this one.

src/systems/python_system_loader/PythonSystemLoader.cc

src/systems/python_system_loader/PythonSystemLoader.hh

Signed-off-by: Addisu Z. Taddese <[email protected]>

This allows multiple PythonSystemLoaders to run albeit with a single python interpreter. Signed-off-by: Addisu Z. Taddese <[email protected]>

Signed-off-by: Addisu Z. Taddese <[email protected]>

azeey

There were a couple of GIL issues that showed up when I started using gz-transport inside the python system. The fix requires releasing the GIL when we run PostUpdate threads and acquire the GIL from the threads each PostUpdate runs in. This also requires an update to gz-transport so that the GIL is acquired before calling subscription callbacks since that also happens from a different thread than the main thread that contains the Python interpreter.

src/systems/python_system_loader/PythonSystemLoader.cc

azeey · 2023-08-24T19:38:43Z

src/systems/python_system_loader/PythonSystemLoader.cc

+}
+
+//////////////////////////////////////////////////
+void PythonSystemLoader::PostUpdate(const UpdateInfo &_info,


Some of the tests were failing because they initialized two Server instances. I fixed it by using pybind11::initialize_interpreter() instead of pybind11::scoped_interpreter so we can skip initialization of the interpreter is already initialized.

src/systems/python_system_loader/PythonSystemLoader.hh

Signed-off-by: Addisu Z. Taddese <[email protected]>

src/ServerPrivate.hh

ahcorde · 2023-08-25T08:41:54Z

src/systems/python_system_loader/PythonSystemLoader.cc

+}
+
+void PythonSystemLoader::Configure(
+    const Entity &_entity, const std::shared_ptr<const sdf::Element> &_sdf,


include ENtity, sdf::Element, EntityComponentManager, EventManager, SystemLoader

Some were already there, added the rest in f30128f

ahcorde · 2023-08-25T08:43:04Z

src/systems/python_system_loader/PythonSystemLoader.cc

+  CallPythonMethod(this->resetMethod, _info, &_ecm);
+}
+
+GZ_ADD_PLUGIN(PythonSystemLoader, System, ISystemConfigure, ISystemPreUpdate,


include system.hh

Do we need to if it's already in "PythonSystemLoader.hh"? Added anyway f30128f

test/integration/python_system_loader.cc

ahcorde · 2023-08-25T08:45:26Z

test/integration/python_system_loader.cc

+                 common::joinPaths(std::string(PROJECT_SOURCE_PATH), "python",
+                                   "test", "plugins"));
+
+  sim::ServerConfig serverConfig;


include: ServerConfig, gz/common/Filesystem.hh, components::Name, components::Model, sim::worldPose

Some of the headers were already there, added the rest in f30128f

test/integration/python_system_loader.cc

Signed-off-by: Addisu Z. Taddese <[email protected]>

tutorials/python_interfaces.md

Signed-off-by: Addisu Z. Taddese <[email protected]>

codecov · 2023-08-28T20:27:59Z

Codecov Report

Merging #2081 (5d11d53) into main (5846393) will decrease coverage by 0.03%.
Report is 2 commits behind head on main.
The diff coverage is 68.03%.

❗ Current head 5d11d53 differs from pull request most recent head dfe27ab. Consider uploading reports for the commit dfe27ab to get more accurate results

@@            Coverage Diff             @@
##             main    #2081      +/-   ##
==========================================
- Coverage   65.32%   65.30%   -0.03%     
==========================================
  Files         321      322       +1     
  Lines       30303    30410     +107     
==========================================
+ Hits        19795    19858      +63     
- Misses      10508    10552      +44

Files Changed	Coverage Δ
include/gz/sim/EventManager.hh	`75.75% <ø> (ø)`
...systems/python_system_loader/PythonSystemLoader.cc	`57.60% <57.60%> (ø)`
src/Link.cc	`98.31% <100.00%> (+0.05%)`	⬆️
src/Server.cc	`87.13% <100.00%> (+0.15%)`	⬆️
src/SimulationRunner.cc	`91.13% <100.00%> (-0.85%)`	⬇️

... and 2 files with indirect coverage changes

azeey requested a review from mjcarroll as a code owner August 17, 2023 23:22

github-actions bot added the 🎵 harmonic Gazebo Harmonic label Aug 17, 2023

mjcarroll reviewed Aug 17, 2023

View reviewed changes

mjcarroll approved these changes Aug 21, 2023

View reviewed changes

azeey added the beta Targeting beta release of upcoming collection label Aug 21, 2023

caguero approved these changes Aug 23, 2023

View reviewed changes

src/systems/python_system_loader/PythonSystemLoader.cc Outdated Show resolved Hide resolved

src/systems/python_system_loader/PythonSystemLoader.cc Show resolved Hide resolved

src/systems/python_system_loader/PythonSystemLoader.hh Show resolved Hide resolved

azeey requested a review from mabelzhang as a code owner August 24, 2023 19:33

azeey added 10 commits August 24, 2023 14:33

Skeleton

5acd4f7

Signed-off-by: Addisu Z. Taddese <[email protected]>

Put pybind11::scoped_interpreter in SystemPrivate

1ecec9f

This allows multiple PythonSystemLoaders to run albeit with a single python interpreter. Signed-off-by: Addisu Z. Taddese <[email protected]>

Implement other system interfaces

4191763

Signed-off-by: Addisu Z. Taddese <[email protected]>

Add example python script

cb48ec5

Signed-off-by: Addisu Z. Taddese <[email protected]>

WIP: Configure

b91b9fb

Signed-off-by: Addisu Z. Taddese <[email protected]>

Compute worldPose when the WorldPose component is not available

9bf542e

Signed-off-by: Addisu Z. Taddese <[email protected]>

Fix passing EventManager, address todos

4d3956b

Signed-off-by: Addisu Z. Taddese <[email protected]>

Add test, fix various segfaults, documentation

2b7469e

Signed-off-by: Addisu Z. Taddese <[email protected]>

Add to python interfaces tutorial

c3417d5

Signed-off-by: Addisu Z. Taddese <[email protected]>

Merge remote-tracking branch 'origin/main' into python_systems

e0404df

azeey force-pushed the python_systems branch from da60ded to e0404df Compare August 24, 2023 19:35

azeey commented Aug 24, 2023

View reviewed changes

Remove commented code, pre-commit

3d3cddc

Signed-off-by: Addisu Z. Taddese <[email protected]>

azeey mentioned this pull request Aug 24, 2023

Acquire the GIL when invoking callbacks gazebosim/gz-transport#431

Merged

8 tasks

azeey added 2 commits August 24, 2023 18:26

Add dependency on python3-transport13, fix tests

e5228d3

Signed-off-by: Addisu Z. Taddese <[email protected]>

Use correct package name

5210ffa

Signed-off-by: Addisu Z. Taddese <[email protected]>

azeey mentioned this pull request Aug 25, 2023

Fix python install directory gazebosim/gz-transport#433

Merged

8 tasks

ahcorde requested changes Aug 25, 2023

View reviewed changes

Fix namespace issue for windows

5c1c57c

Signed-off-by: Addisu Z. Taddese <[email protected]>

azeey mentioned this pull request Aug 25, 2023

gz-transport13: install python bindings osrf/homebrew-simulation#2342

Merged

azeey added 2 commits August 25, 2023 12:17

Merge remote-tracking branch 'origin/main' into python_systems

5c255cc

Address review feedback

f30128f

Signed-off-by: Addisu Z. Taddese <[email protected]>

azeey self-assigned this Aug 25, 2023

ahcorde approved these changes Aug 25, 2023

View reviewed changes

azeey added 2 commits August 25, 2023 17:28

Remove import

1932824

Signed-off-by: Addisu Z. Taddese <[email protected]>

Merge remote-tracking branch 'origin/main' into python_systems

cef644a

azeey mentioned this pull request Aug 25, 2023

gz-msgs10: install python bindings osrf/homebrew-simulation#2345

Merged

ahcorde reviewed Aug 26, 2023

View reviewed changes

tutorials/python_interfaces.md Outdated Show resolved Hide resolved

tutorials/python_interfaces.md Outdated Show resolved Hide resolved

azeey added 4 commits August 28, 2023 10:41

Review feedback, windows fix?

24c3d52

Signed-off-by: Addisu Z. Taddese <[email protected]>

Remove whitespace

0530198

Signed-off-by: Addisu Z. Taddese <[email protected]>

Add python3-gz-msgs10

72f85dc

Signed-off-by: Addisu Z. Taddese <[email protected]>

Disable test on windows

b46c6a7

Signed-off-by: Addisu Z. Taddese <[email protected]>

Merge remote-tracking branch 'origin/main' into python_systems

dfe27ab

azeey merged commit a84d66a into gazebosim:main Aug 31, 2023
3 checks passed

azeey deleted the python_systems branch August 31, 2023 03:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for writing systems in Python #2081

Add support for writing systems in Python #2081

azeey commented Aug 17, 2023 •

edited

Loading

mjcarroll Aug 17, 2023

azeey Aug 18, 2023

azeey Aug 24, 2023

mjcarroll Aug 17, 2023

azeey Aug 18, 2023

mjcarroll Aug 21, 2023

azeey Aug 24, 2023

mjcarroll commented Aug 21, 2023

caguero left a comment

azeey left a comment

azeey Aug 24, 2023

ahcorde Aug 25, 2023

azeey Aug 25, 2023

ahcorde Aug 25, 2023

azeey Aug 25, 2023

ahcorde Aug 25, 2023

azeey Aug 25, 2023

codecov bot commented Aug 28, 2023 •

edited

Loading

Add support for writing systems in Python #2081

Add support for writing systems in Python #2081

Conversation

azeey commented Aug 17, 2023 • edited Loading

🎉 New feature

Summary

Test it

Checklist

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjcarroll commented Aug 21, 2023

caguero left a comment

Choose a reason for hiding this comment

azeey 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

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

codecov bot commented Aug 28, 2023 • edited Loading

Codecov Report

azeey commented Aug 17, 2023 •

edited

Loading

codecov bot commented Aug 28, 2023 •

edited

Loading