Skip to content
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

String parameter with digits and E or e (e.g. "123E5") is always interpreted as double #384

Open
chrmel opened this issue Oct 20, 2023 · 12 comments · May be fixed by #436
Open

String parameter with digits and E or e (e.g. "123E5") is always interpreted as double #384

chrmel opened this issue Oct 20, 2023 · 12 comments · May be fixed by #436
Assignees

Comments

@chrmel
Copy link

chrmel commented Oct 20, 2023

Bug report

When launching a ROS2 node with ros2 launch ... a parameter containing digits and E or e is always interpreted as a double value. Even when the parameter is defined as a string parameter and even when the whole parameter is passed as string.

  • Operating System:
    • Ubuntu 22.04
  • Installation type:
    • binaries
  • Version or commit hash:
    • humble
  • Client library (if applicable):
    • rclpy

Steps to reproduce issue

Create a Python package and have a simple node and launchfile
param_test.py

import signal
import rclpy

from rclpy.node import Node
from rcl_interfaces.msg import ParameterType


class ParamTest(Node):
    def __init__(self):
        Node.__init__(self, 'param_test')

        # parameter declaration and info
        self._param = self.declare_parameter('param', 'abc').get_parameter_value().string_value
        self.get_logger().info(f'String Param: {self._param}')


def main(args=None):
    def raise_keyboard_int(_, __):
        rclpy.logging.get_logger('param_test_node').info('stopping')
        raise KeyboardInterrupt()

    signal.signal(signal.SIGINT, raise_keyboard_int)

    rclpy.init(args=args)
    node = ParamTest()

    try:
        rclpy.spin_once(node)

    except KeyboardInterrupt:
        pass

    finally:
        node.destroy_node()



if __name__ == '__main__':
    main()

test.launch.py

#!/usr/bin/python3

from launch import LaunchDescription
from launch_ros.actions import Node
from launch.actions import DeclareLaunchArgument
from launch.substitutions import TextSubstitution, LaunchConfiguration


def generate_launch_description():
    return LaunchDescription([
        DeclareLaunchArgument(
            'param',
            default_value=TextSubstitution(text='abc'),
            description='String param'
        ),
        Node(
            package='param_test',
            executable='param_test',
            name='mccdaq_daq',
            output='screen',
            emulate_tty=True,
            parameters=[{
                'param': LaunchConfiguration('param'),
            }],
        ),
        ])

Expected behavior

$ ros2 launch param_test test.launch.py param:="123E5"
[INFO] [1697811050.937763500] [param_test]: String Param: 123E5

Actual behavior

$ ros2 launch param_test test.launch.py param:="123E5"
[INFO] [launch]: All log files can be found below /root/.ros/log/2023-10-20-14-11-17-221115-708631e4d2d9-2422
[INFO] [launch]: Default logging verbosity is set to INFO
[INFO] [param_test-1]: process started with pid [2423]
[param_test-1] Traceback (most recent call last):
[param_test-1]   File "/ros2_ws/install/param_test/lib/param_test/param_test", line 33, in <module>
[param_test-1]     sys.exit(load_entry_point('param-test==0.0.0', 'console_scripts', 'param_test')())
[param_test-1]   File "/ros2_ws/install/param_test/lib/python3.10/site-packages/param_test/param_test.py", line 25, in main
[param_test-1]     node = ParamTest()
[param_test-1]   File "/ros2_ws/install/param_test/lib/python3.10/site-packages/param_test/param_test.py", line 13, in __init__
[param_test-1]     self._param = self.declare_parameter('param', 'abc').get_parameter_value().string_value
[param_test-1]   File "/opt/ros/humble/local/lib/python3.10/dist-packages/rclpy/node.py", line 367, in declare_parameter
[param_test-1]     return self.declare_parameters('', [args], ignore_override)[0]
[param_test-1]   File "/opt/ros/humble/local/lib/python3.10/dist-packages/rclpy/node.py", line 494, in declare_parameters
[param_test-1]     self._set_parameters(
[param_test-1]   File "/opt/ros/humble/local/lib/python3.10/dist-packages/rclpy/node.py", line 739, in _set_parameters
[param_test-1]     raise InvalidParameterTypeException(
[param_test-1] rclpy.exceptions.InvalidParameterTypeException: Trying to set parameter 'param' to '12300000.0' of type 'DOUBLE', expecting type 'STRING'

Additional information

Mirght be related to ros2/ros2cli#677?

  • ros2 run ... can be forced to interpret the parameter as a string:
    $ ros2 run param_test param_test --ros-args -p "param:='123E5'"
    [INFO] [1697811050.937763500] [param_test]: String Param: 123E5
  • ros2 launch ... cannot be forced to take this parameter as a string:
    $ ros2 launch param_test test.launch.py param:="123E5"
    ... rclpy.exceptions.InvalidParameterTypeException: Trying to set parameter 'param' to '12300000.0' of type 'DOUBLE', expecting type 'STRING'
    $ ros2 launch param_test test.launch.py param:="'123E5'"
    ... rclpy.exceptions.InvalidParameterTypeException: Trying to set parameter 'param' to '12300000.0' of type 'DOUBLE', expecting type 'STRING'
    $ ros2 launch param_test test.launch.py param:='"123E5"'
    ... rclpy.exceptions.InvalidParameterTypeException: Trying to set parameter 'param' to '12300000.0' of type 'DOUBLE', expecting type 'STRING'
    And the most interesting one:
    $ ros2 launch param_test test.launch.py param:='!!str 123E5'
    ... rclpy.exceptions.InvalidParameterTypeException: Trying to set parameter 'param' to '12300000.0' of type 'DOUBLE', expecting type 'STRING'
    Even using all kinds of escapes, ASCII codes and what ever did not work!
fujitatomoya referenced this issue in fujitatomoya/ros2_test_prover Oct 24, 2023
@fujitatomoya
Copy link
Contributor

fujitatomoya commented Oct 24, 2023

I think this problem is related to https://github.com/ros2/launch_ros/tree/rolling/ros2launch, not in here ros2cli. (probably https://github.com/ros2/launch/tree/rolling/launch.)

@clalancette can you move this issue to https://github.com/ros2/launch_ros?

note: this problem is reproducible with ros2/ros2@1f5bd8e current head today.

@sloretz sloretz transferred this issue from ros2/ros2cli Nov 2, 2023
@danielcranston
Copy link

I ran into this as well (using rclcpp / jazzy binaries) and spent some time digging into what might be the cause.

TL;DR

The issue seems to lie with how the param is dumped to a temporary YAML file (launch_ros/actions/node.py#L376) by the actions.Node, and how that dumped YAML is read and interpreted (by client library code, I presume. Or some intermediate logic?).

Longer description

Regardless of how the param is given to the ros2 launch invocation,

  • param:=123E5
  • param:="123E5"
  • param:="'123E5'"

It gets juggled around a bit by launch_ros processing and ends up as a yaml_evaluated_value (launch_ros/utilities/evaluate_parameters.py#L79), that in all of the above cases ends up being a string with value 123E5 (free of quotes!).

Then the following happens:

  • When being dumped, PYYAML (correctly!) considers it a string, so it does its normal thing and dumps it without quotes.
  • The code that consumes this YAML seemingly interprets it as a double, which leads to the InvalidParameterTypeException that @chrmel mentions (identical/equivalent issue happens with rclcpp as well).

So for me this does not look like a problem in https://github.com/ros2/launch_ros, but rather that the consuming code is at fault for failing to parse the YAML value as a string. What do you think @fujitatomoya @clalancette?

How to address this?

I can see a couple of ways to tackle this:

  1. On the dumper side:
    a. Change action/node.py/utilities/evaluate_parameters.py to ensure that yaml_evaluated_values that got evaluated to a str are dumped with explicit quotes.
  2. On the consumer side:
    a. Change client libraries (or whatever is reading the temporary YAML files) to catch this edge case at YAML read time.
    b. Relax client libraries so that whenever a double value is attempted to be set to a param declared as a string (currently leading to InvalidParameterTypeException), it falls back to first casting the incoming value to a string and trying again.

Related questions and comments

  • Can someone link the code that actually consumes this temporary YAML file? I haven't been able to find it.
  • I did a quick (rclcpp) test of solution 1 above, and indeed with explicit quotes in the temporary YAML the issue disappears.
  • This seems related: Numbers in scientific notation without dot are parsed as string yaml/pyyaml#173. And I see now a few other ROS issues link to it.
  • For clarity, PYYAML is "round-trip-consistent", i.e. a no-quotes 123E5 YAML value is loaded as a string as well (not a double). So there is no discrepancy within PYYAML at least. It also raises the question "What parsing logic/library is the consuming code using?"

I'd be happy to contribute if I could get some more direction on the above.

@danielcranston
Copy link

I found some answers:

Can someone link the code that actually consumes this temporary YAML file? I haven't been able to find it.

So it is indeed client library code. Specifically it's in rcl, which makes sense since this issue is present in both rclcpp and rclpy.

It also raises the question "What parsing logic/library is the consuming code using?"

https://github.com/ros2/libyaml_vendor i.e. https://github.com/yaml/libyaml.

AFAICT both PYYAML and libyaml implement the YAML 1.1 spec, so I guess either the spec is inconclusive in this case, or one of them (PYAML, libyaml) is incorrectly implementing the spec.

@danielcranston
Copy link

So here's an executive summary of the issue:

  1. launch_ros treats these kinds of "digits" as strings
  2. launch_ros prunes away quotes via this PYAML load.
  3. launch_ros, when dumping to temporary YAML params file, writes (via PYYAML) these strings in non-quoted (plain?) form.
  4. the rclcpp/rclpy node reads said params file (in rcl, via libyaml), and libyaml happens to treat these kinds of "digit strings" (when unquoted!) as doubles.

At this point I feel solution 1 (see "On the dumper side") is the best approach. Unless there's something I'm missing, it should always be safe to dump strings explicitly with quotes. Indeed https://www.yaml.info/learn/quote.html even advocates for using explicit quotes (see "Special types").


As for this comment:

I guess either the spec is inconclusive in this case, or one of them (PYAML, libyaml) is incorrectly implementing the spec.

This tests data (Linked from yaml.info, so seem credible) implies that PYAML is correct to treat them as strings, and that libyaml is the one "at fault". Then again I found #319 (comment) (that issue seems very related to this one!) which implies the other way around.

@fujitatomoya
Copy link
Contributor

@danielcranston 1st of all, thank you very much for checking all the details and good explanation!

ROS 2 https://github.com/ros2/libyaml_vendor/blob/rolling/CMakeLists.txt#L14 specifies the version of libyaml, that only supports YAML 1.1 now.
I do not know very much about this history, but this intentionally keeps ROS 2 with YAML 1.1 specification.

The code that consumes this YAML seemingly interprets it as a double,
but rather that the consuming code is at fault for failing to parse the YAML value as a string.

this can be true if it uses YAML 1.2. 123E5 without any quotes should be recognized as string, not float.
but YAML 1.1 recognizes this as float, and that is what rcl_yaml_param_parser does as specified.

after all, i think rcl_yaml_param_parser is no problem.

PYYAML (correctly!) considers it a string, so it does its normal thing and dumps it without quotes.

related to the following comment(more like guess).
this is not good assumption here? libyaml parses this as float, but string. that is why we are having this problem.

For clarity, PYYAML is "round-trip-consistent", i.e. a no-quotes 123E5 YAML value is loaded as a string as well (not a double). So there is no discrepancy within PYYAML at least.

i think pyyaml module uses libyaml internally, but pyyaml addresses this specific case, so that it can support "round-trip-consistent".

At this point I feel solution 1 (see "On the dumper side") is the best approach.

So do I. and i do not think there is downside for this adding quotes for str approach.

i will try to patch it, give me some time.

@fujitatomoya
Copy link
Contributor

@danielcranston @chrmel it would be appreciated if you guys can try #436 to see if that fixes the problem.

@danielcranston
Copy link

Thanks for making the PR @fujitatomoya, and thanks for shedding more light on the background and checking some of my assumptions.

Some replies:

i think pyyaml module uses libyaml internally, but pyyaml addresses this specific case, so that it can support "round-trip-consistent".

Oh, I wasn't aware of that! But I think that, unless PYYAML was installed with --with-libyaml and some CLoader/CDumper stuff is setup, it uses it's own pure Python implementation. And indeed they mention that there are "some subtle (but not really significant) differences between pure Python and LibYAML based parsers and emitters." I wonder what they might be referring to 😄

But regardless, your point about "round-trip-consistent" still stands.

The code that consumes this YAML seemingly interprets it as a double,
but rather that the consuming code is at fault for failing to parse the YAML value as a string.

this can be true if it uses YAML 1.2. 123E5 without any quotes should be recognized as string, not float.
but YAML 1.1 recognizes this as float, and that is what rcl_yaml_param_parser does as specified.

Provided the "tests data" linked above is a correct representation of how the specs should be behaving, I think it's the other way around?

  • In YAML 1.2 (Core), 123E5 without quotes should be recognized as float
  • In YAML 1.1, 123E5 without quotes should be recognized as string

(but it's a bit ambiguous since it only makes clear what the behavior is for lower case)

@fujitatomoya
Copy link
Contributor

I tried the following with rolling ubuntu noble envirnoment.

import yaml

# Data with float and string
data = {
    "float_value": 123e5,
    "string_value": "123e5"
}

# Dump YAML to a file
with open("output.yaml", "w") as file:
    yaml.dump(data, file, default_flow_style=False)

# Read and Load YAML back
with open("output.yaml", "r") as file:
    loaded_data = yaml.safe_load(file)

# Print loaded values and their types
print("Loaded Data:", loaded_data)
print("Type of float_value:", type(loaded_data["float_value"]))
print("Type of string_value:", type(loaded_data["string_value"]))

print("Using libyaml:", yaml.__with_libyaml__)  # True if libyaml is used

data = yaml.safe_load("""
    "float_value": 123e5
""")

print(data)

yaml_data = "value: 123E5"

# Load YAML
loaded_data = yaml.safe_load(yaml_data)

# Print loaded value and its type
print("Loaded Data:", loaded_data)
print("Type of value:", type(loaded_data["value"]))  # Expected: <class 'float'>

print(yaml.__version__)
root@tomoyafujita:~/ros2_ws/colcon_ws# python3 test.py
Loaded Data: {'float_value': 12300000.0, 'string_value': '123e5'}
Type of float_value: <class 'float'>
Type of string_value: <class 'str'>
Using libyaml: True
{'float_value': '123e5'}
Loaded Data: {'value': '123E5'}
Type of value: <class 'str'>
6.0.1
root@tomoyafujita:~/ros2_ws/colcon_ws# cat output.yaml
float_value: 12300000.0
string_value: 123e5
root@tomoyafujita:~/ros2_ws/colcon_ws# dpkg -l libyaml-0-2
Desired=Unknown/Install/Remove/Purge/Hold
| Status=Not/Inst/Conf-files/Unpacked/halF-conf/Half-inst/trig-aWait/Trig-pend
|/ Err?=(none)/Reinst-required (Status,Err: uppercase=bad)
||/ Name              Version       Architecture Description
+++-=================-=============-============-========================================
ii  libyaml-0-2:amd64 0.2.5-1build1 amd64        Fast YAML 1.1 parser and emitter library

according to the result above,

Provided the "tests data" linked above is a correct representation of how the specs should be behaving, I think it's the other way around?
In YAML 1.1, 123E5 without quotes should be recognized as string

this is correct. sorry i mixed up... 😅 thanks for clarification.

based on the above result, the root cause is when we change the python dictionary into yaml via yaml.dump, specifically it changes 123E5 into 12300000.0, that will be loaded as float of course.

after all, i think your deduction is correct. there is nothing wrong with libyaml or pyyaml, they do support round-trip-consistent with YAML 1.1 specification. so i think current fix looks okay to convert the str object into yaml element with quote representator. what do you think?

@danielcranston
Copy link

I agree that the quote representor is a good solution, yeah 👍 To ensure we eliminate any misunderstanding for other readers, I can't resist responding to this though:

root cause is when we change the python dictionary into yaml via yaml.dump, specifically it changes 123E5 into 12300000.0, that will be loaded as float of course.

Almost 🙂

yaml.dump does not change 123E5 into 12300000.0:

daniel@daniel:~$ python3
>>> import yaml
>>> yaml.dump
>>> with open("/tmp/temp_params.yaml", "w") as f:
...     yaml.dump({"param": "123E4"}, f)
... 
>>> 
daniel@daniel:~$ cat /tmp/temp_params.yaml 
param: 123E4
daniel@daniel:~$ python3
>>> import yaml
>>> with open("/tmp/temp_params.yaml") as f:
...     d = yaml.safe_load(f)
... 
>>> print(d)
{'param': '123E5'}
>>> print(type(d["param"]))
<class 'str'>

Instead the root cause is that

  • yaml.dump unfortunately does not wrap 123E5 in quotes.
  • libyaml will load this param value inside this /tmp/temp_params.yaml as a double (you can try it out in isolation).

So I would even go so far as to say that libyaml might not be respecting the YAML 1.1 spec in this instance. At least, it is not consistent with the "tests data" above, since by that reference libyaml should be parsing it as a string, but is not.

Which leads into this line in the code you shared:

print("Type of value:", type(loaded_data["value"]))  # Expected: <class 'float'>

I would expect string, due to the above (again, provided the "tests data" is a valid reference).

@fujitatomoya
Copy link
Contributor

yaml.dump does not change 123E5 into 12300000.0:

really, this is what i got from Ubuntu Focal/Noble.

root@tomoyafujita:~/ros2_ws/work# cat test.py
import yaml

# Data with float and string
data = {
    "float_value": 123e5,
    "string_value": "123e5"
}

# Dump YAML to a file
with open("output.yaml", "w") as file:
    yaml.dump(data, file, default_flow_style=False)

root@tomoyafujita:~/ros2_ws/work# python3 test.py
root@tomoyafujita:~/ros2_ws/work# cat output.yaml
float_value: 12300000.0
string_value: 123e5

float_value: 12300000.0 will be loaded as floating type.

@danielcranston
Copy link

Ah sorry, you are absolutely right.

When I was talking about 123E5 above, I was considering it a Python variable of type str. In your discussion I see now you are considering it a float. In that case indeed it is being converted to 12300000.0.

Sorry for the confusion 🙇

@fujitatomoya
Copy link
Contributor

@danielcranston no worries. this is really complicated, i lost many times 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants