-
Notifications
You must be signed in to change notification settings - Fork 168
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
Fix several bugs & add Bert inc example to pipeline #1492
base: main
Are you sure you want to change the base?
Conversation
olive/workflows/run/config.py
Outdated
def resolve_all_data_configs(config, values): | ||
"""Recursively traverse the config dictionary to resolve all 'data_config' keys.""" | ||
for param_name, param_value in config.items(): | ||
if isinstance(param_value, dict): |
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.
is this recursion for cases like in inc quantization where it has a metric with data config inside it?
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.
not an issue but I think the recursion feels more correct if the order of logic is
if param_name.endswith("data_config"):
_resolve_data_config(config, values, param_name)
elif isinstance(param_value, dict):
resolve_all_data_configs(param_value, values)
since now there is an explicit base case
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.
Sounds good! Updated.
Describe your changes
Fix Olive bugs & Add Bert inc examples to example pipeline.
batch_size
is needed for Inc dataloader. Set default size to 1.QuantizationAwareTraining
pass,train_data_config
is not required if user providestraining_loop_func
.save_safetensors
as false to train argument.Checklist before requesting a review
lintrunner -a
(Optional) Issue link