[WIP] Define parameter initialization mechanisms #242
[WIP] Define parameter initialization mechanisms #242
Conversation
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
|
@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 |
|
Just crossed my mind that we could switch to |
| - `value` is the value, in [YAML](https://yaml.org/spec/1.2/spec.html) syntax, to initialize the parameter with. | ||
| - `!=` is the parameter overriding assignment operator. | ||
|
|
||
| Note the use of the `!=` operator in lieu of the `:=` operator, to help disambiguate parameter initialization statements from name remapping rules. |
There was a problem hiding this comment.
Why not splitting the remapping rules from the parameters with a separator? e.g.:
ros2 run package executable asd:=bsd --params param1:=1 param2:=asd
ros2 run package executable asd:=bsd -- param1:=1 param2:=asdI slightly prefer the second option.
There was a problem hiding this comment.
I'm concerned somewhat about the use of ! as in my experience in shells it needs to be escaped and can be difficult to do so sometimes. I always avoid them in my commit messages because this 🤣.
There was a problem hiding this comment.
I like --params better than --, because you have to remember that we may be sharing the command line with users and -- is not very explicit.
There was a problem hiding this comment.
I know it would break everything, but if I were doing this from scratch I'd use from->to for remapping and key:=value for parameters. :/
There was a problem hiding this comment.
I guess that's similar to ~= for remapping, as you suggested. But I agree that may be too disruptive.
There was a problem hiding this comment.
Yeah. Also, won't from->to have issues with redirection in most shells?
There was a problem hiding this comment.
I recommend against using -- for this. It's very commonly used (e.g. by bash builtins, grep, etc.) to signify the end of CLI opions for the command, after which only positional parameters are accepted (e.g. it's very common for commands which run other commands in a subshell, as those other commands will have their own options). Seeing it used as suggested here would be pretty confusing.
|
So, it looks like we either do something really disruptive or we add CLI options to aid disambiguation e.g. |
|
If we add CLI options, it seems like we should make it a hard requirement to specify With the first command, the user is expected to know that no flag implies a remap. I'm okay with adding CLI options, but I'd choose being more disruptive to gain a better syntax. Is easier for my brain to parse, versus: 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. |
|
As per offline discussion with the entire @ros2/team, the main two contending approaches to disambiguate remapping rules and parameter assignments are: |
|
(1) To have separate operators e.g. An example of this is: ros2 run package executable foo~=bar foo:=bazOn 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 |
|
(2) To combine the current An example of this is: ros2 run package executable --remap foo:=bar --param foo:=bazOther, slight variations on this have to do with how many remapping rules and/or parameter assigments can be passed per CLI option e.g. 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. |
|
Anyone feel free to upvote either approach or suggest alternatives below. |
|
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 Could mistakes such as |
|
I prefer the verbosity of
I suspect if that could be reliably detected this wouldn't be a discussion, no? |
Yep, that's exactly how I feel about it. |
@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 |
That seems unfortunate beyond just this case. Might I then suggest the introduction of |
|
@kyrofa how do we pass the arguments to the executable in that example? |
IIUC, this arguments are going to be parsed at
I still prefer this option. IMO, |
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 |
|
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. |
Command-line arguments are parsed in I think the fear with I personally like the idea of |
|
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. |
|
Alright, perhaps we can tweak my proposal then. Think how Note the use of Either way, |
|
Imo polluting the namespace of possible arguments a user can use is high undesired. Therefore my first choice is the Options without the ROS prefix or especially one letter short options aren't acceptable imo. |
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. |
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. |
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? |
I agree, that would indeed be much cleaner 👍 |
|
I'm not a huge fan of just using 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 So with the previous examples in mind, would you do If we use an option to isolate the ROS options, then I'd prefer the syntax 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 |
I like this one. I was first going to say that I prefer
If we don't use something to separate node and ros arguments, I think we shouldn't use the short options
IMHO, it's too hard to read. But it's just a personal bias. |
I agree, I'm annoyed I suggested it 😛 . I updated the suggestion as well-- explicit is far more clear.
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 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
Yeah my vote remains |
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. |
|
I don't really see a need for I also think we should allow eliding of the @hidmic your example is long, but it wouldn't be much shorter if you get rid of Or: 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. |
|
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 |
|
What about only using URLs to distinguish remaps and params? Then we don't need worry about introducing options: |
That seems painfully verbose to me if you don't need to be that precise.
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. |
|
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 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? |
|
I also prefer the pair of operators for brevity. But, I am okay with the other options 🥁. |
|
@jacobperron did you mean to have A compromise might be that when using |
Yes. Updated my comment.
Agreed. We could consider adding the short-form alias later if there is a strong desire from the community. |
|
|
|
Closing in favor of #245. Thank you all for your input! |
|
This pull request has been mentioned on ROS Discourse. There might be relevant details there: https://discourse.ros.org/t/ros-2-cli-arguments-syntax-feedback-request/9942/1 |
Still a work in progress, as parameter files have not been duly documented.