-
Notifications
You must be signed in to change notification settings - Fork 46
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
Consolidate tgis finetuning #116
Consolidate tgis finetuning #116
Conversation
Signed-off-by: gkumbhat <[email protected]>
Signed-off-by: gkumbhat <[email protected]>
Signed-off-by: gkumbhat <[email protected]>
Signed-off-by: gkumbhat <[email protected]>
… concept Signed-off-by: gkumbhat <[email protected]>
Signed-off-by: gkumbhat <[email protected]>
…for evaluation Signed-off-by: gkumbhat <[email protected]>
Signed-off-by: gkumbhat <[email protected]>
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.
Thanks Gaurav, great work! Some thoughts, but in general this looks a lot better
if not model_name_override.endswith(loaded_base_model): | ||
raise ValueError( | ||
log.error( |
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.
Why is this being made non fatal?
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.
This is because we are now re-training the models with fine-tuning, so model name may not be same as base_model name, os the above condition would fail.
examples/run_fine_tuning.py
Outdated
try: | ||
raw_model_text = model.run(datum.input).generated_text | ||
except Exception as ex: | ||
print(ex) |
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.
oof. Any way to make this more narrow, or is it like a general exception? Or, should we at least catch separately for in vs not in TGIS?
Co-authored-by: Alex Brooks <[email protected]> Signed-off-by: Gaurav Kumbhat <[email protected]>
Signed-off-by: gkumbhat <[email protected]>
Signed-off-by: gkumbhat <[email protected]>
Signed-off-by: gkumbhat <[email protected]>
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.
LGTM!
Consolidate tgis finetuning Signed-off-by: gkumbhat <[email protected]>
closes: #81
Changes
.train
function and related pre-processing function from fine-tuning modules to text-generation local module