-
Notifications
You must be signed in to change notification settings - Fork 149
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
Refactor Initialized Model Export #2224
Conversation
…arseml into export_instantiated_model
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.
Left a few comments, all in all, great changes!
@@ -129,7 +129,8 @@ def remove_leftover_files(self): | |||
torch_onnx_export_transform, _TorchOnnxExport | |||
), "Expected the first transform from self.transform to be _TorchOnnxExport" | |||
for file in torch_onnx_export_transform.leftover_files: | |||
os.remove(file) | |||
if os.path.exists(file): |
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.
just curious, why is this change needed now?
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.
Honestly, I am not quite sure. I would occasionally get an error about deleting a file that didn't exist when exporting and this fixed it
:param integration: Optional name of the integration to use. If not provided, | ||
will attempt to infer it from the source_path. | ||
:return: The name of the integration to use for exporting the model. | ||
""" | ||
|
||
integration = integration or _infer_integration_from_source_path(source_path) | ||
|
||
# attempt to infer transformers based on model attribute | ||
if source_model is not None and hasattr(source_model, "config_class"): |
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 config_class
is a deciding attribute here?
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 really just to tell if its a PreTrainedModel, but I didn't want to have to add the transformers dependency to this part of the repo
Example