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

Removed console spam from misfiring warnings #13625

Merged
merged 5 commits into from
Sep 17, 2021
Merged

Conversation

Rocketknight1
Copy link
Member

A major source of UX annoyance is that when our code is used with Keras, warnings about inappropriate arguments fire every time the model is traced/compiled, which is at least once, and sometimes several times, per training run. I don't know why the warnings were written like this in the first place, but they're definitely not working now, so I'm going to remove them for now and do a deeper dive into the reason for the misbehaviour when I have more time.

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 think the problem is the top model with a classification head sends those arguments to the base model, which then triggers the warnings. I'm not sure just removing the warnings is the right solution since there is a modification of the kwargs that is operated. I see several potential solutions:

  1. remove the warnings and the modification of the kwargs and let the user deal with the error they get (if any). If there is no error at all and this code wasn't necessary in the first place, that would be my first choice.

  2. only check the parameters from the top model, and not the base model. This is could be done by adding a private kwarg to flag that to the input processing function, but this is not ideal since it would need updating all models signatures. Another way can be to use a private attribute of the model class that the model with head would set to False in the base model it uses.

@Rocketknight1
Copy link
Member Author

The warning fires regardless of whether the options are set or not. I think it's only supposed to show when the user initializes a model, and then overrides those arguments when calling the model in graph mode.

However, because of the way inputs_processing handles things the non-overridden inputs are set to the config values and not to None when the arguments are passed to boolean_processing, so it just fires (often several times) every time we fit() any Transformers model. This means that users just get used to ignoring the warning the whole time anyway.

I'm 99% sure we can just choose solution 1) and totally remove the warning with no problems - I'm not really sure why we have this restriction on changing parameters in graph mode anyway in modern Tensorflow!

@sgugger
Copy link
Collaborator

sgugger commented Sep 17, 2021

The warning fires regardless of whether the options are set or not.

Yes, like I said, I think it's because the top model resolves the kwargs passed, then passes them to the base model which also sends the thing to input_processing.

But if you are sure they are unnecessary, let's remove them and the change in the kwargs.

@Rocketknight1
Copy link
Member Author

I'm still a little bit afraid of changing the actual input processing until I do a deep dive into everything! I made a change to the PR: The warning still exists, but it confirms that the overridden value is actually different from the config value before it logs the warning. This eliminates the spam in all cases except the ones when it was actually supposed to display in the first place.

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.

Works for me, this should already remove a lot of spam!

Copy link
Member

@LysandreJik LysandreJik 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 looking into it, looking forward to the deep dive into the inputs processing!

@Rocketknight1 Rocketknight1 merged commit 0eb0287 into master Sep 17, 2021
@Rocketknight1 Rocketknight1 deleted the no_more_console_spam branch September 17, 2021 14:44
Albertobegue pushed a commit to Albertobegue/transformers that referenced this pull request Jan 13, 2022
* Removed misfiring warnings

* Revert "Removed misfiring warnings"

This reverts commit cea90de.

* Retain the warning, but only when the user actually overrides things

* Fix accidentally breaking just about every model on the hub simultaneously

* Style pass
Albertobegue pushed a commit to Albertobegue/transformers that referenced this pull request Jan 27, 2022
* Removed misfiring warnings

* Revert "Removed misfiring warnings"

This reverts commit cea90de.

* Retain the warning, but only when the user actually overrides things

* Fix accidentally breaking just about every model on the hub simultaneously

* Style pass
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.

None yet

3 participants