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

API Changes: execution structure, options pattern, and arcs #427

Closed
wants to merge 12 commits into from

Conversation

mxgrey
Copy link
Collaborator

@mxgrey mxgrey commented Nov 17, 2024

This PR introduces a number of API breakages that I will be proposing with the following goals:

  • Review the API breakages needed for Async Execution #421 in isolation from the introduction of the async features
  • Future-proof the rclrs API so that we can add even more features beyond Async Execution #421 without breaking API again
  • Streamline some aspects of the API to cut down on the noise that users experience

There are three broad categories of breaking API changes that are being introduced here. Each of these categories can be discussed and iterated on independently, and I'm happy to revert or revise each of them as needed, bearing in mind that some breakages cannot be avoided in order to introduce async execution. I've lumped all three into one PR with the intent of completing all foreseeable API breakages in one fell swoop. If it's desired, I can reorganize this into a sequence of incremental API breaking PRs.

The three categories are broken down below:

1. Execution structure (Context -> Execution -> Node relationship)

(spun out in #428)

This is the most crucial change for the async execution to be feasible. Up until now, rclrs has had a loose coupling between the executor and nodes. That means nodes could be added to or removed from the executor freely. This comes with the following disadvantages:

  • It's possible to add the same node to multiple executors and spin them both at the same time. We have protections in place so that this won't violate any contract with rcl, but it would be a confusing arrangement.
  • We need many layers of mutexes, arcs, and weaks to manage the loose ownership.
  • Users don't have a clear order in which to invoke these three top-level objects.
  • For async execution, nodes will need a way of issuing jobs to their executor at any time, which isn't possible if they can be orphaned or bounce around between executors.

Due to the last bullet point in particular, I'm proposing a rigid procedure in which:

  • A user creates a Context
  • The Context can create one or more Executors
  • Each Executor can create one or more Nodes

The minimum publisher example shows a good demonstration of this new procedure.

In this structure, Nodes are no longer allowed to be removed from an Executor or move between Executors. That's a very unlikely use case to begin with, so I don't think there's a downside to this restriction. After #421 we'll be able to introduce custom flavors of Executors, so if anyone really cares about moving their node around between executors, it would be possible to introduce a custom hierarchical executor that can move nodes between other executors.

2. Options Pattern

(spun out in #429)

This is a slight twist on the builder pattern that we were using via NodeBuilder up until now.

Instead of creating a "builder" object which will eventually .build() the final product, we will build an _Options structure which gets passed into a create_ function. Here are some examples:

The user fills up the _Options with whichever fields are relevant to them, and the rest of the option fields will use default values. Note that publishers and subscribers will have different default QoS from clients and services, and this is taken into account by the option builders for these different primitive types.

3. Arcs

(spun out in #430)

This one is very subjective so I don't mind reverting it, but I felt that the prevalence of Arc in the API was adding noise that is seldom relevant to the average user. In this PR, I've refactored the API a little so that Node, Subscription, Publisher, Client, and Service each have a respective _State structure which is public and represents the underlying instance. At the same time, Node, Subscription, etc are now type aliases of the form

  • pub type Node = Arc<NodeState>;
  • pub type Subscription<T> = Arc<SubscriptionState<T>>;

This does not lead to any actual change in the structure or memory management of any of these types; it's a purely superficial renaming. But it reduces how often Arc is tossed around in the API and thereby streamlines the 95% (my own estimate) of use cases where Arc is just an implementation detail. At the same time, it does not interfere with the remaining 5% of use cases where the Arc is relevant, e.g. if a user wants to hold onto a Weak<Node> for some reason.

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
@mxgrey
Copy link
Collaborator Author

mxgrey commented Dec 9, 2024

I'm closing this in favor of the three spun-out PRs #428, #429, and #430.

Those PRs are already diverging from this one based on review feedback, and there's no point trying to keep this PR in sync with them, especially with the overhead of keeping all the PRs in sync with the main branch.

@mxgrey mxgrey closed this Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant