(moveit_py) node can have multiple param files

[MatthijsBurgh]

Description

A node is often started with multiple param files. Using list.index only returns the first occurrence. The new code searches for all occurrences.

Checklist

• Required by CI: Code is auto formatted using clang-format
• Extend the tutorials / documentation reference
• Document API changes relevant to the user in the MIGRATION.md notes
• Create tests, which fail without this PR reference
• Include a screenshot if changing a GUI
• While waiting for someone to review your request, please help review another open pull request to support the maintainers

Please backport to iron and rolling.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c0c6baf) 50.46% compared to head (11ac7c0) 50.84%.
Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2393      +/-   ##
==========================================
+ Coverage   50.46%   50.84%   +0.39%     
==========================================
  Files         385      386       +1     
  Lines       31831    32002     +171     
==========================================
+ Hits        16061    16269     +208     
+ Misses      15770    15733      -37     

see 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[MatthijsBurgh]

@peterdavidfagan any changes I need to make to this PR? Or is it ready for review?

[peterdavidfagan]

Hey @MatthijsBurgh,

This looks good to me, thank you for this contribution.

I have a paper deadline for October 10th, once this passes I'll test the above changes on my side.

[henningkayser]

Thank you for addressing this issue! What I'm not really following is why we have to parse cli_args, and why we can't simply forward all the launch arguments blindly. Maybe @peterdavidfagan can give an explanation for this?

[peterdavidfagan]

It has been a while since I looked at this but below is my understanding.

why we can't simply forward all the launch arguments blindly

Short Answer
The MoveItPy instance in a given Python executable does not directly inherit from the base Node class provided by rclpy. I haven't delved into the rclpy codebase too much but I suspect somewhere they must also parse parameters from the command line arguments due to how launch_ros executes nodes as python executables.

Long Answer
The python launch scripts and in particular launch_ros.actions.Node, constructs a command to start a process for a given executable. Part of this process is constructing the ros_specific_arguments. For python executables (as is the case with this library), the python script gets executed with specific arguments which need to be parsed as the MoveItPy instance is not itself a Node in the traditional sense but rather a Python binding of a moveit_cpp node.

If the user themselves don't pass a config dict from MoveItConfigsBuilder in their Python script, the parameters are automatically taken from the cli args in the constructor as seen here. This honestly could be cleaned up, in terms of the statements used in the constructor. This being said I am unsure if there is a cleaner way to simply forward launch parameter arguments as the instance doesn't inherit from the rclpy definition of a Node.

The logic for constructing cmd arguments from parameters can be found here. This same script provides comments for how the node parameters are parsed etc.

Reason for Changes
As @MatthijsBurgh correctly pointed out it is possible to have more than one param file which is demonstrated in the following loop. I must have missed this when originally creating the bindings. One thing to note is that they also specify individual arguments with -p likely we need to parse these too. It could be worth revisiting to see what rclpy is doing in order to automatically forward node parameters.

rclpy approach
I did a quick search to understand how parameters are being read in rclpy. This looks like a good starting point. In general, their Node constructor contains a lot more parsing logic than the current MoveItPy implementation does. It might be worth considering a refactor of this at some point, for now I think @MatthijsBurgh's changes are worth merging.

[peterdavidfagan]

@MatthijsBurgh I am testing your changes now, thanks for this contribution it is clearly an improvement over what was there before.

Would you be up for performing an interactive rebase of your branch so it only contains commits for the latest changes? This would be useful for commit history readability. I am going to quickly test on my side and then complete a review of the changes.

Update
It looks like the parameters in the existing tutorials need to be updated, the following runtime error is encountered when running the python tutorial. I'll look to update this such that the planning_plugin is specified.

I can see that your changes works so far as specifying param files.

[peterdavidfagan]

Approving these changes, they accomplish the fix of ensuring multiple parameter files get parsed which improves over the existing code. While going through this PR other issues relating to parsing of parameters were encountered. Follow up work will address these issues.

[MatthijsBurgh]

I rebased this PR on main. Dropping the merge commit.

I do think we should merge this PR, not waiting on the other open issues.

[peterdavidfagan]

Agreed on the above.

[MatthijsBurgh]

P.S. Do I still need approval to run workflows as I am still a first time contributor. Since this PR isn't merged yet? Or are your workflow settings strict that at all time outside contributors need approval. In that case, you maybe want to relax that. As that would speed up the process a little bit.

[peterdavidfagan]

P.S. Do I still need approval to run workflows as I am still a first time contributor. Since this PR isn't merged yet? Or are your workflow settings strict that at all time outside contributors need approval. In that case, you maybe want to relax that. As that would speed up the process a little bit.

Yeah the workflows usually require a maintainer to approve. I believe there is a financial cost associated with these workflows so it might be problematic to run them for every update to a pr. I agree though it would be nice to have the permission to manually trigger workflows, at the moment I think only maintainers can do this.

Update: I addressed the config file issue in the tutorials, I agree this is ready to be merged.

[MatthijsBurgh]

@peterdavidfagan friendly ping to trigger CI :)

[peterdavidfagan]

@peterdavidfagan friendly ping to trigger CI :)

Hi @MatthijsBurgh unfortunately I don't have access to trigger CI but @henningkayser or @sjahr may be able to help as they are maintainers and should have access. I have been using your code and all is good hopefully these changes can be merged soon.

[MatthijsBurgh]

@sjahr It seems the CI issues are not related to the changes in this PR. Correct?