Merge pull request #801 from kubeflow/main

[pull] main from kubeflow:main

Author: openshift-merge-bot[bot]

catalog/README.md

@@ -82,7 +82,7 @@ The HuggingFace catalog source allows you to discover and import models from the
 
 #### 1. Set Your API Key
 
-The HuggingFace provider requires an API key for authentication. By default, the service reads the API key from the `HF_API_KEY` environment variable:
+Setting a Hugging Face API key is optional. Hugging Face  requires an API key for authentication for full access to data of models that are private and/or gated. If an API key is NOT set, private models will be entirely unavailable and gated models will have limited metadata. By default, the service reads the API key from the `HF_API_KEY` environment variable:
 
 ```bash
 export HF_API_KEY="your-huggingface-api-key-here"

catalog/internal/catalog/catalog_test.go

@@ -29,8 +29,7 @@ func TestLoadCatalogSources(t *testing.T) {
 		{
 			name:    "test-catalog-sources",
 			args:    args{catalogsPath: "testdata/test-catalog-sources.yaml"},
-			want:    []string{"catalog1"},
-			wantErr: false,
+			want:    []string{"catalog1", "catalog2"},
 		},
 	}
 	for _, tt := range tests {
@@ -63,6 +62,7 @@ func TestLoadCatalogSources(t *testing.T) {
 
 func TestLoadCatalogSourcesEnabledDisabled(t *testing.T) {
 	trueValue := true
+	falseValue := false
 	type args struct {
 		catalogsPath string
 	}
@@ -82,6 +82,12 @@ func TestLoadCatalogSourcesEnabledDisabled(t *testing.T) {
 					Enabled: &trueValue,
 					Labels:  []string{},
 				},
+				"catalog2": {
+					Id:      "catalog2",
+					Name:    "Catalog 2",
+					Enabled: &falseValue,
+					Labels:  []string{},
+				},
 			},
 			wantErr: false,
 		},
@@ -470,6 +476,87 @@ func TestLoadCatalogSourcesWithRepositoryErrors(t *testing.T) {
 	}
 }
 
+func TestLoadCatalogSourcesWithNilEnabled(t *testing.T) {
+	// Test that nil Enabled field is treated as enabled (per OpenAPI spec default: true)
+	mockModelRepo := &MockCatalogModelRepository{}
+	mockArtifactRepo := &MockCatalogArtifactRepository{}
+	mockModelArtifactRepo := &MockCatalogModelArtifactRepository{}
+	mockMetricsArtifactRepo := &MockCatalogMetricsArtifactRepository{}
+
+	services := service.NewServices(
+		mockModelRepo,
+		mockArtifactRepo,
+		mockModelArtifactRepo,
+		mockMetricsArtifactRepo,
+		&MockPropertyOptionsRepository{},
+	)
+
+	// Register a test provider
+	testProviderName := "test-nil-enabled-provider"
+	RegisterModelProvider(testProviderName, func(ctx context.Context, source *Source, reldir string) (<-chan ModelProviderRecord, error) {
+		ch := make(chan ModelProviderRecord, 1)
+
+		modelName := "test-model-nil-enabled"
+		model := &dbmodels.CatalogModelImpl{
+			Attributes: &dbmodels.CatalogModelAttributes{
+				Name: &modelName,
+			},
+		}
+
+		ch <- ModelProviderRecord{
+			Model:     model,
+			Artifacts: []dbmodels.CatalogArtifact{},
+		}
+		close(ch)
+
+		return ch, nil
+	})
+
+	testConfig := &sourceConfig{
+		Catalogs: []Source{
+			{
+				CatalogSource: apimodels.CatalogSource{
+					Id:      "test-catalog-nil-enabled",
+					Name:    "Test Catalog Nil Enabled",
+					Enabled: nil, // Nil should be treated as enabled
+				},
+				Type: testProviderName,
+			},
+		},
+	}
+
+	l := NewLoader(services, []string{})
+	ctx := context.Background()
+
+	// First call updateSources to populate the SourceCollection
+	err := l.updateSources("test-path", testConfig)
+	if err != nil {
+		t.Fatalf("updateSources() error = %v", err)
+	}
+
+	err = l.updateDatabase(ctx)
+	if err != nil {
+		t.Fatalf("updateDatabase() error = %v", err)
+	}
+
+	// Wait for processing
+	time.Sleep(100 * time.Millisecond)
+
+	// Verify that the model WAS saved (because nil Enabled is treated as enabled)
+	if len(mockModelRepo.SavedModels) != 1 {
+		t.Errorf("Expected 1 model to be saved (nil Enabled should be treated as enabled), got %d", len(mockModelRepo.SavedModels))
+	}
+
+	if len(mockModelRepo.SavedModels) > 0 {
+		savedModel := mockModelRepo.SavedModels[0]
+		if savedModel.GetAttributes() == nil || savedModel.GetAttributes().Name == nil {
+			t.Error("Saved model should have attributes with name")
+		} else if *savedModel.GetAttributes().Name != "test-model-nil-enabled" {
+			t.Errorf("Expected model name 'test-model-nil-enabled', got '%s'", *savedModel.GetAttributes().Name)
+		}
+	}
+}
+
 func TestMockRepositoryBehavior(t *testing.T) {
 	mockRepo := &MockCatalogModelRepository{}
 

catalog/internal/catalog/hf_catalog.go

@@ -633,21 +633,31 @@ func newHFModelProvider(ctx context.Context, source *Source, reldir string) (<-c
 	}
 	apiKey := os.Getenv(apiKeyEnvVar)
 	if apiKey == "" {
-		return nil, fmt.Errorf("missing %s environment variable for HuggingFace catalog", apiKeyEnvVar)
+		glog.Infof("No API key configured for Hugging Face. Only public models and limited data for gated models will be available.")
 	}
 	p.apiKey = apiKey
 
 	// Parse base URL (optional, defaults to huggingface.co)
 	// This allows tests to use mock servers by providing a custom URL
 	p.baseURL = defaultHuggingFaceURL
 	if url, ok := source.Properties[urlKey].(string); ok && url != "" {
-		p.baseURL = strings.TrimSuffix(url, "/") // Remove trailing slash if present
+		p.baseURL = strings.TrimSuffix(url, "/")
 	}
 
-	// Validate credentials before proceeding
-	if err := p.validateCredentials(ctx); err != nil {
-		glog.Errorf("HuggingFace catalog credential validation failed: %v", err)
-		return nil, fmt.Errorf("failed to validate HuggingFace catalog credentials: %w", err)
+	if p.apiKey != "" {
+		hasValidPrefix := strings.HasPrefix(p.apiKey, "hf_")
+		if !hasValidPrefix {
+			// API key is set but doesn't have expected prefix, warn and continue without authentication
+			glog.Infof("API key does not have expected 'hf_' prefix. Only public models and limited data for gated models will be available.")
+			p.apiKey = "" // Clear invalid key to prevent its use
+		}
+		if hasValidPrefix {
+			// Validate credentials only if API key has correct format
+			if err := p.validateCredentials(ctx); err != nil {
+				glog.Errorf("HuggingFace catalog credential validation failed: %v", err)
+				return nil, fmt.Errorf("failed to validate HuggingFace catalog credentials: %w", err)
+			}
+		}
 	}
 
 	// Use top-level IncludedModels from Source as the list of models to fetch

catalog/internal/catalog/loader.go

@@ -330,12 +330,18 @@ func (l *Loader) readProviderRecords(ctx context.Context) <-chan ModelProviderRe
 	ch := make(chan ModelProviderRecord)
 	var wg sync.WaitGroup
 
-	// Get all enabled sources from the merged collection.
+	// Get all sources from the merged collection.
 	// This allows sparse overrides to work: a user can enable a disabled source
 	// with just "id" and "enabled: true", inheriting Type and Properties from the base.
 	mergedSources := l.Sources.AllSources()
 
 	for _, source := range mergedSources {
+		// Skip disabled sources - only load catalog data from enabled sources
+		// Per OpenAPI spec, enabled defaults to true, so nil is treated as enabled
+		if source.Enabled != nil && !*source.Enabled {
+			continue
+		}
+
 		// Skip sources that have already been loaded
 		if l.loadedSources[source.Id] {
 			continue

catalog/internal/catalog/sources.go

@@ -159,21 +159,19 @@ func (sc *SourceCollection) merged() map[string]Source {
 
 // AllSources returns all merged sources including Type and Properties.
 // This is used by the loader to get complete source information.
-// Only enabled sources are returned.
+// All sources are returned regardless of enabled status.
 func (sc *SourceCollection) AllSources() map[string]Source {
 	sc.mu.RLock()
 	defer sc.mu.RUnlock()
 
 	result := map[string]Source{}
 	for id, source := range sc.merged() {
-		if source.Enabled != nil && *source.Enabled {
-			result[id] = source
-		}
+		result[id] = source
 	}
 	return result
 }
 
-// All returns all enabled sources as CatalogSource (for the API).
+// All returns all sources as CatalogSource (for the API).
 // This excludes internal fields like Type and Properties.
 func (sc *SourceCollection) All() map[string]model.CatalogSource {
 	result := map[string]model.CatalogSource{}

catalog/internal/catalog/sources_test.go

@@ -412,7 +412,7 @@ func TestSourceCollection_MergeOverride(t *testing.T) {
 			},
 		},
 		{
-			name:        "user can disable a source from default - disabled sources not returned",
+			name:        "user can disable a source from default - disabled sources ARE returned",
 			originOrder: []string{"default.yaml", "user.yaml"},
 			mergeSequence: []struct {
 				origin  string
@@ -441,8 +441,15 @@ func TestSourceCollection_MergeOverride(t *testing.T) {
 					},
 				},
 			},
-			// Disabled sources are not returned by All()
-			expectedSources: map[string]model.CatalogSource{},
+			// Disabled sources ARE returned by All() - with enabled field showing false
+			expectedSources: map[string]model.CatalogSource{
+				"hf": {
+					Id:      "hf",
+					Name:    "Hugging Face",
+					Enabled: apiutils.Of(false),
+					Labels:  []string{},
+				},
+			},
 		},
 		{
 			name:        "sparse override: user enables disabled source with just id and enabled",