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

Distributed inference example for llava_next #3179

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

VladOS95-cyber
Copy link

@VladOS95-cyber VladOS95-cyber commented Oct 20, 2024

Add distributed inference example for LLaVA-NeXT-Video-7B-hf

Before submitting

Who can review?

@sayakpaul @a-r-r-o-w @muellerzr

@VladOS95-cyber
Copy link
Author

Hi @sayakpaul @a-r-r-o-w! This PR is ready for review, please, take a look.

Copy link
Member

@sayakpaul sayakpaul left a comment

Choose a reason for hiding this comment

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

Thanks for working on this so quickly. Left some comments.

examples/inference/distributed/llava_next_video.py Outdated Show resolved Hide resolved
examples/inference/distributed/llava_next_video.py Outdated Show resolved Hide resolved
examples/inference/distributed/llava_next_video.py Outdated Show resolved Hide resolved
examples/inference/distributed/llava_next_video.py Outdated Show resolved Hide resolved
@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@VladOS95-cyber VladOS95-cyber force-pushed the add-video-capture-example-on-distributed-inference branch from 91ee412 to 0a58faa Compare October 24, 2024 14:16
Copy link
Member

@sayakpaul sayakpaul left a comment

Choose a reason for hiding this comment

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

I think there's some gap between the objectives. I thought we agreed on doing distributed captioning?

examples/inference/distributed/llava_next_video.py Outdated Show resolved Hide resolved
examples/inference/distributed/llava_next_video.py Outdated Show resolved Hide resolved
@VladOS95-cyber VladOS95-cyber force-pushed the add-video-capture-example-on-distributed-inference branch 2 times, most recently from c9bb16e to b43581e Compare November 4, 2024 17:51
@VladOS95-cyber
Copy link
Author

Hey @sayakpaul! Please, take a look on recent changes. Does it look ok to you?

@VladOS95-cyber VladOS95-cyber force-pushed the add-video-capture-example-on-distributed-inference branch from 6e8fb0d to 336def5 Compare November 24, 2024 13:52
Copy link
Member

@sayakpaul sayakpaul left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, I left some minor comments. I think this is close to merge.

Could you additionally show some sample outputs (i.e., video - generated caption pairs)?

examples/inference/distributed/llava_next_video.py Outdated Show resolved Hide resolved
examples/inference/distributed/llava_next_video.py Outdated Show resolved Hide resolved
@sayakpaul
Copy link
Member

@muellerzr I think this is now ready for your review.

@sayakpaul sayakpaul requested a review from muellerzr November 24, 2024 14:43
@VladOS95-cyber
Copy link
Author

VladOS95-cyber commented Nov 24, 2024

Thanks for the updates, I left some minor comments. I think this is close to merge.

Could you additionally show some sample outputs (i.e., video - generated caption pairs)?

@sayakpaul Thank you for your review! Do you mean by providing some examples in description to the script? Just in case, as example:
{ "prompt": "USER: <video>\nGenerate caption ASSISTANT:", "video": "/Users/{user_name}/.cache/huggingface/hub/datasets--malterei--LLaVA-Video-small-swift/snapshots/7d712657d4907c4f29c2c6fa17afaa7289a362a7/videos/4255049031.mp4", "generated_text": [ "USER: \nGenerate caption ASSISTANT: \"Practicing his moves: A man in a white shirt and jeans demonstrates his agility and balance on a basketball court, with a water bottle nearby, ready for hydration.\"" ] }

Copy link
Collaborator

@muellerzr muellerzr left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! As long as @sayakpaul is happy I'm happy 🤗

I did leave one suggestion/feedback, we need to destroy process groups now at the end else torch will get angry at us

@VladOS95-cyber
Copy link
Author

Hey @sayakpaul! Do you have any additional comments or suggestions?

Copy link
Member

@sayakpaul sayakpaul left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

@VladOS95-cyber
Copy link
Author

Hi @muellerzr! I think this PR is ready to be merged or there is something else?

@VladOS95-cyber
Copy link
Author

Hi @muellerzr! Just a kind reminder in order to not lose this PR!

@VladOS95-cyber
Copy link
Author

Hi @sayakpaul @muellerzr! Just a kind reminder, I suppose this PR was missed to be merged. Please, take a look when you are available.

@sayakpaul
Copy link
Member

Could I ask you to be a bit more respectful about pinging maintainers? You have pinged @muellerzr already once. So, that should be more than okay.

@VladOS95-cyber
Copy link
Author

Could I ask you to be a bit more respectful about pinging maintainers? You have pinged @muellerzr already once. So, that should be more than okay.

Hi, sure, sry fo that, I just wanted to avoid moving this PR into stale or smth.

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.

4 participants