Move component client/server classes out of the API #215

acmorrow · 2024-02-16T20:33:29Z

I'm still working through the implications of this, particularly w.r.t. to mocks and testing, but this is the general idea:

Move sdk/components/x/x.h to skd/components/x.h.
Move sdk/components/x/client.h to sdk/components/private/x_client.h.
Move sdk/components/x/server.h to sdk/components/private/x_server.h.
And then adjust #include's as necessary.

After this change, the viam/sdk/components directory looks like this in the source directory:

$ ls src/viam/sdk/components
base.cpp            camera.cpp          encoder.cpp         motor.cpp           power_sensor.cpp    sensor.hpp
base.hpp            camera.hpp          encoder.hpp         motor.hpp           power_sensor.hpp    servo.cpp
board.cpp           component.cpp       generic.cpp         movement_sensor.cpp private             servo.hpp
board.hpp           component.hpp       generic.hpp         movement_sensor.hpp sensor.cpp

And like this in the installation directory (i.e.$PREFIX/include/viam/sdk/components):

$ ls build/install/include/viam/sdk/components
base.hpp            camera.hpp          encoder.hpp         motor.hpp           power_sensor.hpp    servo.hpp
board.hpp           component.hpp       generic.hpp         movement_sensor.hpp sensor.hpp

If we like this approach, I'll do the same for services. In hindsight I should have done that one first as there are fewer services than components, but oh well.

stuqdog

Awesome, this looks great! Definitely a fan of the new look.

stuqdog · 2024-02-20T13:16:57Z

src/viam/sdk/tests/mocks/camera_mocks.hpp

+#include <viam/sdk/components/private/camera_client.hpp>
+#include <viam/sdk/components/private/camera_server.hpp>


No need to change especially if we want to keep this PR purely mechanical, but I suspect we can do away with these includes entirely (same in other mocks).

Yes. I want to understand whether tests / mocks actually require access to the concrete FooServer and FooClient types: it would be nice if they don't, because then we can actually not only make them "header private", by not installing the headers, but we can make those types have hidden visibility so they aren't even part of the symbol table for the SDK library.

acmorrow · 2024-02-21T15:03:38Z

Awesome, this looks great! Definitely a fan of the new look.

Cool! I'll get this done for services and promote this to a real review.

acmorrow

@stuqdog - Updated, now services are handled the same way. I ran into one small issue with testing in the mlmodel stuff, so at least one more update is on the way. But please give this a look. I expect given that you were happy with the components changes that you will also be fine with the services changes. The only really interesting change in this review was changing how the mock pipeline stuff worked so it doesn't need to have access to the concrete client types.

acmorrow · 2024-02-23T21:20:51Z

src/viam/sdk/tests/test_utils.hpp

    auto grpc_channel = test_server.grpc_in_process_channel(args);
-    ClientType resource_client(mock->name(), grpc_channel);
+
+    auto resource_client = Registry::lookup_resource_client(API::get<ClientType>())


This is a non-mechanical change, so that tests no longer have a dependency on the concrete FooClient type. Instead of directly constructing ClientType, we ask the registry to do it for us, then downcast.

Potentially we should rename the ClientType template parameter?

Yeah, I think ResourceType is probably better at this point.

acmorrow · 2024-02-23T21:22:04Z

src/viam/sdk/tests/test_mlmodel.cpp

-#include <viam/sdk/services/mlmodel/client.hpp>
-#include <viam/sdk/services/mlmodel/private/proto.hpp>
-#include <viam/sdk/services/mlmodel/server.hpp>
+#include <viam/sdk/services/private/proto.hpp>


This needs some thought: this test currently tests directly some internal proto to vocab conversions. I do want to keep testing that code as it is particularly tricky. I'll give it some thought, as there will probably be other places we need something like this.

I think I'm not quite following what the problem is here. Since it's not being exposed in a header or non-testing code, what is the harm of having this include?

The tests aren't part of the libviamsdk library. Eventually, the types and symbols under private won't even be exported from libviamsdk, so the tests won't be able to use them. Better to clean it up now.

Got it, thanks for clarifying.

acmorrow · 2024-02-23T21:22:45Z

src/viam/sdk/tests/test_encoder.cpp


 #include <boost/test/included/unit_test.hpp>

-#include <viam/api/common/v1/common.pb.h>


I stripped lots of direct #include <viam/api where I could. There are still some in our public headers. We should strive to eliminate them all.

Just created a ticket for this.

acmorrow · 2024-02-23T21:23:57Z

src/viam/sdk/services/private/proto.cpp

 // limitations under the License.

-#include <viam/sdk/services/mlmodel/private/proto.hpp>
+#include <viam/sdk/services/private/proto.hpp>


This file and the associated header should be renamed somehow.

Yeah, would love to see the name change here before we land this PR, proto as a name is much too generic without the more qualified path. What about mlmodel_proto_conversions? It's a little wordy, but it's specific and hidden from users.

I renamed it to services/private/mlmodel.hpp

stuqdog

LGTM!

stuqdog · 2024-02-26T19:04:02Z

src/viam/sdk/services/private/proto.cpp

 // limitations under the License.

-#include <viam/sdk/services/mlmodel/private/proto.hpp>
+#include <viam/sdk/services/private/proto.hpp>


Yeah, would love to see the name change here before we land this PR, proto as a name is much too generic without the more qualified path. What about mlmodel_proto_conversions? It's a little wordy, but it's specific and hidden from users.

stuqdog · 2024-02-26T19:07:44Z

src/viam/sdk/tests/test_mlmodel.cpp

-#include <viam/sdk/services/mlmodel/client.hpp>
-#include <viam/sdk/services/mlmodel/private/proto.hpp>
-#include <viam/sdk/services/mlmodel/server.hpp>
+#include <viam/sdk/services/private/proto.hpp>


I think I'm not quite following what the problem is here. Since it's not being exposed in a header or non-testing code, what is the harm of having this include?

stuqdog · 2024-02-26T19:09:06Z

src/viam/sdk/tests/test_utils.hpp

    auto grpc_channel = test_server.grpc_in_process_channel(args);
-    ClientType resource_client(mock->name(), grpc_channel);
+
+    auto resource_client = Registry::lookup_resource_client(API::get<ClientType>())


Yeah, I think ResourceType is probably better at this point.

stuqdog · 2024-02-26T19:13:00Z

src/viam/sdk/tests/test_encoder.cpp


 #include <boost/test/included/unit_test.hpp>

-#include <viam/api/common/v1/common.pb.h>


Just created a ticket for this.

acmorrow · 2024-02-26T19:22:29Z

@stuqdog - This is ready for another round of review. Changes since last time:

Pushed all "private" stuff into an impl namespace. I like this because it gives us a way to see at a glance in the symbols (or in source usage for that matter) if an internal type is being exported or used somehow. Happy to bikeshed a name. Ideally I'd call it private but that's a keyword of course.
Fixed the mlmodel tests to not use the internal "proto" functions anymore but test them indirectly by way of a mock and a client pipeline.
Renamed the components/private/proto headers to components/private/mlmodel and renamed the mlmodel_details namespace (which used to be public) to mlmodel inside the impl namespace.

stuqdog · 2024-02-26T19:44:12Z

All still looks good to me, thanks!

acmorrow requested a review from stuqdog February 16, 2024 20:33

stuqdog approved these changes Feb 20, 2024

View reviewed changes

acmorrow added 4 commits February 22, 2024 12:44

move component files

44b379d

fixup includes after component file renamings

15aabd1

format

9a2abd5

mocks do not need access to api or private client/server

cf412cf

acmorrow force-pushed the libsplit branch from 52a75fe to cf412cf Compare February 23, 2024 20:10

acmorrow added 4 commits February 23, 2024 15:43

tests do not need direct access to Client/Server types

008d130

format

26cede2

move service files

80a7eb7

fixup service references after move

e81b343

acmorrow marked this pull request as ready for review February 23, 2024 21:19

acmorrow requested a review from a team as a code owner February 23, 2024 21:19

acmorrow requested review from purplenicole730 and stuqdog and removed request for a team February 23, 2024 21:19

acmorrow commented Feb 23, 2024

View reviewed changes

acmorrow added 2 commits February 26, 2024 09:58

push now private client server implementations into impl namespace

c2e4a7b

Test mlmodel tensor serde via mocks rather than private apis

22e6684

stuqdog approved these changes Feb 26, 2024

View reviewed changes

acmorrow added 2 commits February 26, 2024 14:14

Rename proto headers to reflect connection to mlmodel

cda5dae

format

0a6d212

acmorrow added 2 commits February 26, 2024 14:49

Rename template parameter

768b156

list ordering

5636a81

acmorrow merged commit c48fbca into viamrobotics:main Feb 26, 2024

		#include <viam/sdk/components/private/camera_client.hpp>
		#include <viam/sdk/components/private/camera_server.hpp>


		#include <boost/test/included/unit_test.hpp>

		#include <viam/api/common/v1/common.pb.h>

Move component client/server classes out of the API #215

Move component client/server classes out of the API #215

Uh oh!

Conversation

acmorrow commented Feb 16, 2024

Uh oh!

stuqdog left a comment

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 commented Feb 21, 2024

Uh oh!

acmorrow left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuqdog left a comment

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 commented Feb 26, 2024

Uh oh!

stuqdog commented Feb 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants