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

Add support for Less in GNMT #1700

Closed
RanACohen opened this issue Aug 27, 2019 · 13 comments
Closed

Add support for Less in GNMT #1700

RanACohen opened this issue Aug 27, 2019 · 13 comments
Labels
feature request request for unsupported feature or enhancement

Comments

@RanACohen
Copy link
Contributor

When I use GNMT from https://drive.google.com/drive/folders/1cWYUg1V1FFgFo8dF-W0hR2tyezCzY_UV

I get:
runtimeError: [ONNXRuntimeError] : 9 : NOT_IMPLEMENTED : Could not find an implementation for the node Less(9)

@RanACohen
Copy link
Contributor Author

I am using the latest master from today: commit f25847b

@snnn snnn added the feature request request for unsupported feature or enhancement label Aug 28, 2019
@RanACohen
Copy link
Contributor Author

This is for float64 input, I will create a MR for this.
I implemented it locally.

@RanACohen
Copy link
Contributor Author

@snnn How do I create a MR? I don't seem to have permission

@neginraoof
Copy link
Contributor

How is your implementation different from this PR? #1604
Since I'm able to run the ONNX exported model with no issues.

@RanACohen
Copy link
Contributor Author

this model have double (float 64)
just added Less with double support.

@neginraoof
Copy link
Contributor

neginraoof commented Aug 28, 2019

We found the missing input type at this line:
https://github.com/NVIDIA/DeepLearningExamples/blob/3d3ff3e168a391e55cb28c3d5a4df69c9d4f4100/PyTorch/Translation/GNMT/seq2seq/models/attention.py#L81
This refers to int64 type. Can you confirm why we need float64?

@snnn
Copy link
Member

snnn commented Aug 28, 2019

@RanACohen
Copy link
Contributor Author

@neginraoof In the GNMT I posted in the drive link above (made by NVidia I think) there is a casting to float64 (double) and then Less.

@snnn
Copy link
Member

snnn commented Aug 29, 2019

Where did you get the model? How did you convert it? Is it the same one in mlperf https://github.com/mlperf/inference ?

Welcome to submit your change. I can help review.

@RanACohen
Copy link
Contributor Author

@snnn same source as in #1583

@RanACohen
Copy link
Contributor Author

@snnn Thank you Sun, please review #1722

@neginraoof
Copy link
Contributor

neginraoof commented Aug 29, 2019

@RanACohen The onnx model you are trying to run in the Google Drive is created by me. This is the onnx model exported from a fork of GNMT model as a workaround. Now that 'int64' is supported for 'Less' op, you should be able to run the model with no issues. I exported the onnx model from GNMT master branch and uploaded it again (replaces the files). You can try running the new one. There's no need to use double for 'Less' op in this model. Sorry if the uploaded onnx model confused you.

@RanACohen
Copy link
Contributor Author

@neginraoof Thanks a lot! Does not hurt to extend the runtime with Less(double) in any case 😃
I will use the updated one.

@faxu faxu closed this as completed Oct 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request request for unsupported feature or enhancement
Projects
None yet
Development

No branches or pull requests

4 participants