-
Notifications
You must be signed in to change notification settings - Fork 25.3k
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
Add Onnx Config for ImageGPT #19868
Add Onnx Config for ImageGPT #19868
Conversation
The documentation is not available anymore as the PR was closed or merged. |
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 for adding ONNX support for this model @RaghavPrabhakar66 🤗 !
Could you please confirm that the slow tests pass by running:
RUN_SLOW=1 pytest tests/onnx/test_onnx_v2.py -k "imagegpt"
@lewtun With latest changes, all the 4 tests are now passing. |
Thanks for iterating so fast, @RaghavPrabhakar66. Good work! If you have time, you can try to upload an ONNX ImageGPT model to the ONNX organization on the hub if you want. |
@chainyo Sure. |
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 a lot for sharing the test runs @RaghavPrabhakar66 ! I spotted one last thing we need to fix, but then this should be good to go 🚀
from collections import OrderedDict | ||
from typing import Any, Mapping, Optional | ||
|
||
from transformers.feature_extraction_utils import FeatureExtractionMixin |
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.
We use relative imports in the transformers
codebase, so would you mind changing these two lines please?
Also, since we're only using these imports for type checking we should put them under a TYPE_CHECKING
clause like this:
from typing import TYPE_CHECKING
if TYPE_CHECKING:
from ... import FeatureExtractionMixin, TensorType
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.
Ok. One small doubt, Should I put Any
, Mapping
etc under TYPE_CHECKING
clause?
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.
Also, after putting TensorType
under TYPE_CHECKING
clause, I am getting following error
RuntimeError: Failed to import transformers.models.imagegpt.configuration_imagegpt because of the following error (look up to see its traceback):
name 'TensorType' is not defined
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.
It's OK to leave Any
etc as proper imports - the TYPE_CHECKING
clause is usually reserved for imports coming from the transformers
library :)
To fix your issue, you need to put TensorType
in quotes, i.e. "TensorType"
. You can find more information about "forward referencing" here: https://peps.python.org/pep-0484/#forward-references
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.
Thank you for replying and clearing the doubt.
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 for iterating @RaghavPrabhakar66 !
This now LGTM, so gently pinging @sgugger for final approval 🙏
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 one small nit in the doc and we should be good to go!
Thanks for working on this!
Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
@sgugger fixed it. |
Thanks! |
* add Onnx Config for ImageGPT * add generate_dummy_inputs for onnx config * add TYPE_CHECKING clause * Update doc for generate_dummy_inputs Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com> Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
* add Onnx Config for ImageGPT * add generate_dummy_inputs for onnx config * add TYPE_CHECKING clause * Update doc for generate_dummy_inputs Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com> Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
What does this PR do?
Fixes #16308
Add changes to make ImageGPT models available for Onnx conversion.
Who can review?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.
@chainyo