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

improve: improve support for pyannote.audio #688

Merged
merged 8 commits into from
Mar 23, 2022

Conversation

hbredin
Copy link
Contributor

@hbredin hbredin commented Feb 16, 2022

pyannote.audio has two main types of "inference":

  • models are regular PyTorch models that output tensors
  • pipelines encapsulate one or more models, postprocess their outputs and return actual audio segmentation results (start_time, end_time, label) as pyannote.core.Annotation instances

I think both have their own use and target different users (e.g. models for researchers, pipelines for users who just want to get speaker diarization results).

@osanseviero
Copy link
Contributor

Hey there! Sorry for the slow response. As of now, the code snippet is based in the tag of a model. So if a model repo has pyannote tag, it will then show the code snippet. With this change, you're changing the expected tag.

Instead, I would suggest the two code types directly in the same snippet, although my concern would be of the sippet being too long. Maybe we can simplify and just do inference on the whole file. It's not like the transformers code snippets have a full example. WDYT?

@hbredin
Copy link
Contributor Author

hbredin commented Mar 22, 2022

Are you suggesting to only stick with the pyannote-audio-pipeline tag?

@osanseviero
Copy link
Contributor

Maybe just to take a step back and understand the usage. Can you use both Pipelineand Model with the same model repository?

Once you have a repo with the tag pyanote, you will show corresponding code snippet. If you do the change you're suggesting, you will need to modify the repos to have one of the two snippets you suggest (but only one snippet will show)

@hbredin
Copy link
Contributor Author

hbredin commented Mar 23, 2022

Maybe just to take a step back and understand the usage. Can you use both Pipelineand Model with the same model repository?

No, because a Pipeline may internally rely on multiple Models.
There is no bijection between a pipeline and a model.

For instance, pyannote/speaker-diarization pipeline relies on pyannote/segmentation model (for things like voice activity detection) and pyannote/embedding model (for the speaker clustering step).

Once you have a repo with the tag pyanote, you will show corresponding code snippet. If you do the change you're suggesting, you will need to modify the repos to have one of the two snippets you suggest (but only one snippet will show)

Yes, that was the point of this PR :)

If that is too much trouble, we could focus on pipelines (pyannote-audio-pipeline tag) which is probably what beginners would use first. That being said, I will keep tagging pipelines and models with two different tags as I need an easy way to search them using the HFApi.

@osanseviero
Copy link
Contributor

Alright, much clearer now.

My suggestion is then to do the following

  1. Keep pyannote as the tag (key) used here. This will work as the primary tag
  2. You can then have secondary tags, which can be the ones you suggest.
  3. Then, based on the secondary tag, you can show different snippets. See how it's done here: https://github.com/huggingface/huggingface_hub/blob/main/js/src/lib/interfaces/Libraries.ts#L174-L181

This will allow to have a single filter for all pyannote repos in the main libraries filter, but with the benefits that:

  • The code snippets change based on the secondary tag
  • Users can still filter on with the secondary tag.

Do you think this would solve the need?

@hbredin
Copy link
Contributor Author

hbredin commented Mar 23, 2022

Perfect. Will make the changes.

@hbredin
Copy link
Contributor Author

hbredin commented Mar 23, 2022

Done.

@osanseviero osanseviero self-requested a review March 23, 2022 08:56
Copy link
Contributor

@osanseviero osanseviero left a comment

Choose a reason for hiding this comment

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

Thank you very much for the PR! There are some unrelated changes, would you be able to remove them?

Afterwards we can go ahead and merge

js/src/lib/interfaces/Libraries.ts Outdated Show resolved Hide resolved
js/src/lib/interfaces/Libraries.ts Show resolved Hide resolved
Copy link
Contributor

@osanseviero osanseviero left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks 🚀

@osanseviero osanseviero merged commit c8937a9 into huggingface:main Mar 23, 2022
@hbredin
Copy link
Contributor Author

hbredin commented Mar 23, 2022

Thanks! Any update on the "audio-segmentation" widget? ;)

@hbredin hbredin deleted the patch-1 branch March 23, 2022 09:31
@hbredin
Copy link
Contributor Author

hbredin commented Mar 25, 2022

Also, out of curiosity, when will this be deployed?

@osanseviero
Copy link
Contributor

Hey there! Sorry, we're doing a repo cleanup/split to make things much much much better organized, so this is taking a bit longer to get deployed. I'll update you as soon as we do this.

@osanseviero
Copy link
Contributor

The change was also applied in huggingface/hub-docs@e00b7bd, this should be deployed soon.

@osanseviero
Copy link
Contributor

@hbredin the change is now deployed 🚀 thanks @Pierrci!

@osanseviero
Copy link
Contributor

@Narsil we'll need a deploy of this change in api-inference updating the framework name, which requires a small internal PR iirc. Could you help us with this?

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.

2 participants