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.

Sign up for GitHub

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

Jump to bottom

Fix warning situation: UserWarning: max_length is ignored when padding=True" #13829

Merged
merged 4 commits into from Oct 1, 2021
Merged

Fix warning situation: UserWarning: max_length is ignored when padding=True" #13829

merged 4 commits into from Oct 1, 2021

Conversation

shirayu
Copy link
Contributor

@shirayu shirayu commented Oct 1, 2021

What does this PR do?

Fixes #13826

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?

@sgugger @LysandreJik

@nreimers
Copy link
Contributor

nreimers commented Oct 1, 2021

The change looks good to me.

sgugger
sgugger reviewed Oct 1, 2021
View reviewed changes
Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

I don't think the warning should be just completely removed as it has some importance. We could check the truncation argumentand leave the warning if it's not set toTrue`.

@nreimers
Copy link
Contributor

nreimers commented Oct 1, 2021

I don't think the warning should be just completely removed as it has some importance. We could check the truncation argumentand leave the warning if it's not set toTrue`.

You mean truncation=False and max_length=10 => Should that not raise another warning, that max_length is ignored when you have set truncation to False.

I would argue that max_length and padding=True has no connection => hence, create no warning

@sgugger
Copy link
Collaborator

sgugger commented Oct 1, 2021

I don't think users are confused by what is happening when truncation=False, they are confused by what happens when padding=True. Remember that most users don't read the doc so they don't know padding=True means "longest" and not "max length".

@nreimers
Copy link
Contributor

nreimers commented Oct 1, 2021

I don't think users are confused by what is happening when truncation=False, they are confused by what happens when padding=True. Remember that most users don't read the doc so they don't know padding=True means "longest" and not "max length".

Yes, that makes sense.

So this warning can be raised when truncation == False or truncation == 'do_not_truncate'

@shirayu shirayu changed the title Removed wrong warning: UserWarning: max_length is ignored when padding=True" Fix warning situation: UserWarning: max_length is ignored when padding=True" Oct 1, 2021
@shirayu
Copy link
Contributor Author

shirayu commented Oct 1, 2021

Thank you for your comments.
I updated the code and title.

sgugger
sgugger approved these changes Oct 1, 2021
View reviewed changes
Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

Thanks for adapting, I just have a tweak to the warning message.

src/transformers/tokenization_utils_base.py Outdated Show resolved Hide resolved
@shirayu @sgugger
Update the warning message
54c07bd 
Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
@sgugger sgugger merged commit 90f980e into huggingface:master Oct 1, 2021
stas00 pushed a commit to stas00/transformers that referenced this pull request Oct 12, 2021
@shirayu @sgugger @stas00
Fix warning situation: UserWarning: max_length is ignored when paddin…
5b30b69 
…g=True" (huggingface#13829)

* Removed wrong warning

* Raise a warning when `max_length` is given with wrong `truncation`

* Update the error message

* Update the warning message

Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>

Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
lapisfluvialis pushed a commit to lapisfluvialis/transformers that referenced this pull request Oct 27, 2021
@shirayu @sgugger @LysandreJik
Fix warning situation: UserWarning: max_length is ignored when paddin…
3642d13 
…g=True" (huggingface#13829)

* Removed wrong warning

* Raise a warning when `max_length` is given with wrong `truncation`

* Update the error message

* Update the warning message

Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>

Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
Albertobegue pushed a commit to Albertobegue/transformers that referenced this pull request Jan 27, 2022
@shirayu @sgugger
Fix warning situation: UserWarning: max_length is ignored when paddin…
93adfe1 
…g=True" (huggingface#13829)

* Removed wrong warning

* Raise a warning when `max_length` is given with wrong `truncation`

* Update the error message

* Update the warning message

Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>

Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgugger sgugger sgugger approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Tokenizer - Raises wrong "UserWarning: max_length is ignored when padding=True"
3 participants
@shirayu @nreimers @sgugger