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 --include torchscript onnx coreml argument #3137

Merged
merged 5 commits into from
May 12, 2021
Merged

Add --include torchscript onnx coreml argument #3137

merged 5 commits into from
May 12, 2021

Conversation

CristiFati
Copy link
Contributor

@CristiFati CristiFati commented May 12, 2021

Some people might only want to export the models in certain formats (e.g. I only wanted ONNX). Although exporting in all the formats doesn't take much time (in my case, bulk exporting all 8 files took ~1min), this will become more handy when more formats will be available.

Initially I had a patch for line 12 as well (as yesterday I downloaded v5 and I tried running the script from a different folder, which obviously failed), then I cloned the repo and noticed it was already fixed in master, so the only thing left to do was to fix comments.

🛠️ PR Summary

Made with ❤️ by Ultralytics Actions

🌟 Summary

Enhanced model export capabilities for YOLOv5, now supporting CoreML format.

📊 Key Changes

  • Extended the export functionality to include CoreML format, along with TorchScript and ONNX.
  • Simplified the usage command by removing the need to set PYTHONPATH.
  • Added a new --include argument to specify which formats to export.
  • Conditional export statements now check the --include argument for format specification.

🎯 Purpose & Impact

  • 🚀 Purpose: To enable the deployment of YOLOv5 models in a wider range of environments by supporting additional export formats, especially targeting iOS devices with CoreML.
  • Impact: Developers can now more easily convert YOLOv5 models for use in iOS apps and have more control over the export process with the --include option. The potential for simpler deployment processes and broader usage of models in different platforms is increased.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

👋 Hello @CristiFati, thank you for submitting a 🚀 PR! To allow your work to be integrated as seamlessly as possible, we advise you to:

  • ✅ Verify your PR is up-to-date with origin/master. If your PR is behind origin/master an automatic GitHub actions rebase may be attempted by including the /rebase command in a comment body, or by running the following code, replacing 'feature' with the name of your local branch:
git remote add upstream https://github.com/ultralytics/yolov5.git
git fetch upstream
git checkout feature  # <----- replace 'feature' with local branch name
git rebase upstream/master
git push -u origin -f
  • ✅ Verify all Continuous Integration (CI) checks are passing.
  • ✅ Reduce changes to the absolute minimum required for your bug fix or feature addition. "It is not daily increase but daily decrease, hack away the unessential. The closer to the source, the less wastage there is." -Bruce Lee

@glenn-jocher
Copy link
Member

@CristiFati thanks for the PR! I think the assumption is that users would not install packages for export channels that were not of interest to them, like coremltools etc.

One thing also is that coremltools export first requires a TorchScript export. I see you've made some additional improvements also, I'll take a look.

renamed --skip-format to --exclude
@glenn-jocher
Copy link
Member

@CristiFati I updated this a bit to bring the terminology in line with other 'include'/'exclude' functions like check_requirements(), but something doesn't seem right about going with the exclude pathway vs offering an include pathway instead.

i.e. if I want to export onnx, I would rather opt-in to onnx export than opt-out of other exports (which I'm potentially unaware of).

@CristiFati
Copy link
Contributor Author

Thanks for the quick replies. Going one at a time:

  • If CoreML requires TorchScript, then I should modify the condition for TorchScript (widen it)
  • --exclude was my 1st choice then I changed it to --skip-format
  • Chose the exclude approach to be as much backward compatible as possible. Currently everything is exported, and only omit stuff on demand. One potential issue that I can see, if switching to include approach is that after upgrading, some (automation) scripts that rely on everything being exported by default, will no longer work OOTB

Let me know what else I could do.

@glenn-jocher
Copy link
Member

glenn-jocher commented May 12, 2021

@CristiFati yeah that all makes sense. I think what we want is to remove the exclude list and replace it with an include list, which would default to all formats but also allow for a one or two formats if an argument is passed.

parser.add_argument('--include', nargs='+', default=['torchscript', 'onnx', 'coreml'], help='included formats')

@CristiFati
Copy link
Contributor Author

Done, now with that CoreML -> TorchScript dependency: although I don't see one's input depending on the other's output, would it make sense that line 79 to be if 'torchscript' in opt.include or 'coreml' in opt.include: (instead of if 'torchscript' in opt.include:)? Or it's just nonsense?

@glenn-jocher glenn-jocher changed the title Minor changes to export script Add --include torchscript onnx coreml argument May 12, 2021
@glenn-jocher glenn-jocher added the enhancement New feature or request label May 12, 2021
@glenn-jocher glenn-jocher changed the title Add --include torchscript onnx coreml argument Add --include torchscript onnx coreml argument May 12, 2021
@glenn-jocher
Copy link
Member

@CristiFati changes look good, I added a CoreML check for TorchScript.

Merging PR now. Thank you for your contributions!

@glenn-jocher
Copy link
Member

glenn-jocher commented May 12, 2021

Verified working:

Screenshot 2021-05-12 at 19 45 47

@glenn-jocher glenn-jocher merged commit d9b4e6b into ultralytics:master May 12, 2021
@CristiFati
Copy link
Contributor Author

Glad to be helpful. Looking forward to more!

Lechtr pushed a commit to Lechtr/yolov5 that referenced this pull request Jul 20, 2021
* Allow users to skip exporting in formats that they don't care about

* Correct comments

* Update export.py

renamed --skip-format to --exclude

* Switched format from exclude to include (as instructed by @glenn-jocher)

* cleanup

Co-authored-by: Glenn Jocher <glenn.jocher@ultralytics.com>
(cherry picked from commit d9b4e6b)
BjarneKuehl pushed a commit to fhkiel-mlaip/yolov5 that referenced this pull request Aug 26, 2022
* Allow users to skip exporting in formats that they don't care about

* Correct comments

* Update export.py

renamed --skip-format to --exclude

* Switched format from exclude to include (as instructed by @glenn-jocher)

* cleanup

Co-authored-by: Glenn Jocher <glenn.jocher@ultralytics.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants