RSDK-3030 - Refactor registry #208

stuqdog · 2024-01-31T16:21:57Z

High Level Changes

Remove init method for registering resources pre-main. Instead, call a Registry::initialize() method to handle all registration.
Simplify ResourceRegistration2, move it into a register_resource method.
Move all resources onto ResourceRegistration2, removing the need to create unique {ResourceType}Registration classes.
Ensure that resource servers are properly registered when creating a RobotService such that resources acquired from that robot service can actually make gRPC calls.
- In order to accomplish this, I moved management of ResourceServers onto the Server class. I considered creating a top level Viam class (which would own both the servers and the initialize() method currently on Registry), but decided against this because it would then need to be passed around to various methods and thus would complicate user-facing APIs.
- this in turn led to a refactor of the ModuleService. Previously it was managing services by itself but this ended up being pretty duplicative of the new server-based ownership module, so I moved it over to just looking to the Server to manage everything.

…o refactor-registry

stuqdog · 2024-01-31T16:46:42Z

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

    Model mybase_model("viam", "base", "mybase");

+    // // CR erodkin: add note about how we've reduced the comment here.
+    // Make sure to explicity register resources with custom APIs.


Removed the "must be done in main and not in resource implementation" bit of this comment because the idea of doing registration pre-main doesn't really make sense anymore.

stuqdog · 2024-01-31T16:48:05Z

src/viam/sdk/robot/service.hpp


   private:
+    // CR erodkin: this is a bit of a hacky workaround, leave a comment about it
+    API api() const override;


We added the ability to look up API for a ResourceServer so that we can register them properly within the server. Unfortunately the RobotService is technically a ResourceServer! It doesn't really make sense for it to have an API that people can lookup so I just threw it in private to try and hide it. Curious if folks have thoughts on a better approach.

You can't really hide it because you can always call it via the base class pointer. Why doesn't the RobotService have an API? Shouldn't it just be the API of Robot?

There is no Robot base class, and there is no Robot API. Certainly not in the SDK, but (to my knowledge) not anywhere in the codebase. The type of an API (either component or service) breaks down with robot, because component or service is a resource type on the particular robot. It doesn't really make sense in my head to think of the API of a robot.

stuqdog · 2024-01-31T16:53:22Z

src/viam/sdk/rpc/server.hpp

-    // TODO: make `register_service` take one of our types as an arg rather than a
-    // grpc service type, and convert under the hood


I tried to do this but ran into weird issues. We can't just go to registering a ResourceServer type unless we make it clear under the hood that it's a grpc::Service. But, we can't have the ResourceServer base type inherit from grpc::Service because, e.g., MotorServer needs to inherit from MotorService, and then we get an ambiguous type hierarchy inference when trying to register. We could solve this by adding a to_grpc_service method that does appropriate typecasting or something like that, but this is pretty hacky and I don't love it. Add to this the fact that users (even those implementing new services) should never have to touch this method and it seemed not worth looking into this todo anymore.

Happy to revert if folks disagree.

I think it's fine to just take in the grpc::Service*.

stuqdog · 2024-01-31T17:18:08Z

src/viam/sdk/module/service.cpp

    }
 }

-void ModuleService::add_api_from_registry_inlock_(API api, const std::lock_guard<std::mutex>&) {


This was adding managed resource servers/resources from the registry to the module_. But, since all resource servers are now owned/maintained by the Server, this is no longer necessary.

stuqdog · 2024-01-31T17:18:44Z

src/viam/sdk/registry/registry.cpp

-    std::unordered_map<std::string, std::shared_ptr<ModelRegistration>> registry;
-    for (auto& resource : resources_) {
-        registry.emplace(resource.first, resource.second);
+const google::protobuf::ServiceDescriptor* Registry::get_service_descriptor_(


Just moving some logic from the header to the implementation file.

stuqdog · 2024-01-31T17:20:47Z

src/viam/sdk/registry/registry.cpp

    return service_descriptor_;
 }

+void register_resources() {


A bit of a gotcha for Viam developers, we'll now need to add new resources here. It should be hard to miss; client_to_mock_pipeline handles Registry initialization and so tests will fail if resources aren't registered.

stuqdog · 2024-01-31T20:02:54Z

src/viam/sdk/rpc/server.cpp

+    Registry::initialize();
+    for (const auto& rr : Registry::registered_resources()) {
+        auto new_manager = std::make_shared<ResourceManager>();
+        auto server = rr.second->create_resource_server(new_manager, *this);
+        managed_servers_.emplace(server->api(), std::move(server));


Register all known resources when creating the server, to ensure proper gRPC reflection.

I don't like having this logic (Registry initializing, creating of managed resource servers) happening inside the Server constructor. But, it does have the nice property of creating only a single point where all this is handled, and not requiring users to call initialize themselves (which could be a bit of a gotcha).

I tried moving this logic into the Server::start() call where I think it makes a bit more sense, but this created a problem where it was impossible to add a managed resource until the server had already started. This made it impossible, e.g., for the RobotService to make sure that all its managed resources were taken care of at construction.

Another consideration I had was removing static from all Registry methods, having the Server class own a single instance of a registry, and then we can handle all important registry initialization in the Registry constructor. This is nice and clean. But, it created a dependency loop (server.hpp would need to include registry.hpp, and registry.hpp needs to include server.hpp for the register_resource call. Moving the server.register_service call out of registry.hpp and into the server constructor (and thus solving this dependency loop) puts us in the case described above with ambiguous inheritance to grpc::service. We could add a to_grpc_service() method on all ResourceServers to account for this but 1) this creates extra boilerplate, and 2) making Registry non-static imposes other limitations (e.g., we can no longer register resources until a Server has been created). This would involve a fair amount of refactoring and testing and I'm not convinced would be a strict upgrade.

Given that this is relatively hidden from users (they should never have to deal with registration at this level, or deal with the look of this implementation), I thought the usefulness properties and lack of user-facing issues outweighed the slight code-smell that comes from it. Happy to revisit if others disagree.

Yeah I'm really feeling the lack of clarity around what the root of initialization and ownership is in the SDK. I don't think we are going to get there in this review.

😞 yeah that is fair. I suspect a second PR that focuses broadly on this specific question will be important in the near future.

acmorrow

@stuqdog - I'm starting into this review today. I've just taken a quick skim through so far. Is it fair to say that as far as the changes under src/viam/sdk/{components,services}/* that it is more or less identical changes for each of base, board, etc.? If so that will save me some time reviewing as I can take a close look at the changes for one component and one service and basically mark-as-read the others. It will be easier to see the real changes that way.

stuqdog · 2024-02-01T17:01:02Z

@stuqdog - I'm starting into this review today. I've just taken a quick skim through so far. Is it fair to say that as far as the changes under src/viam/sdk/{components,services}/* that it is more or less identical changes for each of base, board, etc.? If so that will save me some time reviewing as I can take a close look at the changes for one component and one service and basically mark-as-read the others. It will be easier to see the real changes that way.

@acmorrow hi yes, that is a fair assessment!

acmorrow · 2024-02-05T16:21:00Z

src/viam/sdk/CMakeLists.txt


 target_sources(viamsdk
  PRIVATE
-    # TODO(RSDK-1742): we have to put `registry` up top here because we need the


acmorrow · 2024-02-05T16:40:00Z

src/viam/sdk/registry/registry.cpp

+}
+
+void Registry::initialize() {
+    if (initialized_) {


This needs some sort of thread safety.

acmorrow · 2024-02-05T16:41:12Z

src/viam/sdk/resource/resource_server_base.cpp

    return manager_;
 };

+std::shared_ptr<ResourceManager>& ResourceServer::resource_manager() {


Return type seems dangerous here, as it would allow you to write:

some_resource_server.resource_manager() = nullptr;

Ahh yeah, good catch. Upon reflection I'm not sure why I added this at all. We already had a const version, and since the inner ResourceManager isn't const we can still call the setter methods. Will remove.

acmorrow · 2024-02-05T16:45:17Z

src/viam/sdk/robot/service.hpp


   private:
+    // CR erodkin: this is a bit of a hacky workaround, leave a comment about it
+    API api() const override;


You can't really hide it because you can always call it via the base class pointer. Why doesn't the RobotService have an API? Shouldn't it just be the API of Robot?

acmorrow · 2024-02-05T17:49:37Z

src/viam/sdk/registry/registry.cpp


+void register_resources() {
+    // Register all components
+    Registry::register_resource<BaseClient, BaseServer, component::base::v1::BaseService>(


Whenever I see code like this, I start thinking about how we can template meta-program it away over a type list of some sort.

Yeah, I tried to figure out some way to do that with boost::mpl::list but got pretty lost in the weeds and walked away with the sense that boost::mpl wasn't able to do what I needed. I'm very very far from an expert on C++ meta-programming however, and more than happy to be pointed in a better direction or otherwise proven wrong!

acmorrow · 2024-02-06T19:35:38Z

src/viam/examples/modules/complex/client.cpp

    // of that type from the server.
-    Registry::register_resource(API::get<Gizmo>(), Gizmo::resource_registration());
-    Registry::register_resource(API::get<Summation>(), Summation::resource_registration());
+    Registry::register_resource<GizmoClient, GizmoServer, GizmoService>(API::get<Gizmo>());


There's four types here now, where there more or less used to be one. Now this knows of the client type, the server type, the GRPC service type, and the "C++ SDK Interface" type (for lack of a better term).

But:

The server type always inherits from the ::Server nested type of the GRPC service type.

The client type always inherits from the C++ SDK Interface Type and manages objects of the ::StubInterface nested type of the GRPC service type.

It feels like the client type should have a well-known typedef for the GRPC service type it is associated with:

class FooClient : public Foo { public: using interface_type = Foo; using service_type = FooService; ... private: using StubType = service_type::StubInterface; };

And something similar for the server type:

class FooServer : public ResourceServer, public FooService::Server { public: using service_type = FooService; using server_type = service_type::Server; };

That'd hopefully let you reduce this registration to something like:

Registry::register_resource<GizmoClient, GizmoServer>();

Because you could obtain all the other types by way of the well known typedefs. Another option might be to make our own traits class(es) and bake this knowledge in there.

I don't think you should do it in this review, but I think as a subsequent step it would make a lot of sense to use either conventional typedefs or traits classes to teach the type system about the intended relationship between these several classes and allow compile time navigation around the graph.

Whatever we do there though should ensure that knowledge of client side stuff doesn't force knowledge of server side stuff (though the other direction is probably fine).

Yeah, I like the look of this. Is there a reason to even include GizmoClient in the register_resource call? If we're fine with knowledge of server side forcing knowledge of client side then I think we can get away with having

using client_type = FooClient; using interface_type = Foo;

in the FooServer declaration, and then reducing down to Registry::register_resource<FooServer>(); for registration.

As I mentioned in my other comment, I'm curious about whether register_resource is trying to do too much. Am I correct that both FooClient and FooServer do need to be registered, but the former only for client usage and the latter only in a relevant module? If that's the case then I'd be pretty happy keeping FooServer even unaware of FooClient, and just having both know the associated base type.

acmorrow · 2024-02-06T20:01:20Z

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


+    // Make sure to explicity register resources with custom APIs.
+    Registry::register_resource<GizmoClient, GizmoServer, GizmoService>(gizmo_api);
+    Registry::register_resource<SummationClient, SummationServer, SummationService>(summation_api);


I find it a little odd that in complex/client, the registration must make reference to [Gizmo,Summation]Server despite not really needing anything to do with servers, and that in the module main that the registration must make reference to [Gizmo,Summation]Client, which it will presumably never use.

Hmm yeah I agree. Will revisit and see if we can clean that up some.

It makes me wonder whether client and server registration should be separate activities.

acmorrow · 2024-02-06T20:08:53Z

src/viam/sdk/resource/resource_server_base.hpp


 class ResourceServer {
   public:
+    virtual API api() const = 0;


Why do ResourceServer's need this now?

My initial thinking was: when initializing the server we store all supported ResourceServer instances, but we want to be able to look them up so we store them by API. This is how we get the API from the ResourceRegistration.

However! I now realize that the key in ResourceRegistration is an API already 🤦 so I think we can get rid of this (and do away with the question of the Robot API) entirely.

acmorrow · 2024-02-06T20:16:23Z

src/viam/sdk/rpc/server.cpp

+    Registry::initialize();
+    for (const auto& rr : Registry::registered_resources()) {
+        auto new_manager = std::make_shared<ResourceManager>();
+        auto server = rr.second->create_resource_server(new_manager, *this);
+        managed_servers_.emplace(server->api(), std::move(server));


Yeah I'm really feeling the lack of clarity around what the root of initialization and ownership is in the SDK. I don't think we are going to get there in this review.

acmorrow · 2024-02-07T15:54:06Z

src/viam/sdk/components/base/base.hpp

    friend bool operator==(const properties& lhs, const properties& rhs);

-    // functions shared across all components
-    static std::shared_ptr<ResourceRegistration> resource_registration();


BTW, I should add that I am happy to see the registration entry points removed from the base classes. It was a little weird that the base classes were responsible for registering the client subclasses and associated servers types.

acmorrow · 2024-02-07T15:59:03Z

src/viam/sdk/registry/registry.hpp

-    static std::unordered_map<std::string, std::shared_ptr<ModelRegistration>> registered_models();
+    /// @brief Provide information on registered resource models.
+    /// @return A map from name to `ModelRegistration` of all registered resource models.
+    static const std::unordered_map<std::string, std::shared_ptr<ModelRegistration>>&


I have some concerns about these return types. First, it is returning a reference to internal state. Are there concurrency concerns? Second, shared_ptr is const, but the ModelRegistration it points to is not.

Ahh good shout out. I think this overlaps with your concerns on the resource_manager method on ResourceServer. I think some revisiting of why we need to expose these types at all, how they're being used, and how to avoid doing so is warranted. I'll do some digging/refactoring.

stuqdog · 2024-02-08T16:43:46Z

src/viam/sdk/registry/registry.hpp

-    virtual ~ResourceRegistration();
-
-    /// @brief Add `Reconfigure` functionality to a resource.
-    std::function<std::shared_ptr<Resource>(std::shared_ptr<Resource>, Name)> create_reconfigurable;


This wasn't doing anything and the (theoretical) semantics of it were confusing. So I got rid of it. Per RSDK-6484, we should just use a Reconfigurable base class instead.

benjirewis

Sorry for the delay on review, but this generally LGTM! I have no big comments to add beyond Drew's.

benjirewis · 2024-02-08T19:54:43Z

src/viam/sdk/registry/registry.hpp

+    ResourceClientRegistration() = default;
 };

+// TODO(RSDK-3030): instead of std::functions, consider making these functions


Should we use a different ticket number if this is still a TODO?

Ooh yes, good catch! Updated.

benjirewis · 2024-02-08T19:56:52Z

src/viam/sdk/rpc/server.hpp

-    // TODO: make `register_service` take one of our types as an arg rather than a
-    // grpc service type, and convert under the hood


I think it's fine to just take in the grpc::Service*.

acmorrow

I'm approving this, more or less as is. I have a few small comments here and there but I don't think they amount to enough that we should do another round of review. I also don't think this is the end of the resource registration story, nor of the driver initialization story. I still have a lot of questions about who owns what, and it feels like there are maybe more concepts in play than are really needed. I also have some lingering concerns around thread safety and potentially racy visibility into internal state. But I'm very pleased with the reduction in boilerplate in the client and server files, and I think the API for registering clients and servers is dramatically improved.

acmorrow · 2024-02-09T18:03:00Z

src/viam/examples/modules/complex/client.cpp

    // of that type from the server.
-    Registry::register_resource(API::get<Gizmo>(), Gizmo::resource_registration());
-    Registry::register_resource(API::get<Summation>(), Summation::resource_registration());
+    Registry::register_resource_client<GizmoClient>();


acmorrow · 2024-02-09T18:12:31Z

src/viam/sdk/registry/registry.cpp

+#include <viam/api/component/base/v1/base.grpc.pb.h>
+#include <viam/api/component/board/v1/board.grpc.pb.h>
+#include <viam/api/component/camera/v1/camera.grpc.pb.h>
+#include <viam/api/component/encoder/v1/encoder.grpc.pb.h>
+#include <viam/api/component/generic/v1/generic.grpc.pb.h>
+#include <viam/api/component/motor/v1/motor.grpc.pb.h>
+#include <viam/api/component/movementsensor/v1/movementsensor.grpc.pb.h>
+#include <viam/api/component/powersensor/v1/powersensor.grpc.pb.h>
+#include <viam/api/component/sensor/v1/sensor.grpc.pb.h>
+#include <viam/api/component/servo/v1/servo.grpc.pb.h>
 #include <viam/api/robot/v1/robot.pb.h>
-
+#include <viam/api/service/generic/v1/generic.grpc.pb.h>
+#include <viam/api/service/mlmodel/v1/mlmodel.grpc.pb.h>
+#include <viam/api/service/motion/v1/motion.grpc.pb.h>


My intuition is that most of these includes are no longer needed, since I'd expect the associated foo/client.hpp and foo/server.hpp to pull them in. But I haven't checked explicitly.

acmorrow · 2024-02-09T18:59:50Z

src/viam/sdk/registry/registry.cpp

 };

-void Registry::register_model(std::shared_ptr<ModelRegistration> resource) {
+void Registry::register_model(std::shared_ptr<const ModelRegistration> resource) {


Could you have a look through these methods and the following for value parameters that could be const& (if they are just observed), or are consumed but aren't std::move'd at the point of consumption? I don't want to nitpick each one but I think several of the methods here could use some small improvements.

Yep will do!

…into refactor-registry

stuqdog added 10 commits January 25, 2024 15:10

minor cleanup

0c1785b

refactor ResourceRegistration2

17eedd3

start building out init

78a677a

Merge branch 'main' of ssh://github.com/viamrobotics/viam-cpp-sdk int…

6eb7767

…o refactor-registry

everything compiling, tests passing

15f5070

everything compiling, all tests passing

eaad4b0

figured out duplicate registration but gizmo not working?

58b2515

cleanup, no regression

a0c47e4

cleanup

e084c1a

clang tidy complaint

bb9b55f

stuqdog commented Jan 31, 2024

View reviewed changes

delete notes to self, import grpcpp channel in mlmodelclient.hpp

fbccc83

stuqdog commented Jan 31, 2024

View reviewed changes

stuqdog added 2 commits January 31, 2024 12:28

some more cleanup and clang tidy stuff

44a965b

const stuff for clang tidy

1902a0f

stuqdog requested review from acmorrow and benjirewis January 31, 2024 19:35

stuqdog marked this pull request as ready for review January 31, 2024 19:35

stuqdog requested a review from a team as a code owner January 31, 2024 19:35

stuqdog requested review from njooma and removed request for a team January 31, 2024 19:35

registry initialize when starting server

5b5530c

stuqdog commented Jan 31, 2024

View reviewed changes

acmorrow reviewed Feb 1, 2024

View reviewed changes

stuqdog marked this pull request as draft February 2, 2024 16:07

stuqdog marked this pull request as ready for review February 2, 2024 16:29

acmorrow reviewed Feb 6, 2024

View reviewed changes

acmorrow reviewed Feb 7, 2024

View reviewed changes

stuqdog added 2 commits February 8, 2024 08:48

remove api method from servers

a5eace3

PR comments

f938ec5

stuqdog commented Feb 8, 2024

View reviewed changes

stuqdog added 2 commits February 8, 2024 11:45

move comment

a70d01b

missing lock, a couple missing consts

80cdd4d

stuqdog requested a review from acmorrow February 8, 2024 17:20

benjirewis approved these changes Feb 8, 2024

View reviewed changes

stuqdog added 3 commits February 8, 2024 15:20

unconst method that messes with mutex

6b6e0e4

fix ticket ##s

188786c

Merge branch 'main' into refactor-registry

8adda84

acmorrow approved these changes Feb 9, 2024

View reviewed changes

stuqdog added 2 commits February 9, 2024 15:09

final PR comments

ad47447

Merge branch 'refactor-registry' of ssh://github.com/stuqdog/cpp-sdk …

baad2ab

…into refactor-registry

stuqdog merged commit cc2851d into viamrobotics:main Feb 12, 2024

stuqdog deleted the refactor-registry branch February 12, 2024 13:58

		// TODO: make `register_service` take one of our types as an arg rather than a
		// grpc service type, and convert under the hood

RSDK-3030 - Refactor registry #208

RSDK-3030 - Refactor registry #208

Uh oh!

Conversation

stuqdog commented Jan 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuqdog Jan 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acmorrow left a comment

Choose a reason for hiding this comment

Uh oh!

stuqdog commented Feb 1, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acmorrow Feb 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuqdog commented Jan 31, 2024 •

edited

Loading

stuqdog Jan 31, 2024 •

edited

Loading

acmorrow Feb 7, 2024 •

edited

Loading