store types as tuple of abstract types #33

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

Replaces #30 now that #24 has merged

Blocked by #34

rosidl_generator_py/resource/_msg.py.em

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?

rosidl_generator_py/resource/_msg.py.em

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

rosidl_generator_py/resource/_msg.py.em

 @[  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

rosidl_runtime_py/rosidl_runtime_py/__init__.py

rosidl_runtime_py/rosidl_runtime_py/convert.py

rosidl_runtime_py/rosidl_runtime_py/set_message.py

rosidl_generator_py/test/test_interfaces.py

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.

rosidl_runtime_py/rosidl_runtime_py/set_message.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?

rosidl_generator_py/resource/_msg.py.em

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

rosidl_runtime_py/rosidl_runtime_py/import_message.py

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

rosidl_runtime_py/rosidl_runtime_py/set_message.py

+                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

rosidl_generator_py/resource/_msg.py.em

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

rosidl_runtime_py/rosidl_runtime_py/set_message.py

@@ -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 <[email protected]> update tests Signed-off-by: Mikael Arguedas <[email protected]> simplify logic by printing member type directly Signed-off-by: Mikael Arguedas <[email protected]> move dict construction to rosidl_runtime_py Signed-off-by: Mikael Arguedas <[email protected]> add utility to import complex message and add support for nested array in set_message Signed-off-by: Mikael Arguedas <[email protected]> update tests and remove coverage for dict to avoid circular dependency Signed-off-by: Mikael Arguedas <[email protected]> more descriptive variable name Signed-off-by: Mikael Arguedas <[email protected]> refactor import logic according to ros2#35 Signed-off-by: Mikael Arguedas <[email protected]> move imports to top of file Signed-off-by: Mikael Arguedas <[email protected]> string maximum size doesn't need quoting or str conversion Signed-off-by: Mikael Arguedas <[email protected]> update docblock Signed-off-by: Mikael Arguedas <[email protected]> slot_types_dict_from_message -> get_message_slot_types Signed-off-by: Mikael Arguedas <[email protected]> check Abstrct type instead of 'list' Signed-off-by: Mikael Arguedas <[email protected]> use NestedType Signed-off-by: Mikael Arguedas <[email protected]> add conditional is namespaces is an empty list Signed-off-by: Mikael Arguedas <[email protected]> one liner Signed-off-by: Mikael Arguedas <[email protected]> extend test suite to cover NesteTypes of NamespacedTypes Signed-off-by: Mikael Arguedas <[email protected]> single message fixture to use Signed-off-by: Mikael Arguedas <[email protected]> use NestedType Signed-off-by: Mikael Arguedas <[email protected]> add coverage for static array of nested messages Signed-off-by: Mikael Arguedas <[email protected]> restore old API Signed-off-by: Mikael Arguedas <[email protected]> SLOT_TYPES improvement from dirk-thomas Signed-off-by: Mikael Arguedas <[email protected]> resolve conflicts Signed-off-by: Mikael Arguedas <[email protected]>

Signed-off-by: Mikael Arguedas <[email protected]>

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 <[email protected]> update tests Signed-off-by: Mikael Arguedas <[email protected]> simplify logic by printing member type directly Signed-off-by: Mikael Arguedas <[email protected]> move dict construction to rosidl_runtime_py Signed-off-by: Mikael Arguedas <[email protected]> add utility to import complex message and add support for nested array in set_message Signed-off-by: Mikael Arguedas <[email protected]> update tests and remove coverage for dict to avoid circular dependency Signed-off-by: Mikael Arguedas <[email protected]> more descriptive variable name Signed-off-by: Mikael Arguedas <[email protected]> refactor import logic according to ros2/rosidl_python#35 Signed-off-by: Mikael Arguedas <[email protected]> move imports to top of file Signed-off-by: Mikael Arguedas <[email protected]> string maximum size doesn't need quoting or str conversion Signed-off-by: Mikael Arguedas <[email protected]> update docblock Signed-off-by: Mikael Arguedas <[email protected]> slot_types_dict_from_message -> get_message_slot_types Signed-off-by: Mikael Arguedas <[email protected]> check Abstrct type instead of 'list' Signed-off-by: Mikael Arguedas <[email protected]> use NestedType Signed-off-by: Mikael Arguedas <[email protected]> add conditional is namespaces is an empty list Signed-off-by: Mikael Arguedas <[email protected]> one liner Signed-off-by: Mikael Arguedas <[email protected]> extend test suite to cover NesteTypes of NamespacedTypes Signed-off-by: Mikael Arguedas <[email protected]> single message fixture to use Signed-off-by: Mikael Arguedas <[email protected]> use NestedType Signed-off-by: Mikael Arguedas <[email protected]> add coverage for static array of nested messages Signed-off-by: Mikael Arguedas <[email protected]> restore old API Signed-off-by: Mikael Arguedas <[email protected]> SLOT_TYPES improvement from dirk-thomas Signed-off-by: Mikael Arguedas <[email protected]> resolve conflicts Signed-off-by: Mikael Arguedas <[email protected]> * add back I100 ignore Signed-off-by: Mikael Arguedas <[email protected]> * use new rosidl_parser.definition types and new test interfaces Signed-off-by: Mikael Arguedas <[email protected]> * restore original test, add test for SLOT_TYPES Signed-off-by: Mikael Arguedas <[email protected]>

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

rosidl_generator_py/resource/_msg.py.em Outdated Show resolved Hide resolved

rosidl_generator_py/resource/_msg.py.em Outdated Show resolved Hide resolved

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 4b5a82d Compare 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

rosidl_runtime_py/rosidl_runtime_py/set_message.py Outdated Show resolved Hide resolved

dirk-thomas reviewed Apr 1, 2019

View reviewed changes

rosidl_generator_py/resource/_msg.py.em Outdated Show resolved Hide resolved

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 <[email protected]>

use new rosidl_parser.definition types and new test interfaces

d6589fd

Signed-off-by: Mikael Arguedas <[email protected]>

restore original test, add test for SLOT_TYPES

64003b6

Signed-off-by: Mikael Arguedas <[email protected]>

mikaelarguedas force-pushed the get_slot_types_2 branch from 712d4da to 64003b6 Compare 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:master May 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

store types as tuple of abstract types #33

store types as tuple of abstract types #33

mikaelarguedas commented Mar 13, 2019 •

edited

Loading

mikaelarguedas commented Mar 28, 2019

dirk-thomas left a comment

dirk-thomas Mar 29, 2019

mikaelarguedas Apr 1, 2019

dirk-thomas Apr 1, 2019

mikaelarguedas Apr 1, 2019

mikaelarguedas commented Apr 1, 2019

dirk-thomas commented Apr 1, 2019

mikaelarguedas commented Apr 1, 2019

dirk-thomas commented Apr 1, 2019

mikaelarguedas commented Apr 1, 2019

dirk-thomas Apr 1, 2019

mikaelarguedas Apr 1, 2019

dirk-thomas left a comment

dirk-thomas Apr 1, 2019

mikaelarguedas Apr 1, 2019

mikaelarguedas commented Apr 3, 2019

dirk-thomas commented Apr 19, 2019

dirk-thomas commented May 1, 2019

mikaelarguedas commented May 2, 2019 •

edited

Loading

mikaelarguedas commented May 3, 2019

dirk-thomas commented May 3, 2019

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

store types as tuple of abstract types #33

store types as tuple of abstract types #33

Conversation

mikaelarguedas commented Mar 13, 2019 • edited Loading

mikaelarguedas commented Mar 28, 2019

dirk-thomas 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

mikaelarguedas commented Apr 1, 2019

dirk-thomas commented Apr 1, 2019

mikaelarguedas commented Apr 1, 2019

dirk-thomas commented Apr 1, 2019

mikaelarguedas commented Apr 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirk-thomas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikaelarguedas commented Apr 3, 2019

dirk-thomas commented Apr 19, 2019

dirk-thomas commented May 1, 2019

mikaelarguedas commented May 2, 2019 • edited Loading

mikaelarguedas commented May 3, 2019

dirk-thomas commented May 3, 2019

mikaelarguedas commented Mar 13, 2019 •

edited

Loading

mikaelarguedas commented May 2, 2019 •

edited

Loading