feat(tools): add HuggingFace config → architecture_config converter#198
feat(tools): add HuggingFace config → architecture_config converter#198rishi-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 new command-line utility designed to streamline the process of adapting HuggingFace model configurations for use within the ModelPack ecosystem. By converting 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 two new Python scripts: tools/hf_to_arch.py and tools/hf_to_arch_test.py. The hf_to_arch.py script converts HuggingFace config.json files into a simplified architecture_config format, specifically for transformer models, including robust validation for required integer fields. The hf_to_arch_test.py script provides comprehensive tests for the conversion utility, covering various valid and error-case scenarios. Feedback suggests improving the integer type check in hf_to_arch.py for clarity, refactoring its command-line argument parsing to use argparse for better maintainability, and migrating the test suite to pytest for a more standard and robust testing framework.
| 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): |
There was a problem hiding this comment.
The current type check for integers is functionally correct but a bit subtle because it relies on bool being a subclass of int in Python, which can be non-obvious. Using type(value) is not int is a more explicit and clearer way to check that the value is strictly an integer and not a boolean.
| if not isinstance(value, int) or isinstance(value, bool): | |
| if type(value) is not int: |
| if len(sys.argv) != 2: | ||
| print(f"usage: {sys.argv[0]} <config.json>", file=sys.stderr) | ||
| sys.exit(1) | ||
|
|
||
| config_path = sys.argv[1] |
There was a problem hiding this comment.
For better command-line interface design and maintainability, consider using the argparse module instead of manually parsing sys.argv. argparse provides automatic help messages, error handling for arguments, and is the standard way to create CLI tools in Python. You will also need to add import argparse at the top of the file.
| 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="Convert HuggingFace config.json to architecture_config format.") | |
| parser.add_argument("config_path", help="Path to the HuggingFace config.json file.") | |
| args = parser.parse_args() | |
| config_path = args.config_path |
| 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.") | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| main() |
There was a problem hiding this comment.
This test file implements a custom test runner by calling test functions from main() and using print statements to show progress. While this works, adopting a standard test framework like pytest would offer several advantages:
- No need for a manual runner (
mainfunction andif __name__ == '__main__'block). - More detailed and readable output on test failures.
- A rich ecosystem of plugins.
- Use of simple
assertstatements with no need for custom failure messages.
Migrating to pytest would involve removing the main function, the if __name__ == '__main__' block, and all print statements for test status. Tests could then be run with the pytest command. This would make the test suite more standard and easier to maintain.
There was a problem hiding this comment.
Pull request overview
Adds a small Python CLI utility to convert a HuggingFace config.json into a minimal transformer architecture_config JSON shape, as an initial step toward automated transformer spec generation (Issue #164).
Changes:
- Introduces
tools/hf_to_arch.pyCLI with required-field mapping and basic validation. - Adds an end-to-end subprocess-based test script
tools/hf_to_arch_test.pycovering success and several failure modes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tools/hf_to_arch.py | New converter CLI that reads HF config JSON, validates required fields, and emits deterministic architecture config JSON. |
| tools/hf_to_arch_test.py | New subprocess-driven tests for the converter’s success and error behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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] |
There was a problem hiding this comment.
convert_hf_config assumes hf_config is a dict-like JSON object. If the input JSON is a list/string/etc., the hf_key not in hf_config / hf_config[hf_key] access can raise a TypeError (or behave unexpectedly) and will bypass the current ValueError handling, resulting in a raw stack trace. Add an explicit top-level type check (e.g., require dict) and raise a ValueError with a clear message when the JSON root is not an object.
| config_path = sys.argv[1] | ||
|
|
||
| try: | ||
| with open(config_path, "r") as f: |
There was a problem hiding this comment.
open(config_path, "r") relies on the platform default encoding, which can cause failures or mis-parsing on systems where the locale encoding isn’t UTF-8. Since HuggingFace config.json files are UTF-8 JSON, open the file with an explicit encoding="utf-8" for consistent behavior across environments.
| with open(config_path, "r") as f: | |
| with open(config_path, "r", encoding="utf-8") as f: |
| 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.") | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| main() |
There was a problem hiding this comment.
This test module is a standalone script (custom main() and print-based status) rather than a framework-discoverable test (e.g., pytest/unittest). In the current repo, CI/make test only runs go test, so these tests won’t execute automatically and may silently regress. Consider integrating a Python test runner into CI/Makefile or converting these checks into the repo’s existing Go test harness; if you intend to use pytest later, also rename to test_*.py and remove the manual main().
| 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.") | |
| if __name__ == "__main__": | |
| main() |
|
/cc @aftersnow |
Summary
Add a small CLI tool that converts a HuggingFace
config.jsoninto theModelPack
architecture_configshape for transformer models.Scope
New tool: tools/hf_to_arch.py
Tests: tools/hf_to_arch_test.py
Mapping
Behavior
Limitations
These constraints are intentional. The goal is to establish a small, verifiable
baseline before expanding coverage.
Relation to #164
This is a first step toward automated generation of transformer specs from
existing models. Follow-ups can extend the mapping and integrate with the
schema and validation layers.