-
Notifications
You must be signed in to change notification settings - Fork 74
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
[DDNM] Support additional parameters to onnxsim #175
Comments
I am well aware of the advantages and disadvantages of that option, as I was the one who inspired the addition of that feature to onnxsim. If possible, would you be willing to share a sample ONNX file for testing? |
Released with additional functionality. I am adding regression tests to my GitHub Actions for each feature revision, so if you can share the onnx before closing the issue, I would be very happy to share it. https://github.com/PINTO0309/onnx2tf/actions/runs/4109150793/jobs/7090651297 https://github.com/PINTO0309/onnx2tf/wiki/model_status https://github.com/PINTO0309/onnx2tf/releases/tag/1.6.0 |
Thanks @PINTO0309 ! I'll share the ONNX model later today, as I'm using mobile data at the moment and the model is fairly big. I returned to troubleshooting conversion today, and I'm unfortunately unable to reproduce the issue I reported originally. However, I'm now running into a new issue, and I have no idea what I changed! (If you'd prefer I open a separate issue I'm happy to do that). The converter is reporting that the output shape is
|
If the model is very large, it is better to run onnxsim at least 5 times in a row before using onnx2tf. The number of OPs is only 1092, so the model does not appear to be very complex. As a rule of thumb, a model with 25,000 OP should run onnxsim approximately 10 times.
Incidentally, the model you are trying to convert probably has none of the effects of |
Fantastic, really appreciate all the help @PINTO0309 ! The model converted successfully with your advice. I've now got yet another question (again, please let me know if you want to make a new issue, as I think you've successfully addressed the original issue I opened here). I've got the following files:
(Here's the folder with the above three files). Here are the image outputs (I'm running it against an image of Marie Curie using the "inpainting" task (a mask is drawn over the mouth and the image is reconstructed):
|
First, you need to try the command I posted in the TensorFlow Forum.
The tool misjudged the correct transposed dimension just before the OP, which is marked Be sure to read this issue before rushing to get answers. The location of the transposition error can be identified from the log, so the JSON file can be used to compensate for the transposition error. Converting your model correctly is easy, but I don't have time right now. It is now 1:00 a.m. in Japan and I need to get some sleep to prepare for work tomorrow. It may take some time to understand how to correct the tool's behavior, but if you want to convert a DDNM model in a hurry, read this tutorial and try to transpose it so that the first
The probability of transposition errors in the Transformer model is quite high in the current version. Although it is expected to take considerable time to modify the system, the following issue is expected to significantly reduce the error rate. |
I am having trouble resolving the arithmetic errors in https://github.com/PINTO0309/onnx2tf/blob/main/json_samples/replace_ddnm.json
This is because the optimization by onnxsim was not properly performed by you. The onnx-tensorflow also fails in many cases to transform models with undefined dimensions.
|
Thanks for all the help @PINTO0309 . For the command you shared, I'm getting:
I also tried with I'll keep working on it, I'm still getting up to speed on your tool - there's quite a lot to read up on. |
How did you generate the |
I have converted your model several times since last night. The figure below is the result of a fairly rigorous accuracy check, so the final output is shown as a lot of This log shows a successful conversion one minute ago.
The JSON is written by hand, trying many conversions and identifying the points where accuracy errors occur one by one. See the issue below for how I identified the problem areas and how I wrote the JSON. I have created a number of repositories in the past and written many READMEs, but many users did not read them no matter how much information I included. Therefore, in this repository, I decided not to include a lot of information in the README. |
I have found a bug in some parameter replacement logic that I will now fix. I noticed that the |
Thanks to you I was able to notice the existence of a fatal bug. I immediately fixed the bug and released it with the parameter replacement function working correctly. https://github.com/PINTO0309/onnx2tf/releases/tag/1.6.1 I will share the URL for downloading the
|
Thanks for the quick responses and bug fixes. I downloaded your However, I'm still running into the same issue when running the tool locally, and I don't know why:
I'm not sure why your command works but mine fails. Maybe it's a version mismatch; I pip installed via pypi, maybe I should try pip installing directly from this repo? |
At the moment, I have no idea why the error is occurring, but from the logs, it looks like you are using Anaconda, so why not try using Docker? |
Sounds like you're onto something. I tried within a docker container and it appears to be running (at least, it's gotten past that error). So it must be a system thing. I'll keep troubleshooting and report what I find. |
Some updates from me: I'm now on
That's a huge speed up! Kudos. I do see two new issues that aren't blocking me, but might be worth reporting:
Might be nice to only emit these files if specified by the user (I'm not seeing anything in the documentation about turning them off)
It's not clear to me how to debug this output error. Neither of these issues is a show stopper for me, and the saved model export is working beautifully 👌 |
Very happy to hear the good news. :) I am currently working on a pull request that will significantly change the behavior of the tool to address the issues you have presented. It will be difficult and quite time consuming. I would appreciate it if you could close this issue once the modification to make the parameter file unnecessary would take a lot of time and is a topic of functional improvement that is very different from the initial issue of this issue. It was a very meaningful issue. Thank you. |
Fantastic, I am looking forward to it! 😄 |
Issue Type
Feature Request
onnx2tf version number
1.5.44
onnx version number
1.13.0
tensorflow version number
Version: 2.11.0
Download URL for ONNX
Not relevant
Parameter Replacement JSON
Not relevant
Description
Right now, a single parameter -
--overwrite-input-shape
- is forwarded toonnxsim
.It would be helpful to provide a mechanism to pass arbitrary parameters to
onnxsim
.For a particular conversion I'm attempting, I see the following warning:
However, I have no way to forward
--no-large-tensor
toonnxsim
.The text was updated successfully, but these errors were encountered: