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 WhisperForConditionalGeneration to respect generation_config? #30623

Closed
wants to merge 2 commits into from

Conversation

mizoru
Copy link

@mizoru mizoru commented May 2, 2024

I had to change a line in WhisperGenerationMixin for it to generate with timestamps during evaluation.
Many parameters of WhisperGenerationMixin.generate are None by deafult. So that when WhisperForConditionalGeneration.generate is called with default parameters and return_timestamps is set to True in generation_config, WhisperGenerationMixin._set_return_timestamps is called and replaces the value in the config with None.
The same thing happens with return_token_timestamps in the method _set_return_outputs, so I'm unsure if this is intended.
Should I maybe open an issue first?

What does this PR do?

Change WhisperGenerationMixin._set_return_timestamps to only replace the value return_timestamps in generation_config if new value is not None.

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?

I can see @patrickvonplaten was the last person who worked on these lines.

mizoru and others added 2 commits April 30, 2024 20:41
…ps=None

When WhisperForConditionalGeneration.generate is called with return_timestamps=None and return_timestamps=True in generation_config WhisperGenerationMixin._set_return_timestamps replaces the value in the config with None, now set to check return_timestamps is not None
@amyeroberts
Copy link
Collaborator

cc @sanchit-gandhi @kamilakesbi

@mizoru
Copy link
Author

mizoru commented May 2, 2024

Sorry, I'm a bit in over my head on how to contribute this properly right now, but:

This should work

    @staticmethod
    def _set_return_timestamps(return_timestamps, is_shortform, generation_config):
        if return_timestamps is None and hasattr(generation_config, "return_timestamps"):
            return_timestamps = generation_config.return_timestamps
        if not is_shortform:
            if return_timestamps is False:
                raise ValueError(
                    "You have passed more than 3000 mel input features (> 30 seconds) which automatically enables long-form generation which "
                    "requires the model to predict timestamp tokens. Please either pass `return_timestamps=True` or make sure to pass no more than 3000 mel input features."
                )

            logger.info("Setting `return_timestamps=True` for long-form generation.")
            return_timestamps = True

        if return_timestamps and not hasattr(generation_config, "no_timestamps_token_id"):
            raise ValueError(
                "You are trying to return timestamps, but the generation config is not properly set. "
                "Make sure to initialize the generation config with the correct attributes that are needed such as `no_timestamps_token_id`. "
                "For more details on how to generate the approtiate config, refer to https://github.com/huggingface/transformers/issues/21878#issuecomment-1451902363"
            )
        generation_config.return_timestamps = return_timestamps

@kamilakesbi
Copy link
Contributor

kamilakesbi commented May 7, 2024

Hi @mizoru,

Thanks for iterating on this!
Could you please open an issue with a min reproducer of the error you get before making these changes?

@kamilakesbi kamilakesbi self-assigned this May 17, 2024
@github-actions github-actions bot closed this Jun 19, 2024
@huggingface huggingface deleted a comment from github-actions bot Jun 21, 2024
@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.

@kamilakesbi
Copy link
Contributor

It will be solved by PR #31296 :)

@kamilakesbi kamilakesbi closed this Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants