-
Notifications
You must be signed in to change notification settings - Fork 2
Detector #10
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
Conversation
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.
had clarifying comments but I'm assuming answers will make me comfortable approving
have to be honest that I don't feel like I can effectively review the vast majority of the object_detector.py
implementation so I just skimmed it.
import torchvision # type: ignore | ||
import numpy as np | ||
from transformers import Owlv2Processor, Owlv2ForObjectDetection, Owlv2VisionModel # type: ignore | ||
from transformers.models.owlv2.modeling_owlv2 import Owlv2Attention # type: ignore | ||
import time | ||
from typing import Union | ||
from torch_scatter import scatter_max # type: ignore | ||
from flash_attn import flash_attn_func # type: ignore |
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.
why are we ignoring typing on these?
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.
none of them come with typing annotations, so mypy is complaining and that can't be resolved .. is there something better I can do?
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.
don't think it demands a change. in the spirit of a well-typed repo we should probably provide justification for ignoring typing in any given change (e.g. linking something like this)
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.
LGTM
Adding the object detector, complete with batched runs. Took out flash attention from the text encoder because it is not obvious that the behavior is the same