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.

Sign up for GitHub

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

Jump to bottom

Support setting parameters atomically #727

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

Support setting parameters atomically #727

wants to merge 2 commits into from

Conversation

ihasdapie
Copy link
Member

This PR implements a --atomic flag for ros2 param set for setting parameters atomically from the command line.

@deepanshubansal01
Copy link
Contributor

deepanshubansal01 commented Jun 23, 2022 •
edited
Loading

I might be wrong but based on my understanding we can only set one parameter currently using ros2 param set and hence setting params in an atomic fashion will not make sense using ros2 param set.

We should expose the --atomic flag for setting params atomically using ros2 service call #721 since we can set multiple params using service. Additionally, we can expose the --atomic flag for setting params using ros2 param load as well.

@clalancette
Copy link
Contributor

We should expose the --atomic flag for setting params atomically using ros2 service call #721 since we can set multiple params using service.

The problem with that, though, is that --atomic there only makes sense for setting parameters, not other service calls. So I don't think the flag belongs on ros2 service call.

Additionally, we can expose the --atomic flag for setting params using ros2 param load as well.

This we should definitely do. The other thing we could do is to fix up ros2 param set so that it can take multiple parameters; at that point, it makes sense to add the --atomic flag.

@deepanshubansal01
Copy link
Contributor

deepanshubansal01 commented Jun 23, 2022 •
edited
Loading

We should expose the --atomic flag for setting params atomically using ros2 service call #721 since we can set multiple params using service.

The problem with that, though, is that --atomic there only makes sense for setting parameters, not other service calls. So I don't think the flag belongs on ros2 service call.

Additionally, we can expose the --atomic flag for setting params using ros2 param load as well.

This we should definitely do. The other thing we could do is to fix up ros2 param set so that it can take multiple parameters; at that point, it makes sense to add the --atomic flag.

I see, makes sense!

@ihasdapie
Copy link
Member Author

Right, I definitely wasn't thinking when I put this PR together 😅. Will update this PR to support multiple parameters & param load --atomic

@clalancette
Copy link
Contributor

Will update this PR to support multiple parameters & param load --atomic

I'll suggest you do two separate PRs; the first one to add multiple param support to set, and then a second one (maybe this one) to add the --atomic flag. They are logically independent things, so they should have different PRs.

@ihasdapie ihasdapie force-pushed the brianc/atomic_parameter_set branch from 5bd9a70 to 13a8d16 Compare June 23, 2022 21:57
@ihasdapie ihasdapie mentioned this pull request Jun 23, 2022
Open
@ihasdapie
Copy link
Member Author

ros2 param set with multiple parameters PR opened: #728

This PR is based off that PR (#728) and now implements --atomic for set and load verbs.

Diff: brianc/multiple_param_set...brianc/atomic_parameter_set

@audrow audrow changed the base branch from master to rolling June 28, 2022 14:23
@ihasdapie ihasdapie force-pushed the brianc/atomic_parameter_set branch from 13a8d16 to fb6efc1 Compare July 22, 2022 22:54
fujitatomoya
fujitatomoya reviewed Jul 28, 2022
View reviewed changes
ros2param/ros2param/api/__init__.py
@@ -50,6 +50,10 @@ def load_parameter_file(*, node, node_name, parameter_file, use_wildcard):
parameters = list(parameter_dict_from_yaml_file(parameter_file, use_wildcard).values())
rclpy.spin_until_future_complete(node, future)
response = future.result()
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think this call will raise exception if there is an actual exception.

ros2param/ros2param/api/__init__.py
future = client.load_parameter_file_atomically(parameter_file, use_wildcard)
parameters = list(parameter_dict_from_yaml_file(parameter_file, use_wildcard).values())
rclpy.spin_until_future_complete(node, future)
response = future.result()
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here

ros2param/ros2param/api/__init__.py
raise RuntimeError('Exception while calling service of node '
f'{node_name}: {future.exception()}')

if response.result.successful:
Copy link
Collaborator

Choose a reason for hiding this comment

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

if response.result.successful is false, user can know that loading parameter operation has been failed?

ros2param/ros2param/api/__init__.py
f'{node_name}: {future.exception()}')

if response.result.successful:
msg = 'Set parameters {} successful'.format(' '.join([i.name for i in parameters]))
Copy link
Collaborator

Choose a reason for hiding this comment

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

this whole operation is atomic, and user knows parameters as input, so i am not sure if we need print all parameters again here. probably this command prints the result only?

ros2param/ros2param/api/__init__.py
client.wait_for_services(timeout_sec=5.0)
future = client.set_parameters_atomically(parameters)
rclpy.spin_until_future_complete(node, future)
response = future.result()
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fujitatomoya fujitatomoya fujitatomoya left review comments

@jacobperron jacobperron Awaiting requested review from jacobperron

@deepanshubansal01 deepanshubansal01 Awaiting requested review from deepanshubansal01

@adityapande-1995 adityapande-1995 Awaiting requested review from adityapande-1995

Assignees

@ihasdapie ihasdapie

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support setting parameters atomically using ros2 service command line
4 participants
@ihasdapie @deepanshubansal01 @clalancette @fujitatomoya