[Misc] Add model config parsing support for more model types#296
[Misc] Add model config parsing support for more model types#296
Conversation
Summary of ChangesHello @beiguo218, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the model configuration parsing capabilities of the system by integrating support for numerous contemporary Hugging Face models. The changes involve adding new configuration structures, updating existing ones to accommodate model-specific parameters like flexible token IDs and advanced RoPE scaling, and introducing the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly expands model support by adding configuration parsing for several new model families, including Gemma, Phi-3, and Qwen variants. The changes are well-structured, with new files for each model and comprehensive unit tests. I've identified a few minor issues in the new tests, such as incorrect comments and a bug in an error message. Additionally, I've suggested some improvements to the test code structure to enhance readability and reduce code duplication, which is particularly beneficial given the number of new test files added.
| // Check parameter count (should be approximately 7B) | ||
| paramCount := config.GetParameterCount() | ||
| expectedCount := int64(30_000_000_000) // 7B parameters | ||
| if paramCount != expectedCount { | ||
| t.Errorf("Expected parameter count to be %d, but got %d", expectedCount, paramCount) | ||
| } | ||
|
|
||
| // Check RoPE theta value (specific to Qwen3) | ||
| if qwen3MoeConfig.RopeTheta != 10000000.0 { | ||
| t.Errorf("Expected RoPE theta to be 5000000.0, but got %f", qwen3MoeConfig.RopeTheta) | ||
| } |
There was a problem hiding this comment.
There are a couple of issues in this test case:
- The comment for the parameter count check is misleading. It says
// 7B parametersbut the expected value is 30B. The comment should be updated to reflect the correct expected value. - The error message for the
RopeThetacheck has an incorrect expected value (5000000.0instead of10000000.0). This could cause confusion when debugging test failures.
| // Check parameter count (should be approximately 7B) | |
| paramCount := config.GetParameterCount() | |
| expectedCount := int64(30_000_000_000) // 7B parameters | |
| if paramCount != expectedCount { | |
| t.Errorf("Expected parameter count to be %d, but got %d", expectedCount, paramCount) | |
| } | |
| // Check RoPE theta value (specific to Qwen3) | |
| if qwen3MoeConfig.RopeTheta != 10000000.0 { | |
| t.Errorf("Expected RoPE theta to be 5000000.0, but got %f", qwen3MoeConfig.RopeTheta) | |
| } | |
| // Check parameter count (should be approximately 30B) | |
| paramCount := config.GetParameterCount() | |
| expectedCount := int64(30_000_000_000) // 30B parameters | |
| if paramCount != expectedCount { | |
| t.Errorf("Expected parameter count to be %d, but got %d", expectedCount, paramCount) | |
| } | |
| // Check RoPE theta value (specific to Qwen3) | |
| if qwen3MoeConfig.RopeTheta != 10000000.0 { | |
| t.Errorf("Expected RoPE theta to be 10000000.0, but got %f", qwen3MoeConfig.RopeTheta) | |
| } |
| if config.AutoMap == nil { | ||
| t.Error("Expected auto_map to be parsed, but it is nil") | ||
| } else { | ||
| expectedAutoConfig := "configuration_deepseek.DeepseekV3Config" | ||
| if config.AutoMap.AutoConfig != expectedAutoConfig { | ||
| t.Errorf("Expected AutoConfig to be '%s', but got '%s'", expectedAutoConfig, config.AutoMap.AutoConfig) | ||
| } | ||
|
|
||
| expectedAutoModel := "modeling_deepseek.DeepseekV3ForCausalLM" | ||
| if config.AutoMap.AutoModelForCausalLM != expectedAutoModel { | ||
| t.Errorf("Expected AutoModelForCausalLM to be '%s', but got '%s'", expectedAutoModel, config.AutoMap.AutoModelForCausalLM) | ||
| } | ||
|
|
||
| expectedAutoModelBase := "modeling_deepseek.DeepseekV3Model" | ||
| if config.AutoMap.AutoModel != expectedAutoModelBase { | ||
| t.Errorf("Expected AutoModel to be '%s', but got '%s'", expectedAutoModelBase, config.AutoMap.AutoModel) | ||
| } | ||
| } |
There was a problem hiding this comment.
The test for auto_map parsing can be made more concise and robust. Using t.Fatal when config.AutoMap is nil will correctly stop the test execution, as subsequent checks would panic. Also, structuring the checks for each field, for example using a table-driven subtest, improves readability and makes it easier to add more checks in the future.
if config.AutoMap == nil {
t.Fatal("Expected auto_map to be parsed, but it is nil")
}
t.Run("AutoMap values", func(t *testing.T) {
testCases := []struct {
name string
got string
expected string
}{
{"AutoConfig", config.AutoMap.AutoConfig, "configuration_deepseek.DeepseekV3Config"},
{"AutoModelForCausalLM", config.AutoMap.AutoModelForCausalLM, "modeling_deepseek.DeepseekV3ForCausalLM"},
{"AutoModel", config.AutoMap.AutoModel, "modeling_deepseek.DeepseekV3Model"},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
if tc.got != tc.expected {
t.Errorf("expected %q, got %q", tc.expected, tc.got)
}
})
}
})| func TestLoadGemma3Config(t *testing.T) { | ||
| configPath := filepath.Join("testdata", "gemma3.json") | ||
|
|
||
| // Load the config | ||
| config, err := LoadGemma3Config(configPath) | ||
| if err != nil { | ||
| t.Fatalf("Failed to load Gemma3 config: %v", err) | ||
| } | ||
|
|
||
| // Check that it's the correct model type | ||
| if config.GetModelType() != "gemma3" { | ||
| t.Errorf("Expected model type 'gemma3' but got '%s'", config.GetModelType()) | ||
| } | ||
| } |
There was a problem hiding this comment.
There's a lot of repeated code for loading the test configuration in this file (and other new test files in this PR). To improve maintainability and reduce duplication, consider creating a helper function to load the Gemma3Config for tests. This pattern can be applied to the other new test files as well (e.g., phi3_test.go, phi3small_test.go, etc.).
func loadGemma3TestConfig(t *testing.T) *Gemma3Config {
t.Helper()
configPath := filepath.Join("testdata", "gemma3.json")
config, err := LoadGemma3Config(configPath)
if err != nil {
t.Fatalf("Failed to load Gemma3 config: %v", err)
}
return config
}
func TestLoadGemma3Config(t *testing.T) {
config := loadGemma3TestConfig(t)
// Check that it's the correct model type
if config.GetModelType() != "gemma3" {
t.Errorf("Expected model type 'gemma3' but got '%s'", config.GetModelType())
}
}| // Check auto_map parsing | ||
| if phi3Config.AutoMap == nil { | ||
| t.Error("Expected auto_map to be parsed, but it is nil") | ||
| } else { | ||
| expectedAutoConfig := "configuration_phi3_v.Phi3VConfig" | ||
| if phi3Config.AutoMap.AutoConfig != expectedAutoConfig { | ||
| t.Errorf("Expected AutoConfig to be '%s', but got '%s'", expectedAutoConfig, phi3Config.AutoMap.AutoConfig) | ||
| } | ||
|
|
||
| expectedAutoModel := "modeling_phi3_v.Phi3VForCausalLM" | ||
| if phi3Config.AutoMap.AutoModelForCausalLM != expectedAutoModel { | ||
| t.Errorf("Expected AutoModelForCausalLM to be '%s', but got '%s'", expectedAutoModel, phi3Config.AutoMap.AutoModelForCausalLM) | ||
| } | ||
| } |
There was a problem hiding this comment.
This test for auto_map can be improved for conciseness and robustness. Using t.Fatal when phi3Config.AutoMap is nil is better as it stops the test immediately. Also, structuring the checks for each field makes the test cleaner.
// Check auto_map parsing
if phi3Config.AutoMap == nil {
t.Fatal("Expected auto_map to be parsed, but it is nil")
}
if got, want := phi3Config.AutoMap.AutoConfig, "configuration_phi3_v.Phi3VConfig"; got != want {
t.Errorf("AutoConfig: got %q, want %q", got, want)
}
if got, want := phi3Config.AutoMap.AutoModelForCausalLM, "modeling_phi3_v.Phi3VForCausalLM"; got != want {
t.Errorf("AutoModelForCausalLM: got %q, want %q", got, want)
}| // Check parameter count (should be approximately 7B) | ||
| paramCount := config.GetParameterCount() | ||
| expectedCount := int64(4_000_000_000) // 7B parameters | ||
| if paramCount != expectedCount { | ||
| t.Errorf("Expected parameter count to be %d, but got %d", expectedCount, paramCount) | ||
| } |
There was a problem hiding this comment.
The comment for the parameter count check is misleading. It says // 7B parameters but the expected value is 4B. Please update the comment to match the expected value.
| // Check parameter count (should be approximately 7B) | |
| paramCount := config.GetParameterCount() | |
| expectedCount := int64(4_000_000_000) // 7B parameters | |
| if paramCount != expectedCount { | |
| t.Errorf("Expected parameter count to be %d, but got %d", expectedCount, paramCount) | |
| } | |
| // Check parameter count (should be approximately 4B) | |
| paramCount := config.GetParameterCount() | |
| expectedCount := int64(4_000_000_000) // 4B parameters | |
| if paramCount != expectedCount { | |
| t.Errorf("Expected parameter count to be %d, but got %d", expectedCount, paramCount) | |
| } |
| // Check parameter count (should be approximately 7B) | ||
| paramCount := config.GetParameterCount() | ||
| expectedCount := int64(235_000_000_000) // 7B parameters | ||
| if paramCount != expectedCount { | ||
| t.Errorf("Expected parameter count to be %d, but got %d", expectedCount, paramCount) | ||
| } |
There was a problem hiding this comment.
The comment for the parameter count check is misleading. It says // 7B parameters but the expected value is 235B. Please update the comment to match the expected value.
| // Check parameter count (should be approximately 7B) | |
| paramCount := config.GetParameterCount() | |
| expectedCount := int64(235_000_000_000) // 7B parameters | |
| if paramCount != expectedCount { | |
| t.Errorf("Expected parameter count to be %d, but got %d", expectedCount, paramCount) | |
| } | |
| // Check parameter count (should be approximately 235B) | |
| paramCount := config.GetParameterCount() | |
| expectedCount := int64(235_000_000_000) // 235B parameters | |
| if paramCount != expectedCount { | |
| t.Errorf("Expected parameter count to be %d, but got %d", expectedCount, paramCount) | |
| } |
2 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
Does this PR introduce a user-facing change?
Users who using model agent or ome agent model metadata now can have more typed models to be parsed properly in terms of model configuration.