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

Add support for spin_until_timeout (#1821) #1874

Merged
merged 1 commit into from
Jun 24, 2022

Conversation

hliberacki
Copy link
Contributor

@hliberacki hliberacki commented Jan 21, 2022

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 <[email protected]>

@alsora
Copy link
Collaborator

alsora commented Jan 21, 2022

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
Copy link
Contributor Author

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
Copy link
Contributor Author

@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
Copy link
Collaborator

alsora commented Jan 21, 2022

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
Copy link
Contributor

@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
Copy link
Collaborator

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 Outdated Show resolved Hide resolved
rclcpp/test/rclcpp/executors/test_executors.cpp Outdated Show resolved Hide resolved
rclcpp/test/rclcpp/executors/test_executors.cpp Outdated Show resolved Hide resolved
rclcpp/test/rclcpp/executors/test_executors.cpp Outdated Show resolved Hide resolved
Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
@alsora
Copy link
Collaborator

alsora commented Mar 17, 2022

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
Copy link
Contributor Author

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
Copy link
Collaborator

alsora commented Mar 17, 2022

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
Copy link
Contributor Author

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
Copy link
Collaborator

SteveMacenski commented Mar 17, 2022

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

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

@clalancette
Copy link
Contributor

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
Copy link
Collaborator

SteveMacenski commented Mar 17, 2022

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

@ivanpauno
Copy link
Member

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()

Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

LGTM with the minor naming issue resolved

@SteveMacenski
Copy link
Collaborator

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
Copy link
Contributor Author

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

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 hliberacki marked this pull request as draft March 25, 2022 15:38
@hliberacki
Copy link
Contributor Author

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?

@@ -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<
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ;)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

@fujitatomoya
Copy link
Collaborator

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

@hliberacki
Copy link
Contributor Author

@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
Copy link
Collaborator

alsora commented Jun 13, 2022

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?

@ivanpauno ivanpauno requested a review from wjwwood June 16, 2022 13:25
@wjwwood wjwwood merged commit 3c8e89d into ros2:master Jun 24, 2022
@Blast545
Copy link
Contributor

👨‍🌾 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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

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

@hliberacki
Copy link
Contributor Author

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.

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
Copy link
Member

wjwwood commented Jun 24, 2022

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
Copy link
Member

Should I proceed with merging this, or wait?

@clalancette
Copy link
Contributor

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
Copy link
Member

wjwwood commented Jun 24, 2022

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 added a commit that referenced this pull request Jun 24, 2022
…uture_complete with spin_until_complete. (#1821) (#1874)"

This reverts commit 3c8e89d.
wjwwood added a commit that referenced this pull request Jun 24, 2022
…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
…_until_future_complete with spin_until_complete. (#1821) (#1874)" (#1956)"

This reverts commit f43a919.
@hliberacki
Copy link
Contributor Author

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
Copy link
Member

wjwwood commented Jun 24, 2022

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
Copy link
Collaborator

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

Can do

@SteveMacenski
Copy link
Collaborator

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
Copy link
Collaborator

ros2/ros2_documentation#2798 Docs PR

@Ryanf55
Copy link

Ryanf55 commented Jun 13, 2023

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
Copy link
Collaborator

@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
Copy link
Collaborator

alsora commented Jun 19, 2023

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

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.

10 participants