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

FastMtCnn Use of Gpu for Face Detection #1145

Merged
merged 5 commits into from Apr 5, 2024
Merged

Conversation

aku-ato
Copy link
Contributor

@aku-ato aku-ato commented Mar 26, 2024

This pull request introduces an enhancement in the face detection component of our project by improving device compatibility and efficiency. The proposed changes involve dynamically assigning the computational device for the face detection model based on the availability of GPU resources.

Key Changes:

  • The device selection process is now dynamic, allowing the model to utilize GPU resources when available, falling back to CPU otherwise. This is achieved by checking torch.cuda.is_available() and setting the device accordingly.
  • This change ensures better performance on systems equipped with GPUs while maintaining compatibility with systems that only have CPUs.

Modified Code:

import torch

# Dynamically set the device based on availability
device = torch.device('cuda:0' if torch.cuda.is_available() else 'cpu')
face_detector = fast_mtcnn(device=device)

Benefits:

  • Improved performance on GPU-enabled systems without compromising functionality on CPU-only systems.
  • Enhanced efficiency and resource utilization, potentially leading to faster face detection in applications.

I believe this modification will make our face detection more robust and adaptable to various hardware configurations. Looking forward to your feedback and suggestions.

Please review and let me know if any further modifications are needed.

@@ -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

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?

@@ -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

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

@serengil
Copy link
Owner

serengil commented Apr 5, 2024

LGTM

@serengil serengil merged commit 1b60cf2 into serengil:master Apr 5, 2024
2 checks passed
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

2 participants