Composition API #599

crystaldust · 2020-07-14T16:04:15Z

The composition API for rclpy.
Add a new rclpy_components sub project just like what rclcpp_components does. The project provides the component manager(essentially a wrapper of Node and executor), which provides the required services described in the composition design doc for the ros2cli to load/unload/list components

Related PR: ros2/ros2cli#554

crystaldust · 2020-07-14T16:21:02Z

Missing features/TODO list records:

The supported_types service(discussed here), need a discussion on where to put this service, put it in the composition_interfaces package, or just the rclpy package.
Need some essential test cases

jacobperron

Looks like a good start! I've left some preliminary feedback. I'll take a look at the connected ros2cli PR and examples next.

jacobperron · 2020-08-28T01:38:44Z

rclpy_components/rclpy_components/component_manager.py

+        self.components = {}  # key: unique_id, value: full node name and component instance
+        self.unique_id_index = 0
+
+        self.executor.spin()


Given that we're also calling spin in the containers, which is a blocking call, I'm surprised that the advertised services work (I haven't tried it myself yet). I would think that we don't need to call spin here.

jacobperron · 2020-08-28T01:42:51Z

rclpy_components/package.xml

+  <license>TODO: License declaration</license>
+
+  <depend>rclpy</depend>
+  <depend>std_msgs</depend>


looks like we're not depending on std_msgs, but instead we need composition_interfaces.

jacobperron · 2020-08-28T01:45:20Z

rclpy_components/rclpy_components/component_container.py

+
+def main():
+    try:
+        _main()


Rather than wrapping all of the logic in _main() in a try-catch, I think we can narrow the scope to the spin() call and remove the redundant "main" function. For example, see how it looks in this demo.

rclpy_components/rclpy_components/component_container.py

jacobperron · 2020-08-28T01:50:52Z

rclpy_components/rclpy_components/component_manager.py

+    from importlib_metadata import entry_points
+
+RCLPY_COMPONENTS = 'rclpy_components'
+logger = get_logger('ComponentManager')


Instead of creating a global logger, why not just use the nodes logger? For example.

jacobperron · 2020-08-28T02:22:44Z

rclpy_components/setup.py

+    zip_safe=True,
+    maintainer='root',
+    maintainer_email='[email protected]',
+    description='TODO: Package description',


Please populate the description.

jacobperron · 2020-08-28T02:23:42Z

rclpy_components/package.xml

+  <version>0.0.0</version>
+  <description>TODO: Package description</description>
+  <maintainer email="[email protected]">root</maintainer>
+  <license>TODO: License declaration</license>


Suggested change

<license>TODO: License declaration</license>

<license>Apache License 2.0</license>

jacobperron · 2020-08-28T02:24:09Z

rclpy_components/package.xml

+  <name>rclpy_components</name>
+  <version>0.0.0</version>
+  <description>TODO: Package description</description>
+  <maintainer email="[email protected]">root</maintainer>


Suggested change

<maintainer email="[email protected]">root</maintainer>

<maintainer email="[email protected]">Jacob Perron</maintainer>

jacobperron · 2020-08-28T02:24:17Z

rclpy_components/package.xml

+<package format="3">
+  <name>rclpy_components</name>
+  <version>0.0.0</version>
+  <description>TODO: Package description</description>


Please populate this description.

jacobperron · 2020-08-28T02:25:40Z

rclpy_components/package.xml

+<?xml-model href="http://download.ros.org/schema/package_format3.xsd" schematypens="http://www.w3.org/2001/XMLSchema"?>
+<package format="3">
+  <name>rclpy_components</name>
+  <version>0.0.0</version>


For maintenance reasons, this should match the version of other packages in this repository.

Suggested change

<version>0.0.0</version>

<version>1.1.0</version>

crystaldust · 2020-08-30T10:24:45Z

Thanks for the review! Just get myself back to work after a LONG business travel. I'll look into the code and make some fixes/improvments

crystaldust · 2020-10-19T10:01:50Z

@jacobperron Hi, I've made some changes following the reviews, make a list here for better track:

Remove the unnecessary spin in ComponentManager's __init__ method
Make up the meta data in package.xml and setup.py, like author info, desc, version, license etc
Surround spin with try-catch, instead of creating a shadow _main function
Call component manager's destroy node when exiting program
Use the node's logger instead of creating a new global one
Use f string for logging
Return error message in response instead of logging it
Use request.id(type integer) as ComponentManager's components member as key

Also with the basic unit/integrated test code added. Let's first make this PR merged and then move to next PRs related to ros2cli and examples

sloretz

Great start! I like the use of entry points for finding components. Any idea why the multithreaded test is failing?

sloretz · 2020-12-23T00:11:44Z

rclpy_components/package.xml

+  <name>rclpy_components</name>
+  <version>1.1.0</version>
+  <description>The dynamic node management package</description>
+  <maintainer email="[email protected]">Zhen Ju</maintainer>


Mind adding yourself as an <author> tag, and adding @claireyywang @ivanpauno and myself as maintainers?

rclpy/rclpy/package.xml

Lines 8 to 10 in 22816b4

<maintainer email="[email protected]">Claire Wang</maintainer>

<maintainer email="[email protected]">Ivan Paunovic</maintainer>

<maintainer email="[email protected]">Shane Loretz</maintainer>

sloretz · 2020-12-23T00:14:08Z

rclpy_components/rclpy_components/component_container_mt.py

+        executor.spin()
+    except KeyboardInterrupt:
+        print('KeyboardInterrupt received, exit')
+        pass


nit - no need for pass here :)

rclpy_components/rclpy_components/component_manager.py

sloretz · 2020-12-23T01:06:16Z

rclpy_components/test/test_component_manager_ut.py

+        assert mock_res.error_message
+        assert (not mock_res.success)
+
+        # Unload the first node


This might become the only test in rclpy that depends on the state of the previous tests. I've never seen this style before. IIUC it works because the test runner sorts tests alphabetically.

No need to change anything here - I'm fine with this as is.

sloretz · 2020-12-23T01:12:35Z

rclpy_components/test/test_component_manager.py

+        unload_res: UnloadNode.Response = self.__class__.unload_node(1000)
+        assert unload_res.error_message != ""
+        assert unload_res.success is False
+        list_res: ListNodes.Response = self.__class__.list_nodes()


nit - don't need to specify __class__ when calling class methods; self.list_nodes() works

sloretz · 2020-12-23T01:22:51Z

rclpy_components/test/test_component_manager.py

+
+    def list_nodes_test(self):
+        container_name = self.__class__.container_name
+        print(f'{container_name}: list_nodes tested within test_load_node and test_unload_node')


nit, I would delete this function since it doesn't test anything.

sloretz · 2020-12-23T01:25:01Z

rclpy_components/test/test_component_manager.py

+        self.list_nodes_test()
+
+
+class TestComponentManagerMT(TestComponentManager):


I see a test failure here when I merged this branch into master locally. Any ideay why?

============================= test session starts ============================== platform linux -- Python 3.8.2, pytest-4.6.9, py-1.8.1, pluggy-0.13.0 cachedir: /home/sloretz/bigssd/ros2/build/rclpy_components/.pytest_cache rootdir: /home/sloretz/bigssd/ros2/src/ros2/rclpy, inifile: pytest.ini plugins: ament-copyright-0.10.0, ament-pep257-0.10.0, ament-flake8-0.10.0, ament-lint-0.10.0, colcon-core-0.6.0, cov-2.8.1, mock-1.10.4 collecting ... collected 13 items test/test_component_manager.py .F [ 15%] test/test_component_manager_ut.py ........ [ 76%] test/test_copyright.py . [ 84%] test/test_flake8.py . [ 92%] test/test_pep257.py . [100%] =================================== FAILURES =================================== _________________ TestComponentManagerMT.test_composition_api __________________ test/test_component_manager.py:187: in test_composition_api self.load_node_test() test/test_component_manager.py:130: in load_node_test load_res = self.__class__.load_node(TEST_COMPOSITION, TEST_COMPOSITION_FOO) test/test_component_manager.py:93: in load_node raise RuntimeError(f'No load service found in /{cls.container_name}') E RuntimeError: No load service found in /TestComponentManagerMT

I remember to add some components specifically for the test, so the test won't have to rely on any pre-existing components. It seems the testing components are not installed, I'll take a look into this.

buschbapti · 2021-07-21T14:37:24Z

Any update on this? We have the need for launching python components and having that integrated would be very neat.

crystaldust · 2021-07-27T01:47:04Z

@buschbapti Thanks for the attention. I'll make a recheck on this and see if it can be merged ASAP.

buschbapti · 2021-07-27T07:59:07Z

I copied your rclpy_components to make this work in our application. Everything runs smoothly but I needed to change the get_fully_qualified_node_name line. Either this function has been deleted in the latest version (galactic) or is not integrated yet.

alpylmz · 2022-04-29T15:34:30Z

This seems to be an important feature and it is almost done! Any update on this?

SteveMacenski · 2022-05-24T00:01:43Z

+1

ihasdapie · 2022-06-02T18:52:37Z

@crystaldust Are you still working on this? If not I'd be take over and help push this PR through.

MathieuxHugo · 2023-07-19T14:34:49Z

Hello any updates on this? Will it be integrated?

domire8 · 2024-02-16T11:02:13Z

@mjcarroll @sloretz @crystaldust just trying to bump this again, we've been using it for 2 years and works as expected

The project provide component container(special Node) and component_manager Signed-off-by: Zhen Ju <[email protected]>

Signed-off-by: Zhen Ju <[email protected]>

This is to get rid of the impact of global remapping rules on node name, which will create nodes with duplicated names. Remove *args in ComponentManager's constructor. Signed-off-by: Zhen Ju <[email protected]>

* Update the meta info in package.xml and setup.py * Try-except executor.spin instead of main in executables * Use f string to format infos * Pass error message to response objectc instead of logging it * Use node's logger instead of init a global one * Call node.get_qualified_name() to assign response obj's full_node_name * Other super tiny changes Signed-off-by: Zhen Ju <[email protected]>

Signed-off-by: Zhen Ju <[email protected]>

Signed-off-by: Shane Loretz <[email protected]>

sloretz · 2024-03-31T16:11:17Z

Since it had been a while I rebased this onto rolling and made some changes

Resolved all flake8 linter issues
Updated setup.py and package.xml to list Zhen Ju as the author with @adityapande-1995 and I as the maintainers
Rewrote tests to use pytest and made each test independent. Previously the tests were dependent and relied on the tests being executed in alphabetical order, but that must have changed (or one of my installed pytest plugins changed it) because the tests weren't passing on my machine.
Fixed code that assumed .keys() and .values() from a dict would always be the same order to use .items()
Made the component_manager use the LoadNode.package_name parameter. The entrypoint specification is still package_name::PluginName, but now the component manager looks for an entry point named that way instead of using only plugin_name.
Removed tests_components package marker file and put the test node into the rclpy_components.test subpackage
Renamed the test node and file so that pytest didn't think it was a test
Added a check to make sure the fully qualified name of the loaded node is unique in the component manager to satisfy requirement in this design doc
Protected component_manager from crashing in case the entrypoint or class itself fail to load or be instantiated
Made node name and namespace get passed in via remap arguments instead of direct arguments to match rclcpp. The remap rules get evaluated with a different priority from the passed in node name so this makes that behavior consistent between the two
Added more tests (failure handling cases + making sure name and namespace remapping worked)
Use non-deprecated entry_points API
Removed importlib_metata as we don't need to support Python less than 3.8 on rolling
Use non-deprecated setup.cfg types.

And with that, this PR LGTM, but I've made so many changes I think it needs another reviewer.

sloretz

LGTM with green CI and a second reviewer!

sloretz · 2024-03-31T16:23:28Z

CI (repos file build: --packages-up-to rclpy_components test: --packages-select rclpy_components)

Linux
Linux-aarch64
Windows

EDIT: the tests on Windows got stuck. I don't have access to a Windows machine at the moment to debug this :(

wjwwood

Code looks reasonable to me, and since it's a new package then it comes with little enough risk in my opinion. I'd say if you @sloretz and @adityapande-1995 are ok with it (since you're the new maintainers of the new package), then lgtm.

Are there any examples?

alberthli · 2024-07-16T22:09:55Z

Any chance this could get merged soon? I'd also be interested in a backport at least through Humble.

hello-amal · 2024-07-17T17:38:39Z

Ditto to the value of a humble backport soon. Thanks all for your hard work!

sloretz · 2024-10-11T17:53:30Z

@Mergifyio Update

mergify · 2024-10-11T17:53:36Z

Update

❌ Sorry but I didn't understand the command. Please consult the commands documentation 📚.

sloretz · 2024-10-11T17:54:02Z

@Mergifyio update

mergify · 2024-10-11T17:54:09Z

update

✅ Branch has been successfully updated

dirk-thomas requested a review from mjcarroll July 30, 2020 16:09

clalancette assigned jacobperron Aug 20, 2020

jacobperron reviewed Aug 28, 2020

View reviewed changes

crystaldust force-pushed the composition-api-pr branch 3 times, most recently from eb9f93a to a05946f Compare September 13, 2020 07:04

crystaldust mentioned this pull request Sep 14, 2020

rosbag fails at importing import_c_library ros2/rosbag2#522

Closed

crystaldust force-pushed the composition-api-pr branch 3 times, most recently from 171d13b to cbe3290 Compare October 19, 2020 07:50

sloretz reviewed Dec 23, 2020

View reviewed changes

audrow mentioned this pull request Feb 26, 2021

Composition API ros2/ros2cli#554

Open

jacobperron removed their assignment Feb 26, 2021

clalancette assigned sloretz Mar 18, 2021

audrow changed the base branch from master to rolling June 28, 2022 14:21

crystaldust added 4 commits March 30, 2024 13:28

Add the rclpy_component sub project

077707d

The project provide component container(special Node) and component_manager Signed-off-by: Zhen Ju <[email protected]>

Refer to ComponentManager from project rclpy_components

1f58c71

Signed-off-by: Zhen Ju <[email protected]>

Set 'use_global_arguments' to False when creating node instance.

1bea84a

This is to get rid of the impact of global remapping rules on node name, which will create nodes with duplicated names. Remove *args in ComponentManager's constructor. Signed-off-by: Zhen Ju <[email protected]>

Add test cases for composition API.

875d84c

Signed-off-by: Zhen Ju <[email protected]>

sloretz force-pushed the composition-api-pr branch from cbe3290 to 875d84c Compare March 30, 2024 22:10

crystaldust requested a review from adityapande-1995 as a code owner March 30, 2024 22:10

sloretz added 14 commits March 31, 2024 00:50

Move test node into test subpackage

874752b

Signed-off-by: Shane Loretz <[email protected]>

Remove unused test package marker file

d23d9be

Signed-off-by: Shane Loretz <[email protected]>

Copyright not needed for empty file I think

734d35e

Signed-off-by: Shane Loretz <[email protected]>

use non-deprecated underscore script_dir and install_scripts

a839f77

Signed-off-by: Shane Loretz <[email protected]>

Add __init__.py for test subpackage

57092ea

Signed-off-by: Shane Loretz <[email protected]>

Consolidate unit tests into one

96a2321

Signed-off-by: Shane Loretz <[email protected]>

flake8 fixes

85e6f60

Signed-off-by: Shane Loretz <[email protected]>

Rename TestFoo to FooNode to avoid pytest trying to load tests from it

c2921d7

Signed-off-by: Shane Loretz <[email protected]>

Rewrite tests to use pytest and make them independent of each other

ed26ff1

Signed-off-by: Shane Loretz <[email protected]>

Misc flake8 fixes and bug fixes

c4cf182

Signed-off-by: Shane Loretz <[email protected]>

test_foo.py -> foo_node.py

1e94788

Signed-off-by: Shane Loretz <[email protected]>

Make node name and namespace be passed through remapping args

a9dd0e7

Signed-off-by: Shane Loretz <[email protected]>

Update author an maintainer entries

8b1066f

Signed-off-by: Shane Loretz <[email protected]>

Update description for package

dcb7da6

Signed-off-by: Shane Loretz <[email protected]>

sloretz approved these changes Mar 31, 2024

View reviewed changes

wjwwood approved these changes Apr 2, 2024

View reviewed changes

Merge branch 'rolling' into composition-api-pr

dc0c975

	<license>TODO: License declaration</license>
	<license>Apache License 2.0</license>

	<maintainer email="[email protected]">root</maintainer>
	<maintainer email="[email protected]">Jacob Perron</maintainer>

	<maintainer email="[email protected]">Claire Wang</maintainer>
	<maintainer email="[email protected]">Ivan Paunovic</maintainer>
	<maintainer email="[email protected]">Shane Loretz</maintainer>

		self.list_nodes_test()


		class TestComponentManagerMT(TestComponentManager):

Composition API #599

Are you sure you want to change the base?

Composition API #599

Conversation

crystaldust commented Jul 14, 2020 • edited Loading

crystaldust commented Jul 14, 2020 • edited Loading

jacobperron 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crystaldust commented Aug 30, 2020

crystaldust commented Oct 19, 2020 • edited Loading

sloretz 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

buschbapti commented Jul 21, 2021

crystaldust commented Jul 27, 2021

buschbapti commented Jul 27, 2021

alpylmz commented Apr 29, 2022

SteveMacenski commented May 24, 2022

ihasdapie commented Jun 2, 2022

MathieuxHugo commented Jul 19, 2023

domire8 commented Feb 16, 2024 • edited Loading

sloretz commented Mar 31, 2024

sloretz left a comment

Choose a reason for hiding this comment

sloretz commented Mar 31, 2024 • edited Loading

wjwwood left a comment

Choose a reason for hiding this comment

alberthli commented Jul 16, 2024

hello-amal commented Jul 17, 2024

sloretz commented Oct 11, 2024

mergify bot commented Oct 11, 2024

❌ Sorry but I didn't understand the command. Please consult the commands documentation 📚.

sloretz commented Oct 11, 2024

mergify bot commented Oct 11, 2024

✅ Branch has been successfully updated

crystaldust commented Jul 14, 2020 •

edited

Loading

crystaldust commented Jul 14, 2020 •

edited

Loading

crystaldust commented Oct 19, 2020 •

edited

Loading

domire8 commented Feb 16, 2024 •

edited

Loading

sloretz commented Mar 31, 2024 •

edited

Loading