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

Substitutions in parameter files #168

Merged
merged 18 commits into from
Aug 11, 2020

Conversation

ivanpauno
Copy link
Member

Equivalent to the ROS 1 feature.
Fixes #96.

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno added enhancement New feature or request in review labels Aug 5, 2020
@ivanpauno ivanpauno requested a review from hidmic August 5, 2020 19:19
@ivanpauno ivanpauno self-assigned this Aug 5, 2020
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Though I have to say I'm surprised that the substitution parser can handle multi-line strings.

launch_ros/launch_ros/actions/node.py Outdated Show resolved Hide resolved
launch_ros/launch_ros/actions/node.py Outdated Show resolved Hide resolved
launch_ros/launch_ros/parameter_descriptions.py Outdated Show resolved Hide resolved
launch_ros/launch_ros/parameter_descriptions.py Outdated Show resolved Hide resolved
launch_ros/launch_ros/parameter_descriptions.py Outdated Show resolved Hide resolved
launch_ros/launch_ros/parameter_descriptions.py Outdated Show resolved Hide resolved
f.write(contents)
yield f.name
finally:
os.unlink(f.name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ivanpauno nit: out of curiosity, why not:

with NamedTemporaryFile() as f:
    f.write(contents)
    yield f.name

instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that won't unlink the file, just close it (unlink = remove from the filesystem).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will remove it if you don't set delete=False (which seems unnecessary in this case).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yes, I guess that works

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in fc999d0.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I now remember why I did this ...

Windows doesn't allow opening the same file twice.
If I yield from within the with statement, then I have a problem when opening the file in the test (see Windows CI results).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Windows doesn't allow opening the same file twice.

That makes no sense.

Copy link
Contributor

@hidmic hidmic Aug 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears you can reopen it, but not easily... (see here). Oh well, nevermind.

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno requested a review from hidmic August 10, 2020 14:34
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno requested a review from hidmic August 10, 2020 18:21
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending green CI

@ivanpauno
Copy link
Member Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@ivanpauno
Copy link
Member Author

Windows again:

  • Windows Build Status

This reverts commit fc999d0.

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno force-pushed the ivanpauno/substitutions-in-param-file branch from 7228ef6 to 521c910 Compare August 11, 2020 19:42
@ivanpauno
Copy link
Member Author

Last force push was because I forgot to sign the revert commit.

@ivanpauno ivanpauno requested a review from hidmic August 11, 2020 20:37
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@ivanpauno ivanpauno merged commit 91097d1 into master Aug 11, 2020
@jacobperron jacobperron deleted the ivanpauno/substitutions-in-param-file branch September 8, 2020 23:51
@gramss
Copy link

gramss commented Sep 16, 2020

Hey,

this looks really cool. Is there a simple user-faced example / documentation available for this?

I want to load a big param file and only replace certain parameter values (1-2 out of ~150). This is needed for CI testing.
Earlier this was done by a custom nav2 tool: https://github.com/ros-planning/navigation2/blob/main/nav2_common/nav2_common/launch/rewritten_yaml.py (But this seemed to stop working with foxy?)

Here is a example code using this: https://github.com/ros-planning/navigation2/blob/b8e9fba422d6e024767d1a5b668703625454fdcb/nav2_system_tests/src/system/test_system_launch.py#L45

Would be cool to maybe merge or replace the custom nav2 with official ros2_launch tools.

Thank you in advise!
Kind regards

@ivanpauno
Copy link
Member Author

Hi @gramss, sorry for the delay but I didn't see your comment before.

I didn't write an example, but it would look something like this:

Node(
  executable=executable_name,
  package=package_name,
  parameters=[ParameterFile('path/to/file', allow_substs=True)]
)

The tests have an example parameter file, you can basically use the normal launch XML style substitutions in any position (see this guide for launch XML style substitutions, most of them are documented there).

Adding an example would actually be a great idea.
Feel free to add one in the paraterfile class docs or in the examples folder.

If you have any question feel free to ping me.

@andre-nguyen
Copy link

@ivanpauno Is there anything blocking us from backporting this to Foxy?

@ivanpauno
Copy link
Member Author

@ivanpauno Is there anything blocking us from backporting this to Foxy?

By default, we backport only fixes and not features.
If there's enough interest to get this backported a patch doing that can be accepted, but that's up to the maintainers of the package @jacobperron @mjeronimo .

@jacobperron
Copy link
Member

I tried cherry-picking this change to Foxy, but there are some conflicts. If someone wants to take the time to resolve them, I'm happy to review a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request in review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support substitution in parameter yaml files
5 participants