Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,34 @@
},
"capabilities": {
"$ref": "#/$defs/ModelCapabilities"
},
"architecture_config": {
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

The new property name architecture_config introduces snake_case into the schema, while other ModelConfig fields use lowerCamelCase (e.g., paramSize, docURL, diffIds). Consider renaming this to architectureConfig (and updating the Go struct tags/tests/tools accordingly) to keep JSON field naming consistent.

Suggested change
"architecture_config": {
"architectureConfig": {

Copilot uses AI. Check for mistakes.
"$ref": "#/$defs/ArchitectureConfig"
}
},
"additionalProperties": false
},
"ArchitectureConfig": {
"type": "object",
"properties": {
"type": {
"type": "string",
"enum": ["transformer"]
},
"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
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
}
Comment on lines +596 to +603
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

The tests and schema are using the new architecture_config field name. If the intent is to follow the existing lowerCamelCase JSON convention for config fields, consider renaming this to architectureConfig here (and in the schema + Go structs) to avoid introducing an inconsistent naming style.

Copilot uses AI. Check for mistakes.
},
"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")
}
}
18 changes: 18 additions & 0 deletions specs-go/v1/config.go
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 26, 2026

Choose a reason for hiding this comment

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

json:"architecture_config,omitempty" is inconsistent with the existing JSON naming in this file (e.g., paramSize, docURL, diffIds). If possible, align the JSON tag with the prevailing lowerCamelCase convention (e.g., architectureConfig) and update the schema/tests accordingly.

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
59 changes: 59 additions & 0 deletions tools/hf_to_arch.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
#!/usr/bin/env python3
"""Convert HuggingFace config.json to architecture_config format."""

import json
import sys


REQUIRED_MAPPINGS = {
"numLayers": "num_hidden_layers",
"hiddenSize": "hidden_size",
"numAttentionHeads": "num_attention_heads",
}


def convert_hf_config(hf_config: dict) -> dict:
"""Convert HuggingFace config to architecture_config format."""
arch_config = {"type": "transformer"}

for arch_key, hf_key in REQUIRED_MAPPINGS.items():
if hf_key not in hf_config:
raise ValueError(f"missing required field: {hf_key}")
value = hf_config[hf_key]
if not isinstance(value, int) or isinstance(value, bool):
Comment on lines +15 to +23
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

convert_hf_config assumes the parsed JSON is a dict/object. If config.json is a JSON array/string/etc, the current checks (e.g., hf_key not in hf_config) will behave incorrectly or raise confusing errors. Consider validating hf_config is a dict up front and raising a clear ValueError if not.

Copilot uses AI. Check for mistakes.
raise ValueError(f"field {hf_key} must be an integer, got {type(value).__name__}")
if value < 1:
raise ValueError(f"field {hf_key} must be >= 1, got {value}")
arch_config[arch_key] = value

return arch_config


def main():
if len(sys.argv) != 2:
print(f"usage: {sys.argv[0]} <config.json>", file=sys.stderr)
sys.exit(1)

config_path = sys.argv[1]
Comment on lines +33 to +37
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.

medium

For more robust and maintainable command-line argument parsing, consider using the argparse module from the standard library. It provides features like automatic help message generation (-h/--help) and is the standard for creating command-line interfaces in Python.

You will need to add import argparse at the top of the file.

Suggested change
if len(sys.argv) != 2:
print(f"usage: {sys.argv[0]} <config.json>", file=sys.stderr)
sys.exit(1)
config_path = sys.argv[1]
parser = argparse.ArgumentParser(description=__doc__.strip())
parser.add_argument("config_path", metavar="config.json", help="Path to HuggingFace config.json")
args = parser.parse_args()
config_path = args.config_path


try:
with open(config_path, "r") as f:
hf_config = json.load(f)
except FileNotFoundError:
print(f"error: file not found: {config_path}", file=sys.stderr)
sys.exit(1)
except json.JSONDecodeError as e:
print(f"error: invalid JSON: {e}", file=sys.stderr)
sys.exit(1)

try:
arch_config = convert_hf_config(hf_config)
except ValueError as e:
print(f"error: {e}", file=sys.stderr)
sys.exit(1)

print(json.dumps(arch_config, indent=2, sort_keys=True))


if __name__ == "__main__":
main()
146 changes: 146 additions & 0 deletions tools/hf_to_arch_test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
#!/usr/bin/env python3
"""Tests for hf_to_arch.py"""

import json
import subprocess
import sys
import tempfile
import os

SCRIPT_PATH = os.path.join(os.path.dirname(__file__), "hf_to_arch.py")


def run_script(config_content: str) -> tuple:
"""Run hf_to_arch.py with given config content, return (exitcode, stdout, stderr)."""
with tempfile.NamedTemporaryFile(mode="w", suffix=".json", delete=False) as f:
f.write(config_content)
f.flush()
temp_path = f.name

try:
result = subprocess.run(
[sys.executable, SCRIPT_PATH, temp_path],
capture_output=True,
text=True,
)
return result.returncode, result.stdout, result.stderr
finally:
os.unlink(temp_path)


def test_valid_config():
"""Valid HuggingFace config produces correct output."""
config = json.dumps({
"num_hidden_layers": 32,
"hidden_size": 4096,
"num_attention_heads": 32,
"vocab_size": 32000,
})

exitcode, stdout, stderr = run_script(config)

assert exitcode == 0, f"expected exit 0, got {exitcode}: {stderr}"
output = json.loads(stdout)
assert output == {
"type": "transformer",
"numLayers": 32,
"hiddenSize": 4096,
"numAttentionHeads": 32,
}, f"unexpected output: {output}"
print("PASS: test_valid_config")


def test_missing_field():
"""Missing required field produces error."""
config = json.dumps({
"num_hidden_layers": 32,
"hidden_size": 4096,
})

exitcode, stdout, stderr = run_script(config)

assert exitcode != 0, "expected non-zero exit for missing field"
assert "num_attention_heads" in stderr, f"error should mention missing field: {stderr}"
print("PASS: test_missing_field")


def test_invalid_json():
"""Invalid JSON produces error."""
exitcode, stdout, stderr = run_script("not valid json {")

assert exitcode != 0, "expected non-zero exit for invalid JSON"
assert "invalid JSON" in stderr.lower() or "json" in stderr.lower(), f"error should mention JSON: {stderr}"
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

This assertion lowercases stderr but checks for the mixed-case substring "invalid JSON", which can never match. Consider checking for "invalid json" (lowercase) or just relying on the existing "json" substring check.

Suggested change
assert "invalid JSON" in stderr.lower() or "json" in stderr.lower(), f"error should mention JSON: {stderr}"
assert "invalid json" in stderr.lower() or "json" in stderr.lower(), f"error should mention JSON: {stderr}"

Copilot uses AI. Check for mistakes.
print("PASS: test_invalid_json")


def test_file_not_found():
"""Non-existent file produces error."""
result = subprocess.run(
[sys.executable, SCRIPT_PATH, "/nonexistent/path/config.json"],
capture_output=True,
text=True,
)

assert result.returncode != 0, "expected non-zero exit for missing file"
assert "not found" in result.stderr.lower(), f"error should mention file not found: {result.stderr}"
print("PASS: test_file_not_found")


def test_invalid_field_type():
"""Non-integer field produces error."""
config = json.dumps({
"num_hidden_layers": "32",
"hidden_size": 4096,
"num_attention_heads": 32,
})

exitcode, stdout, stderr = run_script(config)

assert exitcode != 0, "expected non-zero exit for invalid type"
assert "integer" in stderr.lower(), f"error should mention type: {stderr}"
print("PASS: test_invalid_field_type")


def test_zero_value():
"""Zero value produces error."""
config = json.dumps({
"num_hidden_layers": 0,
"hidden_size": 4096,
"num_attention_heads": 32,
})

exitcode, stdout, stderr = run_script(config)

assert exitcode != 0, "expected non-zero exit for zero value"
assert ">= 1" in stderr, f"error should mention minimum: {stderr}"
print("PASS: test_zero_value")


def test_bool_value():
"""Boolean value produces error."""
config = json.dumps({
"num_hidden_layers": True,
"hidden_size": 4096,
"num_attention_heads": 32,
})

exitcode, stdout, stderr = run_script(config)

assert exitcode != 0, "expected non-zero exit for bool value"
assert "integer" in stderr.lower(), f"error should mention type: {stderr}"
print("PASS: test_bool_value")


def main():
test_valid_config()
test_missing_field()
test_invalid_json()
test_file_not_found()
test_invalid_field_type()
test_zero_value()
test_bool_value()
print("\nAll tests passed.")
Comment on lines +134 to +142
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.

medium

While this test script is functional, structuring it with Python's standard unittest framework would make it more robust and align with common testing practices. unittest provides a test runner, test discovery, and a richer set of assertion methods (e.g., self.assertEqual(), self.assertIn()). This would also remove the need for a custom main function to run tests and manual print statements for test status.

For example, you could structure your tests in a class:

import unittest
# ... other imports

class HFToArchTests(unittest.TestCase):
    def test_valid_config(self):
        # ... test logic ...
        self.assertEqual(exitcode, 0, f"...")
        # ...

if __name__ == '__main__':
    unittest.main()

This approach is more maintainable and scalable as more tests are added.



if __name__ == "__main__":
main()
Comment on lines +134 to +146
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

These Python tests are not picked up by the repository's default make test (which currently runs go test ./... only). If these are meant to run in CI, consider integrating them into the test target/CI workflow (or converting to a Go test that shells out to the tool), otherwise the PR's "Tests included" claim may be misleading.

Copilot uses AI. Check for mistakes.
Loading