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

Rename node-related parameters #122

Merged
merged 18 commits into from Mar 5, 2020
Merged

Conversation

jacobperron
Copy link
Member

@jacobperron jacobperron commented Feb 6, 2020

The 'node-' prefix seems redundant since the attribute is part of a "node" tag.
Add a deprecation warning in case the old name is used.

I couldn't find a reason why 'node-name' was preferred in #73.
I'm suggesting in this PR that we change it 'name', which seems more intuitive for me, especially coming from ROS 1.

The 'node-' prefix seems redundant since the attribute is part of a <node> tag.
Add a deprecation warning in case the old name is used.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron jacobperron requested a review from hidmic Feb 6, 2020
@jacobperron jacobperron added the enhancement New feature or request label Feb 6, 2020
@jacobperron jacobperron self-assigned this Feb 6, 2020
@jacobperron
Copy link
Member Author

jacobperron commented Feb 7, 2020

I also just noticed that the migration guide uses the attribute "name", which is currently inconsistent with the implementation.

@hidmic
Copy link
Contributor

hidmic commented Feb 7, 2020

which is currently inconsistent with the implementation.

You're absolutely right, good catch.

…teProcess

Instead, intercept the 'name' attribute and don't forward to ExecuteProcess.
Add a new attribute 'exec-name' that can be used to set the name of the process.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
del kwargs['name']
exec_name = entity.get_attr('exec-name', optional=True)
if exec_name is not None:
kwargs['name'] = parser.parse_substitution(exec_name)
Copy link
Contributor

@hidmic hidmic Feb 10, 2020

Choose a reason for hiding this comment

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

@jacobperron hmm, while this is a fine patch, it grows the gap between Python and markup-based launch files. This is something we've discussed with @ivanpauno several times, how detrimental to standardization is the fact that launch frontend parsing support allows one to depart from the original action "signature". IMHO, we should apply this exact same change to the Node action constructor itself and the parsing function should just use that. WDYT?

Copy link
Member

@ivanpauno ivanpauno Feb 10, 2020

Choose a reason for hiding this comment

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

The problem with changing the Node signature is that it breaks API in a confusing way:
if somebody was using name to specify the executable name, they will have a surprise when that's now used as node name.
I think will should keep XML attributes as close as possible to Python constructor arguments.

Copy link
Contributor

@hidmic hidmic Feb 10, 2020

Choose a reason for hiding this comment

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

That is also true. But doing so in XML/YAML only but not in Python makes it worst IMO.

Copy link
Member Author

@jacobperron jacobperron Feb 10, 2020

Choose a reason for hiding this comment

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

I agree that this change should happen in the Node constructor, keeping the signature the same as the front-end. I'll do this if there's a consensus to go ahead with this change.

The problem with changing the Node signature is that it breaks API in a confusing way:
if somebody was using name to specify the executable name, they will have a surprise when that's now used as node name.

Yeah, this is a problem. IMO, it seems like a good thing to change long term. In the short-term, we can document the behavior change in the release notes and on whatever documentation exists for launch_ros (note, besides the API docstrings, I only found mention of the "name" attribute in the migration guide and it is already assuming the behavior I expect).

I also understand if there's consensus to leave the implementation as-is in order to avoid breaking the existing behavior.

Copy link
Member

@ivanpauno ivanpauno Feb 10, 2020

Choose a reason for hiding this comment

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

I think that the API break is reasonable.
To avoid breaking user launch files, node_name can be deprecated with a warning.
The users that were using name will have to check the release notes to understand the problem (though I expect not much users using it).
We should also merge the argument name change together with a PR updating the release notes. If not, we will forget later.

If @wjwwood and @hidmic are also ok with breaking API, I think it's fine.

Copy link
Member

@wjwwood wjwwood Feb 10, 2020

Choose a reason for hiding this comment

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

👍 to breaking API to make it make sense, and 👍 for updating the documentation and release notes simultaneously.

Copy link
Member Author

@jacobperron jacobperron Feb 10, 2020

Choose a reason for hiding this comment

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

Thanks for the input, all. I'm on it 😎

jacobperron added 5 commits Feb 10, 2020
…by ExecuteProcess"

This reverts commit 61503c9.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Deprecate 'node_name' and 'node_namespace'.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Add test for deprecated parameters.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
The parameter is named 'exec_name' and forwarded to ExecuteProcess.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron jacobperron force-pushed the jacob/rename_node_name_attr branch from 8010e61 to e7e1325 Compare Feb 10, 2020
@jacobperron
Copy link
Member Author

jacobperron commented Feb 10, 2020

@hidmic @ivanpauno I've reverted the 'name' overriding in the parser (45d4992) and added new Node parameters for name and namespace (827042b), deprecating the old parameters. I've also added a new Node param exec_name for setting the process name (e7e1325)

Copy link
Member

@ivanpauno ivanpauno left a comment

Left some minimal comments.
It would be good to check if something has to be updated in demos/examples/tutorials.

launch_ros/launch_ros/actions/node.py Show resolved Hide resolved
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/actions/node.py Outdated Show resolved Hide resolved
launch_ros/launch_ros/actions/node.py Show resolved Hide resolved
jacobperron added 2 commits Feb 10, 2020
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron
Copy link
Member Author

jacobperron commented Feb 10, 2020

It would be good to check if something has to be updated in demos/examples/tutorials.

Good idea, I'll open up PRs connecting to this for instances I find.

jacobperron added a commit to ros2/demos that referenced this pull request Feb 10, 2020
node_name and node_namespace parameters have been deprecated in ros2/launch_ros#122

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron
Copy link
Member Author

jacobperron commented Feb 10, 2020

LifecylceNode has a required keyword argument named node_name. I've just patched how it is passed to it's parent constructor (586e09c), but what do you think about also renaming it's argument to name? This would be a harder API break, since we would would have to introduce a new argument name that is required.

@jacobperron
Copy link
Member Author

jacobperron commented Feb 11, 2020

Adding to Foxy release notes: ros2/ros2_documentation#522

@ivanpauno
Copy link
Member

ivanpauno commented Feb 11, 2020

LifecylceNode has a required keyword argument named node_name. I've just patched how it is passed to it's parent constructor (586e09c), but what do you think about also renaming it's argument to name? This would be a harder API break, since we would would have to introduce a new argument name that is required.

I think that if we're going to do the change, it makes sense to rename all the node_* of any action/description (LifecycleNode, ComposableNodeContainer, ComposableNode, ...).
To avoid that hard API break in that case, the following can be done:

  def __init__(self, *, name: Optional[Text] = None, node_name: Optional[Text] = None, **kwargs) -> None:

If node_name is passed, a warning is showed.
If both name and node_name are passed, an error is raised.
If only name is passed, we're happy.

In another release, node_name can be removed and name be required.

@jacobperron
Copy link
Member Author

jacobperron commented Feb 12, 2020

If node_name is passed, a warning is showed.
If both name and node_name are passed, an error is raised.
If only name is passed, we're happy.

Also, if both are None, an error raised. I don't think the LifecycleNode action will work otherwise.

I can do this.

Deprecate the node_name parameter.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron
Copy link
Member Author

jacobperron commented Feb 12, 2020

I've also deprecated the node_name parameter for LifecycleNode (09aa4c5) and updated the connected demos PR.

Copy link
Member

@ivanpauno ivanpauno left a comment

LGTM with @wjwwood and @hidmic 👍

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
jacobperron added a commit to ros2/ros2_documentation that referenced this pull request Feb 12, 2020
Specifically, I've made updates that reflect the change made in ros2/launch_ros#122

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
hidmic
hidmic approved these changes Feb 13, 2020
Copy link
Contributor

@hidmic hidmic left a comment

LGTM!

BTW I figure you chose not to add an Optional type annotation for some constructor arguments where a None default is used to convey that argument it's mandatory, but I'd expect mypy to complain.

launch_ros/launch_ros/actions/composable_node_container.py Outdated Show resolved Hide resolved
launch_ros/launch_ros/actions/lifecycle_node.py Outdated Show resolved Hide resolved
jacobperron added 2 commits Feb 14, 2020
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron
Copy link
Member Author

jacobperron commented Feb 14, 2020

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

The same test failures are occurring on the latest nightly builds.

test_launch_ros/.eggs/README.txt Outdated Show resolved Hide resolved
hidmic
hidmic approved these changes Feb 17, 2020
Copy link
Contributor

@hidmic hidmic left a comment

Overall LGTM, pending green CI

jacobperron added 2 commits Feb 17, 2020
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@ivanpauno
Copy link
Member

ivanpauno commented Feb 26, 2020

@jacobperron is this ready to be merged?

@jacobperron
Copy link
Member Author

jacobperron commented Mar 3, 2020

I was hesitating due to the CI failures. I've retriggered CI.

@nuclearsandwich
Copy link
Member

nuclearsandwich commented Mar 4, 2020

@jacobperron I haven't completed the first CI test but in local research ros2/ci#399 is a workaround for the installation paths which should allow the tests to run.

@jacobperron jacobperron changed the title Rename 'node-name' attribute to 'name' Rename Node action parameters Mar 5, 2020
@jacobperron jacobperron changed the title Rename Node action parameters Rename node-related parameters Mar 5, 2020
@jacobperron jacobperron merged commit be0f9a1 into master Mar 5, 2020
1 check passed
@delete-merged-branch delete-merged-branch bot deleted the jacob/rename_node_name_attr branch Mar 5, 2020
jacobperron added a commit to ros2/ros2_documentation that referenced this pull request Mar 5, 2020
Specifically, I've made updates that reflect the change made in ros2/launch_ros#122

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
jacobperron added a commit to ros2/demos that referenced this pull request Mar 5, 2020
* Update launch_ros Node action usage

node_name and node_namespace parameters have been deprecated in ros2/launch_ros#122

* Update LifecycleNode action usage

The parameter 'node_name' is deprecated, replaced by 'name'.

* Update ComposableNode action usage

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
jacobperron added a commit that referenced this pull request Apr 15, 2020
This is a follow-up to #122.

If 'node_executable' is used then a deprecation warning is issued.
If 'node_executable' is used at the same time as 'executable', then a
runtime error is raised. This is similar behaviour to the other
parameters deprecated in #122.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
jacobperron added a commit that referenced this pull request Apr 16, 2020
…140)

This is a follow-up to #122.

If 'node_executable' is used then a deprecation warning is issued.
If 'node_executable' is used at the same time as 'executable', then a
runtime error is raised. This is similar behaviour to the other
parameters deprecated in #122.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
ferranm99 added a commit to ferranm99/ferran-ros that referenced this pull request May 20, 2022
Specifically, I've made updates that reflect the change made in ros2/launch_ros#122

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants