Skip to content

feat(schema): add minimal architecture_config for transformer models #196

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
rishi-jat wants to merge 1 commit into
base: main
Choose a base branch
from
+92 −0
Open

feat(schema): add minimal architecture_config for transformer models #196

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
26 changes: 26 additions & 0 deletions schema/config-schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,34 @@
},
"capabilities": {
"$ref": "#/$defs/ModelCapabilities"
},
"architecture_config": {
"$ref": "#/$defs/ArchitectureConfig"
}
},
"additionalProperties": false
},
"ArchitectureConfig": {
"type": "object",
"properties": {
"type": {
"type": "string",
"enum": ["transformer"]
},
Comment on lines +45 to +57
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
"numLayers": {
"type": "integer",
"minimum": 1
},
"hiddenSize": {
"type": "integer",
"minimum": 1
},
"numAttentionHeads": {
"type": "integer",
"minimum": 1
}
},
"required": ["type", "numLayers", "hiddenSize", "numAttentionHeads"],
"additionalProperties": false
},
"ModelDescriptor": {
Expand Down
48 changes: 48 additions & 0 deletions schema/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -589,3 +589,51 @@ func TestConfig(t *testing.T) {
}
}
}

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")
}
}
Comment on lines +593 to +639
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Mar 25, 2026

Choose a reason for hiding this comment

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

medium

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)
			}
		})
	}
}

Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
}
}
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")
}
}

Copilot uses AI. Check for mistakes.
18 changes: 18 additions & 0 deletions specs-go/v1/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,24 @@ type ModelConfig struct {

// Special capabilities that the model supports
Capabilities *ModelCapabilities `json:"capabilities,omitempty"`

// Architecture-specific configuration parameters
ArchitectureConfig *ArchitectureConfig `json:"architecture_config,omitempty"`
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
ArchitectureConfig *ArchitectureConfig `json:"architecture_config,omitempty"`
ArchitectureConfig *ArchitectureConfig `json:"architectureConfig,omitempty"`

Copilot uses AI. Check for mistakes.
}

// ArchitectureConfig defines architecture-specific parameters for the model.
type ArchitectureConfig struct {
// Type specifies the model architecture type (e.g., "transformer").
Type string `json:"type"`

// NumLayers is the number of layers in the model.
NumLayers int `json:"numLayers"`

// HiddenSize is the dimensionality of the hidden representations.
HiddenSize int `json:"hiddenSize"`

// NumAttentionHeads is the number of attention heads.
NumAttentionHeads int `json:"numAttentionHeads"`
}

// ModelFS describes a layer content addresses
Expand Down
Loading