Skip to content

Conversation

@sambhavnoobcoder
Copy link
Contributor

Problem Statement

There were two issues with the audio classification pipeline's top_k parameter:

  1. Documentation mismatch: The docs stated that when top_k=None, all labels should be returned, but this wasn't implemented
  2. Runtime error: When top_k=None and a model had fewer labels than the default (5), the pipeline would crash

Fixes : #35736

Root Cause Analysis

After investigating the implementation, we found:

  1. The __init__ method was unconditionally setting a default top_k=5, even when explicitly set to None
  2. The _sanitize_parameters method wasn't properly handling the None case, leading to potential crashes when models had fewer labels than the requested/default top_k

Solution

The fix involves:

  1. Properly handling top_k=None in initialization to respect the user's intent to get all labels
  2. Updating _sanitize_parameters to use the model's total number of labels when top_k=None
  3. Maintaining backward compatibility by keeping the default top_k=5 when not explicitly specified
  4. Adding comprehensive tests to verify the behavior

Implementation Details

Changes were made to:

  1. AudioClassificationPipeline.__init__: Modified to properly handle None value
  2. AudioClassificationPipeline._sanitize_parameters: Updated to use all labels when top_k=None
  3. Added new test file test_audio_classification_top_k.py with three test cases:
    • Testing top_k=None returns all labels
    • Testing behavior with models having fewer labels
    • Testing top_k greater than available labels

Testing

Added comprehensive tests that verify:

  1. When top_k=None, all labels are returned
  2. Models with fewer labels than the default work correctly
  3. Large top_k values are properly capped to available labels

Test Results

Screenshot 2025-01-19 at 2 21 30 PM

All test cases pass successfully, confirming the fix works as intended while maintaining backward compatibility.

Note About Warning:
During test execution, we see a deprecation warning: UserWarning:

Passing gradient_checkpointing to a config initialization is deprecated and will be removed in v5 Transformers. Using model.gradient_checkpointing_enable() instead...

This warning is unrelated to our top_k changes. It comes from the model configuration initialization and is about the deprecation of setting gradient checkpointing via config. This is a broader change planned for Transformers v5 to move gradient checkpointing configuration from model initialization to explicit method calls. It doesn't affect our audio classification pipeline's functionality or the top_k parameter behavior.

the warning would go away by exploring a better model with multiple labels supporting gradient checkpointing even now , but i decided to leave the same model in place for originality purposes as it doesn't affect this PR in any way or form .

cc: @Rocketknight1

@Rocketknight1
Copy link
Member

Yes, this looks clean to me, and I really appreciate the broad test coverage!

cc @wilke0818, does this fix the issues you saw?

@wilke0818
Copy link

Yep just tried on the original code I had, as well as did some stress testing of it and it seems to work in all scenarios I can think of. Thanks for the quick responses and fixes! Also did a quick check and it doesn't seem like this issue exists in the other classification pipelines (I don't see any top_k default value setting).

@sambhavnoobcoder
Copy link
Contributor Author

Cool . Then I think we can merge this @Rocketknight1 ?

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.

Yes, approving, and thank you for the PR!

@Rocketknight1 Rocketknight1 merged commit 0de15c9 into huggingface:main Feb 5, 2025
25 checks passed
Copy link
Collaborator

Choose a reason for hiding this comment

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

cc @Rocketknight1 this should not need a single file on it's own, this should go in the pipeline tests

Copy link
Member

Choose a reason for hiding this comment

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

Will fix, my bad for not noticing it was a separate file in the review!

MekkCyber pushed a commit that referenced this pull request Feb 7, 2025
…#35736 (#35771)

* added condition for top_k Doc mismatch fix

* initilation of test file for top_k changes

* added test for returning all labels

* added test for few labels

* tests/test_audio_classification_top_k.py

* final fix

* ruff fix

---------

Co-authored-by: sambhavnoobcoder <[email protected]>
elvircrn pushed a commit to elvircrn/transformers that referenced this pull request Feb 13, 2025
…huggingface#35736 (huggingface#35771)

* added condition for top_k Doc mismatch fix

* initilation of test file for top_k changes

* added test for returning all labels

* added test for few labels

* tests/test_audio_classification_top_k.py

* final fix

* ruff fix

---------

Co-authored-by: sambhavnoobcoder <[email protected]>
sbucaille pushed a commit to sbucaille/transformers that referenced this pull request Feb 16, 2025
…huggingface#35736 (huggingface#35771)

* added condition for top_k Doc mismatch fix

* initilation of test file for top_k changes

* added test for returning all labels

* added test for few labels

* tests/test_audio_classification_top_k.py

* final fix

* ruff fix

---------

Co-authored-by: sambhavnoobcoder <[email protected]>
@sambhavnoobcoder
Copy link
Contributor Author

Hey @Rocketknight1 , sorry for inconvenience on a merged PR , but i was looking through my past contributions and i found that even though i was getting PRs merged like this one , or others like 35859 , 35858 , 35735 or 36345 , I could not see my name / image on the contributors board/list for this repo/ project . Could you look into it and tell me why is that happening ? Is there some other process i am missing out to be listed on the board ? i already posted about this on discord first , but was told to discuss this directly on the github issue itself , hence the query.

@Rocketknight1
Copy link
Member

Hi @sambhavnoobcoder, do you mean here? https://github.com/huggingface/transformers/graphs/contributors

If so, that's part of Github, not managed by us. It's sorted by total commits (PRs merged) to the repo. You'll turn up there eventually!

@sambhavnoobcoder
Copy link
Contributor Author

yes @Rocketknight1 , that was what i was looking at . actually i added the number of commits from all of my PRs that were merged so far , and thought that i could've been there on the basis of that number , but if the metrics/requirements for the board are something different , then i am not sure .

@Rocketknight1
Copy link
Member

Hi @sambhavnoobcoder, actually, one single PR being merged counts as one "commit" to Transformers. Even though the PR branch has multiple commits, it's squished into a single update to the main branch that is recorded as a single commit.

That makes it very hard for the rest of us to catch up with founders like @julien-c and @thomwolf who were able to just make lots of raw commits directly to the repo in the early days 😅

@sambhavnoobcoder
Copy link
Contributor Author

ohh , that makes perfect sense !! haha , i was wondering if i was doing something wrong , but this makes perfect sense . thanks for the quick reply .

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.

Audio-Classification Pipeline top_k Documentation mismatch and bug (possibly generalizes to any classification pipelines)

4 participants