-
Notifications
You must be signed in to change notification settings - Fork 31.9k
Fix duplicate arguments passed to dummy inputs in ONNX export #16045
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
Conversation
|
The documentation is not available anymore as the PR was closed or merged. |
LysandreJik
left a comment
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.
Looks good! Left two comments that should be applied 4 times each :)
src/transformers/onnx/convert.py
Outdated
| `Tuple[List[str], List[str]]`: A tuple with an ordered list of the model's inputs, and the named inputs from | ||
| the ONNX configuration. | ||
| """ | ||
| from ..tokenization_utils_base import PreTrainedTokenizerBase |
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.
I think this can be a top-level import
src/transformers/onnx/convert.py
Outdated
| "The `tokenizer` argument is deprecated and will be removed in version 5 of Transformers. Use `preprocessor` instead.", | ||
| FutureWarning, | ||
| ) | ||
| logger.warning("Overwriting the `preprocessor` argument with `tokenizer` to generate dummmy inputs.") |
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.
Maybe this can be an info as it's more additional information and not really an 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.
(warnings get displayed by default, info is displayed when users ask to have more info)
src/transformers/onnx/convert.py
Outdated
| import onnx | ||
| import tf2onnx | ||
|
|
||
| from ..tokenization_utils_base import PreTrainedTokenizerBase |
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.
Same comment about top-level
src/transformers/onnx/convert.py
Outdated
| "The `tokenizer` argument is deprecated and will be removed in version 5 of Transformers. Use `preprocessor` instead.", | ||
| FutureWarning, | ||
| ) | ||
| logger.warning("Overwriting the `preprocessor` argument with `tokenizer` to generate dummmy inputs.") |
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.
Same comment about logging level
sgugger
left a comment
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.
Good for me with Lysandre's comments! Thanks for working on this!
What does this PR do?
This PR fixes:
generate_dummy_inputs()function during the ONNX export.It also removes problematic TensorFlow integration tests, where the model implementation doesn't have parity with the PyTorch one (e.g.
camembert-baseis missing the causal LM head in TensorFlow). I'll address those issues in separate PRs as it involves touching the TensorFlow modeling files.With these fixes, all slow ONNX tests now pass in all environments (only
torch, onlytensorflow,torchandtensorflow):cc @michaelbenayoun