diff --git a/internal/api/docs/openapi.yaml b/internal/api/docs/openapi.yaml index ec019c347..437ccd0a6 100644 --- a/internal/api/docs/openapi.yaml +++ b/internal/api/docs/openapi.yaml @@ -1505,8 +1505,6 @@ components: type: object AppLocalBrickCreateRequest: properties: - description: - type: string name: type: string type: object @@ -1623,6 +1621,8 @@ components: type: string name: type: string + readme: + type: string require_model: type: boolean status: diff --git a/internal/e2e/client/client.gen.go b/internal/e2e/client/client.gen.go index 199ef5698..79a35d7d6 100644 --- a/internal/e2e/client/client.gen.go +++ b/internal/e2e/client/client.gen.go @@ -170,8 +170,7 @@ type AppListResponse struct { // AppLocalBrickCreateRequest defines model for AppLocalBrickCreateRequest. type AppLocalBrickCreateRequest struct { - Description *string `json:"description,omitempty"` - Name *string `json:"name,omitempty"` + Name *string `json:"name,omitempty"` } // AppLocalBrickCreateResponse defines model for AppLocalBrickCreateResponse. @@ -235,6 +234,7 @@ type BrickInstance struct { Id *string `json:"id,omitempty"` Model *string `json:"model,omitempty"` Name *string `json:"name,omitempty"` + Readme *string `json:"readme,omitempty"` RequireModel *bool `json:"require_model,omitempty"` Status *string `json:"status,omitempty"` @@ -1681,7 +1681,7 @@ func NewCreateAppLocalBrickRequestWithBody(server string, appID string, contentT var pathParam0 string - pathParam0, err = runtime.StyleParamWithLocation("simple", false, "appID", runtime.ParamLocationPath, appID) + pathParam0, err = runtime.StyleParamWithOptions("simple", false, "appID", appID, runtime.StyleParamOptions{ParamLocation: runtime.ParamLocationPath, Type: "string", Format: ""}) if err != nil { return nil, err } diff --git a/internal/e2e/daemon/ai_models_test.go b/internal/e2e/daemon/ai_models_test.go index ed668f77f..03c3fb545 100644 --- a/internal/e2e/daemon/ai_models_test.go +++ b/internal/e2e/daemon/ai_models_test.go @@ -117,7 +117,7 @@ func TestAIModelDetails(t *testing.T) { t.Run("should return full details for a valid custom model ID", func(t *testing.T) { _, err := custommodel.Store(customModelDir.Join("my-model"), custommodel.ModelDescriptor{ ID: "custom-classification-model-eim", - Name: "this the name of the model", + Name: "this is the name of the model", Description: "this is the description of the model", Bricks: []custommodel.BrickConfig{ {ID: "arduino:audio_classification"}, @@ -133,12 +133,12 @@ func TestAIModelDetails(t *testing.T) { got := response.JSON200 require.Equal(t, &client.AIModelItem{ Id: f.Ptr("custom-classification-model-eim"), - Name: f.Ptr("this the name of the model"), + Name: f.Ptr("this is the name of the model"), IsBuiltin: f.Ptr(false), Runner: f.Ptr(""), Description: f.Ptr("this is the description of the model"), BrickIds: &[]string{"arduino:audio_classification"}, - DiskUsage: f.Ptr(222), + DiskUsage: f.Ptr(225), }, got, "The returned model details should match the expected values") // TODO test metadata and model configuration contents and runner @@ -174,6 +174,7 @@ func TestAIModelDetails(t *testing.T) { } func TestAIModelDelete(t *testing.T) { + customModelDir, err := paths.MkTempDir("", "custom-models") require.NoError(t, err) @@ -230,6 +231,7 @@ func TestAIModelDelete(t *testing.T) { _, err := custommodel.Store(customModelDir.Join("my-custom-model"), custommodel.ModelDescriptor{ ID: modelId, + Name: "this the name of the model", Runner: "brick", Bricks: []custommodel.BrickConfig{ {ID: "arduino:audio_classification"}, diff --git a/internal/orchestrator/check.go b/internal/orchestrator/check.go index 421a62c6c..4839c84b0 100644 --- a/internal/orchestrator/check.go +++ b/internal/orchestrator/check.go @@ -46,36 +46,47 @@ func checkBricks(a app.AppDescriptor, index *bricksindex.BricksIndex, modelIndex indexBrick, found := index.FindBrickByID(appBrick.ID) if !found { allErrors = errors.Join(allErrors, fmt.Errorf("brick %q not found", appBrick.ID)) - continue // Skip further validation for this brick since it doesn't exist + continue } + var model *modelsindex.AIModel if len(appBrick.Model) != 0 { - _, modelFound := modelIndex.GetModelByID(appBrick.Model) + m, modelFound := modelIndex.GetModelByID(appBrick.Model) if !modelFound { allErrors = errors.Join(allErrors, fmt.Errorf("model %q for brick %q not found", appBrick.Model, appBrick.ID)) + } else { + model = m } } for appBrickVariableName := range appBrick.Variables { - _, exist := indexBrick.GetVariable(appBrickVariableName) - if !exist { - // TODO: we should return warnings + if _, exist := indexBrick.GetVariable(appBrickVariableName); !exist { slog.Warn("[skip] variable does not exist into the brick definition", "variable", appBrickVariableName, "brick", indexBrick.ID) } } - // Check that all required brick variables are provided by app + // Check that all required brick variables are provided by app or by the model for _, indexBrickVariable := range indexBrick.Variables { - if indexBrickVariable.IsRequired() { - if _, exist := appBrick.Variables[indexBrickVariable.Name]; !exist { - allErrors = errors.Join(allErrors, fmt.Errorf("variable %q is required by brick %q", indexBrickVariable.Name, indexBrick.ID)) + if !indexBrickVariable.IsRequired() { + continue + } + _, providedByApp := appBrick.Variables[indexBrickVariable.Name] + providedByModel := false + if model != nil { + for _, brickConfig := range model.Bricks { + if brickConfig.ID == appBrick.ID { + _, providedByModel = brickConfig.ModelConfiguration[indexBrickVariable.Name] + break + } } } + if !providedByApp && !providedByModel { + allErrors = errors.Join(allErrors, fmt.Errorf("variable %q is required by brick %q", indexBrickVariable.Name, indexBrick.ID)) + } } } return allErrors } - func checkRequiredDevices(bricksIndex *bricksindex.BricksIndex, appBricks []app.Brick, availableDevices peripherals.AvailableDevices) error { requiredDeviceClasses := make(map[peripherals.DeviceClass]bool) diff --git a/internal/orchestrator/check_test.go b/internal/orchestrator/check_test.go index 1d86b6f5e..a05039256 100644 --- a/internal/orchestrator/check_test.go +++ b/internal/orchestrator/check_test.go @@ -438,3 +438,85 @@ func TestCheckRequiredDevice(t *testing.T) { }) } } + +func TestCheckBricksModelProvidesRequiredVariable(t *testing.T) { + bricksIndex := &bricksindex.BricksIndex{ + BuiltInBricks: []bricksindex.Brick{ + { + ID: "arduino:ai-brick-with-required-var", + Name: "AI brick with required variable", + Variables: []bricksindex.BrickVariable{ + { + Name: "A_REQUIRED_VAR", + Description: "a-required-variable-by-the-brick", + DefaultValue: "", + }, + }, + }, + }, + } + + modelIndex := &modelsindex.ModelsIndex{ + InternalModels: []modelsindex.AIModel{ + { + ID: "my-model", + Bricks: []modelsindex.BrickConfig{ + { + ID: "arduino:ai-brick-with-required-var", + ModelConfiguration: map[string]string{ + "A_REQUIRED_VAR": "/models/ootb/ei/my-model.eim", + }, + }, + }, + }, + }, + } + + testCases := []struct { + name string + yamlContent string + expectedError error + }{ + { + name: "valid if required variable is provided by model", + yamlContent: ` +name: App with model providing required variable +bricks: + - arduino:ai-brick-with-required-var: + model: my-model +`, + expectedError: nil, + }, + { + name: "invalid if required variable is not provided by app nor by model", + yamlContent: ` +name: App missing required variable +bricks: + - arduino:ai-brick-with-required-var: +`, + expectedError: errors.New("variable \"A_REQUIRED_VAR\" is required by brick \"arduino:ai-brick-with-required-var\""), + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + tempDir := t.TempDir() + err := paths.New(tempDir).MkdirAll() + require.NoError(t, err) + appYaml := paths.New(tempDir, "app.yaml") + err = os.WriteFile(appYaml.String(), []byte(tc.yamlContent), 0600) + require.NoError(t, err) + + appDescriptor, err := app.ParseDescriptorFile(appYaml) + require.NoError(t, err) + + err = checkBricks(appDescriptor, bricksIndex, modelIndex) + if tc.expectedError == nil { + assert.NoError(t, err) + } else { + require.Error(t, err) + assert.Equal(t, tc.expectedError.Error(), err.Error()) + } + }) + } +} diff --git a/internal/orchestrator/modelsindex/custommodel/custommodel.go b/internal/orchestrator/modelsindex/custommodel/custommodel.go index af0ce8a1d..6562176ba 100644 --- a/internal/orchestrator/modelsindex/custommodel/custommodel.go +++ b/internal/orchestrator/modelsindex/custommodel/custommodel.go @@ -110,10 +110,11 @@ func (a *AiModel) GetDescriptorPath() *paths.Path { } func (a *AiModel) writeDescriptorFile() error { - if !a.ModelDescriptor.IsValid() { - // TODO: provide more details about the invalidity - return errors.New("invalid model descriptor") + err := a.ModelDescriptor.Validate() + if err != nil { + return fmt.Errorf("invalid model descriptor: %w", err) } + descriptorPath := a.GetDescriptorPath() if descriptorPath == nil { return errors.New("model descriptor file path is not set") diff --git a/internal/orchestrator/modelsindex/custommodel/custommodel_test.go b/internal/orchestrator/modelsindex/custommodel/custommodel_test.go index b0b5b987a..2e6cb5c51 100644 --- a/internal/orchestrator/modelsindex/custommodel/custommodel_test.go +++ b/internal/orchestrator/modelsindex/custommodel/custommodel_test.go @@ -90,7 +90,6 @@ func TestLoad(t *testing.T) { } func TestStore(t *testing.T) { - t.Run("it writes descriptor and model file when reader provided", func(t *testing.T) { tempDir := t.TempDir() modelDir := paths.New(tempDir).Join("test-model-with-blob") @@ -126,8 +125,9 @@ func TestStore(t *testing.T) { modelDir := paths.New(tempDir).Join("test-model-no-filename") descr := ModelDescriptor{ - ID: "test-id", - Name: "test", + ID: "test-id", + Name: "test", + Runner: "brick", } blobReader := io.NopCloser(bytes.NewReader([]byte("content"))) @@ -142,8 +142,9 @@ func TestStore(t *testing.T) { modelDir := paths.New(tempDir).Join("test-model-large") descr := ModelDescriptor{ - ID: "test-large", - Name: "large model", + ID: "test-large", + Name: "large model", + Runner: "brick", } // Create 1MB blob diff --git a/internal/orchestrator/modelsindex/custommodel/parser.go b/internal/orchestrator/modelsindex/custommodel/parser.go index 04e693e7f..26d12a3cf 100644 --- a/internal/orchestrator/modelsindex/custommodel/parser.go +++ b/internal/orchestrator/modelsindex/custommodel/parser.go @@ -19,9 +19,12 @@ package custommodel import ( "fmt" + "strings" "github.com/arduino/go-paths-helper" "github.com/goccy/go-yaml" + + "github.com/arduino/arduino-app-cli/internal/orchestrator/bricksindex" ) type ModelDescriptor struct { @@ -52,10 +55,64 @@ func ParseModelDescriptorFile(file *paths.Path) (ModelDescriptor, error) { return descriptor, nil } -func (a *ModelDescriptor) IsValid() bool { - /* TODO: check - 1) brick list are present into the brick-list - 2) metadata are coherent with the source - */ - return true +func (a *ModelDescriptor) Validate() error { + if a.ID == "" { + return fmt.Errorf("invalid model descriptor: id is empty") + } + if a.Name == "" { + return fmt.Errorf("invalid model descriptor: name is empty") + } + source, ok := a.Metadata["source"] + if !ok { + return nil // source is optional + } + + switch source { + case "edgeimpulse": + return validateEdgeImpulseMetadata(a.Metadata) + default: + return fmt.Errorf("invalid model descriptor: unsupported source '%s'", source) + } +} + +func (a *ModelDescriptor) CheckEdgeImpulseBricks(bricksIndex *bricksindex.BricksIndex) error { + for _, brickConfig := range a.Bricks { + brick, ok := bricksIndex.FindBrickByID(brickConfig.ID) + if !ok { + return fmt.Errorf("invalid model descriptor: brick with ID '%s' not found", brickConfig.ID) + } + + for _, variable := range brick.Variables { + if strings.HasPrefix(variable.Name, "EI_") && strings.HasSuffix(variable.Name, "_MODEL") { + if val, ok := brickConfig.ModelConfiguration[variable.Name]; !ok || val == "" { + return fmt.Errorf("invalid model descriptor: missing model configuration for variable '%s' in brick '%s'", variable.Name, brickConfig.ID) + } + } + } + } + return nil +} + +func validateEdgeImpulseMetadata(metadata map[string]string) error { + requiredFields := []string{ + "ei-project-id", + "ei-impulse-id", + "ei-impulse-name", + "ei-deployment-version", + } + for _, field := range requiredFields { + if val, ok := metadata[field]; !ok || val == "" { + return fmt.Errorf("invalid Edge Impulse metadata: missing required field '%s'", field) + } + } + + if metadata["ei-model-type"] != "float32" { + return fmt.Errorf("invalid Edge Impulse metadata: unsupported model type") + } + + if metadata["ei-engine"] != "tflite" { + return fmt.Errorf("invalid Edge Impulse metadata: unsupported engine") + } + + return nil }