Skip to content

Move & rename onnx_export#1685

Merged
fxmarty merged 3 commits into
huggingface:mainfrom
fxmarty:move-onnx-model-export-to-convert
Feb 8, 2024
Merged

Move & rename onnx_export#1685
fxmarty merged 3 commits into
huggingface:mainfrom
fxmarty:move-onnx-model-export-to-convert

Conversation

@fxmarty

@fxmarty fxmarty commented Feb 6, 2024

Copy link
Copy Markdown
Contributor

As per title. It should ideally not be in __main__.py, as this file is kept simply for backward compatibility of pythonm -m optimum.exporters.onnx.

Motivation: not seeing from optimum.exporters.onnx.__main__ import onnx_export in code snippets.

return export_output


def onnx_export_from_model(

@fxmarty fxmarty Feb 6, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function is also renamed to onnx_export_from_model to explicit its role, and documented.

Comment on lines +1010 to +1018
if (
not custom_architecture
and library_name != "diffusers"
and task + "-with-past"
in TasksManager.get_supported_tasks_for_model_type(model_type, "onnx", library_name=library_name)
and not monolith
):
# -with-past is the default.
task = task + "-with-past"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is added compared to main - the default should be the export with KV cache.

"""
library_name = TasksManager._infer_library_from_model(model)

# TODO: call standardize_model_attributes here once its model_name_or_path argument is optional.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As standardize_model_attributes is not called here but only in main_export, calling this function with a timm/diffusers/sentence-transformers model will fail. Only Transformers is working, I believe.

library_name (`Optional[str]`, *optional*)::
The library name of the model. Can be any of "transformers", "timm", "diffusers", "sentence_transformers".
"""
# TODO: make model_name_or_path an optional argument here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mht-sharma Is this argument needed? For timm, the config indeed seem not to be attached to the timm.models.xxx.Xxx (e.g. timm.models.resnet.ResNet) instance, but is this an issue for the export? Do we need any info from the config.json?

For diffusers, sentence-transformers, we are fine.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fxmarty indeed its not required, its mapped to the pretrained_cfg. Created a PR with the changes in respective locations. #1686. Feel free to merge after review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @mht-sharma, can you in #1686 make it so that standardize_model_attributes is called here:

# TODO: call standardize_model_attributes here once its model_name_or_path argument is optional.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

@echarlaix echarlaix left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks @fxmarty

Comment thread optimum/exporters/onnx/convert.py Outdated
Co-authored-by: Ella Charlaix <80481427+echarlaix@users.noreply.github.com>
@fxmarty fxmarty merged commit 32a51af into huggingface:main Feb 8, 2024
young-developer pushed a commit to young-developer/optimum that referenced this pull request May 10, 2024
* move & rename onnx_export

* fix test

* Update optimum/exporters/onnx/convert.py

Co-authored-by: Ella Charlaix <80481427+echarlaix@users.noreply.github.com>

---------

Co-authored-by: Ella Charlaix <80481427+echarlaix@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants