-
Notifications
You must be signed in to change notification settings - Fork 487
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 Support for Decision Transformer Model #2038
Conversation
Thanks @IlyasMoutawwakil for the detail review comments, will check and re-submit again by next week. |
@IlyasMoutawwakil ,
Here is the terminal output for your reference. Please let me know, if it is good for approval. |
cool addition ! I committed a couple of suggestions, mainly about keeping the naming of dimensions closer to the config attributes. |
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
Thanks @IlyasMoutawwakil , your inputs were very valuable, it was a great learning experience while working on this PR !! |
Got to test this again and there were a few issues as in the attached log. Further analyzing, changes had to be done to |
Can you please review the changes. |
triggering tests and checking today ! |
Sure, thanks @IlyasMoutawwakil The branch might be out of sync with the main repo. Please let me know if I should re-sync, copy the PR changes and then commit again. |
@ra9hur yes please, a rebase or merge would be great ! also a |
@IlyasMoutawwakil , I have merged changes in Pardon my ignorance, couldn't quite get what you meant by Last change that I made was only ordering of outputs in OrderDict in base.py, so nothing major. Any references so that I can follow ? |
The export seems to fail due to some outputs mismatch https://github.com/huggingface/optimum/actions/runs/11931260970/job/33256099991?pr=2038#step:5:7057
I mean doing |
Thanks for your guidance !! Regarding output mismatch, it is working fine locally.
|
@ra9hur the export does work for
we get 3/3 tests that are passing, for both supported tasks and |
@IlyasMoutawwakil , I tried your changes locally and executes without errors. Good to go from my side. Checking new build errors, none of those seem to be specific to this PR. |
@IlyasMoutawwakil , Tried running pytest on the tests folder locally while referring to this link: https://huggingface.co/docs/transformers/testing
But, I am not able to exactly reproduce errors as we see here. |
|
the failing test is only related to the missing hf token, PRs from a fork will usually fail that test for security reasons. |
Thanks for the guidance !! |
Fixes #2032
@IlyasMoutawwakil ,
As per your suggestions, I have made necessary changes to support decision transformer model to ONNX. I ran the below command:
Attaching the terminal output:
output_log.txt
The model was exported successfully with a warning:
since
max diff
isnan
, printed the results of the reference and ONNX models. Have attached the results in the below text file.Note: Have extracted results from output_log.txt file to a separate text file for clarity.
output.txt
Excluding
nan
, if we closely observe, the difference seems to be well within set tolerance limits.Please review and let me know your suggestions.