-
Notifications
You must be signed in to change notification settings - Fork 3
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
146 bug voice activity detection import fails #208
base: main
Are you sure you want to change the base?
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.
I have left some minor comments. Feel free to merge and release the patch once those are addressed and all required checks pass
|
||
|
||
def diarize_audios( | ||
audios: List[Audio], | ||
model: SenselabModel = PyannoteAudioModel(path_or_uri="pyannote/speaker-diarization-3.1", revision="main"), | ||
model: Optional[PyannoteAudioModel] = None, |
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.
I would use SenselabModel as an input type here, since in the general api we don't want only PyannoteAudioModel objects. I know it's consistent with the current status of implementation, but doesn't communicate properly the intention of the method. And it's same for all other api methods.
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.
In my opinion, for the static type hints we should use what the function actually supports, not what it would in the future. Because these are explicitly checked by mypy, it seems less confusing to align the type hinting to the actual type check that occurs within the function. And then when we implement other models we can change the type hint. I see your point though and I can make it clearer in the docstring and errors that we would like to support more models in the future.
if model is None: | ||
model = PyannoteAudioModel(path_or_uri="pyannote/speaker-diarization-3.1", revision="main") |
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.
Can you please specify more clearly in the doc string what's the behavior with None? This comment applies for all other cases
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.
Good point. I'll address that
…fault model usage
@fabiocat93 I've made some changes to address your comments. Let me know if it's good to merge now. |
Description
Fixes import errors by not initializing models until a function is called. Also, makes the errors more informative when they do happen.
Related Issue(s)
#146
How Has This Been Tested?
Passes all unit tests, and also some spot checks for the imports described in #146
Types of changes
Checklist: