-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Allow custom model inference and upload #1696
Conversation
Pull Request Test Coverage Report for Build 5827
💛 - Coveralls |
@azhavoro , could you please review, test, and comment? |
for key, labels in db_labels.items(): | ||
if labels in auto_segmentation_labels.keys(): | ||
labels_mapping[auto_segmentation_labels[labels]] = key | ||
if int(mid) == 989898: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use django fixtures or post migrate signals to add proper records to the database. It will allow to not use a magic numbers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I need to check that. I will get back to you. Just to summarize what we do here, we assigned a specific unique number to the default models because we had int
in the request.
frames = frame_provider.get_frames(frame_provider.Quality.ORIGINAL) | ||
for image_num, (image_bytes, _) in enumerate(frames): | ||
image_num = 0 | ||
image_list = list(frame_provider.get_frames(frame_provider.Quality.ORIGINAL)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could there be memory issue here? Why do you need cast to list
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We didn't face any issues while testing, but yes I agree with you. We need a length of the frame_provider to figure out distribution among available GPUs. The Generator class does not have length function. Let me see if we can have an alternative way. Do you have any suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
frame_provider
has __len__
definition https://github.com/opencv/cvat/blob/develop/cvat/apps/engine/frame_provider.py#L83
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, I will update this.
@@ -144,16 +148,26 @@ def save_file_as_tmp(data): | |||
dl_model_id = create_empty(owner=owner) | |||
|
|||
run_tests = bool(model_file or weights_file or labelmap_file or interpretation_file) | |||
if is_custom in ["tensorflow","maskrcnn"]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that these string constants are used many times, could you add enum for them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, do we have any specific file where we should create enum class? or do you have any suggestion where this should be created?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this file will be ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok thanks
@@ -3,11 +3,11 @@ | |||
|
|||
set -e | |||
|
|||
MASK_RCNN_URL=https://github.com/matterport/Mask_RCNN | |||
MASK_RCNN_URL=https://github.com/onepanelio/Mask_RCNN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We made several changes in Mask_RCNN repo to make it work with multiple gpus. So, if we change this then it won't work with multiple GPUs.
Here is an overview of what got changed by this pull request: Complexity increasing per file
==============================
- cvat/apps/auto_segmentation/views.py 8
- cvat/apps/tf_annotation/views.py 6
- cvat/apps/auto_annotation/model_manager.py 1
See the complete overview on Codacy |
@san999p , let's meet next week and discuss the PR. I want to present to you an alternative solution using nuclio serverless platform (https://github.com/nuclio/nuclio): https://github.com/opencv/cvat/tree/nm/serverless. |
sure @nmanovic let me know what time works for you. Maybe send a calendly link? |
@san999p , I will schedule a meeting this week. I will contact Rush and Donald. You can look at the PR to understand future changes: #1767 |
@san999p @nmanovic We have simple implementation where user can:
This gives user freedom to use their own models for automation and also they can use any models from TF model zoo. Let me know your views on this. Will submit PR soon, may be it will give more clear idea. |
@kishan-sib , Let's look at your implementation. The main problem that every DL model needs an It is the primary reason why I'm looking at "serverless approach" to resolve all issues because each DL model will be a separate container. You can look at Mask RCNN implementation as serverless function for nuclio framework here: https://github.com/opencv/cvat/tree/nm/serverless/serverless/tensorflow/matterport/mask_rcnn/nuclio |
I think #1767 closes this PR. |
@savan77 , thanks for the input. I will close the PR. |
This patch allows users to upload any compatible object detection or segmentation model and run automatic annotation. This will close #1028
Motivation and context
Right now, you can only upload custom models that are supported by OpenVINO. Tensorflow Object Detection API is widely used and it would be helpful if we allow support for custom model inference. The same goes for MaskRCNN.
For object detection, you can upload .pb file for model and .csv file for classes. For auto segmentation, you can upload .h5 for model and .csv for classes. The format of csv is slighly different for both of them which we can put it in the docs.
How has this been tested?
This has been tested on local machine.
Checklist
x
] I submit my changes into thedevelop
branchcvat-core, cvat-data and cvat-ui)
I will update CHANGELOG and add documentation once this has been approved. I expect some changes based on your suggestion. Please feel free to suggest any changes that can make this better.
License
Feel free to contact the maintainers if that's a concern.
requesting for a review @nmanovic @azhavoro