[RSDK-9630][RSDK-9633] delete discover components (viamrobotics#4831)

Author: JohnN193

module/modmanager/manager.go

@@ -27,7 +27,6 @@ import (
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/credentials/insecure"
 	"google.golang.org/grpc/status"
-	"google.golang.org/protobuf/types/known/structpb"
 
 	"go.viam.com/rdk/config"
 	"go.viam.com/rdk/ftdc"
@@ -88,7 +87,6 @@ type module struct {
 	sharedConn rdkgrpc.SharedConn
 	client     pb.ModuleServiceClient
 	// robotClient supplements the ModuleServiceClient client to serve select robot level methods from the module server
-	// such as the DiscoverComponents API
 	robotClient robotpb.RobotServiceClient
 	addr        string
 	resources   map[resource.Name]*addedResource
@@ -627,19 +625,19 @@ func (mgr *Manager) Configs() []config.Module {
 	return configs
 }
 
-// AllModels returns a slice of resource.ModuleModelDiscovery representing the available models
+// AllModels returns a slice of resource.ModuleModel representing the available models
 // from the currently managed modules.
-func (mgr *Manager) AllModels() []resource.ModuleModelDiscovery {
+func (mgr *Manager) AllModels() []resource.ModuleModel {
 	moduleTypes := map[string]config.ModuleType{}
-	models := []resource.ModuleModelDiscovery{}
+	models := []resource.ModuleModel{}
 	for _, moduleConfig := range mgr.Configs() {
 		moduleName := moduleConfig.Name
 		moduleTypes[moduleName] = moduleConfig.Type
 	}
 	for moduleName, handleMap := range mgr.Handles() {
 		for api, handle := range handleMap {
 			for _, model := range handle {
-				modelModel := resource.ModuleModelDiscovery{
+				modelModel := resource.ModuleModel{
 					ModuleName: moduleName, Model: model, API: api.API,
 					FromLocalModule: moduleTypes[moduleName] == config.ModuleTypeLocal,
 				}
@@ -1328,10 +1326,6 @@ func (m *module) registerResources(mgr modmaninterface.ModuleManager) {
 		case api.API.IsComponent():
 			for _, model := range models {
 				m.logger.Infow("Registering component API and model from module", "module", m.cfg.Name, "API", api.API, "model", model)
-				// We must copy because the Discover closure func relies on api and model, but they are iterators and mutate.
-				// Copying prevents mutation.
-				modelCopy := model
-				apiCopy := api
 				resource.RegisterComponent(api.API, model, resource.Registration[resource.Resource, resource.NoNativeConfig]{
 					Constructor: func(
 						ctx context.Context,
@@ -1341,34 +1335,6 @@ func (m *module) registerResources(mgr modmaninterface.ModuleManager) {
 					) (resource.Resource, error) {
 						return mgr.AddResource(ctx, conf, DepsToNames(deps))
 					},
-					Discover: func(ctx context.Context, logger logging.Logger, extra map[string]interface{}) (interface{}, error) {
-						extraStructPb, err := structpb.NewStruct(extra)
-						if err != nil {
-							return nil, err
-						}
-
-						//nolint:deprecated,staticcheck
-						req := &robotpb.DiscoverComponentsRequest{
-							Queries: []*robotpb.DiscoveryQuery{
-								{Subtype: apiCopy.API.String(), Model: modelCopy.String(), Extra: extraStructPb},
-							},
-						}
-
-						//nolint:deprecated,staticcheck
-						res, err := m.robotClient.DiscoverComponents(ctx, req)
-						if err != nil {
-							m.logger.Errorf("error in modular DiscoverComponents: %s", err)
-							return nil, err
-						}
-						switch len(res.Discovery) {
-						case 0:
-							return nil, errors.New("modular DiscoverComponents response did not contain any discoveries")
-						case 1:
-							return res.Discovery[0].Results.AsMap(), nil
-						default:
-							return nil, errors.New("modular DiscoverComponents response contains more than one discovery")
-						}
-					},
 				})
 			}
 		case api.API.IsService():

module/modmanager/manager_test.go

@@ -3,7 +3,6 @@ package modmanager
 import (
 	"bytes"
 	"context"
-	"encoding/json"
 	"errors"
 	"fmt"
 	"os"
@@ -42,8 +41,6 @@ import (
 	rutils "go.viam.com/rdk/utils"
 )
 
-type testDiscoveryResult map[string]interface{}
-
 func setupSocketWithRobot(t *testing.T) string {
 	t.Helper()
 
@@ -1537,70 +1534,6 @@ func TestFTDCAfterModuleCrash(t *testing.T) {
 	test.That(t, numModuleElapsedTimeMetricsSeen, test.ShouldBeGreaterThan, 0)
 }
 
-func TestModularDiscoverFunc(t *testing.T) {
-	ctx := context.Background()
-	logger := logging.NewTestLogger(t)
-
-	modPath := rtestutils.BuildTempModule(t, "module/testmodule")
-
-	modCfg := config.Module{
-		Name:    "test-module",
-		ExePath: modPath,
-	}
-
-	parentAddr := setupSocketWithRobot(t)
-
-	mgr := setupModManager(t, ctx, parentAddr, logger, modmanageroptions.Options{UntrustedEnv: false})
-
-	err := mgr.Add(ctx, modCfg)
-	test.That(t, err, test.ShouldBeNil)
-
-	// The "helper" model implements actual (foobar) discovery
-	reg, ok := resource.LookupRegistration(generic.API, resource.NewModel("rdk", "test", "helper"))
-	test.That(t, ok, test.ShouldBeTrue)
-	test.That(t, reg, test.ShouldNotBeNil)
-	test.That(t, reg.Discover, test.ShouldNotBeNil)
-
-	testCases := []struct {
-		name          string
-		params        map[string]interface{}
-		expectedExtra string
-	}{
-		{
-			name:          "Without extra set",
-			params:        map[string]interface{}{},
-			expectedExtra: "default",
-		},
-		{
-			name:          "With extra set",
-			params:        map[string]interface{}{"extra": "not the default"},
-			expectedExtra: "not the default",
-		},
-	}
-
-	for _, tc := range testCases {
-		t.Run(tc.name, func(t *testing.T) {
-			result, err := reg.Discover(ctx, logger, tc.params)
-			test.That(t, err, test.ShouldBeNil)
-			t.Log("Discovery result: ", result)
-
-			jsonData, err := json.Marshal(result)
-			test.That(t, err, test.ShouldBeNil)
-			t.Logf("Raw JSON: %s", string(jsonData))
-
-			var discoveryResult testDiscoveryResult
-			err = json.Unmarshal(jsonData, &discoveryResult)
-			test.That(t, err, test.ShouldBeNil)
-			t.Logf("Casted struct: %+v", discoveryResult)
-
-			test.That(t, len(discoveryResult), test.ShouldEqual, 1)
-			extraStr, ok := discoveryResult["extra"].(string)
-			test.That(t, ok, test.ShouldBeTrue)
-			test.That(t, extraStr, test.ShouldEqual, tc.expectedExtra)
-		})
-	}
-}
-
 func TestFirstRun(t *testing.T) {
 	t.Run("fails", func(t *testing.T) {
 		ctx := context.Background()

module/modmaninterface/interface.go

@@ -24,7 +24,7 @@ type ModuleManager interface {
 	CleanModuleDataDirectory() error
 
 	Configs() []config.Module
-	AllModels() []resource.ModuleModelDiscovery
+	AllModels() []resource.ModuleModel
 	Provides(cfg resource.Config) bool
 	Handles() map[string]module.HandlerMap
 

module/module.go

@@ -8,7 +8,6 @@ import (
 	"net"
 	"os"
 	"path/filepath"
-	"strings"
 	"sync"
 	"time"
 
@@ -25,7 +24,6 @@ import (
 	robotpb "go.viam.com/api/robot/v1"
 	streampb "go.viam.com/api/stream/v1"
 	"go.viam.com/utils"
-	vprotoutils "go.viam.com/utils/protoutils"
 	"go.viam.com/utils/rpc"
 	"golang.org/x/exp/maps"
 	"google.golang.org/grpc"
@@ -249,7 +247,6 @@ func NewModule(ctx context.Context, address string, logger logging.Logger) (*Mod
 		return nil, err
 	}
 	// We register the RobotService API to supplement the ModuleService in order to serve select robot level methods from the module server
-	// such as the DiscoverComponents API
 	if err := m.server.RegisterServiceServer(ctx, &robotpb.RobotService_ServiceDesc, m); err != nil {
 		return nil, err
 	}
@@ -561,66 +558,6 @@ func (m *Module) AddResource(ctx context.Context, req *pb.AddResourceRequest) (*
 	return &pb.AddResourceResponse{}, nil
 }
 
-// DiscoverComponents is DEPRECATED!!! Please use the Discovery Service instead.
-// DiscoverComponents takes a list of discovery queries and returns corresponding
-// component configurations.
-//
-//nolint:deprecated,staticcheck
-func (m *Module) DiscoverComponents(
-	ctx context.Context,
-	req *robotpb.DiscoverComponentsRequest,
-) (*robotpb.DiscoverComponentsResponse, error) {
-	var discoveries []*robotpb.Discovery
-
-	for _, q := range req.Queries {
-		// Handle triplet edge case i.e. if the subtype doesn't contain ':', add the "rdk:component:" prefix
-		if !strings.ContainsRune(q.Subtype, ':') {
-			q.Subtype = "rdk:component:" + q.Subtype
-		}
-
-		api, err := resource.NewAPIFromString(q.Subtype)
-		if err != nil {
-			return nil, fmt.Errorf("invalid subtype: %s: %w", q.Subtype, err)
-		}
-		model, err := resource.NewModelFromString(q.Model)
-		if err != nil {
-			return nil, fmt.Errorf("invalid model: %s: %w", q.Model, err)
-		}
-
-		resInfo, ok := resource.LookupRegistration(api, model)
-		if !ok {
-			return nil, fmt.Errorf("no registration found for API %s and model %s", api, model)
-		}
-
-		if resInfo.Discover == nil {
-			return nil, fmt.Errorf("discovery not supported for API %s and model %s", api, model)
-		}
-
-		results, err := resInfo.Discover(ctx, m.logger, q.Extra.AsMap())
-		if err != nil {
-			return nil, fmt.Errorf("error discovering components for API %s and model %s: %w", api, model, err)
-		}
-		if results == nil {
-			return nil, fmt.Errorf("error discovering components for API %s and model %s: results was nil", api, model)
-		}
-
-		pbResults, err := vprotoutils.StructToStructPb(results)
-		if err != nil {
-			return nil, fmt.Errorf("unable to convert discovery results to pb struct for query %v: %w", q, err)
-		}
-
-		pbDiscovery := &robotpb.Discovery{
-			Query:   q,
-			Results: pbResults,
-		}
-		discoveries = append(discoveries, pbDiscovery)
-	}
-
-	return &robotpb.DiscoverComponentsResponse{
-		Discovery: discoveries,
-	}, nil
-}
-
 // ReconfigureResource receives the component/service configuration from the parent.
 func (m *Module) ReconfigureResource(ctx context.Context, req *pb.ReconfigureResourceRequest) (*pb.ReconfigureResourceResponse, error) {
 	var res resource.Resource

module/testmodule/main.go

@@ -49,19 +49,6 @@ func mainWithArgs(ctx context.Context, args []string, logger logging.Logger) err
 		helperModel,
 		resource.Registration[resource.Resource, resource.NoNativeConfig]{
 			Constructor: newHelper,
-			Discover: func(ctx context.Context, logger logging.Logger, extra map[string]interface{}) (interface{}, error) {
-				extraVal, ok := extra["extra"]
-				if !ok {
-					extraVal = "default"
-				}
-				extraValStr, ok := extraVal.(string)
-				if !ok {
-					return nil, errors.New("'extra' value must be a string")
-				}
-				return map[string]interface{}{
-					"extra": extraValStr,
-				}, nil
-			},
 		})
 	err = myMod.AddModelFromRegistry(ctx, generic.API, helperModel)
 	if err != nil {

resource/discovery.go

@@ -0,0 +1,23 @@
+package resource
+
+import (
+	pb "go.viam.com/api/robot/v1"
+)
+
+type (
+	// ModuleModel holds the API and Model information of models within a module.
+	ModuleModel struct {
+		ModuleName      string
+		API             API
+		Model           Model
+		FromLocalModule bool
+	}
+)
+
+// ToProto converts a ModuleModel into the equivalent proto message.
+func (mm *ModuleModel) ToProto() *pb.ModuleModel {
+	return &pb.ModuleModel{
+		Model: mm.Model.String(), Api: mm.API.String(), ModuleName: mm.ModuleName,
+		FromLocalModule: mm.FromLocalModule,
+	}
+}

resource/module_model.go

@@ -121,9 +121,6 @@ type Registration[ResourceT Resource, ConfigT any] struct {
 	// NOTE: This is currently an experimental feature and subject to change.
 	WeakDependencies []Matcher
 
-	// Discover looks around for information about this specific model.
-	Discover DiscoveryFunc
-
 	// configType can be used to dynamically inspect the resource config type.
 	configType reflect.Type
 
@@ -293,7 +290,6 @@ func makeGenericResourceRegistration[ResourceT Resource, ConfigT ConfigValidator
 	reg := Registration[Resource, ConfigValidator]{
 		// NOTE: any fields added to Registration must be copied/adapted here.
 		WeakDependencies: typed.WeakDependencies,
-		Discover:         typed.Discover,
 		isDefault:        typed.isDefault,
 		api:              typed.api,
 		configType:       typed.configType,