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.

Sign up for GitHub

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

Jump to bottom

add GPT-J ONNX config to Transformers #16274

Merged
merged 4 commits into from
Mar 23, 2022
Merged

add GPT-J ONNX config to Transformers #16274

merged 4 commits into from
Mar 23, 2022

Conversation

chainyo
Copy link
Contributor

@chainyo chainyo commented Mar 19, 2022 •
edited
Loading

What does this PR do?

I'm looking for contributing to Transformers repository by adding more OnnxConfig to available models on the hub.
I have created a little organization ONNXConfig for all to track the models that needs support for ONNX.

This is the first contribution since CamemBERT OnnxConfig some months ago.

I took example on GPT2 and GPT-Neo OnnxConfig but I'm not sure if everything is good or if GPT-J needs special things to be added.

So this PR is a work in progress. If anyone can send me ressources to read to understand if it lacks anything, it would be awesome! 🤗

Who can review?

Models GPT2 / GPT-Neo
@LysandreJik @michaelbenayoun

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Mar 19, 2022 •
edited
Loading

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

lewtun
lewtun approved these changes Mar 21, 2022
View reviewed changes
Copy link
Member

@lewtun lewtun left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for adding this very clean implementation @chainyo 🔥 !

The PR looks good and I've just left a few minor suggestions.

I think one useful check would be to see if you're able to export the model with the LM head and do a simple greedy search decoding with ONNX Runtime of some prompt. We've had some users report issues with TensorRT in #15640 and I'm curious to know if the same exists with ONNX Runtime (I don't think so, but nice to check).

src/transformers/models/gptj/configuration_gptj.py Show resolved Hide resolved
src/transformers/onnx/features.py Outdated Show resolved Hide resolved
src/transformers/onnx/features.py Show resolved Hide resolved
@chainyo chainyo mentioned this pull request Mar 21, 2022
Closed
@chainyo
Copy link
Contributor Author

chainyo commented Mar 23, 2022

So what is the next step to make it merged ?

@lewtun
Copy link
Member

lewtun commented Mar 23, 2022

Hey @chainyo the last thing to check is that the slow tests pass with:

RUN_SLOW=1 python -m pytest tests/onnx/test_onnx_v2.py -k "gpt-j"

We only run these on the main branch, so it would be good to know that they pass before we merge.

Apart from that, the PR looks really good - gently pinging @sgugger or @LysandreJik for final approval

sgugger
sgugger approved these changes Mar 23, 2022
View reviewed changes
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.

Thanks for your contribution!

@sgugger sgugger merged commit 029b0d9 into huggingface:main Mar 23, 2022
@chainyo chainyo deleted the add-gptj-onnx-config branch March 24, 2022 07:55
FrancescoSaverioZuppichini pushed a commit that referenced this pull request Mar 24, 2022
@chainyo @lewtun @FrancescoSaverioZuppichini
add GPT-J ONNX config to Transformers (#16274)
241264a 
* add GPT-J ONNX config to Transformers

* remove token-classification features mapping

Co-authored-by: lewtun <lewis.c.tunstall@gmail.com>

* add question-answering features mapping

Co-authored-by: lewtun <lewis.c.tunstall@gmail.com>

* add GPT2 config init to GPT2 config + copie shebang for fix-copies

Co-authored-by: ChainYo <t.chaigneau.tc@gmail.com>
Co-authored-by: lewtun <lewis.c.tunstall@gmail.com>
@chainyo chainyo mentioned this pull request Apr 14, 2022
Merged
@lewtun lewtun mentioned this pull request May 4, 2022
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lewtun lewtun lewtun approved these changes

@sgugger sgugger sgugger approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@chainyo @HuggingFaceDocBuilderDev @lewtun @sgugger