ros2 / design

Design documentation for ROS 2.0 effort
http://design.ros2.org/
Apache License 2.0
215 stars 194 forks source link

[WIP] Define parameter initialization mechanisms #242

Closed hidmic closed 4 years ago

hidmic commented 5 years ago

Still a work in progress, as parameter files have not been duly documented.

hidmic commented 5 years ago

@wjwwood parameter assignment CLI syntax isn't particularly bright, but I couldn't find any other operator that resonated well and was not already taken by some shell. Also, I did not use := on purpose, as it makes it hard for rcl to tell remapping rules from parameter assignment when incomplete addressing syntax is used (e.g. that:=this) and it would preclude remapping rules for parameters (if we ever want that).

hidmic commented 5 years ago

Just crossed my mind that we could switch to ~= for remapping rules (that somewhat resembles the matching that undergoes when remapping if one thinks of bash regex matching operator) and := for parameter assignment. Though it's absolutely disruptive coming from a ROS 1 setting.

hidmic commented 5 years ago

So, it looks like we either do something really disruptive or we add CLI options to aid disambiguation e.g. --param/-p for parameters and --remap/-r for remapping rules (just to keep things consistent). What does the @ros2/team think about this?

jacobperron commented 5 years ago

If we add CLI options, it seems like we should make it a hard requirement to specify --param or --remap to remove ambiguity. E.g. given the following two commands:

ros2 run package executable foo:=bar --param foo:=baz
ros2 run package executable --remap foo:=bar --param foo:=baz

With the first command, the user is expected to know that no flag implies a remap. With the second command, it is more explicit what is happening and easier to understand IMO.

I'm okay with adding CLI options, but I'd choose being more disruptive to gain a better syntax.

ros2 run package executable foo~=bar foo:=baz

Is easier for my brain to parse, versus:

ros2 run package executable --remap foo:=bar --param foo:=baz

I think the expectation is that there are differences transitioning from ROS 1 to ROS 2, so there's no better time to break syntax IMO.

hidmic commented 4 years ago

As per offline discussion with the entire @ros2/team, the main two contending approaches to disambiguate remapping rules and parameter assignments are:

hidmic commented 4 years ago

(1) To have separate operators e.g. ~= for remapping rules and := for parameter assignment.

An example of this is:

  ros2 run package executable foo~=bar foo:=baz

On one hand, this keeps the syntax short and simple as it used to be for ROS 1. There's no risk of it interacting with user defined command line options either.

On the other hand, it is disruptive coming from a ROS 1 setting where := always applies. To be brief, it relies on a user knowing both operators and their side effects. Also, with a minimum Hamming distance of 1, it may be prone to errors e.g. try grok the difference between foo~=bar foo:=baz and foo:=bar foo~=baz.

hidmic commented 4 years ago

(2) To combine the current := operator with a CLI option prefix e.g. --remap for name remapping and --param for parameter assignment.

An example of this is:

  ros2 run package executable --remap foo:=bar --param foo:=baz

Other, slight variations on this have to do with how many remapping rules and/or parameter assigments can be passed per CLI option e.g. --remap foo:=bar --remap that:=this vs. --remaps foo:=bar that:=this and whether := or = is used e.g. --param foo:= baz vs --param foo=baz.

This makes it decidedly more verbose but explicit and less likely to inadvertently result in undesired behavior. It will interact with user defined command line options though.

hidmic commented 4 years ago

Anyone feel free to upvote either approach or suggest alternatives below.

artivis commented 4 years ago

I agree with @jacobperron that transitioning for ROS 1 to ROS 2 people expect changes. But the change of the remapping rule's operator to ~= is subtiles enough to get quickly used to it contrarily to --remap/--param, which are too verbose to my taste.

Could mistakes such as foo~=bar foo:=baz <--> foo:=bar foo~=baz be caught by the cli tool and produce an informative error message The operator '~=' is used for topic remapping ! It cannot be applied to a parameter. ???

kyrofa commented 4 years ago

I prefer the verbosity of --remap/-r and --param/-p. I'll never remember which is which if we use different variations of = (~= and whatnot), but the extra flag makes it pretty obvious.

Could mistakes such as foo~=bar foo:=baz <--> foo:=bar foo~=baz be caught by the cli tool [...] ?

I suspect if that could be reliably detected this wouldn't be a discussion, no?

clalancette commented 4 years ago

I prefer the verbosity of --remap/-r and --param/-p. I'll never remember which is which if we use different variations of = (~= and whatnot), but the extra flag makes it pretty obvious.

Yep, that's exactly how I feel about it.

dirk-thomas commented 4 years ago

I prefer the verbosity of --remap/-r and --param/-p.

@kyrofa Just to clarify: you are aware of the trade off that these options would share the same namespace as custom command line options of any node? So no node a user writes could use custom arguments named --remap, -r, --param, and -p anymore since those would conflict.

kyrofa commented 4 years ago

So no node a user writes could use custom arguments named --remap, -r, --param, and -p anymore since those would conflict.

That seems unfortunate beyond just this case. Might I then suggest the introduction of -- as "end of options", whereby anything that follows it is positional and intended for the node? That's what various other tools do for this exact reason (e.g. grep, lxd, bash built-ins, etc.). It could look like this:

$ ros2 run --remap foo:=bar --param foo:=baz -- package executable --param my-unrelated-thing
wjwwood commented 4 years ago

@kyrofa how do we pass the arguments to the executable in that example?

ivanpauno commented 4 years ago

So no node a user writes could use custom arguments named --remap, -r, --param, and -p anymore since those would conflict.

That seems unfortunate beyond just this case. Might I then suggest the introduction of -- as "end of options", whereby anything that follows it is positional and intended for the node? That's what various other tools do for this exact reason (e.g. grep, lxd, bash built-ins, etc.). It could look like this:

$ ros2 run -r foo:=bar -p foo:=baz -- package executable -p my-option

IIUC, this arguments are going to be parsed at rcl level, the ros2run command line tool isn't parsing them. You can just pass the same parameters directly to the executable.

I prefer the verbosity of --remap/-r and --param/-p. I'll never remember which is which if we use different variations of = (~= and whatnot), but the extra flag makes it pretty obvious.

I still prefer this option. IMO, ~= and := is too difficult to read.

kyrofa commented 4 years ago

IIUC, this parameter are going to be parsed at rcl level, the ros2run command line tool isn't parsing them.

Oh of course, I'm sorry, that changes things. How do ROS 2 nodes typically support their own CLI params today, given that some of them aren't destined to be interpreted by the node itself? I've always just used actual ROS params myself. Is this even an issue (and if it is, does the introduction of --params etc. make it appreciably worse)?

kyrofa commented 4 years ago

I guess what I'm saying is that, given that nodes handle their own argv but RCL needs to claim at least a portion of it, there's no way to avoid this clash without a restructure. Perhaps we should be encouraging folks to just use ROS parameters instead of parsing argv on their own, and let ROS own argv.

wjwwood commented 4 years ago

How do ROS 2 nodes typically support their own CLI params today, given that some of them aren't destined to be interpreted by the node itself? I've always just used actual ROS params myself. Is this even an issue (and if it is, does the introduction of --params etc. make it appreciably worse)?

Command-line arguments are parsed in rclcpp::init(), and non-ROS arguments are ignored. There are a few methods that will extract the ROS arguments, leaving the non-ROS arguments behind, so the user can use the rest, e.g. rclcpp::remove_ros_arguments (http://docs.ros2.org/dashing/api/rclcpp/namespacerclcpp.html#ad4968a767791995c011c57be22cd40c8). Then the user can use the rest for what ever they want.

I think the fear with --param/--remap is that it is far more likely to collide than foo:=bar, which I agree with, especially if we allow -p/-r for brevity. However, I don't know if that is enough to exclude it as the solution, because I also think := and ~= are probably error prone.


I personally like the idea of --ros-remap and --ros-param, because I don't mind typing or tab completing that in exchange for less collision and more readability, though I recognize others may not like it.

wjwwood commented 4 years ago

We'd certainly recommend users not process their own command line arguments unless they really need them, preferring parameters instead, but the fact is some people will want or need them, so we want to be as kind to them as possible too.

kyrofa commented 4 years ago

Alright, perhaps we can tweak my proposal then. Think how --cmake-args works in catkin:

$ ros2 run package executable --remap foo:=bar --param foo:=baz  --node-args --param my-unrelated-thing

Note the use of --node-args to separate args meant for ROS versus args meant for the node. We could swap that around if it was more clear to special-class the ROS args instead (although I prefer the other way, keeping well-defined ROS args first and allowing the potentially infinite node args last):

$ ros2 run package executable --param my-unrelated-thing --ros-args --remap foo:=bar --param foo:=baz

Either way, remove_ros_arguments can take that into account and leave even clashing options in place to be consumed by the node. Thoughts?

dirk-thomas commented 4 years ago

Imo polluting the namespace of possible arguments a user can use is high undesired. Therefore my first choice is the := / ~= syntax. As a secondary I would consider options starting with --ros-.

Options without the ROS prefix or especially one letter short options aren't acceptable imo.

kyrofa commented 4 years ago

Imo polluting the namespace of possible arguments a user can use is high undesired.

I don't disagree, which is why I suggested a method that works around that issue. Even without it, though, it seems wise to be careful designing around a use-case that is discouraged.

dirk-thomas commented 4 years ago

... given that processing one's own CLI args isn't recommended, it seems wise to be careful designing around a use-case that is discouraged.

Imo parsing your own command line arguments is by no means discouraged. It is a standard pattern and telling people they shouldn't use it (and restrict what they can do with it in ROS) seems like an anti pattern to me.

kyrofa commented 4 years ago

It is a standard pattern and telling people they shouldn't use it (and restrict what they can do with it in ROS) seems like an anti pattern to me.

I actually don't disagree with that either, haha! But this problem isn't new-- we're in this situation today. RCL shares CLI args with the node today, which already makes implementing one's own CLI arguments different than how it's done outside of ROS. Shying away from adding more options to RCL just seems like kicking the can down the road here, and sacrificing some usability for it.

The only way to get away from the can is to actually work around the core issue, which is that it's impossible today to separate the argument namespaces for RCL from that for the node itself. I proposed a way to do exactly that-- what do you think about it?

dirk-thomas commented 4 years ago

I proposed a way to do exactly that-- what do you think about it?

I agree, that would indeed be much cleaner :+1:

wjwwood commented 4 years ago

I'm not a huge fan of just using -- because it's not obvious without documentation which side is the ROS stuff and which side is your stuff.

If you say the ROS arguments should come first, because "this is a ROS executable", then that would make sense, but I'm not sure what makes a ROS executable versus a program that might use ROS only a bit or conditionally. Like our demos obviously having exec <ros arg> -- <extra args> makes sense. But for rviz that would be annoying, like rviz -d my_conf.rviz would become rviz -- -d my_conf.rviz (or rviz --node-args -d my_conf.rviz based on another of the suggestions), or even just rviz -h would no longer work right. And what if gazebo wanted to use ROS conditionally (depending on which models were loaded), but also has some use of their own for --, it could get sticky.

So with the previous examples in mind, would you do exec <exec args> -- <ros args>? I think this is better for most cases, but still might collide with some applications that also use --.

If we use an option to isolate the ROS options, then I'd prefer the syntax exec <exec_arg> --ros-args <ros_arg1> <ros_arg2> --, where we have an explicit start flag (e.g. --ros-args) and an explicit end flag (--), which could be elided if nothing follows. You have a lot of different patterns then rviz -h, rviz -d my_conf.rviz --ros-args __node:=my_rviz, gazebo --world ... --ros-args __log_level:=debug -- --verbose -- --something-else, rviz --ros-args __node:=my_rviz -- --stereo --ros-args --remap foo:=bar -- (multiple occurrences is useful when there are user defined ros arguments as well as ones passed by something like launch), and so on. Also the algorithm for extracting the ROS arguments is simple, even simpler than right now.


However, I will point out that this feature (separating the ros arguments) may affect our decision on how to specify remappings separately from parameter overrides, it doesn't make the decision for us. So what do you guys want along side the separated args?

I like something like exec --ros-args -r foo:=bar --remap ping:=pong -p tattuqoltuae:=42 --param fav_color:=green -- --extra-app-arg, using --remap/-r and --param/-p within the explicit tag.

ivanpauno commented 4 years ago
$ ros2 run package executable --param my-unrelated-thing --ros-args --remap foo:=bar --param foo:=baz

I like this one. I was first going to say that I prefer --node-args options, but the comment above changed my opinion. Accepting -- after --ros-args for finishing with ros specific parameters and continuing with others is also a good idea.

I think the fear with --param/--remap is that it is far more likely to collide than foo:=bar, which I agree with, especially if we allow -p/-r for brevity. However, I don't know if that is enough to exclude it as the solution, because I also think := and ~= are probably error prone.

I personally like the idea of --ros-remap and --ros-param, because I don't mind typing or tab completing that in exchange for less collision and more readability, though I recognize others may not like it.

If we don't use something to separate node and ros arguments, I think we shouldn't use the short options -p/-r, as the probability of collisions is really high. --ros-param and --ros-remap are ok, but I slightly prefer --param and --remap (I understand they can collide with user arguments, but for me is acceptable).

Imo polluting the namespace of possible arguments a user can use is high undesired. Therefore my first choice is the := / ~= syntax.

IMHO, it's too hard to read. But it's just a personal bias.

kyrofa commented 4 years ago

I'm not a huge fan of just using -- because it's not obvious without documentation which side is the ROS stuff and which side is your stuff.

I agree, I'm annoyed I suggested it :stuck_out_tongue: . I updated the suggestion as well-- explicit is far more clear.

If we use an option to isolate the ROS options, then I'd prefer the syntax exec --ros-args --, where we have an explicit start flag (e.g. --ros-args) and an explicit end flag (--), which could be elided if nothing follows.

Interesting idea. It is more flexible than requiring one complete set to be ordered before the other complete set, although I still dislike the use of -- that departs from the convention established elsewhere. It's painfully verbose to use e.g. --end-ros-args or something, but I think I lean that direction.

Before we continue down this road though, you mentioned that this simplifies the algorithm for extracting ROS arguments. We probably want to consider ADDING to the algorithm rather than replacing it with this. Specifically, I'm not sure we want to force the common case of ros2 run package node -r foo:=bar into ros2 run package node --ros-args -r foo:=bar --end-ros-args. I think we should cater to the common case, and by default they should continue being in the same namespace as the node args. Then we can SUPPORT --ros-args/--end-ros-args in case a given node handles its own CLI arguments, in which case the caller will know and can use the appropriate flags to separate them.


However, I will point out that this feature (separating the ros arguments) may affect our decision on how to specify remappings separately from parameter overrides, it doesn't make the decision for us. So what do you guys want along side the separated args?

I like something like exec --ros-args -r foo:=bar --remap ping:=pong -p tattuqoltuae:=42 --param fav_color:=green -- --extra-app-arg, using --remap/-r and --param/-p within the explicit tag.

Yeah my vote remains --remap/-r and --param/-p, ignoring the kinks to be worked out with separating the args.

hidmic commented 4 years ago

I think by default they should continue being in the same namespace as the node args, but that we can SUPPORT --ros-args/--end-ros-args in case a given node handles its own CLI arguments

I agree. As it stands, though explicit and hard to get wrong, having to type:

ros2 run package executable --ros-args --param rosparam://my/node/some/param:=2 --end-ros-args ...

to unambiguously set a parameter for a node is painfully verbose.

wjwwood commented 4 years ago

I don't really see a need for --end-ros-args over --, to be honest. As long as they need to be matched with each other, removing all the ros args is easily done (even without depending on a ros library to do so) and it won't collide with others that use -- already, e.g. it should be fine to use it with lldb which uses -- to separate lldb args and application args.

I also think we should allow eliding of the -- if it is the end of the arguments.


@hidmic your example is long, but it wouldn't be much shorter if you get rid of --ros-args, most of the complexity comes from the rosparam url, which could also be shortened. Assuming we do as I suggested and avoid --end-ros-args and also allow it to be elided, your example could become:

ros2 run package executable --ros-args --param /my.node/some/param:=2 -- ...

Or:

ros2 run package executable --ros-args --param /my.node/some/param:=2

Note I'm using the proposed parameter addressing syntax (as I understand it), as implemented right now it would have to be more like the "node targeted" remapping arguments, e.g. --param /my/node:/some/param:=2.

wjwwood commented 4 years ago

Personally, I'm not in favor of supporting both, mixing and isolated ros arguments. If we continue to support mixing arguments then the developer of the application can instead ask their users to "put your ROS arguments between --ros-args and --" and then she can extract them and pass only that to rclcpp::init. There's no need for us to do it for them at that point.

jacobperron commented 4 years ago

What about only using URLs to distinguish remaps and params? Then we don't need worry about introducing options:

ros2 run package executable rosparam://my.node/some/param:=2 rosremap:/foo:=bar
hidmic commented 4 years ago

What about only using URLs to distinguish remaps and params?

That seems painfully verbose to me if you don't need to be that precise.

@hidmic your example is long, but it wouldn't be much shorter if you get rid of --ros-args, most of the complexity comes from the rosparam url, which could also be shortened.

I know, I took it far enough to show how bad it can get. But yes, with enough elision rules it may not be that annoying.

hidmic commented 4 years ago

Alright, it seems the consensus goes towards explicit options to tell remapping rules and parameter assignments apart, plus command line arguments' namespacing to minimize pollution, as in @wjwwood sample:

I like something like exec --ros-args -r foo:=bar --remap ping:=pong -p tattuqoltuae:=42 --param fav_color:=green -- --extra-app-arg, using --remap/-r and --param/-p within the explicit tag.

I personally would've preferred having a pair of operators, just for the sake of brevity, but this doesn't look bad at all. @jacobperron @dirk-thomas @artivis do you agree?

jacobperron commented 4 years ago

I also prefer the pair of operators for brevity. But, I am okay with the other options :drum:. Specifically, the --ros-args option: exec --ros-args -r foo:=bar -p tattuqoltuae:=42 -- --extra-app-arg

wjwwood commented 4 years ago

@jacobperron did you mean to have foo:bar rather than foo:=bar there?


A compromise might be that when using ros2 run that ros2 run pkg exec foo~=bar --extra-app-arg tattuqoltuae:=42 could be automatically translated into exec --ros-args -r foo:=bar -p tattuqoltuae:=42 -- --extra-app-arg. We could easily handle the extra complexity in ros2 run since it would be in a Python tool, but it keeps the parsing of the arguments simple in the target application which may be C++, Python, or something else. We would do this purely for the convenience, but on the other hand it has some draw backs (like argument reordering, inconsistent behavior, and the aforementioned issues with the similar operators).

jacobperron commented 4 years ago

@jacobperron did you mean to have foo:bar rather than foo:=bar there?

Yes. Updated my comment.

A compromise might be that when using ros2 run that ros2 run pkg exec foo~=bar --extra-app-arg tattuqoltuae:=42 could be automatically translated into exec --ros-args -r foo:=bar -p tattuqoltuae:=42 -- --extra-app-arg

Agreed. We could consider adding the short-form alias later if there is a strong desire from the community.

hidmic commented 4 years ago

~Alright, I'll update this PR to reflect the outcome of this discussion.~ I'll just drop this branch, nothing here will be kept.

hidmic commented 4 years ago

Closing in favor of #245. Thank you all for your input!