store types as tuple of abstract types by mikaelarguedas · Pull Request #33 · ros2/rosidl_python

mikaelarguedas · 2019-03-13T23:01:59Z

Replaces #30 now that #24 has merged

Blocked by #34

mikaelarguedas · 2019-03-28T23:21:37Z

@dirk-thomas this has been rebased and modified to match changes from #34 and #35.

In order to test that it fixes ros2/ros2cli#59, it needs to be used along ros2/ros2cli#209 (commit ros2/ros2cli@571d1eb)

dirk-thomas

A few other packages are currently using get_fields_and_field_types: rqt_py_common, rqt_plot, rqt_publisher, rqt_service_caller. Do you think it is possible to update those to use the new API too?

dirk-thomas · 2019-03-29T22:29:06Z

+'@(str(type_.maximum_size))')@
 @[ elif isinstance(type_, NamespacedType)]@
-@('/'.join([type_.namespaces[0], type_.name]))@
+['@("', '".join(type_.namespaces))'], '@(type_.name)')@


This is wrong if namespaces is empty.

In which scenario would someone use a NamespacedType without namespace?
Is the expected syntax the following ?
NamespacedType([], 'myname')
instead of
NamespacedType([''], 'myname')
I can fix it in here, wouldn't the other places in this file using this logic need to be updated as well ?
e.g.
rosidl_python/rosidl_generator_py/resource/_msg.py.em
Line 247 in 4fe2166
from@('.'.join(type_.namespaces)) import@(type_.name)

In which scenario would someone use a NamespacedType without namespace?
There isn't a specific case where it is used atm but the code could be defensive and not rely on that assumption.
NamespacedType([], 'myname')
Correct.
wouldn't the other places in this file using this logic need to be updated as well ?
Probably, doesn't have to happen in this PR though. It's more a future proofing thing.

Sounds good, added coverage for the empty namespaces in b95d02d

mikaelarguedas · 2019-04-01T14:10:25Z

A few other packages are currently using get_fields_and_field_types: rqt_py_common, rqt_plot, rqt_publisher, rqt_service_caller. Do you think it is possible to update those to use the new API too?

Yes it is possible, does this mean adding an explicit dependency on rosidl_runtime_py in all these packages ?

dirk-thomas · 2019-04-01T15:16:14Z

does this mean adding an explicit dependency on rosidl_runtime_py in all these packages ?

Only if they would use any symbols directly from that package. Otherwise no, each message package will already depend on rosidl_runtime_py.

mikaelarguedas · 2019-04-01T16:20:25Z

Only if they would use any symbols directly from that package. Otherwise no, each message package will already depend on rosidl_runtime_py.

Ok so either these packages use what is provided in the message itself (__slots__ and SLOT_TYPES) or they depend explicitely on rosidl_runtime_py to be able to use the dictionary directly.

Do you have a preference for the rqt packages?

dirk-thomas · 2019-04-01T16:24:05Z

Do you have a preference for the rqt packages?

I don't have a preference. Either way is fine with me.

mikaelarguedas · 2019-04-01T16:39:33Z

Do you have a preference for the rqt packages?

I don't have a preference. Either way is fine with me.

Sounds good.
I don't know when I'll be able to allocate time for these, in case anyone else is interested in updating them in the meantime

dirk-thomas · 2019-04-01T16:51:34Z

+ field_elem_type = import_message_from_namespaced_type(rosidl_type)
+ for n in range(len(value)):
+ submsg = field_elem_type()
+ set_message_fields(submsg, value[n])


value[n] looks weird here? Can you please extend the existing test of set_message_fields to cover this case of a nested namespaced type to make sure this works as expected.

added test coverage in ad65970

dirk-thomas

I triggered a set of builds to check if the current state passes:

Linux
Linux-aarch64
macOS
Windows

dirk-thomas · 2019-04-01T17:57:20Z

@@ -33,4 +38,13 @@ def set_message_fields(msg: Any, values: Dict[str, str]) -> None:
 except TypeError:
 value = field_type()


I am surprised this works in cases where the field type is numpy.ndarray or array.array. Can you please also add a test with a static array (which would cover the numpy case).

Yeah it surprised me as well that the code comparing to list didn't need to be updated after the numpy array PR..
Static array test case added in 712d4da

mikaelarguedas · 2019-04-03T19:53:43Z

@dirk-thomas looking briefly at the impacted rqt packages:

In multiple instances it looks like the code is meant to be interacting with the user friendly string representing a message and not a Python type per se.
It looks like there is ongoing discussion on what that user friendly str identifying a type should be: ros2/design#220

In some cases it's unclear if it is a design decision to rely on the msg type to be reflected in that dict.
e.g. in rqt_py_common, this API is used to reconstruct a msg file from a class. It looks to diverge significantly from the ROS 1 approach of providing the msg file on disk, and may require quite some custom code to be convert back from abstract type to msg format.
Is it a design decision to reconstruct msg files programmatically in the ROS 2 version?

Overall the amount of code + design in rqt to grasp and maybe rewrite will be significant, and I don't think I will have time to adapt it.

Is there a path forward for this PR without these rqt changes? I can reopen a PR not using the abstract types but the msg ones if this allows a fix for ros2/ros2cli#59 to be merged

dirk-thomas · 2019-04-19T22:59:22Z

I don't think this ticket has to consider what might come out of that design ticket. Until that is actually merged this PR can focus on the current state of master.

Is it a design decision to reconstruct msg files programmatically in the ROS 2 version?

I am not entirely sure what you mean but I would answer with: no.

Is there a path forward for this PR without these rqt changes?

As long as the patch removes currently available API which is used by rqt it can only land if it also updates the code using the API.

That being said the alternatives are:

keep the old API in parallel to the newly introduced one
don't change the API at all and work with what is there

dirk-thomas · 2019-05-01T15:09:55Z

@mikaelarguedas Friendly ping.

mikaelarguedas · 2019-05-02T18:37:25Z

So I looked at this again. as updating all the rqt packages is not something I will be able to do, I moved to a minimal patch not modifying message generation at all but just the runtime functions (a redo of https://github.com/ros2/ros2cli/pull/197/files).

I submitted the patch at #52.

keep the old API in parallel to the newly introduced one

that was a valuable alternative, I considered this, the current blocker for such an approach is that the rosidl_runtime_py package is not a dependency of the message packages by default, and that it cannot be for now because rosidl_runtime_py depends on message_packages such as test_msgs for testing introducing a circular dependency package dependency.
As there seem to be a lot of changes on the testing of packages these days, I discarded making the required changes for now and submitted the least intrusive change.

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>

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>

mikaelarguedas · 2019-05-03T00:01:13Z

As per discussion here and in #52.

This PR now does the following:

Adds SLOT_TYPES to all messages
Does not modify existing _fields_and_field_types or get_fields_and_field_types API
Adds import_message_from_namespaced_type and get_message_slot_types to rosidl_runtime_py
Update set_message to support Sequence and Arrays of ROS messages
Add test to rosidl_generator_py and rosidl_runtime_py to cover new feature

dirk-thomas · 2019-05-03T03:26:02Z

Thanks for the patch.

* 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>

mikaelarguedas added the in progress Actively being worked on (Kanban column) label Mar 13, 2019

mikaelarguedas mentioned this pull request Mar 13, 2019
store types as tuple and return ordered dict #30
Closed

mikaelarguedas added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Mar 13, 2019

mikaelarguedas mentioned this pull request Mar 14, 2019
add support for nested arrays ros2/ros2cli#207
Closed

dirk-thomas reviewed Mar 15, 2019
View reviewed changes

Comment threadrosidl_generator_py/resource/_msg.py.em Outdated
Comment threadrosidl_generator_py/resource/_msg.py.em Outdated

dirk-thomas added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Mar 15, 2019

dirk-thomas mentioned this pull request Mar 19, 2019
Porting rosapi package. RobotWebTools/rosbridge_suite#388
Merged

dirk-thomas mentioned this pull request Mar 28, 2019
Add rosidl_runtime_py package #34
Merged

mikaelarguedas force-pushed the get_slot_types_2 branch from fcec207 to 4b5a82dCompare March 28, 2019 22:30

mikaelarguedas added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Mar 29, 2019

dirk-thomas reviewed Mar 29, 2019
View reviewed changes

dirk-thomas reviewed Apr 1, 2019
View reviewed changes

Comment threadrosidl_runtime_py/rosidl_runtime_py/set_message.py Outdated

dirk-thomas reviewed Apr 1, 2019
View reviewed changes

Comment threadrosidl_generator_py/resource/_msg.py.em Outdated

dirk-thomas reviewed Apr 1, 2019
View reviewed changes

dirk-thomas added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Apr 19, 2019

mikaelarguedas mentioned this pull request May 2, 2019
Import message and support setting values of complex arrays/sequence #52
Closed

dirk-thomas mentioned this pull request May 2, 2019
add SLOT_TYPES to each message #54
Closed

mikaelarguedas added 4 commits May 3, 2019 01:06

add back I100 ignore
e2443ad
Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>

use new rosidl_parser.definition types and new test interfaces
d6589fd
Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>

restore original test, add test for SLOT_TYPES
64003b6
Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>

mikaelarguedas force-pushed the get_slot_types_2 branch from 712d4da to 64003b6Compare May 2, 2019 23:54

mikaelarguedas requested a review from dirk-thomas May 3, 2019 00:05

mikaelarguedas added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels May 3, 2019

dirk-thomas merged commit 78b765d into ros2:masterMay 3, 2019

dirk-thomas removed the in review Waiting for review (Kanban column) label May 3, 2019

mikaelarguedas deleted the get_slot_types_2 branch May 3, 2019 09:43

This was referenced May 3, 2019
move import_message_type from ros2topic to rosidl_runtime_py ros2/ros2cli#231
Closed
[ros2 topic pub][ros2 service call] Cannot populate nested arrays ros2/ros2cli#59
Closed

dirk-thomas mentioned this pull request Jul 19, 2019
Erroneous mypy specifications #70
Closed

		@@ -33,4 +38,13 @@ def set_message_fields(msg: Any, values: Dict[str, str]) -> None:
		except TypeError:
		value = field_type()

Uh oh!

Conversation

mikaelarguedas commented Mar 13, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mikaelarguedas commented Mar 28, 2019

Uh oh!

dirk-thomas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mikaelarguedas commented Apr 1, 2019

Uh oh!

dirk-thomas commented Apr 1, 2019

Uh oh!

Uh oh!

mikaelarguedas commented Apr 1, 2019

Uh oh!

Uh oh!

dirk-thomas commented Apr 1, 2019

Uh oh!

mikaelarguedas commented Apr 1, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dirk-thomas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikaelarguedas commented Apr 3, 2019

Uh oh!

dirk-thomas commented Apr 19, 2019

Uh oh!

dirk-thomas commented May 1, 2019

Uh oh!

mikaelarguedas commented May 2, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikaelarguedas commented May 3, 2019

Uh oh!

dirk-thomas commented May 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mikaelarguedas commented Mar 13, 2019•
edited
Loading

mikaelarguedas commented May 2, 2019•
edited
Loading