Update remapping order of ns and node to not affect each other #299

iuhilnehc-ynos · 2020-09-22T02:27:39Z

I encountered an issue about remapping two nodes that originally have a different name to the same new name but with a different namespace.

ex.
there are two nodes in examples_rclcpp_minimal_composition, and we want to remap node names
publisher_node -> /publisher_different_ns/test_same_node
subscriber_node -> /subscriber_different_ns/test_same_node

we want to do remapping names like the following way,

$ ros2 run examples_rclcpp_minimal_composition composition_composed \
    --ros-args \
    -r publisher_node:__ns:=/publisher_different_ns \
    -r publisher_node:__node:=test_same_node \
    -r subscriber_node:__ns:=/subscriber_different_ns \
    -r subscriber_node:__node:=test_same_node

rather than using

$ ros2 run examples_rclcpp_minimal_composition composition_composed \
    --ros-args \
    -r test_same_node:__ns:=/publisher_different_ns \ 
    -r publisher_node:__node:=test_same_node \
    -r test_same_node:__ns:=/subscriber_different_ns \
    -r subscriber_node:__node:=test_same_node

, since the existing remapping order that parsing __node before __ns causes the latter command to use the same node_name prefix of __ns parameter for two nodes.

Related to ros2/rcl#806

Signed-off-by: Chen Lihui [email protected]

Signed-off-by: Chen Lihui <[email protected]>

iuhilnehc-ynos · 2020-09-22T02:29:45Z

I have no idea if we should apply for using the FQN prefix and remapping order. To push this commit is mainly for discussion.

fujitatomoya · 2020-09-23T07:48:05Z

articles/141_static_remapping.md

+- A user gives the rules `talker:__ns:=/my_namespace` then `/talker:__node:=foo`
+- The final node name is the default `talker` because the namespace remap is applied before the node name remap


what if user wants to remap talker into namespace A and B? I believe this is really common use case. I am not so convinced to change order. I think Node name -> Namespace would be understandable and usable for user.

Thanks for your response. @fujitatomoya

what if user wants to remap talker into namespace A and B?

Sorry, I don't catch it. How can we remap one node talker into two different namespaces A and B?

Node name -> Namespace would be understandable

It has a limitation.
Imageing there are two nodes publisher_node and subscriber_node in an application.
/publisher_node
/subscriber_node

I want to remap these two nodes as following (ros2/rcl#806),
/publisher_node -----> '/ns1/aaa'
/subscriber_node -----> '/ns2/aaa'

Maybe this case is suited for the real world.
/left_camera -----> /left/camera
/right_camera -----> /right/camera

It seems we can not remap them with current remapping order.

How can we remap one node talker into two different namespaces A and B?

bad example, ignore it... sorry😢

It seems we can not remap them with current remapping order.
/left_camera -----> /left/camera
/right_camera -----> /right/camera

I don't see this is because of remapping order, but implementation. I think we should have some kinda justification to change order to support two nodes with the same name but in different namespaces.

I don't see this is because of remapping order, but implementation.

That's right, users can set two nodes with Node("camera", "/left") and Node("camera", "/right") in implementation.
but we can't ask users to do this, if somebody just wants to use remapping, there is a limitation.

some kinda justification

Actually, the sample about the 'camera' above is the reason why I open this issue.

I found no discussion about why ros2 use this kind of remapping order(node name -> namespace), after checking ros2/rcl#217 and #181, it seems that ros2 follow the same implementation order to ros1. (If I remember correctly, ros1 doesn't have the remapping order for __name and __ns because it just use one node in an application. )

clalancette · 2020-10-07T17:44:59Z

@sloretz Since you wrote the original design document here, could you maybe weigh in on this design change?

articles/141_static_remapping.md

ivanpauno · 2020-10-15T13:37:24Z

With this proposal, how can the following example be solved:

remap /ns1/node /ns2/node to /new_node1 /new_node2 respectively.

?

This isn't currently possible, but it wasn't uncommon in ROS 1 I think.

iuhilnehc-ynos · 2020-10-16T01:54:20Z

remap /ns1/node /ns2/node to /new_node1 /new_node2 respectively.

That's interesting. I think this case should be also supported.

$ ros2 run package exec_name \
    --ros-args \
    -r /ns1/node:__ns:=/ \              # the FQN must use original value
    -r /ns1/node:__node:=new_node1 \    # the FQN must use original value
    -r /ns2/node:__ns:=/ \
    -r /ns2/node:__node:=new_node2

and remap publisher_node subscriber_node to /publisher/test_node /subscriber/test_node

$ ros2 run package exec_name \
    --ros-args \
    -r /publisher_node:__ns:=/publisher \
    -r /publisher_node:__node:=test_node \
    -r /subscriber_node:__ns:=/subscriber \
    -r /subscriber_node:__node:=test_node

after using the original value for prefix, it also breaks the backward compatibility.
ex. /ns1/node -> /new_node1

before

$ ros2 run package exec_name \
    --ros-args \
    -r new_node1:__ns:=/ \
    -r node:__node:=new_node1 \

after

$ ros2 run package exec_name \
    --ros-args \
    -r node:__ns:=/ \
    -r node:__node:=new_node1 \

Edit: If you agree with the above information, I'll update the design, otherwise I think it's time to close this issue.

ivanpauno · 2020-10-16T12:35:49Z

Edit: If you agree with the above information, I'll update the design, otherwise I think it's time to close this issue.

I think that a proposal that handle both cases above correctly would be acceptable.

Signed-off-by: Chen Lihui <[email protected]>

iuhilnehc-ynos · 2020-10-19T02:00:31Z

In this PR, I would not add the FQN prefix in it because it's different from the remapping order. If it's necessary, I'll create a new PR for it.

@ivanpauno
I have updated the design, please review it.

ivanpauno · 2020-10-19T13:15:21Z

@iuhilnehc-ynos I think the proposal is reasonable.

It would be great if you can edit the post description and include a motivation of the changes and examples of what the change enables (so future reviewers can understand the rationale of the change).

In this PR, I would not add the FQN prefix in it because it's different from the remapping order. If it's necessary, I'll create a new PR for it.

Combined with allowing FQN in prefixes, this allow even more use cases.
I don't think that both changes need to be done at the same time, but it would be good to explain how it works in the description.

sloretz · 2020-10-19T22:56:00Z

articles/141_static_remapping.md

@@ -340,7 +339,7 @@ Within each category, the rules are applied in the order in which the user gave

 - A node has name `talker`
 - A user gives the rules `talker:__ns:=/my_namespace` then `talker:__node:=foo`
- The final namespace is the default `/` because the node name remap is applied before the namespace remap
+- The final node name is `foo` because the namespace and node name remap are applied without affecting each other


What's the final fully qualified name? Current design says it would be /foo because the node name remapping happened first. Is this PR proposing the final output would be /my_namespace/foo?

What's the final fully qualified name?

It's /my_namespace/foo which you had already answered. I should make the document clear, I'll update it.

iuhilnehc-ynos · 2020-10-20T02:06:06Z

@ivanpauno

I don't think that both changes need to be done at the same time, but it would be good to explain how it works in the description.

Thanks for your understanding. I have updated the description.

Signed-off-by: Chen Lihui <[email protected]>

ivanpauno · 2020-10-20T12:56:52Z

articles/141_static_remapping.md

@@ -324,8 +324,7 @@ The replacement must be a single token which will become the node's new name.

 Remapping rules are applied in the following order:

-1. Node name remapping
-1. Namespace remapping
+1. Namespace or Node name remapping


so now name are namespace rules are applied in the order they appear, right?

It would be great to make that clear here

order they appear

This is not the case, they order doesn't matter.
Their prefix is always the node name before remapping, maybe that can be made more clear.

This is not the case, they order doesn't matter.

They're still ordered within a rule, right? If I give talker:__node:=foo and talker:__node:=bar, then is the final name is foo?

@ivanpauno

name and namespace rules are applied in the order they appear, right?

they order doesn't matter

the order of name and namespace rules doesn't matter.
I think users like to understand some rules by referring to an example description.

A user gives the rules talker:__ns:=/my_namespace then talker:__node:=foo, or vice versa

@sloretz

They're still ordered within a rule, right?

I supposed you mean the order of multiple items about __node or __ns, the answer is yes.

If I give talker:__node:=foo and talker:__node:=bar, then is the final name is foo?

Yes.
Actually, the example description already showed messages,

talker's namespace is /foo because that rule was given first
though the above example uses talker:__ns:=/foo and __ns:=/bar

About this kind of information, I'll not update it in this PR.

sloretz · 2020-10-20T21:43:53Z

articles/141_static_remapping.md

- A user gives the rules `talker:__ns:=/my_namespace` then `talker:__node:=foo`
- The final namespace is the default `/` because the node name remap is applied before the namespace remap
+- A user gives the rules `talker:__ns:=/my_namespace` then `talker:__node:=foo`, or vice versa
+- The final fully qualified name is `/my_namespace/foo` because the namespace and node name remap are applied without affecting each other


the namespace and node name remap are applied without affecting each other

I would change this wording to: the namespace and node name remap rules both match against the orignal node name before remappings are applied.

Thank you. I'll update it later.

sloretz · 2020-10-20T21:46:14Z

Implementation considerations - it seems possible to implement this by passing the original name arg to rcl_remap_node_namespace() - which could be done by calling rcl_remap_node_namespace() before rcl_remap_node_name() so the name variable is unchanged.

https://github.com/ros2/rcl/blob/e9c87e279bf7509c940b87fd4d6c5dff301bc735/rcl/src/rcl/node.c#L220-L240

sloretz

The idea itself seems good to me. I couldn't think of any use cases it breaks (doesn't seem to break any in the design doc), and it seems to enable a new use case of remapping nodes to have the same name but different FQN.

It looks a bit weird when swapping node names,

 -r alice:__node:=bob
 -r bob:__node:=alice
 -r alice:__ns:=/land
 -r bob:__ns:=/wonder

Before this change results in /wonder/bob and /land/alice while with this proposal it results in /wonder/alice and /land/bob.

But then it doesn't look so weird if I order them this way

 -r alice:__node:=bob
 -r alice:__ns:=/land
 -r bob:__node:=alice
 -r bob:__ns:=/wonder

I think it should be made clearer in the Match Part of a Rule section that whether nodename: refers to the pre-remaping name or the post-remapping name depends on the rule. For __ns and __node it's the pre-remapping node name that gets matched against, while for topic and service rules it's the post-remapping node name that matters.

iuhilnehc-ynos · 2020-10-21T04:47:14Z

But then it doesn't look so weird if I order them this way

Thanks for your example.

I think it should be made clearer in the Match Part of a Rule section that whether nodename: refers to the pre-remaping name or the post-remapping name depends on the rule.

But there already exist information about this pre-remapping and post-remapping in Order of Applying Remapping Rules
Within each category, the rules are applied in the order in which the user gave them.
If adding this kind of information in the Match Part of a Rule section, it seems a little duplicated.

Co-Authored-By: Shane Loretz <[email protected]> Signed-off-by: Chen Lihui <[email protected]>

sloretz · 2020-10-23T20:48:46Z

But there already exist information about this pre-remapping and post-remapping in Order of Applying Remapping Rules
Within each category, the rules are applied in the order in which the user gave them.
If adding this kind of information in the Match Part of a Rule section, it seems a little duplicated.

If not the match rule section, how about another example showing node name remapping and topic name remapping together? I think it'd be helpful because this case looks a little confusing:

 -r alice:__node:=bob
 -r alice:foo:=baz
 -r bob:foo:=bar

Signed-off-by: Chen Lihui <[email protected]>

ivanpauno · 2020-10-27T17:42:58Z

Hi @iuhilnehc-ynos, here some feedback from the ros2 team meeting:

Was this proposal the result of a real use case? i.e. were you actually trying to remap /node1, /node2 to /ns1/node and /ns2/node in a real application?
To avoid a breaking change (that in many cases will be a silent breakage), could this option been enabled with a prefix? e.g.:

ros2 run package_name exec_name --ros-args --inorder-name-ns \
    -r publisher_node:__ns:=/publisher_different_ns \
    -r publisher_node:__node:=test_same_node \
    -r subscriber_node:__ns:=/subscriber_different_ns \
    -r subscriber_node:__node:=test_same_node

In that way, we avoid breaking previous code (this use case seems to be not too common, so making it more verbose wouldn't be a big issue).

Bikeshedding: --inorder-name-ns could have a better name.

iuhilnehc-ynos · 2020-10-28T02:50:57Z

Was this proposal the result of a real use case? i.e. were you actually trying to remap /node1, /node2 to /ns1/node and /ns2/node in a real application?

No. These cases can be avoided in users' applications.

To avoid a breaking change (that in many cases will be a silent breakage), could this option been enabled with a prefix?

Yes, adding this option can avoid the breaking change, but it seems not kindly for users because there are two kinds of rules.

If you don't mind, I want to close this issue.

fujitatomoya · 2020-10-28T03:04:35Z

@iuhilnehc-ynos
CC: @ivanpauno

--inorder-name-ns sounds okay to me.
maybe there would be some corner cases, but it would be really rare.
can avoid limitation with adding --inorder-name-ns (at least user can what they want to do.)

I do not have any strong justification to push this design either.

ivanpauno · 2020-10-28T13:25:38Z

Yes, adding this option can avoid the breaking change, but it seems not kindly for users because there are two kinds of rules.

Agreed, but "silent breakage" (the one that doesn't make your build fail and makes your program behave different than before) is a big issue for existing users.

If you don't mind, I want to close this issue.

Sounds good to me.
If the current order of remapping rules is a problem for some use cases, adding the --inorder-name-ns option would be the best path forward.
It's not super nice to have two different orders for remapping rules, but this is a corner case that most users won't run into.

If you have any other alternatives to avoid breaking pre-existing code, that also would be acceptable.

Thanks for the proposal @iuhilnehc-ynos !

ivanpauno · 2020-10-29T18:12:52Z

Based on the previous comments, I'm going to close this.
If there is an intention to push a non-breaking alternative of the proposal forward feel free to reopen.

Update with nodename prefix to support FQN and remapping order

873ada9

Signed-off-by: Chen Lihui <[email protected]>

iuhilnehc-ynos mentioned this pull request Sep 22, 2020

Name remapping specific to a fully-qualified node's name ros2/rcl#296

Open

fujitatomoya reviewed Sep 23, 2020

View reviewed changes

iuhilnehc-ynos changed the title ~~Update with nodename prefix to support FQN and remapping order~~ Update for nodename prefix to support FQN and remapping order Sep 25, 2020

iuhilnehc-ynos changed the title ~~Update for nodename prefix to support FQN and remapping order~~ Update for nodename prefix to support FQN and remapping order of __ns and __node Sep 25, 2020

ivanpauno reviewed Oct 14, 2020

View reviewed changes

articles/141_static_remapping.md Outdated Show resolved Hide resolved

update namespace and node name remapping to not affect each other

93a1b51

Signed-off-by: Chen Lihui <[email protected]>

iuhilnehc-ynos changed the title ~~Update for nodename prefix to support FQN and remapping order of __ns and __node~~ Update remapping order of __ns and __node to not affect each other Oct 19, 2020

ivanpauno requested a review from sloretz October 19, 2020 13:15

ivanpauno added the enhancement New feature or request label Oct 19, 2020

sloretz reviewed Oct 19, 2020

View reviewed changes

make the example description more clear

520d649

Signed-off-by: Chen Lihui <[email protected]>

ivanpauno reviewed Oct 20, 2020

View reviewed changes

sloretz reviewed Oct 20, 2020

View reviewed changes

sloretz requested changes Oct 20, 2020

View reviewed changes

update with better wording

31ca246

Co-Authored-By: Shane Loretz <[email protected]> Signed-off-by: Chen Lihui <[email protected]>

ivanpauno mentioned this pull request Oct 22, 2020

Name remapping specific to a fully-qualified node's name ros2/rcl#807

Open

Add an example for mixed rules

33770a1

Signed-off-by: Chen Lihui <[email protected]>

ivanpauno closed this Oct 29, 2020

This was referenced Oct 29, 2020

Remap two nodes of one executable file with same node name in different namespaces ros2/rcl#806

Open

Update for remapping order of __ns and __node to not affect each other ros2/rcl#837

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update remapping order of ns and node to not affect each other #299

Update remapping order of ns and node to not affect each other #299

iuhilnehc-ynos commented Sep 22, 2020 •

edited

Loading

iuhilnehc-ynos commented Sep 22, 2020

fujitatomoya Sep 23, 2020

iuhilnehc-ynos Sep 23, 2020

iuhilnehc-ynos Sep 23, 2020 •

edited

Loading

fujitatomoya Sep 23, 2020

iuhilnehc-ynos Sep 23, 2020

clalancette commented Oct 7, 2020

ivanpauno commented Oct 15, 2020

iuhilnehc-ynos commented Oct 16, 2020 •

edited

Loading

ivanpauno commented Oct 16, 2020

iuhilnehc-ynos commented Oct 19, 2020

ivanpauno commented Oct 19, 2020

sloretz Oct 19, 2020

iuhilnehc-ynos Oct 20, 2020

iuhilnehc-ynos commented Oct 20, 2020

ivanpauno Oct 20, 2020

ivanpauno Oct 20, 2020

sloretz Oct 20, 2020

iuhilnehc-ynos Oct 21, 2020

sloretz Oct 20, 2020 •

edited

Loading

iuhilnehc-ynos Oct 21, 2020

sloretz commented Oct 20, 2020

sloretz left a comment •

edited

Loading

iuhilnehc-ynos commented Oct 21, 2020 •

edited

Loading

sloretz commented Oct 23, 2020

ivanpauno commented Oct 27, 2020

iuhilnehc-ynos commented Oct 28, 2020

fujitatomoya commented Oct 28, 2020

ivanpauno commented Oct 28, 2020

ivanpauno commented Oct 29, 2020

		- A user gives the rules `talker:__ns:=/my_namespace` then `/talker:__node:=foo`
		- The final node name is the default `talker` because the namespace remap is applied before the node name remap

Update remapping order of __ns and __node to not affect each other #299

Update remapping order of __ns and __node to not affect each other #299

Conversation

iuhilnehc-ynos commented Sep 22, 2020 • edited Loading

iuhilnehc-ynos commented Sep 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iuhilnehc-ynos Sep 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clalancette commented Oct 7, 2020

ivanpauno commented Oct 15, 2020

iuhilnehc-ynos commented Oct 16, 2020 • edited Loading

ivanpauno commented Oct 16, 2020

iuhilnehc-ynos commented Oct 19, 2020

ivanpauno commented Oct 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iuhilnehc-ynos commented Oct 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sloretz Oct 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sloretz commented Oct 20, 2020

sloretz left a comment • edited Loading

Choose a reason for hiding this comment

iuhilnehc-ynos commented Oct 21, 2020 • edited Loading

sloretz commented Oct 23, 2020

ivanpauno commented Oct 27, 2020

iuhilnehc-ynos commented Oct 28, 2020

fujitatomoya commented Oct 28, 2020

ivanpauno commented Oct 28, 2020

ivanpauno commented Oct 29, 2020

Update remapping order of ns and node to not affect each other #299

Update remapping order of ns and node to not affect each other #299

iuhilnehc-ynos commented Sep 22, 2020 •

edited

Loading

iuhilnehc-ynos Sep 23, 2020 •

edited

Loading

iuhilnehc-ynos commented Oct 16, 2020 •

edited

Loading

sloretz Oct 20, 2020 •

edited

Loading

sloretz left a comment •

edited

Loading

iuhilnehc-ynos commented Oct 21, 2020 •

edited

Loading