-
Notifications
You must be signed in to change notification settings - Fork 27.2k
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
Change logging level from warning to info for max_steps
overriding num_train_epochs
#34810
Conversation
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 ! cc @muellerzr for a second look
@@ -661,7 +661,7 @@ def __init__( | |||
raise ValueError("The `data_collator` should be a simple callable (function, class with `__call__`).") | |||
|
|||
if args.max_steps > 0 and args.num_train_epochs > 0: | |||
logger.warning("max_steps is given, it will override any value given in num_train_epochs") | |||
logger.info("max_steps is given, it will override any value given in num_train_epochs") |
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.
I don't mind switching to info since we already have documented that setting max_steps
will overwrite num_train_epochs
. Another compromise would be to indicate to the user to pass num_train_epochs = -1
in order to remove the warning but then the code becomes bloated for nothing.
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. |
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.
Agree with Marc, this change seems fine. cc @LysandreJik for final :)
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.
Sounds good to me, thanks all!
Unrelated errors, merging |
What does this PR do?
Passing
max_step
is a supported feature, and therefore should not raise a warning. I suggest lowering the level to info.Alternatively, we could have an additional parameter like
ignore_max_step_warning
but this seems overengineered.Considering that the use of
max_steps
is already documented:transformers/src/transformers/training_args.py
Line 307 in ce1d328
lowering the level to info seems to be the best solution.
Technically, we can ignore this warnings with
But it's not intuitive at all IMO.
As for warnings, I'd say that a good practice is that they should always be removable by the user:
either by modifying their code, in the event of a feature being misused, deprecated, etc. E.g example:
transformers/src/transformers/models/speech_to_text/feature_extraction_speech_to_text.py
Lines 235 to 238 in ce1d328
or by acknowledging, in the case of very important information upgraded to warning; we add a parameter for acknowledging. E.g:
example:
allow_missing_key
inload_pytorch_checkpoint_in_tf2_model
Related: huggingface/trl#2350
Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
@muellerzr and @SunMarc