Add normalize_parameters and evaluate_paramters by sloretz · Pull Request #192 · ros2/launch

sloretz · 2019-03-01T18:41:50Z

Probably complete, but in progress while I self review. I wrote this a while ago and am just now coming back to it.

Like #173 but for parameters. This refactors the type checking and evaluation of parameters in Node into new functions normalize_parameters() and evaluate_parameters(). The reason is to make this logic reusable for composable nodes #160.

I made some changes along the way

parameter dictionaries are flattened to one level with names containing .
- Reason: python type hints can't yet do recursive types
- rcl_yaml_param_parser supports this just fine.
Lists of parameters may be any Sequence, not just List
- Reason: I wanted to use make evaluated parameters normalize_parameters() immutable, meaning tuple is used for the list of parameters. I didn't succeed since they still use dictionaries, but this is a little closer.
Parameter types are checked in Node.__init__() instead of when the action is executed
- Reason: Not really intentional; it just made sense to call normalize_parameters() there
- Of course substitutions are still evaluatated when the action is executed
Types of lists output by evaluated_parameters() are made uniform
- Reason: Composable nodes will need to know the list type so it can be passed in a ros service.
- if a list contains int and float, the list type is float
- if a list contains dissimilar types, the list type is str

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

sloretz · 2019-03-01T22:09:52Z

CI (testing launch, launch_ros, and test_launch_ros)

Linux
Linux-aarch64
macOS
Windows

launch_ros/launch_ros/utilities/normalize_parameters.py

jacobperron · 2019-03-05T18:48:51Z

launch_ros/launch_ros/utilities/normalize_parameters.py

+from ..parameters_type import SomeParameterValue
+
+
+def _normalize_parameter_array_value(value: SomeParameterValue) -> ParameterValue:


As it looks like value must be a Sequence, does it make sense to change the argument annotation to value: Sequence?

I think I would need to make it more more specific to make mypy happy Union[Sequence[str], Sequence[int], Sequence[Substitution]]. Since the function is prefixed with leading underscore I'll leave it as is for now.

Would it be Sequence[SomeSubstitutions]? I.e. would you want in the end a single string or a list of strings?

Would it be Sequence[SomeSubstitutions]? I.e. would you want in the end a single string or a list of strings?

For value or on the return type? For value, the types int and float could passed in via a dictionary in the launch description. For the return type the values need to be given to composable nodes via a ROS service, which means the types have to be kept to know which type to set in the parameter message. Sequence[Substitution] wouldn't work because it only outputs Text.

I was speaking about value because I thought that's what @jacobperron was talking about.

I meant as one of the choices in the union you mentioned, not just Sequence[SomeSubstitutionsType] (and I do mean that instead of Sequence[Substitution]).

I don't have a strong opinion, but I was extrapolating from the function name (contains array) and @jacobperron's question. Currently SomeParameterValue -> Union[SomeSubstitutionsType, _SingleValueType, _MultiValueType], and SomeSubstitutionsType is a list of substitutions, but they're meant to be evaluated and then concatenated, so they'd equate to a "single value type" in the end. If you want to represent a "multi value type" (what I assume is a list of values) then you'd need a list of SomeSubstitutionsType.

Maybe there's nothing to be done here, I was just trying to make sure that you're not making the assumption that SomeSubstitutionsType ends up being a list of values after evaluations.

Ah you're right. Added Sequence[SomeSubstitutionsType] (and mypy appeasement required afterwards) to SomeParameterValue in 65f23a6. The normalized type ParameterValue already had the right type for list of lists of substitutions.

launch_ros/launch_ros/utilities/normalize_parameters.py

test_launch_ros/test/test_launch_ros/test_normalize_parameters.py

jacobperron · 2019-03-05T19:21:54Z

test_launch_ros/test/test_launch_ros/test_normalize_parameters.py

+    assert evaluate_parameters(LaunchContext(), norm) == expected
+
+
+def test_dictionary_with_dissimilar_array():


I'm curious to know if a dissimilar array with a substitution type will also work. In particular:

[True, 1, TextSubstitution(text='foo')]

and

[TextSubstitution(text='foo'), True, 1]

Good catch, there's a bug

test/test_launch_ros/test_normalize_parameters.py:180: in test_dictionary_with_dissimilar_array norm = normalize_parameters(orig) ../../../../build/launch_ros/launch_ros/utilities/normalize_parameters.py:183: in normalize_parameters normalized_params.append(normalize_parameter_dict(param)) ../../../../build/launch_ros/launch_ros/utilities/normalize_parameters.py:162: in normalize_parameter_dict normalized[tuple(name)] = _normalize_parameter_array_value(value) ../../../../build/launch_ros/launch_ros/utilities/normalize_parameters.py:84: in _normalize_parameter_array_value return tuple(normalize_to_list_of_substitutions(cast(SomeSubstitutionsType, value))) ../../../../build/launch/launch/utilities/normalize_to_list_of_substitutions_impl.py:42: in normalize_to_list_of_substitutions return [normalize(y) for y in cast(Iterable, subs)] ../../../../build/launch/launch/utilities/normalize_to_list_of_substitutions_impl.py:42: in <listcomp> return [normalize(y) for y in cast(Iterable, subs)] ../../../../build/launch/launch/utilities/normalize_to_list_of_substitutions_impl.py:36: in normalize "'str' or 'launch.Substitution' were expected.".format(type(x))) E TypeError: Failed to normalize given item of type '<class 'bool'>', when only 'str' or 'launch.Substitution' were expected.

Details

Added tests and fixed bug in a4ac02b

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

jacobperron

LGTM with green CI

sloretz · 2019-03-06T00:31:55Z

CI (Testing launch_ros and test_launch_ros)

Linux
Linux-aarch64
macOS
Windows

launch_ros/launch_ros/utilities/normalize_parameters.py

dirk-thomas · 2019-05-21T19:13:27Z

launch_ros/launch_ros/utilities/normalize_parameters.py

+from launch.utilities import ensure_argument_type
+from launch.utilities import normalize_to_list_of_substitutions
+
+from ..parameters_type import ParameterFile


This line seems to result in a flake8 warning in the dev job: http://build.ros2.org/job/Ddev__launch_ros__ubuntu_bionic_amd64/21/consoleFull#console-section-17

@sloretz Can you please double check this.

That's odd, this PR is from March. Is this a flake8 regression? ParameterFile is used in a type annotation. https://github.com/ros2/launch_ros/blob/2d5d3eac831788d54c41069378733837b48b44c8/launch_ros/launch_ros/utilities/normalize_parameters.py#L175

Maybe using the PEP526 typing syntax will workaround the issue?

The dev job users the Debian package of flake8 which is different from the latest release we use on ci.ros2.org.

Add normalize_parameters and evaluate_paramters

e5b13fd

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

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

sloretz self-assigned this Mar 1, 2019

sloretz added 4 commits March 1, 2019 12:02

Misc mypy complaint fixes

1d493eb

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

More mypy appeasement

a304468

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

fix bugs introduced whil appeasing mypy

b0f8c77

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Another mypy complaint fixed

a938a19

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

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

jacobperron self-requested a review March 5, 2019 18:57

jacobperron reviewed Mar 5, 2019

View reviewed changes

sloretz added 8 commits March 5, 2019 12:34

fix checking str and substitution

d66bcb3

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Avoid indentation

1e5d3dc

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

keys -> key

21be882

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

substututions -> substitutions

d835512

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Add test with float as first element in numeric list

b1f2286

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Add test with string before substitution

46b1495

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Add more dissimilar tests and fix bugs

a4ac02b

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

SomeParameterValue multi value substitutions

65f23a6

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

jacobperron approved these changes Mar 6, 2019

View reviewed changes

sloretz merged commit 6b6b67d into master Mar 6, 2019

sloretz deleted the normalize_parameters branch March 6, 2019 01:46

sloretz removed the in review Waiting for review (Kanban column) label Mar 6, 2019

sloretz mentioned this pull request Mar 22, 2019

Launch composable C++ nodes ros2/launch_ros#6

Closed

4 tasks

dirk-thomas reviewed Apr 19, 2019

View reviewed changes

launch_ros/launch_ros/utilities/normalize_parameters.py Show resolved Hide resolved

sloretz mentioned this pull request Apr 19, 2019

Simplify isinstance calls ros2/launch_ros#18

Merged

dirk-thomas reviewed May 21, 2019

View reviewed changes

		from ..parameters_type import SomeParameterValue


		def _normalize_parameter_array_value(value: SomeParameterValue) -> ParameterValue:

		assert evaluate_parameters(LaunchContext(), norm) == expected


		def test_dictionary_with_dissimilar_array():

Conversation

sloretz commented Mar 1, 2019

Uh oh!

sloretz commented Mar 1, 2019

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!

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!

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!

jacobperron left a comment

Choose a reason for hiding this comment

Uh oh!

sloretz commented Mar 6, 2019

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants