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

Bug in ros2node for showing action servers #954

Open
julianmueller opened this issue Dec 12, 2024 · 10 comments
Open

Bug in ros2node for showing action servers #954

julianmueller opened this issue Dec 12, 2024 · 10 comments
Labels
more-information-needed Further information is required

Comments

@julianmueller
Copy link

Hi,
the following line causes an error for me:

https://github.com/ros2/ros2cli/blob/acefd9c0d773e7a067a6c458455eebaa2fbc6751/ros2node/ros2node/api/__init__.py#L128C28-L128C69

The called function get_action_server_names_and_types_by_node() does not actually exist here in rclpy.node, see: https://github.com/ros2/rclpy/blob/rolling/rclpy/rclpy/node.py

@clalancette
Copy link
Contributor

Can you please tell us what version of ROS 2 you are using, and the steps you took to get the error? It looks to me like this exists on Rolling, but it is possible that we missed a backport or something.

@julianmueller
Copy link
Author

I'm using the current LTS, ROS2 Jazzy. Where do you find the function in the rclpy.node module?

@julianmueller
Copy link
Author

Ah I see, thanks for the references!

But still, in https://github.com/ros2/ros2cli/blob/acefd9c0d773e7a067a6c458455eebaa2fbc6751/ros2node/ros2node/api/__init__.py#L128C28-L128C69 the function is called as a method of the rclpy.node.Node class, but it should rather be a function call from the rclpy.action.graph module

@clalancette
Copy link
Contributor

But still, in https://github.com/ros2/ros2cli/blob/acefd9c0d773e7a067a6c458455eebaa2fbc6751/ros2node/ros2node/api/__init__.py#L128C28-L128C69 the function is called as a method of the rclpy.node.Node class, but it should rather be a function call from the rclpy.action.graph module

In theory, both should work, given that it is exported from the Node class. I think at this point it would be helpful to know what commands you are running and what is not working for you.

@fujitatomoya fujitatomoya added the more-information-needed Further information is required label Dec 13, 2024
@julianmueller
Copy link
Author

I called:

get_action_client_info(node=ros2_node, remote_node_name=node_name, include_hidden=True)

with ros2_nodeas an instance of a ROS2 Node class, so a running node and get the python error:

AttributeError: 'Node' object has no attribute 'get_action_client_names_and_types_by_node'. Did you mean: 'get_client_names_and_types_by_node'?

This supports my previous claim, that the Node class does not have the method get_action_server_names_and_types_by_node.

My suggested fix would be:

def get_action_server_info(*, node, remote_node_name, include_hidden=False):
    remote_node = parse_node_name(remote_node_name)
    names_and_types = get_action_server_names_and_types_by_node(
        node, remote_node.name, remote_node.namespace)
    return [
        TopicInfo(
            name=n,
            types=t)
        for n, t in names_and_types if include_hidden or not _is_hidden_name(n)]

and same for the get_action_client_info()

@christophebedard
Copy link
Member

How are you building or running ROS 2? Which distro are you using? If you're building ROS 2 from source, which branches are you using?

@fujitatomoya
Copy link
Collaborator

@julianmueller

with ros2_nodeas an instance of a ROS2 Node class, so a running node and get the python error:

of course not, there is no such method in Node class.

that the Node class does not have the method get_action_server_names_and_types_by_node.

No it does not. but ros2cli does not actually uses the Node class, it uses NodeStrategy. (that is why there is no problem in ros2cli commands)

but i understand your claim here, the following implementation generates the confusion...
and it should be implemented as rclpy.graph API to avoid these special cases.

# TODO(hidmic): generalize/standardize rclpy graph API
# to not have to make a special case for
# rclpy.action
def get_action_names_and_types(self):
return rclpy.action.get_action_names_and_types(self.node)
def get_action_client_names_and_types_by_node(self, remote_node_name, remote_node_namespace):
return rclpy.action.get_action_client_names_and_types_by_node(
self.node, remote_node_name, remote_node_namespace)
def get_action_server_names_and_types_by_node(self, remote_node_name, remote_node_namespace):
return rclpy.action.get_action_server_names_and_types_by_node(
self.node, remote_node_name, remote_node_namespace)

it would be appreciated if you could consider the contribution to address above TODO.

@julianmueller
Copy link
Author

Thanks @fujitatomoya !
Maybe also type annotations and docstrings would be helpful, to know what type which argument should be and what it does. I'd be glad to contribute to that, if wanted.

@clalancette
Copy link
Contributor

Maybe also type annotations and docstrings would be helpful, to know what type which argument should be and what it does. I'd be glad to contribute to that, if wanted.

Yep, that would be great. We've been (slowly) adding type annotations to the entire ROS 2 codebase, but as there is a lot of code it has taken quite a while. If you'd like to contribute to this effort, we'd be happy to review it. I just ask that you do small PRs adding docs or annotations, rather than one giant PR. Small PRs are easier to review and run CI for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
more-information-needed Further information is required
Projects
None yet
Development

No branches or pull requests

4 participants