-
Notifications
You must be signed in to change notification settings - Fork 44
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
map Arrays to numpy.ndarray() and Sequences to array.array() #35
Conversation
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
18df5bf
to
6475261
Compare
A great example of the performance improvement this change provides is to run the following two commands:
Without this patch the Python CLI takes about 600-700ms to convert the incoming ROS message into a Python object. As a consequence only every 15th-20th frames is being shown and the command line tool uses 100% of one CPU core. With the patch the conversion time is reduced by two orders of magnitude to about 7-8ms and the command line tool can print information for every image message without utilizing a full CPU core. Also the memory usage is reduced for the Python tool since an |
Great, i think quite a few people would be helped by this (it would make the python API useable for us at least). See also ros2/ros2#509 and various threads on discourse. |
@@ -18,6 +19,52 @@ from rosidl_parser.definition import Sequence | |||
from rosidl_parser.definition import String | |||
from rosidl_parser.definition import WString | |||
}@ | |||
@#<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you consider using a different delimiter here? These look a lot like git conflict markers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This kind of delimiter already exists in numerous em templates throughout the various generators. So for this PR I would rather stick with it. But a set of follow up PRs could replace them with whatever else is preferred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry, I guess I hadn't noticed it before. It really jumped out at me this time. I'm good with it staying, then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but for tiny comments and pending green CI
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
…ray() Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
6475261
to
739c50f
Compare
@hidmic CI builds have already been run and are referenced in the comment above. |
Oh, I see now that test failures are unrelated. LGTM then. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, sorry for long review, was testing it out locally 👍
CI failures are unrelated to this change.
Merging without squashing to keep the commits separate for easier history reading. Since these changes won't be backported the separated commits shouldn't be a problem. |
Addressed compiler warnings about unused variables in release mode in a follow up commit: 4f11d87. |
Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> update tests Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> simplify logic by printing member type directly Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> move dict construction to rosidl_runtime_py Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> add utility to import complex message and add support for nested array in set_message Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> update tests and remove coverage for dict to avoid circular dependency Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> more descriptive variable name Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> refactor import logic according to ros2#35 Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> move imports to top of file Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> string maximum size doesn't need quoting or str conversion Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> update docblock Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> slot_types_dict_from_message -> get_message_slot_types Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> check Abstrct type instead of 'list' Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> use NestedType Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> add conditional is namespaces is an empty list Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> one liner Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> extend test suite to cover NesteTypes of NamespacedTypes Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> single message fixture to use Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> use NestedType Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> add coverage for static array of nested messages Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> restore old API Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> SLOT_TYPES improvement from dirk-thomas Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> resolve conflicts Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
* store types as constant and return ordered dict Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> update tests Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> simplify logic by printing member type directly Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> move dict construction to rosidl_runtime_py Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> add utility to import complex message and add support for nested array in set_message Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> update tests and remove coverage for dict to avoid circular dependency Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> more descriptive variable name Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> refactor import logic according to #35 Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> move imports to top of file Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> string maximum size doesn't need quoting or str conversion Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> update docblock Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> slot_types_dict_from_message -> get_message_slot_types Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> check Abstrct type instead of 'list' Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> use NestedType Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> add conditional is namespaces is an empty list Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> one liner Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> extend test suite to cover NesteTypes of NamespacedTypes Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> single message fixture to use Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> use NestedType Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> add coverage for static array of nested messages Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> restore old API Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> SLOT_TYPES improvement from dirk-thomas Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> resolve conflicts Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> * add back I100 ignore Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> * use new rosidl_parser.definition types and new test interfaces Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> * restore original test, add test for SLOT_TYPES Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
* store types as constant and return ordered dict Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> update tests Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> simplify logic by printing member type directly Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> move dict construction to rosidl_runtime_py Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> add utility to import complex message and add support for nested array in set_message Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> update tests and remove coverage for dict to avoid circular dependency Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> more descriptive variable name Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> refactor import logic according to ros2/rosidl_python#35 Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> move imports to top of file Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> string maximum size doesn't need quoting or str conversion Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> update docblock Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> slot_types_dict_from_message -> get_message_slot_types Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> check Abstrct type instead of 'list' Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> use NestedType Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> add conditional is namespaces is an empty list Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> one liner Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> extend test suite to cover NesteTypes of NamespacedTypes Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> single message fixture to use Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> use NestedType Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> add coverage for static array of nested messages Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> restore old API Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> SLOT_TYPES improvement from dirk-thomas Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> resolve conflicts Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> * add back I100 ignore Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> * use new rosidl_parser.definition types and new test interfaces Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> * restore original test, add test for SLOT_TYPES Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
Implements the latest version of the design article describing the type mapping for Python (including the latest change ros2/design#217).