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

Fix test_pipelines_video_classification that was always failing #35842

Merged

Conversation

CalOmnie
Copy link
Contributor

What does this PR do?

tests/test_pipelines_video_classification.py was always failing because the output of the video classifier, when run with one file path (as opposed to a list), is returning a list[dict] whereas the testing code was expecting a list[list[dict]], this PR corrects that assumption.

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.
It would be nice if someone could check that pytest tests/test_pipelines_video_classification.py actually fails for them too, it is possible that the issue comes from my environment

@Rocketknight1
Copy link
Member

cc @ydshieh, can you confirm this test was failing?

@ydshieh
Copy link
Collaborator

ydshieh commented Jan 23, 2025

@CalOmnie I am not able to see test_pipelines_video_classification.py failing on our CI report.

Could you provide a link to a job run page where this test is failing or a full command that run a specific test that shows this failure?

@CalOmnie
Copy link
Contributor Author

Hey @ydshieh thanks for taking the time to go over this PR. I shared this command in the PR description but I probably should have made a dedicated section for it, I ran the tests using:

pytest tests/test_pipelines_video_classification.py

I assume it is not part of your automated testing, or it would have been found earlier. That or an issue with my own environment.

@ydshieh
Copy link
Collaborator

ydshieh commented Jan 23, 2025

OK, it should be tests/pipelines/test_pipelines_video_classification.py. But with that, I can't see it from the Slack report as the message is truncated.

This test is indeed failing on main and fixed in this PR.

@ydshieh
Copy link
Collaborator

ydshieh commented Jan 23, 2025

I find the docstring for VideoClassificationPipeline.__call__ in src/transformers/pipelines/video_classification.py is misleading:

        Return:
            A dictionary or a list of dictionaries containing result. If the input is a single video, will return a
            dictionary, if the input is a list of several videos, will return a list of dictionaries corresponding to
            the videos.

Even with a single video (path string), the returned value is still a list of dictionary instead of the mentioned dictionary in the above docstring. (It's a list of length top_k).

@CalOmnie Would you like to double check this part and update the docstring if necessary?

@Rocketknight1 The changes in this PR LGTM.

@CalOmnie
Copy link
Contributor Author

@ydshieh Apologies for the wrong path. I updated the docstring to reflect that the pipeline now returns top_k results, even with a single video.

@ydshieh
Copy link
Collaborator

ydshieh commented Jan 23, 2025

@Rocketknight1 Maybe you can double check the change in src/transformers/pipelines/video_classification.py and then we are ready to go?

Copy link
Member

@Rocketknight1 Rocketknight1 left a comment

Choose a reason for hiding this comment

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

@ydshieh the changes in the actual pipeline are just docstring edits, and I think the new docstring is more correct, so I'm happy to approve this!

@ydshieh ydshieh merged commit b5aaf87 into huggingface:main Jan 23, 2025
23 checks passed
bursteratom pushed a commit to bursteratom/transformers that referenced this pull request Jan 31, 2025
…ggingface#35842)

* Fix test_pipelines_video_classification that was always failing

* Update video pipeline docstring to reflect actual return type

---------

Co-authored-by: Louis Groux <louis.cal.groux@gmail.com>
elvircrn pushed a commit to elvircrn/transformers that referenced this pull request Feb 13, 2025
…ggingface#35842)

* Fix test_pipelines_video_classification that was always failing

* Update video pipeline docstring to reflect actual return type

---------

Co-authored-by: Louis Groux <louis.cal.groux@gmail.com>
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