-
Notifications
You must be signed in to change notification settings - Fork 27.4k
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 bug about add_special_tokens and so on #31496
Fix bug about add_special_tokens and so on #31496
Conversation
This PR makes sense to me, and sorry for the oversight! One suggestion, though - we could probably simplify it by including it in the code block above. In other words: if "add_special_tokens" in generate_kwargs:
preprocess_params["add_special_tokens"] = generate_kwargs["add_special_tokens"]
add_special_tokens = generate_kwargs["add_special_tokens"]
if "padding" in generate_kwargs:
preprocess_params["padding"] = generate_kwargs["padding"] would become if "add_special_tokens" in generate_kwargs:
add_special_tokens = preprocess_params["add_special_tokens"] = generate_kwargs.pop("add_special_tokens")
else:
add_special_tokens = False
if "padding" in generate_kwargs:
preprocess_params["padding"] = generate_kwargs.pop("padding") and then we wouldn't need the extra |
@Rocketknight1 It's obviously better than adding the extra for-pop statements. I revised as you wrote. Thanks a lot! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now! cc @amyeroberts for core maintainer review, and thanks for this bugfix!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing!
Only request before merge is to add a test which would have caught this bug
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. |
I added a test case in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
What does this PR do?
When applying add_special_tokens and/or padding arg(s) to
TextGenerationPipeline.__call__()
, we get an exception like:We need to remove both add_special_tokens and padding fields from generate_kwargs in
TextGenerationPipeline.TextGenerationPipeline()
.Until transformers v4.40.2, these kwargs were used only for preprocess but used for forward_params from v4.41.0.
Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?