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

fix loading parameter behavior from yaml file. #1193

Open
wants to merge 3 commits into
base: rolling
Choose a base branch
from

Conversation

fujitatomoya
Copy link
Collaborator

address ros2/ros2cli#863

in this PR, it tries to address a few things as following.

  • parameter_dict_from_yaml_file do not use temporary list, it now parses parameter yaml file and directly stores the contents in the dictionary.
  • parameter_dict_from_yaml_file is now capable of parsing the yaml file with multiple keys with namespace and base nodename.
  • load_parameter_file of Parameter Client should specifies the target node to parse the parameter yaml file. this should be done to avoid the original problem reported address ros2 param load sets the unexpected parameter value from yaml ros2cli#863
  • add more unit tests to parameter_dict_from_yaml_file accordingly.

rclpy/test/test_parameter_client.py Show resolved Hide resolved
rclpy/rclpy/parameter_client.py Show resolved Hide resolved
rclpy/rclpy/parameter.py Show resolved Hide resolved
@sloretz
Copy link
Contributor

sloretz commented Nov 16, 2023

@clalancette assigned for you to review 🧇

rclpy/rclpy/parameter.py Show resolved Hide resolved
rclpy/rclpy/parameter.py Outdated Show resolved Hide resolved
raise RuntimeError(f'YAML file is not a valid ROS parameter file for node {n}')
param_dict.update(value['ros__parameters'])
abs_name = _get_absolute_node_name(n)
ns, node_basename = abs_name.rsplit('/', 1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can skip doing this split at all if abs_name is in the param_file.keys(). That is, first check to see if the abs_name exists, and only if it doesn't, go ahead and do the split and look for the namespace.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can do this but this is the different behavior from rcl parameter yaml parser, to keep the same behavior i think we need to keep this. (that said, if this is not designed behavior, i think we need to fix rcl parameter yaml parser 1st with deprecation period, because this would break the user space... it is likely that user application starts loading parameter via ros2 command line argument.)

here is an example of ros2 command line argument.

root@tomoyafujita:~/ros2_ws/colcon_ws# cat params.yaml
/foobar/parameter_blackboard:
  ros__parameters:
    test-abs: foobar-absolute
/foobar:
  parameter_blackboard:
    ros__parameters:
      test-rel: foobar-relative
parameter_blackboard:
  ros__parameters:
    test-ns: no-namespace
/parameter_blackboard:
  ros__parameters:
    test-abs-ns: no-namespace-abs
/demo/parameter_blackboard:
  ros__parameters:
    test-abs: demo-absolute
/demo:
  parameter_blackboard:
    ros__parameters:
      test-rel: demo-relative
/**:
  ros__parameters:
    debug: true
root@tomoyafujita:~/ros2_ws/colcon_ws# ros2 run demo_nodes_cpp parameter_blackboard --ros-args --params-file params.yaml
[INFO] [1701194971.305007151] [parameter_blackboard]: Parameter blackboard node named '/parameter_blackboard' ready, and serving '9' parameters already!
root@tomoyafujita:~/ros2_ws/colcon_ws# ros2 param dump parameter_blackboard
/parameter_blackboard:
  ros__parameters:
    debug: true
    qos_overrides:
      /parameter_events:
        publisher:
          depth: 1000
          durability: volatile
          history: keep_last
          reliability: reliable
    start_type_description_service: true
    test-abs-ns: no-namespace-abs
    test-ns: no-namespace
    use_sim_time: false

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that we should maintain the same behavior as rcl_param_yaml_parser.

But I'd actually like to back-up a bit and look at parameter_dict_from_yaml_file. In particular, it has 1 required argument (parameter_file), and 3 optional arguments (use_wildcard, target_nodes, and namespace). I took a look through the core and through all of the code released into Rolling, and there are only a handful of non-test calls to this function:

We never use the target_nodes or the namespace arguments at all.

So my question is: should we just remove these parameters and the functionality around them? It would make this function much easier to understand and test. But maybe I am missing something, I'd like to hear your thoughts.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clalancette thanks!

ros2 param load <node> <parameter_file> case, i think we need to use target_nodes parameter to specify <node> with namespace from the yaml file. this is the original issue described ros2/ros2cli#863, wrong parameters are loaded to the <node>.

this can work at this moment, only because it depends on the bug which this PR trying to address.

we can see, https://github.com/ros2/demos/blob/a29e65623e8ace15b8253ab33f19f3d8165c3cfe/demo_nodes_py/demo_nodes_py/parameters/params.yaml#L1

parameter yaml file only has /param_test_target absolute node name, which should never be loaded to

https://github.com/ros2/demos/blob/a29e65623e8ace15b8253ab33f19f3d8165c3cfe/demo_nodes_py/demo_nodes_py/parameters/async_param_client.py#L28

parameter_blackboard, because yaml file does not have parameter_blackboard parameters at all.

ParameterClient created on remote node, so when loading we can validate the parameters from the yaml file what needs to be loaded against remote node.

We never use the target_nodes or the namespace arguments at all.

parameter_dict_from_yaml_file can just convert yaml into dictionary, and then caller side can do filtering based on those target_nodes, namespace and wildcard, but i do not think that is really useful. filtering logic should be implemented in parameter_dict_from_yaml_file. so that we can get what needs to be loaded based on target node names, namespace and wildcard. (i guess that was original intention for this parameter_dict_from_yaml_file but not tested very good...)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I see. Sorry, I was looking at the original code and not this PR when I wrote my previous comment.

Let me back up even further and ask a question. Is the intention for parameter_dict_from_yaml_file to be the Python equivalent of https://github.com/ros2/rcl/blob/246fd1de5c073e60573abb014a787f012a241419/rcl_yaml_param_parser/include/rcl_yaml_param_parser/parser.h#L96-L98 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure the original intention, sorry. i was just trying to fix ros2/ros2cli#863, and also check if it has the same behavior with rcl_yaml_param_parser. not sure if this answers your question though...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clalancette friendly ping. can we resolve this comment?

rclpy/rclpy/parameter.py Outdated Show resolved Hide resolved
rclpy/rclpy/parameter.py Show resolved Hide resolved
  Ref: ros2/ros2cli#863

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

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@fujitatomoya
Copy link
Collaborator Author

@clalancette can you check my comments above?

@fujitatomoya
Copy link
Collaborator Author

@clalancette friendly ping.

@fujitatomoya
Copy link
Collaborator Author

@fujitatomoya
Copy link
Collaborator Author

@clalancette friendly ping, please have a look at #1193 (comment) the only one left unresolved.

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 this pull request may close these issues.

4 participants