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 rt info for OpenVINO ModelAPI #2891

Merged
merged 7 commits into from
Jun 5, 2023

Conversation

Wovchena
Copy link
Contributor

@Wovchena Wovchena commented May 29, 2023

Refer to openvinotoolkit/model_api#71

πŸ€– Generated by Copilot at 97a2323

Summary

πŸ“πŸš€πŸ§ 

Added runtime information to OpenVINO model exported from yolo/engine/exporter.py. This enables the OpenVINO Inference Engine to use the model and process the results.

OpenVINO model
gets more runtime info now
useful for winter

Walkthrough

  • Add runtime information to OpenVINO model (link)

πŸ› οΈ PR Summary

Made with ❀️ by Ultralytics Actions

🌟 Summary

Enhancements to OpenVINO model export with additional runtime information.

πŸ“Š Key Changes

  • Additional runtime information is now set for the OpenVINO model during export, which includes:
    • Model identifier and type.
    • Reverse input channel flag.
    • Pad value, scale values, and IOU (Intersection over Union) threshold.
    • Label names, formatted to replace spaces with underscores.
    • Resize type for models not classified as 'classify'.

🎯 Purpose & Impact

  • 🎨 Enhanced Model Introspection: This PR adds important metadata to the model, allowing better understanding and usage of the model once exported.
  • βš™οΈ Improved Compatibility: Setting relevant runtime information ensures better compatibility with the OpenVINO toolkit.
  • πŸ“ˆ Increased Flexibility: Users can now have direct access to model information like the processing steps and thresholding right within the model file, which could simplify deployment processes.
  • πŸ‘ Better Performance Tuning: Including IOU threshold and pad values enables users to fine-tune the performance of the model according to specific needs.

@glenn-jocher
Copy link
Member

glenn-jocher commented May 30, 2023

@AlexKoff88 @Wovchena I've updated this PR in f3b6624 to omit classification models from the letterbox input type, and cleaned up the class names. Please take a look at my changes. If everything looks good I'll go ahead and merge!

@glenn-jocher glenn-jocher marked this pull request as ready for review May 30, 2023 18:01
@glenn-jocher glenn-jocher added the TODO Items that needs completing label May 30, 2023
@Wovchena
Copy link
Contributor Author

@glenn-jocher, thank you.

openvinotoolkit/model_api#71 isn't ready yet. So there's no rush to merge ultralytics PR unless there is a specific date by which this PR needs to be merged.

Plus I didn't figure out how to extract confidence threshold from export_openvino() yet.

@Wovchena
Copy link
Contributor Author

@glenn-jocher,

        labels = []
        for i in range(len(self.model.names)):
            labels.append(self.model.names[i].replace(' ', '_'))
        ov_model.set_rt_info(labels, ['model_info', 'labels'])

Was written that way on purpose. That way ensures that self.model.names.keys() are all in range(len(self.model.names)). The reason is that while saving to .xml the keys are lost.
Moreover I suspect there is no grantee on order how self.model.names.values() is iterated in [x.replace(' ', '_') for x in self.model.names.values()], ['model_info', 'labels'].

@glenn-jocher
Copy link
Member

glenn-jocher commented May 31, 2023

@glenn-jocher,

        labels = []
        for i in range(len(self.model.names)):
            labels.append(self.model.names[i].replace(' ', '_'))
        ov_model.set_rt_info(labels, ['model_info', 'labels'])

Was written that way on purpose. That way ensures that self.model.names.keys() are all in range(len(self.model.names)). The reason is that while saving to .xml the keys are lost. Moreover I suspect there is no grantee on order how self.model.names.values() is iterated in [x.replace(' ', '_') for x in self.model.names.values()], ['model_info', 'labels'].

Got it! The class names dict is assumed sorted everywhere in Ultralytics, i.e. see
https://docs.ultralytics.com/datasets/detect/coco/#applications

Screenshot 2023-06-01 at 00 53 47

In any case I've updated this to force sorted order again in 9242c7f

@Wovchena
Copy link
Contributor Author

Wovchena commented Jun 1, 2023

There's still concern about

That way ensures that self.model.names.keys() are all in range(len(self.model.names)). The reason is that while saving to .xml the keys are lost.

But if you think it's also fine, I'm ok with your appriach.

@glenn-jocher
Copy link
Member

@Wovchena thank you for your feedback. Regarding the concern about self.model.names.keys() being lost while saving to .xml, the names dictionary in YOLOv8 is assumed to be sorted everywhere in the Ultralytics repo. Therefore, in the interest of consistency and reliable behavior, we opt for forcing a sorted order in export_openvino() by using range(len(self.model.names)) to ensure the keys are in order. However, we also acknowledge that there may be alternative approaches that can work, and would welcome any suggestions or feedback on this.

@glenn-jocher
Copy link
Member

@Wovchena ok I've tested the current implementation and confirm that it will always sort a dict to a list by the dict keys, so no need to worry there. I think this is all good then, I'll go ahead and merge!

@glenn-jocher glenn-jocher changed the title Add rt info for ModelAPI (https://github.com/openvinotoolkit/model_api/pull/71) Add rt info for OpenVINO ModelAPI Jun 5, 2023
@glenn-jocher
Copy link
Member

@Wovchena actually, are you waiting on any changes from your side before I merge? I remember you mentioned a confidence threshold parameter you'd like to introduce.

@Wovchena
Copy link
Contributor Author

Wovchena commented Jun 5, 2023

You can merge. This PR doesn't invalidate anything. I can bring missing changes later.

@glenn-jocher glenn-jocher merged commit 01273c5 into ultralytics:main Jun 5, 2023
10 checks passed
@glenn-jocher
Copy link
Member

@Wovchena PR merged!

@glenn-jocher glenn-jocher removed the TODO Items that needs completing label Jun 5, 2023
darouwan pushed a commit to darouwan/ultralytics that referenced this pull request Jun 14, 2023
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Glenn Jocher <glenn.jocher@ultralytics.com>
Co-authored-by: Ayush Chaurasia <ayush.chaurarsia@gmail.com>
0iui0 pushed a commit to 0iui0/ultralytics that referenced this pull request Jan 3, 2024
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Glenn Jocher <glenn.jocher@ultralytics.com>
Co-authored-by: Ayush Chaurasia <ayush.chaurarsia@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants