RSDK-5881 Add Stoppable class #186

benjirewis · 2023-11-29T16:38:46Z

RSDK-5881

Adds a Stoppable class with a purely virtual stop method from which stoppable resources should inherit.

stuqdog

A couple minor things but this looks great, thanks for hopping on it so fast!

src/viam/sdk/robot/service.cpp

src/viam/sdk/resource/resource.cpp

src/viam/sdk/resource/resource.hpp

src/viam/sdk/robot/service.cpp

src/viam/examples/modules/tflite/main.cpp

src/viam/sdk/services/mlmodel/mlmodel.hpp

src/viam/examples/modules/tflite/main.cpp

src/viam/sdk/resource/resource.cpp

stuqdog

lgtm!

acmorrow

I think this is very close, but I think stop in TFLite was actually reachable and should continue to be.

acmorrow · 2023-12-01T21:37:48Z

src/viam/sdk/module/service.cpp

@@ -133,7 +134,7 @@ ::grpc::Status ModuleService_::ReconfigureResource(

    // if the type isn't reconfigurable by default, replace it
    try {
-        res->stop();
+        Stoppable::stop_if_stoppable(res);


So ... am I reading this right that the only way to reach this code is if reconfigure threw an exception that did not derive from std::exception?

Also,

That sounds correct; my understanding of that code is to call stop if the resource does not actually implement a reconfigure method (and throws some exception that's not std::exception?) and reconstruct the resource instead of reconfiguring.

That makes me wonder if we need a Reconfigurable mixin, much like Stoppable. The status quo is that all Resource implementations must implement reconfigure, but with an escape hatch where throwing an exception (but only one that doesn't derive from std::exception!) is the signal that the resource doesn't actually support reconfiguration. I'm not sure I'm too keen on that design. Maybe a ticket is in order?

Yeah I think a Reconfigurable class is better. Filed https://viam.atlassian.net/browse/RSDK-5967.

acmorrow · 2023-12-01T21:49:41Z

src/viam/sdk/module/service.cpp

@@ -194,7 +195,7 @@ ::grpc::Status ModuleService_::RemoveResource(
    }

    try {
-        res->stop();
+        Stoppable::stop_if_stoppable(res);


Here and above, in the past, if the implementation object inside a module had an override of Resource::stop, it'd get called here. Now, it won't, unless the implementation object explicitly derives from Stoppable. That seems like a breaking change for modules.

Correct if I'm understanding you.

Now, it won't, unless the implementation object explicitly derives from Stoppable. That seems like a breaking change for modules.

I actually thought this was desirable, and that we were OK with the sort of "opt-in" behavior here of "if you want your resource to be stopped on modular removal or StopAll, you must explictitly inherit Stoppable and override stop".

What I'm trying to get at is that if you have an existing resource implemented (presumably as a module), it almost certainly has a stop method defined, because it had to: Resource::stop was pure virtual so you had to provide a definition.

And, that definition had paths that called it: from ModuleService::reconfigureResource if a non-std::exception exception was thrown, and from ModuleService::RemoveResource. And the steps taken in that stop may have been important.

After this change, the resource defined in the module will still have a stop method, but it won't be Resource::stop and it won't be Stoppable::stop: it will just be the stop method of the resource. None of the management code here can ever cause that stop to be invoked.

So it is something of a breaking change, in that if the implementation of your module was relying on stop in order to manage shutdown, it won't be getting called anymore. As an example, you needed to make changes to the TFLite module here.

So it is something of a breaking change, in that if the implementation of your module was relying on stop in order to manage shutdown, it won't be getting called anymore. As an example, you needed to make changes to the TFLite module here.

Yeah it's a good point, and after discussing offline, I think we should have MLModelServiceTFLite : public Stoppable for now despite Stoppable now modeling both gRPC stop methods and stop (acting as a "cleanup" method) for modular resources.

acmorrow · 2023-12-01T21:56:49Z

src/viam/examples/modules/tflite/main.cpp

@@ -71,19 +71,16 @@ class MLModelServiceTFLite : public vsdk::MLModelService {
        // here. It should be safe to tear down all state
        // automatically without needing to wait for anything more to
        // drain.
+        close();


Please see my comment over in module/service.cpp, but I suspect the status quo is that the stop method was in fact reachable from ModuleService_, probably from RemoveResource, and maybe from ReconfigureResource under some circumstances. I wonder if the move here is to leave this as close, and have MLModelServiceTFLite derive from Stoppable instead. It's a little wrong, in that it doesn't model a "stop GRPC", but at least it keeps the stop that the ModuleService and RobotService want to call on resources wired into the stopp-y behavior of this type.

Can we try putting this back to having a stop method but having MLModelServiceTFLite derive from Stoppable? I think that would mean that the stop_if_stoppable calls from ModuleService_ would actually do the right thing, and then we wouldn't need to introduce the new close concept.

Yep! Derived MLModelServiceTFLite from Stoppable and maintained its stop implementation present on main.

acmorrow

This LGTM except that 1) I think the TFLite service should be made to derive from Stoppable rather than introducing the new close stuff, please see my inline comments to that effect, and 2) I think there is a subtle breaking change here, which maybe needs some messaging so module implementers know what to do.

acmorrow · 2023-12-05T16:11:00Z

src/viam/sdk/module/service.cpp

@@ -133,7 +134,7 @@ ::grpc::Status ModuleService_::ReconfigureResource(

    // if the type isn't reconfigurable by default, replace it
    try {
-        res->stop();
+        Stoppable::stop_if_stoppable(res);


That makes me wonder if we need a Reconfigurable mixin, much like Stoppable. The status quo is that all Resource implementations must implement reconfigure, but with an escape hatch where throwing an exception (but only one that doesn't derive from std::exception!) is the signal that the resource doesn't actually support reconfiguration. I'm not sure I'm too keen on that design. Maybe a ticket is in order?

acmorrow · 2023-12-05T16:16:59Z

src/viam/sdk/module/service.cpp

@@ -194,7 +195,7 @@ ::grpc::Status ModuleService_::RemoveResource(
    }

    try {
-        res->stop();
+        Stoppable::stop_if_stoppable(res);


What I'm trying to get at is that if you have an existing resource implemented (presumably as a module), it almost certainly has a stop method defined, because it had to: Resource::stop was pure virtual so you had to provide a definition.

And, that definition had paths that called it: from ModuleService::reconfigureResource if a non-std::exception exception was thrown, and from ModuleService::RemoveResource. And the steps taken in that stop may have been important.

After this change, the resource defined in the module will still have a stop method, but it won't be Resource::stop and it won't be Stoppable::stop: it will just be the stop method of the resource. None of the management code here can ever cause that stop to be invoked.

So it is something of a breaking change, in that if the implementation of your module was relying on stop in order to manage shutdown, it won't be getting called anymore. As an example, you needed to make changes to the TFLite module here.

acmorrow · 2023-12-05T16:19:28Z

src/viam/examples/modules/tflite/main.cpp

@@ -71,19 +71,16 @@ class MLModelServiceTFLite : public vsdk::MLModelService {
        // here. It should be safe to tear down all state
        // automatically without needing to wait for anything more to
        // drain.
+        close();


Can we try putting this back to having a stop method but having MLModelServiceTFLite derive from Stoppable? I think that would mean that the stop_if_stoppable calls from ModuleService_ would actually do the right thing, and then we wouldn't need to introduce the new close concept.

benjirewis requested review from acmorrow and stuqdog November 29, 2023 16:39

benjirewis assigned ale7714 Nov 29, 2023

benjirewis marked this pull request as ready for review November 29, 2023 16:48

benjirewis requested a review from a team as a code owner November 29, 2023 16:48

benjirewis requested review from purplenicole730 and removed request for a team November 29, 2023 16:48

stuqdog requested changes Nov 30, 2023

View reviewed changes

src/viam/sdk/robot/service.cpp Outdated Show resolved Hide resolved

src/viam/sdk/resource/resource.cpp Outdated Show resolved Hide resolved

src/viam/sdk/resource/resource.cpp Outdated Show resolved Hide resolved

acmorrow requested changes Nov 30, 2023

View reviewed changes

benjirewis requested review from stuqdog and acmorrow November 30, 2023 17:18

stuqdog approved these changes Nov 30, 2023

View reviewed changes

benjirewis added 2 commits December 1, 2023 20:46

RSDK-5881 Add Stoppable class

85e0f77

comments

c6e6d78

benjirewis force-pushed the stoppable-resource branch from 29ab467 to 98c0038 Compare December 1, 2023 20:51

MLModelService not Stoppable

98c0038

acmorrow reviewed Dec 1, 2023

View reviewed changes

acmorrow approved these changes Dec 5, 2023

View reviewed changes

benjirewis mentioned this pull request Dec 5, 2023

RSDK-5902 - misc. clang tidy fixes #188

Merged

MLModelServiceTFLite : public Stoppable

f9e4190

benjirewis merged commit 25b38e2 into viamrobotics:main Dec 6, 2023
2 checks passed

benjirewis deleted the stoppable-resource branch December 6, 2023 00:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RSDK-5881 Add Stoppable class #186

RSDK-5881 Add Stoppable class #186

benjirewis commented Nov 29, 2023 •

edited by jira bot

Loading

stuqdog left a comment

stuqdog left a comment

acmorrow left a comment

acmorrow Dec 1, 2023

benjirewis Dec 1, 2023

acmorrow Dec 5, 2023

benjirewis Dec 6, 2023

acmorrow Dec 1, 2023

benjirewis Dec 1, 2023

acmorrow Dec 5, 2023

benjirewis Dec 6, 2023

acmorrow Dec 1, 2023 •

edited

Loading

acmorrow Dec 5, 2023

benjirewis Dec 6, 2023

acmorrow left a comment

acmorrow Dec 5, 2023

acmorrow Dec 5, 2023

acmorrow Dec 5, 2023

RSDK-5881 Add Stoppable class #186

RSDK-5881 Add Stoppable class #186

Conversation

benjirewis commented Nov 29, 2023 • edited by jira bot Loading

stuqdog left a comment

Choose a reason for hiding this comment

stuqdog left a comment

Choose a reason for hiding this comment

acmorrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acmorrow Dec 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acmorrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjirewis commented Nov 29, 2023 •

edited by jira bot

Loading

acmorrow Dec 1, 2023 •

edited

Loading