Skip to content

fix(modelconfig): return empty architecture when Mistral Architectures is missing #602

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
abinggo wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(modelconfig): return empty architecture when Mistral Architectures is missing #602

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a4898ee
fix(modelconfig): return empty architecture when Mistral Architecture…
abinggo May 8, 2026
7eefeb7
address review: implement IsEmbedding() and add embedding-positive test
abinggo May 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions pkg/hfutil/modelconfig/mistral.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ func (c *MistralConfig) GetArchitecture() string {
if len(c.Architectures) > 0 {
return c.Architectures[0]
}
return "MistralModel"
return ""

@YouNeedCryDear YouNeedCryDear May 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is the empty string as architecture going to be handled by the downstream matcher? Does the corresponding runtime have explicit "" as architecture?

@abinggo abinggo May 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@YouNeedCryDear Thanks for the review. I verified the downstream handling in pkg/modelagent/config_parser.go — all usages of GetArchitecture() normalize via strings.ToLower() and use strings.Contains() for pattern matching. When architecture is "", no specific patterns match, and determineModelCapabilitiesFromHF() correctly falls through to the default ModelCapabilityTextToText at line 586. The embedding check at line 575 relies on IsEmbedding() (which is now self-contained in MistralConfig) rather than the architecture string directly, so this is safe.
This is also consistent with how BaseModelConfig, PhiConfig, and DeepSeekV3Config handle missing architectures — they all return "".

}

func (c *MistralConfig) GetModelType() string {
Expand All @@ -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

@OpheliaLjh OpheliaLjh May 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment here is misleading. It says IsEmbedding “returns true only when the architecture explicitly indicates an embedding model” but the implementation unconditionally returns false. The architecture-based check the comment is describing lives in config_parser.go's (mistral && mistralmodel) condition, not here.

Two clean options:

Option A — implement what the comment describes, which also lets you remove the special case from config_parser.go entirely:

func (c *MistralConfig) IsEmbedding() bool {
    return c.GetArchitecture() == "MistralModel"
}

Option B — keep return false but rewrite the comment to say so plainly:

// IsEmbedding returns false. MistralConfig covers all Mistral-derived architectures,
// not just embedding models. Genuine embedding models (e.g. e5-mistral-7b-instruct)
// are detected via the (mistral && mistralmodel) architecture match in config_parser.go.
func (c *MistralConfig) IsEmbedding() bool {
    return false
}

@OpheliaLjh OpheliaLjh May 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment here is misleading. It says IsEmbedding "returns true only when the architecture explicitly indicates an embedding model" but the implementation unconditionally returns false. The architecture-based check the comment is describing lives in config_parser.go's (mistral && mistralmodel) condition, not here.

Two clean options:

Option A — implement what the comment describes, which also lets you remove the special case from config_parser.go entirely:

func (c *MistralConfig) IsEmbedding() bool {
	return c.GetArchitecture() == "MistralModel"
}

Option B — keep return false but rewrite the comment to say so plainly:

// IsEmbedding returns false. MistralConfig covers all Mistral-derived architectures,
// not just embedding models. Genuine embedding models (e.g. e5-mistral-7b-instruct)
// are detected via the (mistral && mistralmodel) architecture match in config_parser.go.
func (c *MistralConfig) IsEmbedding() bool {
	return false
}

}

// Register the Mistral model handler
Expand Down
30 changes: 30 additions & 0 deletions pkg/hfutil/modelconfig/mistral_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}
}

@OpheliaLjh OpheliaLjh May 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test covers the negative case (no architectures → not embedding) but there is no counterpart for the positive case. Please add a test with a fixture that has "architectures": ["MistralModel"] set and assert:

  • GetArchitecture() == "MistralModel"
  • GetModelType() == "mistral"

That combination is what config_parser.go relies on to classify e5-mistral as EMBEDDING — it is the load-bearing path and easy to accidentally break in a future refactor. Without a test here, nothing catches a regression.

@OpheliaLjh OpheliaLjh May 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test covers the negative case (no architectures → not embedding) but there is no counterpart for the positive case. Please add a test with a fixture that has "architectures": ["MistralModel"] set and assert:

  • GetArchitecture() == "MistralModel"
  • GetModelType() == "mistral"

That combination is what config_parser.go relies on to classify e5-mistral as EMBEDDING — it is the load-bearing path and easy to accidentally break in a future refactor. Without a test here, nothing catches a regression.


func TestMistralInstructConfig(t *testing.T) {
configPath := filepath.Join("testdata", "mistral_7b_instruct.json")

Expand Down
21 changes: 21 additions & 0 deletions pkg/hfutil/modelconfig/testdata/mistral_no_architectures.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
}