Move create_node and create_node_builder back to the rclrs module

[esteve]

Closes #161

[nnmm]

Why do we not want it as a method of context?

[esteve]

@nnmm doesn't really make sense to have them there, Context is about state, not logic. rclrs had create_node in the top level module, but the change was merged by mistake. Plus it doesn't match the API of the other client libraries.

If you have questions, please don't mark your review as needing changes, it blocks the PR from being merged when it already has an approval.

[nnmm]

Yes, I almost never use "changes requested" for this reason, but in this case the change doesn't seem beneficial to me, so I intentionally wanted to block it until we've had a chance to reach an agreement.

[jhdcs]

I thought this had been discussed earlier? If I recall correctly, neither rclcpp nor rclpy have a context::create_node function anymore. This change was to bring us to a similar state. Right?

[nnmm]

The origin of the current API might be an accident, but that doesn't necessarily mean it's bad.

As for API similarity, the crux is that as far as I know, the most common pattern in both rclpy and rclcpp is to never provide a context, but to use the global default context. E.g. from the official pub-sub example

rclcpp::init(argc, argv);
rclcpp::spin(std::make_shared<MinimalPublisher>());

So, of course there it makes sense for Context to not have a create_node function, since it would practically never be used. We do not have an implicit/default global context, so having this creation method is quite convenient – at least if you prefer methods on the type over free methods, which I believe most people do.

[nnmm]

I'm not opposed to having both, by the way. Although I don't understand why rclpy or rclrs should need the create_node function when they have Node classes/structs they could just instantiate. Seems like you added it @esteve, so maybe you know :) ros2/rclpy@c311569

Another comment: rclpy.create_node accepts many more arguments than rclrs::create_node.

[esteve]

Yes, I almost never use "changes requested" for this reason, but in this case the change doesn't seem beneficial to me, so I intentionally wanted to block it until we've had a chance to reach an agreement.

We did discuss this, as @jhdcs pointed out. And there's an agreement, two of us agree on this change. We've had instances were two people agreed on something and one was not fully on board (e.g. I was the one who didn't agree), but we as a project went with the change, because there's really no other way than going with what the majority decides.

I'm not opposed to having both, by the way.

Why both? There should be one clear API to create nodes. And it should be the one that resembles the other client libraries the most, because that's what people learn.

Although I don't understand why rclpy or rclrs should need the create_node function when they have Node classes/structs they could just instantiate.

rclcpp does not have a create_node function any more, the way of creating a Node is via inheritance. There are other more intricate ways of creating a Node in rclcpp, but for the sake of simplicity, let's say that inheritance is the only way.

Seems like you added it @esteve, so maybe you know :) ros2/rclpy@c311569

Yes, I did. At the time it wasn't clear how we'd instantiate Nodes, so I mimicked the create_node API from rclcpp. And there was no context back then.

Another comment: rclpy.create_node accepts many more arguments than rclrs::create_node.

What does that have anything to do with this discussion? I don't understand.

[nnmm]

We did discuss this, as @jhdcs pointed out. And there's an agreement, two of us agree on this change. We've had instances were two people agreed on something and one was not fully on board (e.g. I was the one who didn't agree), but we as a project went with the change, because there's really no other way than going with what the majority decides.

Yes, majority voting is fine (though I feel that we could also ping people in our Matrix chat as a tiebreaker, some of them are quite experienced in Rust/ROS). Please don't take my "changes requested" as insistence that your PR must not be merged, but just as a way to delay until the arguments have been heard. I'll remove it soon, one way or another. I don't know which earlier discussion you're talking about, but in the issue, I didn't get a chance to express my view before you opened the PR.

rclcpp does not have a create_node function any more, the way of creating a Node is via inheritance. There are other more intricate ways of creating a Node in rclcpp, but for the sake of simplicity, let's say that inheritance is the only way.

I'm not sure if we're talking about the same thing here – my question was about rclpy and rclrs, not rclcpp. At least I still don't understand why rclpy and rclrs need this function, apart from backwards compatibility, when they have Node/NodeBuilder.

What does that have anything to do with this discussion? I don't understand.

If your goal is API similarity, a function with a very different signature doesn't achieve that (completely).

[nnmm]

Anyway, I haven't yet seen a direct response to the point I've been trying to make. I'll restate it to maybe make it clearer:

• The comparison with rclcpp and rclpy has to take into consideration that an explicit context is required in our client library, but not these two. Therefore, my argument is that Context::create_node()/Context::create_node_builder() is the most natural way of creating a node for us, but rclpy.Node()/rclpy.create_node() is for rclpy, and the rclcpp::Node constructor (via inheritance) is for rclcpp. We shouldn't blindly copy APIs when they exist under different constraints. Internal consistency of our API favors the Context having this function.
  • Plus, I don't even know any reason why the existence of the function in rclpy is a good idea today. I agree there should be only one way to do things, all else being equal, and the Node class is already there.
  • Another constraint is that Python has kwargs, which makes this function both expressive enough to cover all node options, and short enough to be readable. So we can't really have exactly the same in Rust.
  • We could of course also introduce an implicit global context, but that's a whole other discussion and I'd lean towards "no" a priori.

[esteve]

Yes, majority voting is fine (though I feel that we could also ping people in our Matrix chat as a tiebreaker, some of them are quite experienced in Rust/ROS).

The process is quite well established, up until now we've trusted each others' judgement and when two of us didn't agree, the third one would help resolve the tie. It's proven quite successful and I'm happy that we've respected the process so far, even when I was on the "losing" side. Regarding Matrix, ROS2 developers don't ask on Discourse every API change, nothing would get done ever.

but in the issue, I didn't get a chance to express my view before you opened the PR.

We don't wait for everyone to express their view before we open a PR, that has never happened.

If your goal is API similarity, a function with a very different signature doesn't achieve that (completely).

My goal with ros2-rust is an API that is conceptually similar and idiomatic, the current API has the responsibilities between Context and Node swapped. Eventually, I'd like to achieve composition like rclpy and rclcpp, given that we can't inherit in Rust, a default implementation of the Node trait might possibly be a solution, but if we keep Context::create_node, that seems like a broken API. Forcing ourselves to use rclrs::create_node will break the link between Context and Node, so it might make composition more feasible.

Anyway, I haven't yet seen a direct response to the point I've been trying to make. I'll restate it to maybe make it clearer:

Because only 30 minutes have passed, that's why the Matrix chat is such a terrible channel for having discussions. GitHub threads lend themselves to more asynchronous conversations.

The comparison with rclcpp and rclpy has to take into consideration that an explicit context is required in our client library, but not these two. Therefore, my argument is that Context::create_node()/Context::create_node_builder() is the most natural way of creating a node for us, but rclpy.Node()/rclpy.create_node() is for rclpy, and the rclcpp::Node constructor (via inheritance) is for rclcpp. We shouldn't blindly copy APIs when they exist under different constraints. Internal consistency of our API favors the Context having this function.

I think it's the opposite, Context by its very name, is just a placeholder where we put data that is shared between Nodes, adding more logic to it is counter-intuitive.

* Plus, I don't even know any reason why the existence of the function in `rclpy` is a good idea today. I agree there should be only one way to do things, all else being equal, and the `Node` class is already there.

That is a separate issue, and I agree that inheriting from Node is a better API for rclpy.

* Another constraint is that Python has kwargs, which makes this function both expressive enough to cover all node options, and short enough to be readable. So we can't really have exactly the same in Rust.

That has nothing to do with this PR.

* We could of course also introduce an implicit global context, but that's a whole other discussion and I'd lean towards "no" a priori.

That is a separate issue, only tangentially related to this PR, but not what we're discussing here.

[nnmm]

@esteve I'll reply to the meta/process issues elsewhere. But on the topic itself:

I still find my points convincing, and prefer not to follow rclpy here. The argument from the composition API is a bit vague imo, I'm not even sure we ever need a Node trait. But at this point I don't feel like anyone has incomplete information, and I'll remove the blocker from my review.

I'd just like to hear from @jhdcs whether his opinion has stayed the same after hearing mine.

Discussion done

[jhdcs]

My opinion is unchanged at the moment.

In my mind, rclrs has to walk a fine, rather strange line of trying to marry things that ROS users expect from other client libraries, with things that Rust users expect. For this particular instance, I think leaning toward what a ROS user would expect (which is what Esteve's PR here seems to be doing) is the correct call.

In the end, I feel that using rclrs should feel like using ROS2, just from a slightly different perspective. If we want to look at an API or a method of doing things, I would suggest we focus on rclcpp rather than rclpy, since C++ and Rust have more in common (in my opinion).

With respect to the issue that an explicit context is required in our client library, as opposed to the other libraries (which tend to start things off with a <client_library>::init(), or an equivalent function), I feel that this is not really an argument that lends itself well towards the old way of creating nodes via context - just because we currently require an explicit context does not mean we will always require one. Indeed, my work on trying to add in signal handling is making me think of adopting the other client libraries' method of initialization. Namely, I'm debating creating an rclrs::init() function, with a default context.

Anyways, this has been a long and meandering post, so to sum up:

• I think rclrs looking more like the other client libraries to a user is a good thing
• This PR will help to accomplish that

[jhdcs]

However, I will note there appear to be build errors.

[nnmm]

Yeah, agree that rclcpp is a closer neighbor than rclpy.

[nnmm]

@esteve Don't forget to also update the tutorial.

[esteve]

@nnmm there doesn't seem to be any reference to create_node in the tutorial, unless I've missed something.

[jhdcs]

@esteve @nnmm I don't see any references either. Everything seems to use Node::new()

[nnmm]

Oh, that's fine then.