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

'ros2 param load' fails when double parameter in parameter file is in scientific notation #834

Open
bijoua29 opened this issue May 26, 2023 · 10 comments
Labels
more-information-needed Further information is required

Comments

@bijoua29
Copy link

Bug report

Required Info:

  • Operating System:
    • Ubuntu 22.04
  • Installation type:
    • binaries
  • Version or commit hash:
    • latest rolling release
  • DDS implementation:
    • N/A
  • Client library (if applicable):
    • ros2cli

Steps to reproduce issue

  1. Run an application that loads parameters from a parameter file, that includes a parameter specified using scientfic notation 5e-06, using
ros2 run parameter_test parameter_test --ros-args --params-file parameter_test/config/params.yaml
  1. Load parameters again from the same parameter file using
ros2 param load param_test parameter_test/config/params.yaml

Expected behavior

Parameters are loaded properly in step 1
Parameters are loaded properly in step 2

Actual behavior

Parameters are loaded properly in step 1
However, in step 2, it results in following error:

Set parameter test_double failed: Wrong parameter type, parameter {test_double} is of type {double}, setting it to {string} is not allowed.

Additional information

Attached is package that was used for test
parameter_test.zip

fujitatomoya added a commit to fujitatomoya/ros2_test_prover that referenced this issue Jun 7, 2023
… scientific notation

  ros2/ros2cli#834

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya
Copy link
Collaborator

I believe that root cause is python yaml does not support scientific notation.

https://github.com/ros2/rclpy/blob/20125a4de2d955948db9ff58810efd03cb5603f3/rclpy/rclpy/parameter.py#L194

fixed with yaml/pyyaml#173

@bijoua29
Copy link
Author

bijoua29 commented Jun 7, 2023

@fujitatomoya I don't believe this to be the issue. I have tried 5.0e-06 and get the same error message.

@fujitatomoya
Copy link
Collaborator

I don't believe this to be the issue. I have tried 5.0e-06 and get the same error message.

what do you mean? i think this is because of yaml/pyyaml#173, and could be addressed by https://github.com/yaml/pyyaml/pull/174/files.
are you saying that this #834 has another issues?

@bijoua29
Copy link
Author

bijoua29 commented Jun 7, 2023

Maybe I'm missing something. My understanding was that pyyaml#174 fixes the issue where scientific notation without a dot in the mantissa is not resolved as a float but as a string. What I am saying is that in my repro steps above, I changed the parameter to have a dot in the mantissa and it still resolves it as a string. I'm not sure where the issue is, with respect to #834, but it exists regardless of whether there is a dot or not in the mantissa.

@fujitatomoya
Copy link
Collaborator

@bijoua29 thanks for the explanation.

@clalancette
Copy link
Contributor

I'm also not able to reproduce this with standard ROS 2 tooling.

That is to say, I took your code and removed the references to generate_parameter_library, and just used a regular declare_parameter. You can see exactly what I did in https://github.com/clalancette/parameter_test .

With that setup, your tests above work just fine for me, in Rolling, Iron, and Humble. Can you check to confirm that the same thing works for you?

@clalancette clalancette added the more-information-needed Further information is required label Jun 15, 2023
@bijoua29
Copy link
Author

bijoua29 commented Jun 18, 2023

@clalancette I ran your modified package with standard ROS 2 tooling i.e. without generate_parameter_library, and it still fails for me on the latest Rolling.

That is, the following works fine:

ros2 run parameter_test parameter_test --ros-args --params-file parameter_test/config/params.yaml

but the following fails after the node is launched:

ros2 param load param_test parameter_test/config/params.yaml

with the error:

Set parameter test_double failed: Wrong parameter type, parameter {test_double} is of type {double}, setting it to {string} is not allowed.

Interestingly, if I have 0.00006 (no scientific notation) in the parameter file, it succeeds when launching but still fails on the subsequent param load.

I am now curious what is different about your setup from mine.

@clalancette
Copy link
Contributor

but the following fails after the node is launched:

ros2 param load param_test parameter_test/config/params.yaml

You are correct, that is failing. Thanks for pointing it out.

That said, this does look to be a consequence of yaml/pyyaml#174 not being in yet. I locally modified my PyYAML sources to have that PR in place, and with that there, the ros2 param load works properly. So I think we are more-or-less waiting on that one to be merged and released.

@bijoua29
Copy link
Author

@clalancette Thanks for confirming my findings.

I am not hopeful that yaml/pyyaml#174 is going to be merged anytime soon since it has been open for 5 years. So, did you install pyyaml locally using 'setup.py install'? Not sure how I could automate that since I am running in a docker container.

@clalancette
Copy link
Contributor

So, did you install pyyaml locally using 'setup.py install'?

No, I edited the system version of resolver.py locally to add in the code from that PR to test with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
more-information-needed Further information is required
Projects
None yet
Development

No branches or pull requests

3 participants