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

roslaunch: add support for "param" tag in substitution args #723

Open
mishrasubhransu opened this issue Dec 30, 2015 · 21 comments
Open

roslaunch: add support for "param" tag in substitution args #723

mishrasubhransu opened this issue Dec 30, 2015 · 21 comments

Comments

@mishrasubhransu
Copy link

Say the contents of a file tf_stp_area1.yaml were

world2map: "127 108 0 0 0 0.73454 0.67855 world map"

I feel it would be very convenient to be able to do the following in a launch file

<launch>
  <arg name="location" value="area1"/>
  <rosparam command="load" file="$(find my_pkg)/params/tf_stp_$(arg location).yaml" />
  <node pkg="tf2_ros" type="static_transform_publisher" name="world_to_map" args="$(param world2map)" />
</launch>
@hkorre
Copy link

hkorre commented Nov 8, 2016

Was just looking for a way to do this exact thing today.

@doisyg
Copy link

doisyg commented Feb 7, 2017

same here

@tahsinkose
Copy link
Contributor

Would lift the redundant work of duplicating numbers and strings into launch file. This should also work for <arg> substitution.

@eva-dierkes
Copy link

I'm searching for the same solution...

@tahsinkose
Copy link
Contributor

I have implemented it. If you don't want to wait for official merge, you can directly clone my own fork and try it. I couldn't do an extensive testing, so I'm waiting feedbacks from you for any type of problems.

@rickstaa
Copy link

@tahsinkose Thanks for implementing this! I think this would be an amazing feature to have.

@tahsinkose
Copy link
Contributor

I'm glad that it helps for you @rickstaa . Unfortunately, I don't have the time to re-inspect the PR and have it merged into the master. I know that @dirk-thomas and other maintainers have shifted their focus on ROS 2 development, which has a Python-based launch system, therefore they may not be as helpful as they can for the ROS 1.

If you like you can take on this simple feature and have it pass the tests.

@rickstaa
Copy link

rickstaa commented Sep 30, 2019

@tahsinkose Ah thanks I understand. Thanks again for the nice pull request!

@doisyg
Copy link

doisyg commented Oct 1, 2019

I would love to see it merged too

@mechatheo
Copy link

Hello,

this issue is already quite old, i saw a recent PR: #2097 which solves it. What is the state of this, is there a good chance that this will be accepted?

@tahsinkose
Copy link
Contributor

I'm expecting some maintainer to have a look at it. As most of the efforts are channeled into ROS2 dev, there is not much people left to accept/reject/review stuff. Who should I ping for this?

@FabianSchurig
Copy link

I would love to see it merged, too :)

@luisrayas3
Copy link

I've also been looking for a way to load an arg from a yaml file (and also for structured args, i.e. lists & dicts). That said, I have a few issues with the particular approach suggested here and implemented in #2097.

  1. Confusion of responsibilities. The param and rosparam tags are used to load parameters into the ROS parameter server, not propagate values through launch files. This is already what args do. This proposal totally confuses these concepts. A die-hard pragmatist may say "so what?"; the rest of my points are specific issues that arise from this confusion of responsibilities.

  2. Spurious parameters loaded into the ROS parameter namespace. In order to use this feature, I always need to invoke the param/rosparam constructs, which will still add the parameters to the parameter server. It seems virtually guaranteed that this will result in the need to add parameters which have no business being ROS parameters.

  3. Complication of resolution semantics. Currently, the resolution semantics are very simple to understand: args cannot be re-defined while the last value wins for params, which is fine because they can be loaded after all launch-stuff is resolved and are only used by the launched stuff. Now, at best I lose referential transparency, at worst I totally lose clarity about where my values are coming from:

<param name="foo" value="bar" />
<node name="n1" pkg="p" type="t" args="$(param foo)" />
<param name="foo" value="baz" />
<node name="n2" pkg="p" type="t" args="$(param foo)" />

If n1 gets "bar" and n2 "baz" then I have violated referential transparency, if n1 gets "baz" then some later code can mess up my intentions. Either way, it's going to add cognitive load and decrease overall understand-ability.

Instead, I would suggest the fix be an extension(s) that leverages the existing args. A simple solution for the 1st issue of loading the value from a file, a load_yaml function usable in eval could be introduced so one can do <arg name="foo" value="$(eval load_yaml('blah.yaml'))" />. For the issue of args always being strings I think an extension to arg (e.g. type="...") or a new tag would be more appropriate.

@tahsinkose
Copy link
Contributor

tahsinkose commented Dec 12, 2020

I've also been looking for a way to load an arg from a yaml file (and also for structured args, i.e. lists & dicts)

@luisrayas3 I think your use-case is different from what is asked in this issue. From the responses, I can say that there IS definitely need for this support as ROS parameters are not just needed as run-time constructs in practice. We also need them in launch-time occasionally, if not often. Without this capability, the only way to use the same information is via specifying a duplicate arg of the particular params loaded from yaml files.

  1. I have to disagree with you. This is not about changing the responsibility of tags. It is about providing facilitative substitution rules one can benefit from in order to reduce overall lines of code in launch files.
  2. I think this is invalid due to your misconception on the conflict of responsibilities. If you just need args, then you just use args. No param/rosparam/yaml file needed.
  3. This is a valid concern. I didn't check this explicitly. But the proper behavior for both args to get the latest foo, which is baz.

Instead, I would suggest the fix be an extension(s) that leverages the existing args. A simple solution for the 1st issue of loading the value from a file, a load_yaml function usable in eval could be introduced so one can do

As I said, this is a different use-case. You want to be able to not bloat ROS parameter server when using yaml configurations that are only desired for launch-time. You are right, the example in the first comment of this issue perfectly fits your description. But it lacks the run-time usage of ROS parameters, which actually renders the usefulness of this substitution rule.

Consider the following example:

!-- grippers.yaml --
robot:
    gripper: two_fingers
<launch>
  <rosparam command="load" file="$(find my_pkg)/params/grippers.yaml" />
  <group if="$(eval param('/robot/gripper') == 'two_fingers')">
     ...
  </group>
  <group if="$(eval param('/robot/gripper') == 'hand')">
     ...
  </group>
 
</launch>
...
ros::NodeHandle nh;
const std::string gripper = nh.param<std::string>("/robot/gripper", <def_val>);
...

@luisrayas3
Copy link

Thanks @tahsinkose for your thoughtful response. I'm still not convinced. I do agree that ostensibly, we have different use cases but I believe they can be understood as the same underlying deficiency, and that it would be better to identify it as a deficiency in args and not to patch around it with param substitutions.

I think the response to (1) belies itself. args provide facilitative substitution already, we shouldn't need params to do the same thing. The only case where this solution could do something that an arg extension couldn't do would be to read parameter values that were set before the launch was started. I think this could be reasonable as long as the param values were not mutually updated and read by the launch (the same way env tags and substitutions do not interact), however I don't think that solves the issue we want to solve.

To expound my 2nd bullet, the problem is that this solution renders params as strictly more powerful than args. So it's not about "if you just need args then you just use args", it's about the fact that when params become more powerful than args, one will be forced to bloat one's parameter space when one would have just liked to use args.

Upon further reflection, I actually don't think it is possible to leave $(param) substitutions until after everything is resolved because they will be needed to do the resolution (e.g. in your example, we need to know which group to load). Therefore, I would expect it would be necessary for n1 to get "bar" in the provided example.

Importantly, this also means params will be the only thing that can be both mutated and read by launch files. As a result, param substitutions would be the only substitution expression which could expand to two different values in two different places in the same scope. IMHO these kind of semantics just don't belong in any declarative xml, even if it's arguably a corner case.

And I just don't think it's needed, one can already set params from args, so with "extended" args (syntax made up) the provided example becomes:

<launch>
  <arg name="grippers" type="extended" value="$(eval load_yaml(find('my_pkg') + '/params/grippers.yaml'))" />
  <rosparam subst_value="True">$(arg grippers)</rosparam>
  <group if="$(eval robot['gripper'] == 'two_fingers')">
     ...
  </group>
  <group if="$(eval robot['gripper'] == 'hand')">
     ...
  </group>
 
</launch>

@tahsinkose
Copy link
Contributor

tahsinkose commented Jan 1, 2021

@luisrayas3 thanks for the additional clarification.

I do understand your concern about the mutability of params and share to some extent. Even perhaps this was the initial rationale behind how the params are designed, so that they are exclusively orthogonal -no substitution rules as in the case for args- to launch semantics and directly affect program code.

The only case where this solution could do something that an arg extension couldn't do would be to read parameter values that were set before the launch was started

Thanks for explicit statement. This issue was started with -and only with- this very use-case in mind. You are right that the subsequently changed params will lead to conflicting program behavior and the debugging would be very subtle. However, I believe that boils down to the user to make sure the finality of the param used in substitution. To put it in the opposite way, the params that are subject to change should not be used in substitution rules. The substitution-arg env and env declaration makes a nice exclusion at this point:

Values set using the tag will not be seen by $(env ...), so the tag cannot be used to parametrize launch files.

It is not programmatically possible to make this exclusion among substiution-arg param and the param declaration, but convenient documentation and tutorial can be added to wiki on how to avoid such usages.

I fully understood the 2nd argument in the previous comment and still my point stands. I disagree with you about the "be forced to bloat parameter space" part. If some parameter does matter also in program code, then it must naturally be a param independent from its inclusion into launch semantics. Otherwise it should be defined solely in launch files. Your use-case, which tries to reduce such declarative statements and only bound to args, is a different problem from the one pinpointed with the issue. Because this issue, AFAIU, targets the reusability of the same information with a direct accessor(($param ...), rather than bouncing them back and forth with arg.

From a very similar perspective, your suggested extension could be criticized for disabling the ability of using default keyword as it virtually becomes impossible to feed such a complex dictionary from the higher levels of the launch chain, especially with an indefinite number and type of fields. It must be limited to value keyword and only to the complex types. After raising this concern, I must state that it is not unimplementable as in the case for param substitution. If you decide to implement that feature, please feel free to ping me as a reviewer (even if I don't count yet). I'd love to contribute back as you do in here.

Your hack is really nice and a little bit dirty 🙂. First, you willingly don't use the full load support of the yaml files into Parameter Server for the sake of your extension. Second, it suffers from the same soft spot of param substitution. Just consider a latter rosparam (or param) statement that happens to change /grippers. Same story again. After this point, let us please focus only on the reported issue from the perspective of param substitution in the course of discussion as your use-case is different, even if slightly.

Your suggested solution makes me think more deeply on how the param substitution might be foundationally flawed and thus avoided at the first place. Admitting both the theoretical and practical weakpoints, I still believe that this is a practical problem and could be solved with proposed method in relevant PR as long as users do know what they are doing.

Lastly, I'm open to a voting session by the authorized maintainers whether to accept this as a real issue and thus the "fix" PR. @jacobperron could you please read the discussion spanning last 4-5 comments and provide some feedback on how to react? All in all, I believe @luisrayas3 denoted serious downsides of the proposed param substitution, of which should be taken into consideration of maintainers.

EDIT:

It is not programmatically possible to make this exclusion among substiution-arg param and the param declaration, but convenient documentation and tutorial can be added to wiki on how to avoid such usages.

On second thought, this could be actually done programmatically. Each $(param ...) substituted parameter can be stored in a special data structure with its value. Then at the end of launch resolution the value in data structure and finale value on Parameter Server could be compared and launch can be accordingly failed or succeeded with verbose failure messages.

I'll start implementing the final feature after settling the discussion in the scope of #2097.

@jacobperron
Copy link
Contributor

jacobperron commented Jan 8, 2021

After reviewing the discussion above, I lean towards the position that we shouldn't allow $(param ...) substitutions.

The only case where this solution could do something that an arg extension couldn't do would be to read parameter values that were set before the launch was started

Thanks for explicit statement. This issue was started with -and only with- this very use-case in mind. You are right that the subsequently changed params will lead to conflicting program behavior and the debugging would be very subtle. However, I believe that boils down to the user to make sure the finality of the param used in substitution.

As I understand, this is a feature request to make reusing values from a parameters YAML file in a launch file more convenient, not necessarily to use parameter values directly (which is a tricky rabbit hole as we're learning). At a glance, $(param ...) seems ill-defined: it's not clear if it will substitute the value from the most recent tag, the last tag, or the a value from the parameter server.

It seems more straight forward to add a feature to <arg> tags and/or $(arg ...) substitutions to achieve the same result, but with less confusion.

I'd like to play around with #2097 to get a feel for how it works in different cases, but it seems I hit an exception trying to run the example in this issues description. @tahsinkose Can you run the example (perhaps it's just a problem on my end)?

@tahsinkose
Copy link
Contributor

tahsinkose commented Jan 20, 2021

At a glance, $(param ...) seems ill-defined: it's not clear if it will substitute the value from the most recent tag, the last tag, or the a value from the parameter server.

It will behave as a temporal entity. So the ROS parameter tree is being built up during the launch tree crawling. When $(param ...) substitution is used, it will fetch the value at that time instant of the ROS parameter tree. In the lack of such a value, it will raise an error. With this definition, there is no ill-defined or unclear concept. What @luisrayas3 pointed out is that such temporal logic would not be convenient for a declarative language such as .launch files. Also subsequently changed parameters might lead to conflicting program behavior.

It seems more straight forward to add a feature to tags and/or $(arg ...) substitutions to achieve the same result, but with less confusion.

Unfortunately it is not as straightforward as it seems. The extended arg type proposed by @luisrayas3 seems indeed good, but there are subtle problems of it. $(arg ...) substitution of that particular argument would be only well-defined for ROS parameters, as they support complex types. But the args are meant to resolve to simple types. It could be possible to restrict the usage of an extended type arg out of rosparam statement, but same level of safety guarantee could be implemented for $(param ...) substitution as well, at the end of resolution.

Another problem is the possible arg conflicts between the file-loaded and launch-specified args. Consider the example:

<arg name="some_arg" value="foo"/>
<arg name="placeholder" type="extended" value="$(eval load_yaml(find('my_pkg') + '/params/args.yaml'))" />
# args.yaml
some_arg: bar

Also what $(arg placeholder) will resolve to? Normally it should resolve to a map, but args don't support complex structures. In fact args don't have any property for type specification at all.

@tahsinkose Can you run the example (perhaps it's just a problem on my end)?

I had run it before creating the relevant PR and it was working. If you could share the exact launch and yaml file, I can rapidly test it.

All in all, the addition of such extension on args require much more effort than already spent in the relevant PR. Also the missing gap for resolution of placeholder arg used to load args from yaml file must be properly bridged. Even after assuming to have all these, I believe this complicates arg more than it is originally designed for.

I feel like we should end this discussion with one final comment from your side @jacobperron as my cycles for returning back to this issue are sparsed enough. I wouldn't mind the declination of the PR, but I'm not going to implement @luisrayas3's suggestion either as I believe both approaches have their pros&cons and both boil down to the cautious usage from the users.

@ndahn
Copy link

ndahn commented Jul 19, 2023

I feel like this discussion was a little misguided unfortunately. This was (and is at least for me) specifically about how static_transform_publisher can be used.

In my use case I would like to decouple the launch file from the specific transformation, as it may be occasionally updated by a script. This would be solved easily if static_transform_publisher accepted rosparams as an alternative to command line args.

I will look into this tomorrowish and provide a PR.

@ndahn
Copy link

ndahn commented Jul 21, 2023

PR

@rickstaa
Copy link

PR

Amazing!

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

No branches or pull requests