adding the YOLACT instance segmentation model

[Abdullah-Elkasaby]

No description provided.

[adrianboguszewski]

Thank you for your contribution. Before we do the review please do the self-check first (copy the list and tick the item when it's done):

• each function is described by docstrings and type hints
• notebook contains explicit descriptions and explanatory diagrams
• the notebook doesn't use any data (image, video, etc.) that is not CC4.0 licensed
• there is a README.md file in consistent style (look at other notebooks)
• the notebook is added to the main README
• there are no grammar, punctuation or typo issues (use any free tool for that e.g. Grammarly)
• there are no committed files besides notebook and readme (please use images or videos from data dir)
• your PR doesn't change any other notebooks
• all CI checks passed

[adrianboguszewski]

And it shouldn't be as submodule but committed directly :)

[Abdullah-Elkasaby]

@adrianboguszewski Thanks for the feedback, I will be working on the changes.

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[Abdullah-Elkasaby]

@adrianboguszewski
I have made some changes and ran the treon CI checks and passed, could you have a look?

• Each function is described by docstrings and type hints.
• The notebook doesn't use any data (image, video, etc.) that is not CC4.0 licensed.
• There is a README.md file in consistent style (look at other notebooks).
• The notebook contains explicit descriptions and explanatory diagrams.
• The notebook is added to the main README.
• There are no grammar, punctuation, or typo issues (use any free tool for that, e.g., Grammarly).
• There are no committed files besides the notebook and README (please use images or videos from the data directory).
• Your PR doesn't change any other notebooks.
• Treon CI checks passed.

here's a screenshot of running it locally and the fork repo has it run too.

[Abdullah-Elkasaby]

@adrianboguszewski the docker treon check faild, however the notebook that did was not mine.
could you also provide hints to help with the remaining checks?

[adrianboguszewski]

Docker treon failing is not related to your work. However, the code check CI is. Please fix all flake8 bugs. You can run it locally to see where to improve the code style. See these instructions

[adrianboguszewski]

Two more comments:

• What is the utils directory? Do you need all of these utils?
• Descriptions are missing. The notebook is educational content, so it should contain an explanation of why it was done like that. See other notebooks for the reference

[Abdullah-Elkasaby]

Two more comments:

• What is the utils directory? Do you need all of these utils?
• Descriptions are missing. The notebook is educational content, so it should contain an explanation of why it was done like that. See other notebooks for the reference

The model does need all of those, the original model was even more complex and had multiple dependencies to multiple directories.
Are a description in the readme file and comments in the notebook of the utils directory enough?

[Abdullah-Elkasaby]

@adrianboguszewski I have some changes and the code check passed, however there's some error regarding Binder that I don't get.

[adrianboguszewski]

No worries about Binder. However, we must solve the following issue somehow:

Is there any other place where you can download the weights? Is this pointing to your Google Drive?

[adrianboguszewski]

This output also doesn't look good (like wrong output - mixed areas)

[Abdullah-Elkasaby]

@adrianboguszewski the drive is not mine, it's available on the the main repo of the model, I can set it up on my drive for now, however I don't know if that will be suitable. as for the mixed areas problem, I will try to to work on but I might need some help.

[adrianboguszewski]

If it's points to the main repo, please find a way to skip that access denied error

[Abdullah-Elkasaby]

@aleksandr-mokrov @eaidova any updates?

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

aleksandr-mokrov commented on 2024-01-23T00:47:08Z
----------------------------------------------------------------

Line #6.    from data.config import cfg, set_cfg

No need to copy whole project. Just clone it and set path, like this:

yolact_repo = 'yolact'
if not Path(yolact_repo).exists():

    !git clone https://github.com/dbolya/yolact.git
sys.path.append(yolact_repo)

There are no something that you need to change.

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

aleksandr-mokrov commented on 2024-01-23T00:47:09Z
----------------------------------------------------------------

Line #16.    sys.path.append('..')

Replace by

import urllib.request
urllib.request.urlretrieve(

    url='https://raw.githubusercontent.com/openvinotoolkit/openvino_notebooks/main/notebooks/utils/notebook_utils.py',

  filename='notebook_utils.py'

)
from notebook_utils import download_file

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

aleksandr-mokrov commented on 2024-01-23T00:47:10Z
----------------------------------------------------------------

Line #8.    output_path = f"{WEIGHTS_PATH}/{MODEL_NAME}.pth"

Check if the path exists.

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

aleksandr-mokrov commented on 2024-01-23T00:47:11Z
----------------------------------------------------------------

Line #7.    model_path = SavePath.from_str(args.trained_model)

Why do you use class SavePath?

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

aleksandr-mokrov commented on 2024-01-23T00:47:12Z
----------------------------------------------------------------

Line #10.    set_cfg(args.config)

Make sure the resulting cfg has the desired attributes

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

aleksandr-mokrov commented on 2024-01-23T00:47:13Z
----------------------------------------------------------------

Line #32.            preds = net.detect({'loc': dets_out[0], 'conf': dets_out[1], 'mask':dets_out[2], 'priors': dets_out[3], 'proto': dets_out[4]}, net)

Use original model output and remove this line. You can create wrapper class for convertation, for example:

class ModelWrapper(torch.nn.Module):
  def init(self, model):

    super().init()

    self.model = model
  def forward(self, batch):

    outputs = self.model(batch)[0]

    return outputs['detection']

Or replace the detection method by method that only returns required output.

aleksandr-mokrov commented on 2024-01-23T18:23:47Z
----------------------------------------------------------------

An example implementation of the last option to keep the same behavior but not change the source code:

def detect(pred_outs, net):
  return pred_outs['loc'], pred_outs['conf'], pred_outs['mask'], pred_outs['priors'], pred_outs['proto']
net.detect = detect

[aleksandr-mokrov]

@Abdullah-Elkasaby currents PR affects a lot of files that are not relevant to current example. Looks like you merged main into you branch in the wrong way.

[Abdullah-Elkasaby]

@Abdullah-Elkasaby currents PR affects a lot of files that are not relevant to current example. Looks like you merged main into you branch in the wrong way

I don't see the conficts on my side idk why

[aleksandr-mokrov]

I don't see the conficts on my side idk why

There are no conflicts. Problem is that there changes that affects a lot of files from other notebooks. This happens when you do not resolve the conflict manually correctly. I believe that the problem in this commit: ed491f3

[Abdullah-Elkasaby]

ed491f3

I have reverted back those changes, it should be up to date with the current repo now

[aleksandr-mokrov]

ed491f3

I have reverted back those changes, it should be up to date with the current repo now

Great! Check also files under .github/workflows diretory and .docker/Pipfile.lock. And resolve comments starting with https://app.reviewnb.com/openvinotoolkit/openvino_notebooks/pull/1067/discussion/#comment-1905093771. It will simplify this example.

[Abdullah-Elkasaby]

ed491f3

I have reverted back those changes, it should be up to date with the current repo now

Great! Check also files under .github/workflows diretory and .docker/Pipfile.lock. And resolve comments starting with https://app.reviewnb.com/openvinotoolkit/openvino_notebooks/pull/1067/discussion/#comment-1905093771. It will simplify this example.

I have made the changes regarding making the code related to initilazing the model to the same as the one used in the original model.

However, I don't understand your comment about github workflows and docker directory, could you please clarify?

[aleksandr-mokrov]

I have made the changes regarding making the code related to initilazing the model to the same as the one used in the original model.

There are a lot of copied files from yolact repository. Some of them has some changes. It is possible to avoid it. I left tips on how to do this. Such as cloning a repository and making changes to objects rather than files. I checked it and it works. There are few changes required to the notebook, but we eliminate the need to bring a bunch of third-party files into our repository.

However, I don't understand your comment about github workflows and docker directory, could you please clarify?

Look into "File changed" tab. There are changes in github workflows and docker directory. These changes should not happen.

[aleksandr-mokrov]

An example implementation of the last option to keep the same behavior but not change the source code:

def detect(pred_outs, net):
  return pred_outs['loc'], pred_outs['conf'], pred_outs['mask'], pred_outs['priors'], pred_outs['proto']
net.detect = detect

---

View entire conversation on ReviewNB

[andrei-kochin]

@Abdullah-Elkasaby any updates here on requested changes?

[github-actions]

This PR will be closed in a week because of 2 weeks of no activity.

[github-actions]

This PR was closed because it has been stalled for 2 week with no activity.