From a4898eeaef3ebabe6cc1ffac6a243fdf1d4dc54a Mon Sep 17 00:00:00 2001 From: abinggo <107740309+abinggo@users.noreply.github.com> Date: Fri, 8 May 2026 20:33:55 +0800 Subject: [PATCH 1/2] fix(modelconfig): return empty architecture when Mistral Architectures field is missing Previously, MistralConfig.GetArchitecture() returned "MistralModel" as a fallback when the Architectures field was empty. Combined with the config_parser's special case for (modelType="mistral" && architecture contains "mistralmodel"), this caused any Mistral model with a missing Architectures field to be misclassified as an EMBEDDING model. Additionally, MistralConfig.IsEmbedding() unconditionally returned true, which is incorrect since MistralConfig is used for all Mistral-based models, not just embedding models. Fix: - GetArchitecture() now returns "" when Architectures is empty, consistent with BaseModelConfig and other model configs (phi, deepseek_v3) - IsEmbedding() now returns false; genuine embedding models like intfloat/e5-mistral-7b-instruct explicitly set Architectures to ["MistralModel"] in their config.json and are still correctly classified via the config_parser special case Fixes #601 Co-Authored-By: Claude Opus 4.7 --- pkg/hfutil/modelconfig/mistral.go | 9 ++++-- pkg/hfutil/modelconfig/mistral_test.go | 30 +++++++++++++++++++ .../testdata/mistral_no_architectures.json | 21 +++++++++++++ 3 files changed, 57 insertions(+), 3 deletions(-) create mode 100644 pkg/hfutil/modelconfig/testdata/mistral_no_architectures.json diff --git a/pkg/hfutil/modelconfig/mistral.go b/pkg/hfutil/modelconfig/mistral.go index c05abc85a..576af1f2f 100644 --- a/pkg/hfutil/modelconfig/mistral.go +++ b/pkg/hfutil/modelconfig/mistral.go @@ -117,7 +117,7 @@ func (c *MistralConfig) GetArchitecture() string { if len(c.Architectures) > 0 { return c.Architectures[0] } - return "MistralModel" + return "" } func (c *MistralConfig) GetModelType() string { @@ -140,9 +140,12 @@ func (c *MistralConfig) HasVision() bool { return false } -// IsEmbedding returns true since this is an embedding model +// IsEmbedding returns true only when the architecture explicitly indicates an embedding model. +// Genuine embedding models like intfloat/e5-mistral-7b-instruct set Architectures to +// ["MistralModel"] in their config.json, which the config_parser recognizes via the +// (modelType=="mistral" && architecture=="mistralmodel") special case. func (c *MistralConfig) IsEmbedding() bool { - return true + return false } // Register the Mistral model handler diff --git a/pkg/hfutil/modelconfig/mistral_test.go b/pkg/hfutil/modelconfig/mistral_test.go index 4c3b54e39..e07f8802f 100644 --- a/pkg/hfutil/modelconfig/mistral_test.go +++ b/pkg/hfutil/modelconfig/mistral_test.go @@ -84,6 +84,36 @@ func TestLoadModelWithMistral(t *testing.T) { t.Logf("Mistral model parameter count via generic loader: %s", FormatParamCount(paramCount)) } +// TestMistralConfigNoArchitectures verifies that when the Architectures field +// is missing from config.json, GetArchitecture() returns an empty string rather +// than a misleading fallback like "MistralModel" which would cause the model to +// be misclassified as an embedding model by the config parser. +// Regression test for https://github.com/ome-projects/ome/issues/601 +func TestMistralConfigNoArchitectures(t *testing.T) { + configPath := filepath.Join("testdata", "mistral_no_architectures.json") + + config, err := LoadModelConfig(configPath) + if err != nil { + t.Fatalf("Failed to load Mistral config without architectures: %v", err) + } + + // GetArchitecture must return empty string when Architectures is absent + arch := config.GetArchitecture() + if arch != "" { + t.Errorf("Expected empty architecture when Architectures field is missing, got '%s'", arch) + } + + // IsEmbedding must return false for a generic Mistral model + if config.IsEmbedding() { + t.Error("Expected IsEmbedding() to return false for a generic Mistral model without explicit embedding architecture") + } + + // Verify it's still recognized as a Mistral model + if config.GetModelType() != "mistral" { + t.Errorf("Expected model type 'mistral', got '%s'", config.GetModelType()) + } +} + func TestMistralInstructConfig(t *testing.T) { configPath := filepath.Join("testdata", "mistral_7b_instruct.json") diff --git a/pkg/hfutil/modelconfig/testdata/mistral_no_architectures.json b/pkg/hfutil/modelconfig/testdata/mistral_no_architectures.json new file mode 100644 index 000000000..fcbca17f5 --- /dev/null +++ b/pkg/hfutil/modelconfig/testdata/mistral_no_architectures.json @@ -0,0 +1,21 @@ +{ + "bos_token_id": 1, + "eos_token_id": 2, + "hidden_act": "silu", + "hidden_size": 4096, + "initializer_range": 0.02, + "intermediate_size": 14336, + "max_position_embeddings": 32768, + "model_type": "mistral", + "num_attention_heads": 32, + "num_hidden_layers": 32, + "num_key_value_heads": 8, + "rms_norm_eps": 1e-05, + "rope_theta": 10000.0, + "sliding_window": 4096, + "tie_word_embeddings": false, + "torch_dtype": "bfloat16", + "transformers_version": "4.34.0.dev0", + "use_cache": true, + "vocab_size": 32000 +} From 7eefeb782a544a822a0ada12ffcf4e08d8894842 Mon Sep 17 00:00:00 2001 From: abinggo <107740309+abinggo@users.noreply.github.com> Date: Sat, 9 May 2026 11:05:23 +0800 Subject: [PATCH 2/2] address review: implement IsEmbedding() and add embedding-positive test - IsEmbedding() now returns true when architecture is "MistralModel" (Option A from review), making the logic self-contained - Add TestMistralConfigEmbedding with mistral_embedding.json fixture to cover the e5-mistral positive path Signed-off-by: abinggo <107740309+abinggo@users.noreply.github.com> --- pkg/hfutil/modelconfig/mistral.go | 9 +++--- pkg/hfutil/modelconfig/mistral_test.go | 29 +++++++++++++++++++ .../testdata/mistral_embedding.json | 22 ++++++++++++++ 3 files changed, 55 insertions(+), 5 deletions(-) create mode 100644 pkg/hfutil/modelconfig/testdata/mistral_embedding.json diff --git a/pkg/hfutil/modelconfig/mistral.go b/pkg/hfutil/modelconfig/mistral.go index 576af1f2f..efb087206 100644 --- a/pkg/hfutil/modelconfig/mistral.go +++ b/pkg/hfutil/modelconfig/mistral.go @@ -4,6 +4,7 @@ import ( "encoding/json" "fmt" "os" + "strings" ) // MistralConfig defines the configuration for Mistral models @@ -140,12 +141,10 @@ func (c *MistralConfig) HasVision() bool { return false } -// IsEmbedding returns true only when the architecture explicitly indicates an embedding model. -// Genuine embedding models like intfloat/e5-mistral-7b-instruct set Architectures to -// ["MistralModel"] in their config.json, which the config_parser recognizes via the -// (modelType=="mistral" && architecture=="mistralmodel") special case. +// IsEmbedding returns true when the architecture is "MistralModel", which indicates +// an embedding model (e.g. intfloat/e5-mistral-7b-instruct). func (c *MistralConfig) IsEmbedding() bool { - return false + return strings.EqualFold(c.GetArchitecture(), "MistralModel") } // Register the Mistral model handler diff --git a/pkg/hfutil/modelconfig/mistral_test.go b/pkg/hfutil/modelconfig/mistral_test.go index e07f8802f..e67a880c6 100644 --- a/pkg/hfutil/modelconfig/mistral_test.go +++ b/pkg/hfutil/modelconfig/mistral_test.go @@ -114,6 +114,35 @@ func TestMistralConfigNoArchitectures(t *testing.T) { } } +// TestMistralConfigEmbedding verifies that a Mistral model with +// "architectures": ["MistralModel"] is correctly identified as an embedding +// model (e.g. intfloat/e5-mistral-7b-instruct). +// This is the load-bearing positive case for config_parser.go's EMBEDDING classification. +func TestMistralConfigEmbedding(t *testing.T) { + configPath := filepath.Join("testdata", "mistral_embedding.json") + + config, err := LoadModelConfig(configPath) + if err != nil { + t.Fatalf("Failed to load Mistral embedding config: %v", err) + } + + // GetArchitecture must return "MistralModel" + arch := config.GetArchitecture() + if arch != "MistralModel" { + t.Errorf("Expected architecture 'MistralModel', got '%s'", arch) + } + + // GetModelType must return "mistral" + if config.GetModelType() != "mistral" { + t.Errorf("Expected model type 'mistral', got '%s'", config.GetModelType()) + } + + // IsEmbedding must return true for MistralModel architecture + if !config.IsEmbedding() { + t.Error("Expected IsEmbedding() to return true for a model with 'MistralModel' architecture") + } +} + func TestMistralInstructConfig(t *testing.T) { configPath := filepath.Join("testdata", "mistral_7b_instruct.json") diff --git a/pkg/hfutil/modelconfig/testdata/mistral_embedding.json b/pkg/hfutil/modelconfig/testdata/mistral_embedding.json new file mode 100644 index 000000000..557da2000 --- /dev/null +++ b/pkg/hfutil/modelconfig/testdata/mistral_embedding.json @@ -0,0 +1,22 @@ +{ + "architectures": ["MistralModel"], + "bos_token_id": 1, + "eos_token_id": 2, + "hidden_act": "silu", + "hidden_size": 4096, + "initializer_range": 0.02, + "intermediate_size": 14336, + "max_position_embeddings": 32768, + "model_type": "mistral", + "num_attention_heads": 32, + "num_hidden_layers": 32, + "num_key_value_heads": 8, + "rms_norm_eps": 1e-05, + "rope_theta": 10000.0, + "sliding_window": 4096, + "tie_word_embeddings": false, + "torch_dtype": "float16", + "transformers_version": "4.36.0", + "use_cache": true, + "vocab_size": 32000 +}