feat(schema): add minimal architecture_config for transformer models#196
feat(schema): add minimal architecture_config for transformer models#196rishi-jat wants to merge 1 commit intomodelpack:mainfrom
Conversation
Signed-off-by: Rishi Jat <rishijat098@gmail.com>
Summary of ChangesHello, 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 introduces a foundational mechanism to store basic transformer architecture details within the existing model configuration. By adding a minimal 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. 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 introduces an ArchitectureConfig to the model configuration, defining architecture-specific parameters like type, numLayers, hiddenSize, and numAttentionHeads in both the JSON schema and Go types. Initial validation tests for this new configuration have been added. The feedback suggests refactoring these tests into a table-driven format to improve maintainability and expand test coverage to include all schema constraints, such as minimum values and enum types.
| func TestArchitectureConfigValid(t *testing.T) { | ||
| validJSON := `{ | ||
| "descriptor": {"name": "test-model"}, | ||
| "config": { | ||
| "paramSize": "8b", | ||
| "architecture_config": { | ||
| "type": "transformer", | ||
| "numLayers": 32, | ||
| "hiddenSize": 4096, | ||
| "numAttentionHeads": 32 | ||
| } | ||
| }, | ||
| "modelfs": { | ||
| "type": "layers", | ||
| "diffIds": ["sha256:1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef"] | ||
| } | ||
| }` | ||
|
|
||
| err := schema.ValidatorMediaTypeModelConfig.Validate(strings.NewReader(validJSON)) | ||
| if err != nil { | ||
| t.Fatalf("expected valid architecture_config to pass, got error: %v", err) | ||
| } | ||
| } | ||
|
|
||
| func TestArchitectureConfigMissingRequiredField(t *testing.T) { | ||
| // Missing numLayers field | ||
| invalidJSON := `{ | ||
| "descriptor": {"name": "test-model"}, | ||
| "config": { | ||
| "paramSize": "8b", | ||
| "architecture_config": { | ||
| "type": "transformer", | ||
| "hiddenSize": 4096, | ||
| "numAttentionHeads": 32 | ||
| } | ||
| }, | ||
| "modelfs": { | ||
| "type": "layers", | ||
| "diffIds": ["sha256:1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef"] | ||
| } | ||
| }` | ||
|
|
||
| err := schema.ValidatorMediaTypeModelConfig.Validate(strings.NewReader(invalidJSON)) | ||
| if err == nil { | ||
| t.Fatalf("expected architecture_config with missing numLayers to fail validation") | ||
| } | ||
| } |
There was a problem hiding this comment.
The new tests for ArchitectureConfig are a great start. To improve test coverage and maintainability, I suggest consolidating them into a single, table-driven test. This aligns with the testing style used elsewhere in this file (e.g., TestConfig) and makes it easier to add more validation cases.
The new test should cover not only missing required fields but also other constraints from the schema, such as minimum: 1 for numeric fields and the enum constraint for the type field.
func TestArchitectureConfig(t *testing.T) {
testCases := []struct {
name string
config string
wantErr bool
}{
{
name: "valid",
config: `{
"descriptor": {"name": "test-model"},
"config": {
"paramSize": "8b",
"architecture_config": {
"type": "transformer", "numLayers": 32, "hiddenSize": 4096, "numAttentionHeads": 32
}
},
"modelfs": {"type": "layers", "diffIds": ["sha256:abc"]}
}`,
wantErr: false,
},
{
name: "missing numLayers",
config: `{
"descriptor": {"name": "test-model"},
"config": {
"paramSize": "8b",
"architecture_config": {
"type": "transformer", "hiddenSize": 4096, "numAttentionHeads": 32
}
},
"modelfs": {"type": "layers", "diffIds": ["sha256:abc"]}
}`,
wantErr: true,
},
{
name: "numLayers is zero",
config: `{
"descriptor": {"name": "test-model"},
"config": {
"paramSize": "8b",
"architecture_config": {
"type": "transformer", "numLayers": 0, "hiddenSize": 4096, "numAttentionHeads": 32
}
},
"modelfs": {"type": "layers", "diffIds": ["sha256:abc"]}
}`,
wantErr: true,
},
// TODO: Add more cases for other required fields and constraints.
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
err := schema.ValidatorMediaTypeModelConfig.Validate(strings.NewReader(tc.config))
if (err != nil) != tc.wantErr {
t.Errorf("Validate() error = %v, wantErr %v", err, tc.wantErr)
}
})
}
}There was a problem hiding this comment.
Pull request overview
Adds a minimal, optional architecture-level configuration to the model config schema (starting with a basic transformer shape) to support incremental progress toward a unified transformer specification (#164).
Changes:
- Added a new
ArchitectureConfigtype and an optionalarchitecture_configfield toModelConfig. - Extended the JSON schema with an
ArchitectureConfigdefinition (required fields; strict object). - Added schema validation tests for a valid
architecture_configand a missing-required-field failure.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| specs-go/v1/config.go | Introduces ArchitectureConfig struct and wires it into ModelConfig JSON serialization. |
| schema/config-schema.json | Adds schema definitions/properties for architecture_config and its required transformer fields. |
| schema/config_test.go | Adds validation tests for the new schema behavior around architecture_config. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Capabilities *ModelCapabilities `json:"capabilities,omitempty"` | ||
|
|
||
| // Architecture-specific configuration parameters | ||
| ArchitectureConfig *ArchitectureConfig `json:"architecture_config,omitempty"` |
There was a problem hiding this comment.
The JSON property name architecture_config is the only snake_case field in ModelConfig; all existing config fields use lower camelCase (e.g., paramSize, diffIds, createdAt). Consider renaming this to architectureConfig (and updating the schema/tests accordingly) to keep the public JSON API consistent.
| ArchitectureConfig *ArchitectureConfig `json:"architecture_config,omitempty"` | |
| ArchitectureConfig *ArchitectureConfig `json:"architectureConfig,omitempty"` |
| "architecture_config": { | ||
| "$ref": "#/$defs/ArchitectureConfig" | ||
| } | ||
| }, | ||
| "additionalProperties": false | ||
| }, | ||
| "ArchitectureConfig": { | ||
| "type": "object", | ||
| "properties": { | ||
| "type": { | ||
| "type": "string", | ||
| "enum": ["transformer"] | ||
| }, |
There was a problem hiding this comment.
ModelConfig already has an architecture field, and architecture_config introduces another architecture discriminator (type). As written, the schema allows contradictory values (e.g., architecture: "cnn" alongside architecture_config.type: "transformer"). Consider removing the nested type field or adding a schema dependency so that when architecture_config is present, architecture is required and constrained to "transformer" to keep configs internally consistent.
| if err == nil { | ||
| t.Fatalf("expected architecture_config with missing numLayers to fail validation") | ||
| } | ||
| } |
There was a problem hiding this comment.
The new schema adds constraints beyond “required fields” (enum restriction on type, minimum: 1 checks, and additionalProperties: false), but the added tests only cover the missing-required-field case. Please add at least one negative test covering an invalid type value and one covering an unknown extra field under architecture_config so these constraints are exercised.
| } | |
| } | |
| func TestArchitectureConfigInvalidType(t *testing.T) { | |
| // Invalid type value that is not part of the allowed enum | |
| invalidJSON := `{ | |
| "descriptor": {"name": "test-model"}, | |
| "config": { | |
| "paramSize": "8b", | |
| "architecture_config": { | |
| "type": "invalid-type", | |
| "numLayers": 32, | |
| "hiddenSize": 4096, | |
| "numAttentionHeads": 32 | |
| } | |
| }, | |
| "modelfs": { | |
| "type": "layers", | |
| "diffIds": ["sha256:1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef"] | |
| } | |
| }` | |
| err := schema.ValidatorMediaTypeModelConfig.Validate(strings.NewReader(invalidJSON)) | |
| if err == nil { | |
| t.Fatalf("expected architecture_config with invalid type to fail validation") | |
| } | |
| } | |
| func TestArchitectureConfigUnknownExtraField(t *testing.T) { | |
| // Unknown extra field under architecture_config should be rejected when additionalProperties is false | |
| invalidJSON := `{ | |
| "descriptor": {"name": "test-model"}, | |
| "config": { | |
| "paramSize": "8b", | |
| "architecture_config": { | |
| "type": "transformer", | |
| "numLayers": 32, | |
| "hiddenSize": 4096, | |
| "numAttentionHeads": 32, | |
| "unknownField": 123 | |
| } | |
| }, | |
| "modelfs": { | |
| "type": "layers", | |
| "diffIds": ["sha256:1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef"] | |
| } | |
| }` | |
| err := schema.ValidatorMediaTypeModelConfig.Validate(strings.NewReader(invalidJSON)) | |
| if err == nil { | |
| t.Fatalf("expected architecture_config with unknown extra field to fail validation") | |
| } | |
| } |
|
/cc @aftersnow |
Summary
Add an optional
architecture_configfield to ModelConfig to represent basic transformer structure.Context
There is ongoing work toward a unified transformer specification (#164), but the current spec has no place to store architecture-level information. This introduces a minimal starting point so that work can proceed incrementally.
What’s included
ArchitectureConfigstruct in specs-go/v1/config.goarchitecture_configfield on ModelConfig (omitempty)Design notes
typeto "transformer" for nowOut of scope
Follow-up
This enables future work on: