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

Introduce logging_strategy training argument #10267

Merged

Conversation

tanmay17061
Copy link
Contributor

@tanmay17061 tanmay17061 commented Feb 18, 2021

Introduce logging_strategy training argument
in TrainingArguments and TFTrainingArguments. (#9838)

What does this PR do?

  1. Introduce a logging_strategy argument in TrainingArguments.
  2. Define a LoggingStrategy enumeration. This is similar to EvalStrategy.

Fixes #9838

Before submitting

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors which may be interested in your PR.

As changes in trainer: @sgugger

@tanmay17061 tanmay17061 force-pushed the new-training-arg-logging-epochs branch 2 times, most recently from d70feae to d89b14f Compare February 18, 2021 20:36
@tanmay17061
Copy link
Contributor Author

Currently WIP.
Thanks!

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.

This is a useful addition but it needs to be backward compatible: this means the default should give the same behavior as before (which is not the case right now as highlighted by the failing tests) so "steps" for the default logging_strategy and keep the eval_steps default untouched either.

Could you amend your PR in that direction? Thanks!

@tanmay17061 tanmay17061 force-pushed the new-training-arg-logging-epochs branch from d89b14f to 6e9a757 Compare February 19, 2021 05:12
@tanmay17061
Copy link
Contributor Author

tanmay17061 commented Feb 19, 2021

Thanks, yes that worked out!
IMO, defaulting eval_steps to logging_steps is not a good decision any longer.
With logging_strategy introduced, it seems more intuitive to decouple both. In case user chooses logging_strategy="epoch", logging_steps is no longer a valid quantity.
What is your take on this?

@sgugger
Copy link
Collaborator

sgugger commented Feb 19, 2021

In case user chooses logging_strategy="epoch", logging_steps is no longer a valid quantity.

It will just be ignored in that case, so there is no weird behavior for the user.

@tanmay17061
Copy link
Contributor Author

Sure! That makes sense.
You can review the changes and let me know if any changes required.
Thanks

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.

LGTM! I'm just wondering is we should add the LoggingStrategy.NO in case a user doesn't want any log since it comes for free (I don't think you'll need to change any line of code apart adding this to the enum).

This is because the next step is to do the same thing for saves (if you want to tackle it in a next PR!) which also would have the values steps/epochs/no so we could rename EvaluationStrategy to something more generic like TimeInterval (or any better name) which we would use for the evaluation strategy, saving strategy and logging strategy.

Introduce logging_strategy training argument
in TrainingArguments and TFTrainingArguments. (huggingface#9838)
@tanmay17061 tanmay17061 force-pushed the new-training-arg-logging-epochs branch from 6e9a757 to b3ed60c Compare February 19, 2021 14:24
@tanmay17061
Copy link
Contributor Author

Yes, something like TimeInterval (or TimeStrategy) will be a good generic enum.
I can work on this generic enum and saving_strategy earlier next week most probably. Will raise a PR soon enough.
For now I've made the amends to introduce LoggingStrategy.NO.

@sgugger
Copy link
Collaborator

sgugger commented Feb 19, 2021

Great! We can merge this in the meantime.
Looking forward to your next PR!

@sgugger sgugger merged commit 709c86b into huggingface:master Feb 19, 2021
@tanmay17061 tanmay17061 deleted the new-training-arg-logging-epochs branch February 20, 2021 09:32
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.

logging_epochs argument for TrainingArguments
2 participants