-
Notifications
You must be signed in to change notification settings - Fork 271
Refactor IO config to yaml in assets #2311
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.
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request introduces a major refactor of the HuggingFace ONNX I/O configuration system, migrating from hardcoded Python class registries to a flexible YAML-driven approach. The changes enable easier extensibility and clearer configuration management for model optimization workflows.
Changes:
- Replaced Python-based OnnxConfig class registries with YAML configuration files for tasks and diffusers components
- Introduced unified dummy input generators that read specifications from YAML rather than class hierarchies
- Consolidated I/O configuration generation into a single task-driven API with automatic optional input filtering based on model signatures
Reviewed changes
Copilot reviewed 29 out of 34 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| olive/assets/io_configs/tasks.yaml | Defines input/output specifications for 15+ HuggingFace tasks |
| olive/assets/io_configs/diffusers.yaml | Defines I/O specs for diffusers components (SD, SDXL, SD3, Flux, Sana) |
| olive/assets/io_configs/defaults.yaml | Provides default dimension values and attribute aliases for config resolution |
| olive/common/hf/io_config/yaml_loader.py | YAML loading and config resolution utilities with LRU caching |
| olive/common/hf/io_config/task_config.py | New unified API for generating I/O configs and dummy inputs |
| olive/common/hf/io_config/input_generators.py | Refactored to use YAML-driven input generation |
| olive/common/hf/model_io.py | Updated to use new task-driven API |
| olive/common/hf/wrapper.py | Migrated to use resolve_alias for config attribute access |
| test/common/hf/io_config/*.py | Updated tests for new YAML-driven system |
Comments suppressed due to low confidence (1)
olive/common/hf/io_config/task_config.py:1
- Missing check for whether 'with_past' key exists in task_spec before accessing it. This will raise a KeyError if a task doesn't define with_past overrides. Add a check:
if self.use_past_in_inputs and 'with_past' in self.task_spec:
# -------------------------------------------------------------------------
b5aeb2d to
51c3a9a
Compare
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
| height: [sample_size, image_size, vision_config.image_size] | ||
| width: [sample_size, image_size, vision_config.image_size] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous entries include the key in the value i.e. name is included in the list of aliases. These two entries don't. Should they? Or should the above 4 be updated to exclude the key name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call! No need to put itself in the list. I'll update this.
| # Add aliases if the same concept has different names | ||
| aliases: | ||
| my_custom_dim: [custom_dim, my_dim, special_dim] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above!
| height: [sample_size, image_size, vision_config.image_size] | ||
| width: [sample_size, image_size, vision_config.image_size] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same problem here too. height and weight are not included in the list of values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename this file something more specific - yaml_loader is too generic and says nothing about the actual functionality it provides.
YAML implementation is anyways private to the file. Public API seems more like utility functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated to io_resolver.
|
|
||
| def _load_yaml(filename: str) -> dict[str, Any]: | ||
| """Load a YAML file from the config directory.""" | ||
| config_files = files("olive.assets.io_configs") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't importlib require user to create a whl and install the package for the system to pickup the change to config files.
(Unless of course if the user has the cloned repo in editable mode.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we move assets folder into olive package, so this change is for developers. If user has its own io config, they can specify it in the config file.
|
Suggestion: Current implementation requires user to clone the git repo and modify it. This approach has challenges, most importantly dev vs. released version. Instead, rather than piling all the configuration in a single file, have one file hold configuration for one task. This will allow you to accept new task configuration on command line without explicitly modifying cloned repo. This approach also allows working with released version - which is always recommended compared to using a git clone. P.S. That's the approach both lm-eval and bb implements. |
We now have 22 tasks, having separate config files for each task makes maintenance difficult. Most use cases would be adding new task IO rather than modifying existing ones. How about keeping the current configuration files, but also allowing override with external config files? |
How often do we expect to change any of these ones they are committed? For all intents and purposes, this is readonly data, unlikely to change ever. But if it makes it simpler, all for it. On the implementation front, you could basically assume any number of tasks per file. So, there shouldn't be any difference whether you are loading the Olive released or the user provided one. Basically, the code path should remain the same. |
i think in real life, it is rare that user wants to provide his own io config for his own task. if a task is not supported by Olive, then we should add support it. |
This pull request introduces a major refactor of the HuggingFace ONNX I/O configuration system in Olive. The changes move away from hardcoded Python class registries and instead adopt a YAML-driven approach for specifying input/output configurations for both tasks and diffusers pipelines. This enables easier extensibility and clearer, data-driven configuration management. Additionally, the update adds default dummy input shapes and rich configuration templates for tasks and diffusers components.
The most important changes are:
YAML-based IO Configuration System:
defaults.yaml,tasks.yaml,diffusers.yaml) inolive/assets/io_configs/that define default dummy input shapes, task templates, and diffusers component/pipeline specifications for ONNX export. These files replace the previous Python-based registries and provide a more flexible and maintainable configuration system. [1] [2] [3]Refactor of IO Config Python Interface:
olive/common/hf/io_config/__init__.pyto remove all hardcoded class registries and mapping logic, and instead import new YAML-driven utility functions (generate_dummy_inputs,get_io_config,get_diffusers_io_config, etc.). This greatly simplifies the interface and shifts responsibility to the new YAML configuration system.Legal and Organizational Improvements:
olive/assets/__init__.pyandolive/assets/io_configs/__init__.py.## Describe your changesChecklist before requesting a review
lintrunner -a(Optional) Issue link