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.

Sign up for GitHub

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

FastMtCnn Use of Gpu for Face Detection #1145

Merged
merged 5 commits into from
Apr 5, 2024
Merged

FastMtCnn Use of Gpu for Face Detection #1145

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3d87338
use GPU in fastMtCnn
aku-ato Mar 26, 2024
2b7c0a7
Conditional use of GPU on FastMtCnn
aku-ato Mar 26, 2024
345b671
Conditional use of GPU on FastMtCnn
aku-ato Mar 26, 2024
b4c16f6
Merge branch 'serengil:master' into master
aku-ato Apr 5, 2024
131a7fb
move torch import from global in order to match optional dependency p…
aku-ato Apr 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion deepface/detectors/FastMtCnn.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import cv2
import numpy as np
from deepface.models.Detector import Detector, FacialAreaRegion
import torch
Copy link
Owner

@serengil serengil Mar 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

this is not a must dependency in deepface and I do not want to add new dependencies to make the package smaller. users will have import error if this import is put in global. right?


# Link -> https://github.com/timesler/facenet-pytorch
# Examples https://www.kaggle.com/timesler/guide-to-mtcnn-in-facenet-pytorch
Expand Down Expand Up @@ -68,7 +69,9 @@ def build_model(self) -> Any:
"Please install using 'pip install facenet-pytorch' "
) from e

face_detector = fast_mtcnn(device="cpu")
device = torch.device('cuda:0' if torch.cuda.is_available() else 'cpu')
face_detector = fast_mtcnn(device=device)
Copy link
Owner

Choose a reason for hiding this comment

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

to import fast_mtcnn, we enforce users to install facenet_pytorch instead of torch. not sure installing facenet_pytorch will install torch, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your review and feedback on the pull request. I appreciate your concern regarding the addition of new dependencies and the potential impact on the package size and user experience.

Regarding the dependency on facenet_pytorch, it's important to note that while facenet_pytorch indeed does not directly require torch, it does require torchvision. The torchvision package, in turn, lists torch as one of its dependencies. Consequently, installing torchvision (as a requirement for facenet_pytorch) will ensure that torch is also installed if it is not already present in the environment.

This chain of dependencies means that users will end up with torch installed as part of the process, indirectly satisfying the requirement for torch without explicitly listing it as a direct dependency for our project.

I hope this clarifies the dependency chain and addresses your concern. I am open to further discussion and suggestions on how we can improve this implementation.

Best regards,

Copy link
Owner

@serengil serengil Mar 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

okay I can confirm that installing facenet-pytorch will install torchvision, and installing torchvision will install torch

(base) sefik@L-GCCDXDDT:~$ pip show facenet-pytorch
Name: facenet-pytorch
Version: 2.5.3
Summary: Pretrained Pytorch face detection and recognition models
Home-page: https://github.com/timesler/facenet-pytorch
Author: Tim Esler
Author-email: tim.esler@gmail.com
License:
Location: /home/sefik/miniconda3/lib/python3.9/site-packages
Requires: numpy, pillow, requests, torchvision
Required-by:


(base) sefik@L-GCCDXDDT:~$ pip show torchvision
Name: torchvision
Version: 0.15.2
Summary: image and video datasets and models for torch deep learning
Home-page: https://github.com/pytorch/vision
Author: PyTorch Core Team
Author-email: soumith@pytorch.org
License: BSD
Location: /home/sefik/miniconda3/lib/python3.9/site-packages
Requires: numpy, pillow, requests, torch
Required-by: facenet-pytorch, timm, ultralytics

But still putting import torch to the line 5 will throw exception for users who do not have facenet-pytorch. Because when i run pip install deepface, it will not install torch (it is not a must dependency) and when i import deepface as from deepface import DeepFace, then torch will be attempted to be imported but it will throw exception because I do not have that dependency. So, this change breaks deepface code as is.

As you can see facenet_pytorch is imported in line 64 instead of global level: https://github.com/serengil/deepface/blob/master/deepface/detectors/FastMtCnn.py#L64

You may consider to import torch in that try except block. Otherwise, I cannot merge this PR because it will break the deepface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You may consider to import torch in that try except block. Otherwise, I cannot merge this PR because it will break the deepface.

Sure, I'll do


return face_detector


Expand Down