Skip to content

Merge alpha: remove top_k workaround + cache key fix#498

Merged
satra merged 1 commit into
mainfrom
alpha
Apr 30, 2026
Merged

Merge alpha: remove top_k workaround + cache key fix#498
satra merged 1 commit into
mainfrom
alpha

Conversation

@satra

@satra satra commented Apr 30, 2026

Copy link
Copy Markdown
Collaborator

HF bug fixed upstream. Pass top_k to pipeline constructor. Remove batch_size from cache key.

* Remove stale top_k workaround — HF bug fixed upstream

The transformers#35736 bug (top_k in pipeline constructor) was fixed
in Feb 2025. Remove the workaround comment and pass top_k directly
to pipeline() constructor as intended.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Remove batch_size from pipeline cache key (it's a call-time param)

batch_size is passed at pipe() call time, not pipeline() constructor.
Including it in the cache key created unnecessary duplicate pipelines.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@satra
satra merged commit ae773d5 into main Apr 30, 2026
10 checks passed

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates the Hugging Face audio classification pipeline by removing batch_size from the cache key and re-enabling the top_k and function_to_apply parameters in the pipeline constructor. Feedback indicates that including top_k and function_to_apply in the cache key is inefficient and memory-intensive because these parameters are already handled during inference. Additionally, it was noted that the batch_size parameter is currently ignored during the inference call.

Comment on lines +47 to 58
key = f"{model.path_or_uri}-{model.revision}-{top_k}-{function_to_apply}-{device.value}"
if key not in cls._pipelines:
cls._pipelines[key] = cast(
Pipeline,
pipeline( # type: ignore[call-overload]
task="audio-classification",
model=model.path_or_uri,
revision=model.revision,
# top_k=top_k, #TODO: this causes a bug in the pipeline that has been reported to transformers
# https://github.com/huggingface/transformers/issues/35736
function_to_apply=function_to_apply, # TODO: parameter ignored in transformer code, bug reported
# https://github.com/huggingface/transformers/issues/35739
top_k=top_k,
function_to_apply=function_to_apply,
device=device.value,
),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Including top_k and function_to_apply in the cache key and pipeline constructor is inefficient. These parameters are already passed during the inference call in classify_audios_with_transformers (line 149). By including them in the cache key, a new model instance is loaded into memory for every unique combination of these parameters, which is unnecessary and memory-intensive. Removing them from the key and constructor allows the same pipeline to be reused for different output configurations.

Additionally, the batch_size parameter is now unused in this function and is also missing from the pipeline call at line 149. This means the user-provided batch size is currently ignored during inference, defaulting to 1. You should remove the unused parameters from this function's signature and ensure batch_size is passed to the pipe() call in classify_audios_with_transformers.

        key = f"{model.path_or_uri}-{model.revision}-{device.value}"
        if key not in cls._pipelines:
            cls._pipelines[key] = cast(
                Pipeline,
                pipeline(  # type: ignore[call-overload]
                    task="audio-classification",
                    model=model.path_or_uri,
                    revision=model.revision,
                    device=device.value,
                ),
            )

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.

1 participant