Add Onnx Config for ImageGPT #19868
Conversation
|
The documentation is not available anymore as the PR was closed or merged. |
lewtun
left a comment
lewtun
left a comment
There was a problem hiding this comment.
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. |
lewtun
left a comment
lewtun
left a comment
There was a problem hiding this comment.
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.
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, TensorTypeThere was a problem hiding this comment.
Ok. One small doubt, Should I put Any, Mapping etc under TYPE_CHECKING clause?
There was a problem hiding this comment.
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 definedThere was a problem hiding this comment.
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.
Thank you for replying and clearing the doubt.
lewtun
left a comment
lewtun
left a comment
There was a problem hiding this comment.
Thanks for iterating @RaghavPrabhakar66 !
This now LGTM, so gently pinging @sgugger for final approval 🙏
sgugger
left a comment
sgugger
left a comment
There was a problem hiding this comment.
Just one small nit in the doc and we should be good to go!
Thanks for working on this!
Co-authored-by: Sylvain Gugger <[email protected]>
|
@sgugger fixed it. |
|
Thanks! |
5 participants
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