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

removes decord #33987

Merged
merged 3 commits into from
Oct 17, 2024
Merged

removes decord #33987

merged 3 commits into from
Oct 17, 2024

Conversation

vrnvu
Copy link
Contributor

@vrnvu vrnvu commented Oct 6, 2024

The only usage that was left of decord is removed in this PR

The helper functions read_video_pyav and sample_frame_indices are the used in all the documentation https://github.com/search?q=repo%3Ahuggingface%2Ftransformers%20read_video_pyav&type=code and video_classification https://github.com/huggingface/transformers/blob/main/src/transformers/pipelines/video_classification.py#L132

ad added 2 commits October 6, 2024 14:03
optimize

np

Revert "optimize"

This reverts commit faa136b.

helpers as documentation

pydoc

missing keys
Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

Very welcome for this! Thanks for the cleanup 🤗
Could you confirm that the test_pipeline_video_classification tests work well locally?

@ArthurZucker
Copy link
Collaborator

Regarding the failing test:

VideoClassificationPipeline requires the PyAv library but it was not found in your environment. You can install it with:

pip install av

Please note that you may need to restart your runtime after installation.
FAILED tests/models/timesformer/test_modeling_timesformer.py::TimesformerModelTest::test_pipeline_video_classification_fp16 - ImportError: 
VideoClassificationPipeline requires the PyAv library but it was not found in your environment. You can install it with:

pip install av

Please note that you may need to restart your runtime after installation.

the workflow for pipelines needs a small fix!

@vrnvu
Copy link
Contributor Author

vrnvu commented Oct 7, 2024

Fixed the requirement, I wrongly confused "vision" with "video" and thought I didn't need the av requirement. 🤕

How can I troubleshoot them? Are they flaky?

I dont see how they are related with this PR.

Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

Thanks, yep the other tests are unrelated!

@ArthurZucker
Copy link
Collaborator

But the tests you edited are skipped by the CIs because they need av (which was not installed)

Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

Small nit, LGTM otherwise!

if i > end_index:
break
if i >= start_index and i in indices:
frames.append(frame)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
frames.append(frame)
frames.append(frame.to_rgb())

any reason we don't use this?
or directly convert before stacking?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, we return the transformation at the end to directly transform the numpy data structure

return np.stack([x.to_ndarray(format="rgb24") for x in frames])

It's more efficient than converting the frames to rgb:
https://github.com/PyAV-Org/PyAV/blob/main/av/video/frame.pyx#L252
https://github.com/PyAV-Org/PyAV/blob/main/av/audio/frame.pyx#L168

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

@ArthurZucker
Copy link
Collaborator

Okay merging then! 🤗

@ArthurZucker ArthurZucker merged commit 7f50885 into huggingface:main Oct 17, 2024
22 of 25 checks passed
NielsRogge pushed a commit to NielsRogge/transformers that referenced this pull request Oct 21, 2024
* removes decord dependency

optimize

np

Revert "optimize"

This reverts commit faa136b.

helpers as documentation

pydoc

missing keys

* make fixup

* require_av

---------

Co-authored-by: ad <[email protected]>
BernardZach pushed a commit to BernardZach/transformers that referenced this pull request Dec 5, 2024
* removes decord dependency

optimize

np

Revert "optimize"

This reverts commit faa136b.

helpers as documentation

pydoc

missing keys

* make fixup

* require_av

---------

Co-authored-by: ad <[email protected]>
BernardZach pushed a commit to innovationcore/transformers that referenced this pull request Dec 6, 2024
* removes decord dependency

optimize

np

Revert "optimize"

This reverts commit faa136b.

helpers as documentation

pydoc

missing keys

* make fixup

* require_av

---------

Co-authored-by: ad <[email protected]>
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.

3 participants