Add normalize_parameters and evaluate_paramters

[sloretz]

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

[sloretz]

CI (testing launch, launch_ros, and test_launch_ros)

• Linux
• Linux-aarch64
• macOS
• Windows

[jacobperron]

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

[sloretz]

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.

[wjwwood]

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

[sloretz]

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.

[wjwwood]

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.

[sloretz]

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.

[jacobperron]

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]

[sloretz]

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.

[sloretz]

Added tests and fixed bug in a4ac02b

[jacobperron]

LGTM with green CI

[sloretz]

CI (Testing launch_ros and test_launch_ros)

• Linux
• Linux-aarch64
• macOS
• Windows

[dirk-thomas]

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.

[sloretz]

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?

[dirk-thomas]

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