Skip to content
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

Mrbean/codegen onnx #17903

Merged
merged 2 commits into from
Jun 28, 2022
Merged

Mrbean/codegen onnx #17903

merged 2 commits into from
Jun 28, 2022

Conversation

sam-h-bean
Copy link
Contributor

@sam-h-bean sam-h-bean commented Jun 27, 2022

What does this PR do?

Codegen was added with an ONNX config but not with the model added to the features manager so trying to actually export an ONNX config is failing.

11497 ± RUN_SLOW=1 pytest tests/onnx/test_onnx_v2.py -k "codegen" -v
===================================================================================== test session starts ======================================================================================
platform darwin -- Python 3.9.10, pytest-7.1.2, pluggy-1.0.0 -- /Users/marklar/workspace/transformers/venv/bin/python3
cachedir: .pytest_cache
hypothesis profile 'default' -> database=DirectoryBasedExampleDatabase('/Users/marklar/workspace/transformers/.hypothesis/examples')
rootdir: /Users/marklar/workspace/transformers, configfile: setup.cfg
plugins: xdist-2.5.0, forked-1.4.0, timeout-2.1.0, hypothesis-6.47.0, dash-2.5.0
collected 371 items / 367 deselected / 4 selected                                                                                                                                              

tests/onnx/test_onnx_v2.py::OnnxExportTestCaseV2::test_pytorch_export_029_codegen_causal_lm PASSED                                                                                       [ 25%]
tests/onnx/test_onnx_v2.py::OnnxExportTestCaseV2::test_pytorch_export_030_codegen_default PASSED                                                                                         [ 50%]
tests/onnx/test_onnx_v2.py::OnnxExportTestCaseV2::test_pytorch_export_on_cuda_029_codegen_causal_lm PASSED                                                                               [ 75%]
tests/onnx/test_onnx_v2.py::OnnxExportTestCaseV2::test_pytorch_export_on_cuda_030_codegen_default PASSED                                                                                 [100%]

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

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.

@patil-suraj @JingyaHuang

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Jun 27, 2022

The documentation is not available anymore as the PR was closed or merged.

Comment on lines 181 to +183
("camembert", "camembert-base"),
("convbert", "YituTech/conv-bert-base"),
("codegen", "Salesforce/codegen-350M-multi"),

Choose a reason for hiding this comment

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

small detail but this line should be in between camembert and convbert similar to how it is in the src/transformers/onnx/features.py file

Copy link
Member

Choose a reason for hiding this comment

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

I think that this line should go to PYTORCH_EXPORT_WITH_PAST_MODELS with gpt2 and gpt-neo. Since it's a OnnxConfigWithPast config
@sam-h-bean

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for noticing @NouamaneTazi, to make the test more organised, it would be better to place it in with_past model list.

Copy link
Contributor

@JingyaHuang JingyaHuang left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for adding the feature!

Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

LGTM, @lewtun you can merge if you're happy with the changes.

Thanks for the fix!

Copy link
Member

@lewtun lewtun left a 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 the fix and checking the slow tests pass @sam-h-bean 🚀 !!

This LGTM, so will go ahead with the merge

@lewtun lewtun merged commit b424f0b into huggingface:main Jun 28, 2022
@sam-h-bean sam-h-bean deleted the mrbean/codegen-onnx branch June 28, 2022 15:13
younesbelkada pushed a commit to younesbelkada/transformers that referenced this pull request Jun 29, 2022
viclzhu pushed a commit to viclzhu/transformers that referenced this pull request Jul 18, 2022
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.

7 participants