Skip to content

Commit a9ea1b1

Browse files
chambridgeclaude
andauthored
fix(catalog): make performance metrics cache case-insensitive (kubeflow#2537)
Model lookups were failing when metadata.json IDs had different casing than the stored display name. Normalize cache keys to lowercase using strings.ToLower() in both buildModelDirCache() (write) and Load() (read) to ensure case-insensitive matching. Also adds collision detection with a warning log when two model IDs differ only by case, and fixes modelCount to only increment for genuinely new cache entries. Includes comprehensive tests for case-insensitive lookups (5 table- driven cases) and collision last-write-wins behavior. Signed-off-by: Chris Hambridge <chambrid@redhat.com> Co-authored-by: Claude <noreply@anthropic.com>
1 parent be33bac commit a9ea1b1

2 files changed

Lines changed: 138 additions & 7 deletions

File tree

catalog/internal/catalog/modelcatalog/performance_metrics.go

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ type PerformanceMetricsLoader struct {
128128
metricsArtifactRepo dbmodels.CatalogMetricsArtifactRepository
129129
modelTypeID int32
130130
metricsArtifactTypeID int32
131-
// Cache of model ID -> directory path mapping to avoid repeated directory scans
131+
// Cache of lowercase model ID -> directory path for case-insensitive lookups
132132
modelDirCache map[string]string
133133
}
134134

@@ -179,7 +179,7 @@ func NewPerformanceMetricsLoader(path []string, modelRepo dbmodels.CatalogModelR
179179
return loader, nil
180180
}
181181

182-
// buildModelDirCache scans directories once and builds a cache of model ID -> directory path
182+
// buildModelDirCache scans directories once and builds a cache of lowercase model ID -> directory path
183183
func (pml *PerformanceMetricsLoader) buildModelDirCache() error {
184184
modelCount := 0
185185
for _, rootPath := range pml.path {
@@ -213,9 +213,14 @@ func (pml *PerformanceMetricsLoader) buildModelDirCache() error {
213213
return nil // Continue with other directories
214214
}
215215

216-
// Add to cache
217-
pml.modelDirCache[metadata.ID] = dirPath
218-
modelCount++
216+
// Add to cache (lowercase key for case-insensitive matching)
217+
cacheKey := strings.ToLower(metadata.ID)
218+
if existing, ok := pml.modelDirCache[cacheKey]; ok {
219+
glog.Warningf("Case-insensitive cache collision: %q (dir: %s) overwrites existing entry (dir: %s)", metadata.ID, dirPath, existing)
220+
} else {
221+
modelCount++
222+
}
223+
pml.modelDirCache[cacheKey] = dirPath
219224
glog.V(3).Infof("Cached model directory: %s -> %s", metadata.ID, dirPath)
220225

221226
return nil
@@ -243,8 +248,8 @@ func (pml *PerformanceMetricsLoader) Load(ctx context.Context, record ModelProvi
243248

244249
// Namespaced name is source_id:model_name
245250
namespacedName := *attrs.Name
246-
// Resolve directory from cache: cache is keyed by metadata.ID (display name)
247-
displayName := DisplayNameFromStoredName(namespacedName)
251+
// Resolve directory from cache: cache is keyed by lowercase metadata.ID (display name)
252+
displayName := strings.ToLower(DisplayNameFromStoredName(namespacedName))
248253
dirPath, found := pml.modelDirCache[displayName]
249254
if !found {
250255
glog.V(2).Infof("No performance metrics directory found for model %s", namespacedName)

catalog/internal/catalog/modelcatalog/performance_metrics_test.go

Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@ package modelcatalog
22

33
import (
44
"encoding/json"
5+
"os"
6+
"path/filepath"
57
"strings"
68
"testing"
79
)
@@ -1180,6 +1182,130 @@ func TestMetadataJSONEdgeCases(t *testing.T) {
11801182
}
11811183
}
11821184

1185+
func TestBuildModelDirCache_CaseInsensitive(t *testing.T) {
1186+
// Create a temp directory structure with metadata.json files using mixed-case IDs
1187+
tmpDir := t.TempDir()
1188+
1189+
tests := []struct {
1190+
name string
1191+
metaID string // ID written to metadata.json
1192+
lookupKey string // key used to look up in cache (simulates DisplayNameFromStoredName output)
1193+
wantFound bool
1194+
}{
1195+
{
1196+
name: "exact case match",
1197+
metaID: "sarvam-30b-FP8-Dynamic",
1198+
lookupKey: "sarvam-30b-fp8-dynamic",
1199+
wantFound: true,
1200+
},
1201+
{
1202+
name: "all uppercase metadata ID",
1203+
metaID: "MODEL-ABC-FP16",
1204+
lookupKey: "model-abc-fp16",
1205+
wantFound: true,
1206+
},
1207+
{
1208+
name: "already lowercase",
1209+
metaID: "already-lowercase",
1210+
lookupKey: "already-lowercase",
1211+
wantFound: true,
1212+
},
1213+
{
1214+
name: "mixed case with org prefix",
1215+
metaID: "RedHatAI/Granite-3B-Code",
1216+
lookupKey: "redhatai/granite-3b-code",
1217+
wantFound: true,
1218+
},
1219+
{
1220+
name: "no match returns not found",
1221+
metaID: "some-model",
1222+
lookupKey: "different-model",
1223+
wantFound: false,
1224+
},
1225+
}
1226+
1227+
for _, tt := range tests {
1228+
t.Run(tt.name, func(t *testing.T) {
1229+
// Create a subdirectory with a metadata.json for this test case
1230+
modelDir := filepath.Join(tmpDir, tt.name)
1231+
if err := os.MkdirAll(modelDir, 0o755); err != nil {
1232+
t.Fatalf("failed to create model dir: %v", err)
1233+
}
1234+
metaContent := `{"id": "` + tt.metaID + `"}`
1235+
if err := os.WriteFile(filepath.Join(modelDir, "metadata.json"), []byte(metaContent), 0o644); err != nil {
1236+
t.Fatalf("failed to write metadata.json: %v", err)
1237+
}
1238+
1239+
// Build cache with only this directory
1240+
loader := &PerformanceMetricsLoader{
1241+
path: []string{modelDir},
1242+
modelDirCache: make(map[string]string),
1243+
}
1244+
if err := loader.buildModelDirCache(); err != nil {
1245+
t.Fatalf("buildModelDirCache() error = %v", err)
1246+
}
1247+
1248+
// Lookup using the lowercased key (same as Load does)
1249+
_, found := loader.modelDirCache[strings.ToLower(tt.lookupKey)]
1250+
if found != tt.wantFound {
1251+
t.Errorf("cache lookup for %q: got found=%v, want found=%v (cache keys: %v)",
1252+
tt.lookupKey, found, tt.wantFound, cacheKeys(loader.modelDirCache))
1253+
}
1254+
})
1255+
}
1256+
}
1257+
1258+
func TestBuildModelDirCache_CollisionWarning(t *testing.T) {
1259+
// Two directories with metadata IDs that differ only by case should result
1260+
// in the second overwriting the first (last-write-wins). The collision
1261+
// warning is logged but we verify the cache ends up with a single entry.
1262+
tmpDir := t.TempDir()
1263+
1264+
dir1 := filepath.Join(tmpDir, "dir1")
1265+
dir2 := filepath.Join(tmpDir, "dir2")
1266+
for _, d := range []string{dir1, dir2} {
1267+
if err := os.MkdirAll(d, 0o755); err != nil {
1268+
t.Fatalf("failed to create dir: %v", err)
1269+
}
1270+
}
1271+
if err := os.WriteFile(filepath.Join(dir1, "metadata.json"), []byte(`{"id": "Model-FP8"}`), 0o644); err != nil {
1272+
t.Fatalf("failed to write metadata.json: %v", err)
1273+
}
1274+
if err := os.WriteFile(filepath.Join(dir2, "metadata.json"), []byte(`{"id": "model-fp8"}`), 0o644); err != nil {
1275+
t.Fatalf("failed to write metadata.json: %v", err)
1276+
}
1277+
1278+
loader := &PerformanceMetricsLoader{
1279+
path: []string{tmpDir},
1280+
modelDirCache: make(map[string]string),
1281+
}
1282+
if err := loader.buildModelDirCache(); err != nil {
1283+
t.Fatalf("buildModelDirCache() error = %v", err)
1284+
}
1285+
1286+
// Both IDs normalize to "model-fp8", so the cache should have exactly one entry
1287+
if len(loader.modelDirCache) != 1 {
1288+
t.Errorf("expected 1 cache entry after collision, got %d: %v", len(loader.modelDirCache), cacheKeys(loader.modelDirCache))
1289+
}
1290+
// filepath.Walk visits in lexicographic order, so dir2 (last-write) wins
1291+
cachedPath, found := loader.modelDirCache["model-fp8"]
1292+
if !found {
1293+
t.Fatalf("expected cache key %q, got keys: %v", "model-fp8", cacheKeys(loader.modelDirCache))
1294+
}
1295+
if cachedPath != dir2 {
1296+
t.Errorf("expected collision winner to be %s (last walked), got %s", dir2, cachedPath)
1297+
}
1298+
}
1299+
1300+
// cacheKeys returns all keys from a map for diagnostic output.
1301+
func cacheKeys(m map[string]string) []string {
1302+
keys := make([]string, 0, len(m))
1303+
for k := range m {
1304+
keys = append(keys, k)
1305+
}
1306+
return keys
1307+
}
1308+
11831309
func generateLongString(length int) string {
11841310
var result strings.Builder
11851311
char := "a"

0 commit comments

Comments
 (0)