jina-ai / executors

internal-only
Apache License 2.0
31 stars 12 forks source link

Split Yolov5 in GPU/CPU versions #200

Open jacobowitz opened 2 years ago

jacobowitz commented 2 years ago

While working on GPU support for a couple of executors in this issue, I've stumbled over a problem for the Yolov5 segmenter, which can be seem in this draft pr. CI always fails to install pycocotools on our custom GPU github action runners. That is fairly annoying, because we dont even need this dependency, it is only introduced because the original upstream authors of yolov5 dont provide a Pypi package and we are using this fork instead. There also have been some related discussions already: here and here

So this issue contains two sub tasks:

hmen97 commented 2 years ago

Hi @jacobowitz it looks like this pull request has added all the changes required for a setup.py but the authors are holding off on integrating it as it would involve adding a yolov5 base directory, so they are looking for other options. Meanwhile the author has mentioned this 3rd party Pypi package as the best way to do inference right now which is the same as the fork you referenced. Maybe we can add the real implementation as a submodule?

jacobowitz commented 2 years ago

@hmen97 thank you for digging into this issue! Its a pity that yolov5 authors won't refactor their repo to enable a proper Pypi package :( I think adding their repo as a submodule is not an option as it would break a lot of automation and also does not fit in this general purpose repository here. I think another option might be to fork the fork we are using and change the requirements.txt to a more minimal one. Maybe a better option would be a PR against the existing fork to structure the requirements.txt a bit better? I think the unnecessary dependencies should be marked as such there.

hmen97 commented 2 years ago

Yeah I was thinking along the same lines, based on what you have said

CI always fails to install pycocotools on our custom GPU github action runners.

If I make a PR against the forked repo so that unecessary dependencies like pycocotools become optional/can be skipped when running setup.py, I think that should solve the issue, but in case they don't accept that change then I'll fork the fork. Is that alright?

jacobowitz commented 2 years ago

@hmen97 that sounds great!

CatStark commented 2 years ago

Hi @hmen97 did you have any chance to look into this? it this ticket still relevant to you? :)

cristianmtr commented 2 years ago

Hey @hmen97

We've unassigned you from the issue for now, and moved the issue back to our backlog. If you want to continue your work on it, let me know.