Add support for spin_until_timeout (#1821) #1874

hliberacki · 2022-01-21T13:06:16Z

Introduce executors new spin_for method, replace spin_until_future_complete with spin_until_complete. (#1821)

* Introduce spin_for method.
* Introduce spin_until_complete.
* Deprecate spin_until_future_complete.
* Replace usage of deprecated method.
* Update unit-tests.

Signed-off-by: Hubert Liberacki <hliberacki@gmail.com>

rclcpp/include/rclcpp/executor.hpp

alsora · 2022-01-21T13:41:24Z

I can see value in this new utility.
However, the implementation is almost a copy-paste of spin_until_future_complete.

What if we make this new function more generic, by taking as an extra argument a function that signals that we need to exit early.
Then, spin_until_future_complete would just invoke your new function passing as "early_exit" argument something like this

      // Check if the future is set, return SUCCESS if it is.
      status = future.wait_for(std::chrono::seconds(0));
      if (status == std::future_status::ready) {
        return FutureReturnCode::SUCCESS;
      }

hliberacki · 2022-01-21T13:53:00Z

Thanks for the feedback.

You are right, it is basically the simplified version of spin_until_future_complete.

I did it as it is, only because it's my first change and I did not want to break anything - but my initial idea was to make one generic method.

I'll make it more generic.

hliberacki · 2022-01-21T14:33:05Z

@alsora I was also thinking about API stability, because this could be easily changed to spin_until_complete and then define the completion predicate as an lambda or simply template parameter, which is already there. Yet again I wasn't sure how much API can change within versions.

alsora · 2022-01-21T14:37:30Z

I think we wouldn't break any API as we would keep spin_until_future_complete with the existing signature and just change its internal implementation to invoke this more generic spin_until_complete.

clalancette · 2022-01-21T14:37:51Z

@alsora I was also thinking about API stability, because this could be easily changed to spin_until_complete and then define the completion predicate as an lambda or simply template parameter, which is already there. Yet again I wasn't sure how much API can change within versions.

In general, it is allowed to change API between versions in a "tick-tock" cycle. That is, for public APIs, we deprecate the old API in one release, add in the new one. In the next release, we remove the old one. This gives time for downstream users to switch to the new API. Note, however, that if you deprecate an API, all of the core of ROS 2 has to change at the same time, since we count deprecation warnings as regressions in the core.

In this particular case, this is a very commonly used function. So I wouldn't break the API here without really good justification.

SteveMacenski · 2022-01-21T20:34:33Z

Interestingly, I suggested a similar API change awhile back #1712. The reason this hasn't started yet is due to the lack of desire for churn in the API without a cause. Once we change something more significant or support non-futures, I would jump on that opportunity to make the change in name to coincide with a necessary change.

rclcpp/include/rclcpp/executor.hpp

rclcpp/test/rclcpp/executors/test_executors.cpp

ivanpauno

I'm not sure if it's a good idea to continue adding more flavors of spin_*(), but the implementation lgtm!

rclcpp/include/rclcpp/executor.hpp

alsora · 2022-03-17T08:48:58Z

Hi @hliberacki
it looks like there are still some open comments in this PR.

In particular:

correctly handle setting and un-setting is_spinning_
whether to merge this new function into an existing one or not

hliberacki · 2022-03-17T08:59:25Z

Hi @hliberacki it looks like there are still some open comments in this PR.

In particular:
* correctly handle setting and un-setting `is_spinning_`

* whether to merge this new function into an existing one or not

Hi @alsora,

Yeah sorry for that it's pending for that long - I had some fire in my day-to-day project.
I'm getting back to this PR, to make it done.

At the moment I would need to know which way would be the best fit, your first comment #1874 (comment) - which is significantly easier or @SteveMacenski comment #1874 (comment) to extend spin() with timeout argument.

Even though I find @SteveMacenski solution more robust - it will be a lot of changes because I need to change most of spin calls in the project (both in Python and C++) - which is a lot ;). Making it a default argument, since doesn't drastically change the amount of work.

Some part of the work is already done, but still, I would need to fix tests - parts of python codebase and probably nodes which I did not take look into.

I could also take your suggestion and we can create a second ticket - to handle altering spin API (adding timeout).

How do you feel about it?

And again sorry for being idle that long - not gonna happen again :)

alsora · 2022-03-17T11:23:36Z

No worries!
Just keep in mind that if we want this feature to be available in ROS 2 H-release, it will need to be merged before the 4th of April.

About whether to integrate this into spin or into spin_until_.. I can see pro and cons to both approaches.
I would personally prefer to leave spin untouched.

The spin() function is the default one, that we recommend most people to use (e.g. create a main that loads a node and make it spin forever).
For this reason, I would like for this function to be as fast and as simple as possible and to encourage the good pattern of setup a ROS node, make it spin and forget about it.

Then, next to this, I can see a variety of other "spin functionalities" that can be provided by one or more functions.
All these alternatives would follow the paradigm of "spin until X happens", where X would be:

a timeout
no more work available
a future is ready
etc

In theory all these "spin until X" could be done using a single, configurable function.

hliberacki · 2022-03-17T14:23:01Z

No worries! Just keep in mind that if we want this feature to be available in ROS 2 H-release, it will need to be merged before the 4th of April.

About whether to integrate this into spin or into spin_until_.. I can see pro and cons to both approaches. I would personally prefer to leave spin untouched.

The spin() function is the default one, that we recommend most people to use (e.g. create a main that loads a node and make it spin forever). For this reason, I would like for this function to be as fast and as simple as possible and to encourage the good pattern of setup a ROS node, make it spin and forget about it.

Then, next to this, I can see a variety of other "spin functionalities" that can be provided by one or more functions. All these alternatives would follow the paradigm of "spin until X happens", where X would be:
* a timeout

* no more work available

* a future is ready

* etc
In theory all these "spin until X" could be done using a single, configurable function.

Ok, it seems a better fit for the moment. I will take care of that this week

SteveMacenski · 2022-03-17T17:33:00Z

Even though I find @SteveMacenski solution more robust - it will be a lot of changes because I need to change most of spin calls in the project (both in Python and C++) - which is a lot ;)

Think about it this way, every time anyone uses the usual spin() API you can think to yourself that you made an impact there! I would think of changing a bunch of stuff as a big positive. Plus you brought up a good point and its a compliment that I want to interact with it every day 😉

I know its unfair that different maintainers are telling you different things -- just to see if my summarization of this conversation is correct to see if we can derive some sense of consensus

I suggested spin(timeout) and got 👍 from @fujitatomoya and @hliberacki since we didn't like the spin_until_timeout API.
@ivanpauno didn't like to keep adding more of the spin_... apis
@alsora doesn't like changing spin() since its the most used and perfers to create a single named function spin_XYZ that can be overloaded for use with futures, timeouts, etc as needed

It sounds to me like the 2 options to make the most people happy

Do spin(timeout)
Create a new spin_until_complete() API that we change all existing spin_until_future_completes to & add the timeout option. Which is interestingly the same idea I brought up about a year ago in [API Rename] spin_until_future_complete to spin_until_complete #1712

I actually prefer the second idea, which would make me, Alberto, and Ivan happy

clalancette · 2022-03-17T17:43:55Z

Keep in mind that #1712 (comment) still applies here; if we are going to rename it, we'll have to do all of the steps listed in that comment.

SteveMacenski · 2022-03-17T17:47:19Z

Definitely! I'm just trying to build consensus for a direction at this point -- I'm fine with either option

ivanpauno · 2022-03-17T18:04:51Z

Do spin(timeout)

Would this be a new overload of the current spin()?
That sounds fine to me, though I like a bit more having a differently named method.

Create a new spin_until_complete()

I would rather not see the word "until" in the name for this overload.
I think it's common in cpp that "until" methods receive a timepoint, and "for" methods receive a duration/period.
So in this case I would expect the name to be spin_for() (if I'm understanding the implementation correctly).
e.g.: std::this_thread::sleep_for() vs std::this_thread::sleep_until()
e.g.: asio::io_context::run_until() vs asio::io_context::run_for()

ivanpauno

LGTM with the minor naming issue resolved

SteveMacenski · 2022-03-17T18:37:31Z

I don't have a strong preference for spin_for vs spin_until, though I have a slight preference for until but I think that's mostly just because that's the phrasing we have now. That's not a good technical reason so I'll yield to for if others agree

hliberacki · 2022-03-17T20:10:01Z

Even though I find @SteveMacenski solution more robust - it will be a lot of changes because I need to change most of spin calls in the project (both in Python and C++) - which is a lot ;)

Think about it this way, every time anyone uses the usual spin() API you can think to yourself that you made an impact there! I would think of changing a bunch of stuff as a big positive. Plus you brought up a good point and its a compliment that I want to interact with it every day 😉

I know its unfair that different maintainers are telling you different things -- just to see if my summarization of this conversation is correct to see if we can derive some sense of consensus

I suggested spin(timeout) and got 👍 from @fujitatomoya and @hliberacki since we didn't like the spin_until_timeout API.

@ivanpauno didn't like to keep adding more of the spin_... apis

@alsora doesn't like changing spin() since its the most used and perfers to create a single named function spin_XYZ that can be overloaded for use with futures, timeouts, etc as needed

It sounds to me like the 2 options to make the most people happy

Do spin(timeout)

Create a new spin_until_complete() API that we change all existing spin_until_future_completes to & add the timeout option. Which is interestingly the same idea I brought up about a year ago in [API Rename] spin_until_future_complete to spin_until_complete #1712

I actually prefer the second idea, which would make me, Alberto, and Ivan happy

For starters thanks for the feedback @SteveMacenski . I guess following your previous idea - #1712 would be a good tradeoff between overloading spin and creating spin_until_timeout - which I originally did.

I know its unfair that different maintainers are telling you different things -- just to see if my summarization of this >conversation is correct to see if we can derive some sense of consensus

That's not a problem at all, I am just still a newbie Ros wise so it's hard for me to pick the best approach.

I will then implement as it's described in #1712. As per naming, to me for and until sounds equaly good.

hliberacki · 2022-03-25T15:43:58Z

Since the most concern was about the end API - I thought that it might be beneficial that I just push how I've updated the public API executors - from one of my first commits. I've done far more than this single change- but this is still a draft that I will push as a complete result. I'd like to only know whether you feel ok with API approach (executor.hpp). Then I would not need to update all the unit tests if the API is not fine. I am about to push more implementation this weekend. @SteveMacenski it this what you meant while you were describing #1712?

hliberacki · 2022-03-25T15:47:46Z

rclcpp/include/rclcpp/executor.hpp

@@ -319,6 +320,53 @@ class Executor
  virtual void
  spin_once(std::chrono::nanoseconds timeout = std::chrono::nanoseconds(-1));

+  // TODO(hliberacki): Add documentation
+  template<typename FutureT, typename DurationT = std::chrono::milliseconds,
+    // typename std::enable_if_t<


I was wondering about that? This is commented out since this type_trait is far from being readable or pretty - yet it's more accurate than a single negation - typename std::enable_if_t<!std::is_invocable_v<FutureT>, bool> = true. Not sure what suites the codebase better.

Also I was wondering about C++17 support - I know that several companies are based on ros2 (like Apex) and they uses C++14 - can we freely use all c++17 features? If so I will just drop if constexpr instead of SFINAE

@alsora @SteveMacenski - ideas would be appreciated - I am still a newbie here hence the question ;)

I believe Humble targets C++17, so I see no reason why not to use any C++17 features you like, but I am definitely not the right person to ask. Someone at OR would have to jump in to let us know if we should be trying to minimize its use to help in C++14 compatibility for other important users

typename std::enable_if_t<!std::is_invocable_v, bool> = true

It'll be hard to understand in the future why std::is_invocable_v was used.
I would rather have the more accurate condition (even if it doesn't look pretty) or two overloads.

@ivanpauno At the moment i've decided to use if constexpr and reduce second check whether the Condition is a of type Future or SharedFuture. I did it because realize that there are more use cases, like using src/ros2/rclcpp/rclcpp/include/rclcpp/client.hpp fromsrc/ros2/rclcpp/rclcpp/include/rclcpp/client.hpp.

If you still would like to have more grain granularity - then I would suggest to create Concept like requirement of the API for the given condition. So it would have to have wait_for(timeout) method which returns enum class future_status. I did not introduce it for now, on purpose because I am not sure whether it will be valuable to have that type_trait which is a bit hard to read. But If needed I can introduce it - no problem with that.

rclcpp/include/rclcpp/executor.hpp

fujitatomoya · 2022-03-25T16:20:26Z

@hliberacki is this PR still draft? you are still working on this as PoC?

hliberacki · 2022-03-25T16:28:22Z

@hliberacki is this PR still draft? you are still working on this as PoC?

I'm still working on that. i have changed status from open to draft to mark that the patch that i currently committed is just a question about the the API ans not an actual final solution - to avoid confusion.

i have done far more and intensified my work since yesterday - so this is definitely on going (at the moment I'm extending unit tests for executors). but since I'm doing it which take a bit of time I'd like to confirm how the revievers feel about the API itself so i would not polish thing (document, unit test) that has to be changed. thats why I've pushed it in that state, hopefully it makes sense :)

also I know that there is a release in the begging of April therefore i wanted to have some very basic feedback before the weekend, because i would like to finalize it during the weekend or be close to do it.

alsora · 2022-06-13T10:08:53Z

Yes, the PR is good to go for me.
@wjwwood do you consider your requested changes to be addressed?
If so, would you mind merging the whole thing?

Blast545 · 2022-06-24T15:11:45Z

👨‍🌾 This seems to have introduced several build regressions on the linux buildfarm, it's failing on system_tests, can you take a look? @hliberacki

See:
https://build.ros2.org/view/Rci/job/Rci__nightly-release_ubuntu_jammy_amd64/118/console
https://ci.ros2.org/view/nightly/job/nightly_linux_debug/2338/console

I'm opening a revert PR to be sure this is causing the problem and merge before EOD if we don't find another way to address it. @clalancette

clalancette · 2022-06-24T15:17:50Z

We definitely missed merging at least ros2/system_tests#499 , there may be others. I'll go see if I can track them down.

clalancette · 2022-06-24T15:31:56Z

So the complete list that needs to be merged alongside this is:

I'm not at all sure all of this churn was worth a pretty subjective name change, but here we are.

So we can either revert this (which is the easiest thing to do), or we can merge forward all of the rest of the bits. The problem with merging forward is that not all of the PRs are approved, so someone is going to have to go through, make sure that they are reviewed and approved, and then finish merging them up.

@fujitatomoya @wjwwood @SteveMacenski @alsora Can you go through and review all of the dependent PRs here, and then, assuming they are good, merge the rest? If we can't get it done by the end of the day, then @Blast545 let's revert.

hliberacki · 2022-06-24T15:47:04Z

@clalancette most of that (all?) has already been reviewed, so it shall be rather straight forward.

hliberacki · 2022-06-24T16:04:15Z

So the complete list that needs to be merged alongside this is:

Update spin_until_complete rclpy#919

Replace deprecated spin_until_future_complete system_tests#499

Replace deprecated spin_until_future_complete ros2cli#704

Replace deprecated spin_until_future_complete with spin_until_complete launch_ros#306

Replace deprecated spin_until_future_complete geometry2#512

Replace deprecated spin_until_future_complete examples#339

Update usage of spin_until_future_complete demos#556

Deprecated spin_until_future_complete ros-visualization/interactive_markers#90

https://gitlab.com/ros-tracing/ros2_tracing/-/merge_requests/291

Replace deprecated spin_until_future_complete ros-visualization/rqt_reconfigure#111

I'm not at all sure all of this churn was worth a pretty subjective name change, but here we are.

So we can either revert this (which is the easiest thing to do), or we can merge forward all of the rest of the bits. The problem with merging forward is that not all of the PRs are approved, so someone is going to have to go through, make sure that they are reviewed and approved, and then finish merging them up.

@fujitatomoya @wjwwood @SteveMacenski @alsora Can you go through and review all of the dependent PRs here, and then, assuming they are good, merge the rest? If we can't get it done by the end of the day, then @Blast545 let's revert.

unfortunately replying on my phone, so tis hard to check the list myself but
all of the related PRs are linked with this PR - they are listed in the comment section.

wjwwood · 2022-06-24T16:51:21Z

Ah this is my fault, I should have merged the corresponding prs, I was on my phone and didn't see the cross-references. In retrospect it's obvious that this was required.

christophebedard · 2022-06-24T17:40:08Z

https://gitlab.com/ros-tracing/ros2_tracing/-/merge_requests/291

Should I proceed with merging this, or wait?

clalancette · 2022-06-24T17:41:57Z

Should I proceed with merging this, or wait?

Let's wait for the moment. We're still not sure whether we are going forward or back here. We'll ping you once we want you to merge.

wjwwood · 2022-06-24T18:05:37Z

So, @clalancette and I discussed it and I'm going to revert this.

The rclpy pr is not ready and it blocks the merge of the system_tests pr because that pr changes both C++ and Python uses of this function, but since this pr was merged prematurely by me, system_tests is emitting deprecation warnings (currently escalated to errors, but we're changing that in ros2/system_tests#503).

I could have split the system_tests pr to be C++ and Python separately, but I don't have time to do that right this second, and so I'm going to revert this for now until we can do this properly and all at once.

…uture_complete with spin_until_complete. (#1821) (#1874)" This reverts commit 3c8e89d.

…uture_complete with spin_until_complete. (#1821) (#1874)" (#1956) This reverts commit 3c8e89d.

…_until_future_complete with spin_until_complete. (#1821) (#1874)" (#1956)" This reverts commit f43a919.

hliberacki · 2022-06-24T18:22:33Z

So, @clalancette and I discussed it and I'm going to revert this.

The rclpy pr is not ready and it blocks the merge of the system_tests pr because that pr changes both C++ and Python uses of this function, but since this pr was merged prematurely by me, system_tests is emitting deprecation warnings (currently escalated to errors, but we're changing that in ros2/system_tests#503).

I could have split the system_tests pr to be C++ and Python separately, but I don't have time to do that right this second, and so I'm going to revert this for now until we can do this properly and all at once.

@wjwwood thanks for taking care of that. i will try to fix your findings in rclpy as soon as possible.

wjwwood · 2022-06-24T18:22:59Z

I've revert this in #1956 and opened an un-revert #1957 which we can merge once the rest or the prs are approved.

Sorry again for the noise.

SteveMacenski · 2022-06-24T18:40:39Z

Can you go through and review all of the dependent PRs here

Can do

SteveMacenski · 2022-06-24T18:48:03Z

Something that also should probably be updated is documentation on docs.ros.org. Migration guide mention for Iron + update the tutorials. I took a look and its a pretty trivial number of changes. I'll do that now

SteveMacenski · 2022-06-24T18:52:40Z

ros2/ros2_documentation#2798 Docs PR

Ryanf55 · 2023-06-13T18:40:05Z

Hey there, it seems like this didn't get in to humble. Looking at the examples in humble, it's not clear to me how to handle timeouts in services in humble. Would anyone be able to assist with a demo on how to not block an application if a service server fails to return? I can open up a separate issue if needed.

fujitatomoya · 2023-06-15T22:24:37Z

@clalancette @wjwwood @alsora any objection for taking these PRs? i think this is useful. you can see the entire PR list here, #1874 (comment)

alsora · 2023-06-19T10:26:24Z

Yes, I agree that this is a very useful feature to have.

hliberacki force-pushed the hliberacki/timeout branch from f69da46 to 5a6122a Compare January 21, 2022 13:21

alsora reviewed Jan 21, 2022

View reviewed changes

rclcpp/include/rclcpp/executor.hpp Outdated Show resolved Hide resolved

SteveMacenski reviewed Jan 21, 2022

View reviewed changes

rclcpp/include/rclcpp/executor.hpp Outdated Show resolved Hide resolved

fujitatomoya reviewed Jan 21, 2022

View reviewed changes

clalancette assigned alsora Feb 10, 2022

ivanpauno reviewed Mar 7, 2022

View reviewed changes

rclcpp/include/rclcpp/executor.hpp Outdated Show resolved Hide resolved

ivanpauno reviewed Mar 17, 2022

View reviewed changes

hliberacki marked this pull request as draft March 25, 2022 15:38

hliberacki force-pushed the hliberacki/timeout branch from 5a6122a to fd895b5 Compare March 25, 2022 15:39

hliberacki commented Mar 25, 2022

View reviewed changes

rclcpp/include/rclcpp/executor.hpp Outdated Show resolved Hide resolved

ivanpauno requested a review from wjwwood June 16, 2022 13:25

wjwwood approved these changes Jun 24, 2022

View reviewed changes

wjwwood merged commit 3c8e89d into ros2:master Jun 24, 2022

wjwwood added a commit that referenced this pull request Jun 24, 2022

Revert "Introduce executors new spin_for method, replace spin_until_f…

b48d8df

…uture_complete with spin_until_complete. (#1821) (#1874)" This reverts commit 3c8e89d.

wjwwood mentioned this pull request Jun 24, 2022

Revert "Add support for spin_until_timeout (#1821)" #1956

Merged

wjwwood added a commit that referenced this pull request Jun 24, 2022

Revert "Introduce executors new spin_for method, replace spin_until_f…

f43a919

…uture_complete with spin_until_complete. (#1821) (#1874)" (#1956) This reverts commit 3c8e89d.

wjwwood added a commit that referenced this pull request Jun 24, 2022

Revert "Revert "Introduce executors new spin_for method, replace spin…

0f90756

…_until_future_complete with spin_until_complete. (#1821) (#1874)" (#1956)" This reverts commit f43a919.

wjwwood mentioned this pull request Jun 24, 2022

Revert "Revert "Add support for spin_until_timeout (#1821)"" #1957

Closed

clalancette mentioned this pull request Jun 24, 2022

Remove Werror from test_quality_of_service. ros2/system_tests#503

Merged

SteveMacenski mentioned this pull request Jun 24, 2022

renaming spin until future complete to spin until complete + add to migration guide for Iron ros2/ros2_documentation#2798

Closed

audrow mentioned this pull request Feb 13, 2023

Revert "Revert "adding API change for spin_until_complete"" ros2/ros2_documentation#3334

Closed

christophebedard mentioned this pull request Apr 2, 2024

Add support for spin_until_complete (take 2) #2475

Open

Add support for spin_until_timeout (#1821) #1874

Add support for spin_until_timeout (#1821) #1874

Conversation

hliberacki commented Jan 21, 2022 • edited Loading

alsora commented Jan 21, 2022

hliberacki commented Jan 21, 2022

hliberacki commented Jan 21, 2022

alsora commented Jan 21, 2022

clalancette commented Jan 21, 2022

SteveMacenski commented Jan 21, 2022

ivanpauno left a comment

Choose a reason for hiding this comment

alsora commented Mar 17, 2022

hliberacki commented Mar 17, 2022

alsora commented Mar 17, 2022 • edited Loading

hliberacki commented Mar 17, 2022

SteveMacenski commented Mar 17, 2022 • edited Loading

clalancette commented Mar 17, 2022

SteveMacenski commented Mar 17, 2022 • edited Loading

ivanpauno commented Mar 17, 2022

ivanpauno left a comment

Choose a reason for hiding this comment

SteveMacenski commented Mar 17, 2022

hliberacki commented Mar 17, 2022

hliberacki commented Mar 25, 2022

hliberacki Mar 25, 2022

Choose a reason for hiding this comment

SteveMacenski Mar 25, 2022

Choose a reason for hiding this comment

ivanpauno Mar 25, 2022

Choose a reason for hiding this comment

hliberacki Mar 28, 2022

Choose a reason for hiding this comment

fujitatomoya commented Mar 25, 2022

hliberacki commented Mar 25, 2022

alsora commented Jun 13, 2022

Blast545 commented Jun 24, 2022

clalancette commented Jun 24, 2022

clalancette commented Jun 24, 2022

hliberacki commented Jun 24, 2022

hliberacki commented Jun 24, 2022

wjwwood commented Jun 24, 2022

christophebedard commented Jun 24, 2022

clalancette commented Jun 24, 2022

wjwwood commented Jun 24, 2022

hliberacki commented Jun 24, 2022

wjwwood commented Jun 24, 2022

SteveMacenski commented Jun 24, 2022

SteveMacenski commented Jun 24, 2022

SteveMacenski commented Jun 24, 2022

Ryanf55 commented Jun 13, 2023

fujitatomoya commented Jun 15, 2023

alsora commented Jun 19, 2023

hliberacki commented Jan 21, 2022 •

edited

Loading

alsora commented Mar 17, 2022 •

edited

Loading

SteveMacenski commented Mar 17, 2022 •

edited

Loading

SteveMacenski commented Mar 17, 2022 •

edited

Loading