move wandb/comet logger init to train() to allow parallel logging
Author: krfricke
Pull request: yes

Moving the logger setup to the `train()` function allows parallel runs (e.g. in hyperparameter search) to log each run individually.

Alternative to #6791


Comments:
# [Codecov](https://codecov.io/gh/huggingface/transformers/pull/6850?src=pr&el=h1) Report
> Merging [#6850](https://codecov.io/gh/huggingface/transformers/pull/6850?src=pr&el=desc) into [master](https://codecov.io/gh/huggingface/transformers/commit/b8e4906c974101d328bdd01245bc1695f9b07088?el=desc) will **increase** coverage by `0.17%`.
> The diff coverage is `78.57%`.

[![Impacted file tree graph](https://codecov.io/gh/huggingface/transformers/pull/6850/graphs/tree.svg?width=650&height=150&src=pr&token=9qOlN6Hb1c)](https://codecov.io/gh/huggingface/transformers/pull/6850?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #6850      +/-   ##
==========================================
+ Coverage   80.44%   80.61%   +0.17%     
==========================================
  Files         161      161              
  Lines       30113    30119       +6     
==========================================
+ Hits        24224    24281      +57     
+ Misses       5889     5838      -51     
```


| [Impacted Files](https://codecov.io/gh/huggingface/transformers/pull/6850?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/transformers/trainer.py](https://codecov.io/gh/huggingface/transformers/pull/6850/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy90cmFpbmVyLnB5) | `54.95% <78.57%> (+0.27%)` | :arrow_up: |
| [src/transformers/modeling\_tf\_mobilebert.py](https://codecov.io/gh/huggingface/transformers/pull/6850/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ190Zl9tb2JpbGViZXJ0LnB5) | `24.55% <0.00%> (-72.36%)` | :arrow_down: |
| [src/transformers/modeling\_tf\_flaubert.py](https://codecov.io/gh/huggingface/transformers/pull/6850/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ190Zl9mbGF1YmVydC5weQ==) | `24.53% <0.00%> (-63.81%)` | :arrow_down: |
| [src/transformers/modeling\_marian.py](https://codecov.io/gh/huggingface/transformers/pull/6850/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ19tYXJpYW4ucHk=) | `60.00% <0.00%> (-30.00%)` | :arrow_down: |
| [src/transformers/modeling\_t5.py](https://codecov.io/gh/huggingface/transformers/pull/6850/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ190NS5weQ==) | `76.70% <0.00%> (-6.07%)` | :arrow_down: |
| [src/transformers/activations.py](https://codecov.io/gh/huggingface/transformers/pull/6850/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9hY3RpdmF0aW9ucy5weQ==) | `85.00% <0.00%> (-5.00%)` | :arrow_down: |
| [src/transformers/modeling\_utils.py](https://codecov.io/gh/huggingface/transformers/pull/6850/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ191dGlscy5weQ==) | `87.50% <0.00%> (-0.56%)` | :arrow_down: |
| [src/transformers/modeling\_bart.py](https://codecov.io/gh/huggingface/transformers/pull/6850/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ19iYXJ0LnB5) | `93.90% <0.00%> (-0.51%)` | :arrow_down: |
| [src/transformers/tokenization\_utils\_base.py](https://codecov.io/gh/huggingface/transformers/pull/6850/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy90b2tlbml6YXRpb25fdXRpbHNfYmFzZS5weQ==) | `93.76% <0.00%> (-0.28%)` | :arrow_down: |
| [src/transformers/generation\_utils.py](https://codecov.io/gh/huggingface/transformers/pull/6850/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9nZW5lcmF0aW9uX3V0aWxzLnB5) | `97.21% <0.00%> (+0.27%)` | :arrow_up: |
| ... and [8 more](https://codecov.io/gh/huggingface/transformers/pull/6850/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/huggingface/transformers/pull/6850?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/huggingface/transformers/pull/6850?src=pr&el=footer). Last update [b8e4906...a27cdd3](https://codecov.io/gh/huggingface/transformers/pull/6850?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


I don't think you want to do the logger setup within `train` as users can call `Trainer` for evaluation only as well.
It probably needs to stay within `__init__` but should also go into a hyperparameter search function, maybe `_objective`.

What's important is for loggers to setup at `__init__` and at each parameter search.

However @sgugger will have a better idea in how to organize this function.

Yes, the train method could be called several times on the same Trainer, or the Trainer could be used for evaluation only, and those logging platforms should be setup once only, so the init looks best. Maybe we could add a private attribute `_has_been_setup` that could be checked inside the log method before reporting to wandb/comet and call the setup method if needed? Would that work for the hp search with Ray?

That sounds good. Should it still be setup in the init then? For hyperparameter search this doesn't really make sense (and creates an "empty" run in wandb), and if it is setup on logging calls anyway we wouldn't necessarily need it there. But happy to leave it there, too.

We can leave the setup to the first time we try to log something or the first call to train then (if there is a check to the same flag, we can call the setup method several times safely).

I think the first time we try to log makes sense, and also allow to use `Trainer` in eval only.

If people just want to call multiple times `train`, it would be nice if it was straightforward for them to choose between logging to the same run or logging to a new run. Hyperparameter search would obviously automatically choose to log to a new run.

Note that logging several `train` calls to the same run is actually not currently supported due to `global_step` being reset to 0 [here](https://github.com/huggingface/transformers/blob/54cfefc2ac9e3e1c0968a2ed0dd3c711eee76196/src/transformers/trainer.py#L645) which will cause issues at least in both Tensorboard and W&B.

I adjusted the PR so the loggers will be initialized on the first call to `log()`. Is this what you had in mind?

Yes. I just think we should add the line to setup at the beginning of log, so that the loggers get initialized if we try to log something.

Okay, so the current position is good? (When clicking the "Files changed" link it looks like it's in `_hp_search_setup`, but it's actually right at the beginning of `log`)

Looks great!

Oh yeah, sorry I looked too fast. LGTM!