feat(wandb): save model as artifact
Author: borisdayma
Pull request: yes

# What does this PR do?

<!--
Congratulations! You've made it this far! You're not quite done yet though.

Once merged, your PR is going to appear in the release notes with the title you set, so make sure it's a great title that fully reflects the extent of your awesome contribution.

Then, please replace this with a description of the change and which issue is fixed (if applicable). Please also include relevant motivation and context. List any dependencies (if any) that are required for this change.

Once you're done, someone will review your PR shortly (see the section "Who can review?" below to tag some potential reviewers). They may suggest changes to make the code even better. If no one reviewed your PR after a week has passed, don't hesitate to post a new comment @-mentioning the same persons---sometimes notifications get lost.
-->

<!-- Remove if not applicable -->

**EDIT**

The logic has been simplified.
Model is just saved to a temporary folder and uploaded as artifact at the end of training.

**ORIGINAL message**

Save trained model as artifact.

A few different possibilities:

* log model at `on_save` callback -> the issue is there could quickly be too many checkpoints to upload, high bandwidth…

* log model at `on_train_end`

   * when we have access to `state.best_model_checkpoint`, we should just upload that folder
   * we could upload entire `output_dir` but it could be very large (same problem as `on_save`, ideally we only upload one model
   * we can save last model in a separate folder and upload it -> issue is that we don't have access to `Trainer.save_model` (where are the 2-way callbacks 😉)
   * we could just use `_sorted_checkpoints` and log only last element (which would also be the best model when metrics are given)

I'm thinking I should actually go with the last option (use of `_sorted_checkpoints`). What do you think?

## Before submitting
- [ ] This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
- [X] Did you read the [contributor guideline](https://github.com/huggingface/transformers/blob/master/CONTRIBUTING.md#start-contributing-pull-requests),
      Pull Request section?
- [ ] Was this discussed/approved via a Github issue or the [forum](https://discuss.huggingface.co/)? Please add a link
      to the it if that's the case.
- [X] Did you make sure to update the documentation with your changes? Here are the
      [documentation guidelines](https://github.com/huggingface/transformers/tree/master/docs), and
      [here are tips on formatting docstrings](https://github.com/huggingface/transformers/tree/master/docs#writing-source-documentation).
- [ ] Did you write any new necessary tests?


## 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. @sgugger 

<!-- Your PR will be replied to more quickly if you can figure out the right person to tag with @

 If you know how to use git blame, that is the easiest way, otherwise, here is a rough guide of **who to tag**.
 Please tag fewer than 3 people.

 Trainer: @sgugger
 -->


Comments:
Ok I submitted #8121 for the typo in `_new_step`.

As for this PR, I'm thinking the logic should be:
* use folder referenced by the last item returned from `_sorted_checkpoints`
* in case it's empty, we should probably save the current checkpoint locally and upload it (since we specifically requested an upload to wandb)

After experimenting a bit more:

1. Should we upload a model only if `_sorted_checkpoints(…)`  is non-empty?
   * we don't necessarily get the last model (eg save every 100 steps with 520 steps total)

2. Should we just save current state model at end of training in `args.output_dir + "\wandb"`
   * we need to have access to `Trainer.save_model` from `WandbCallback`
   * we could decide to use `state.best_model_checkpoint` when present instead
   * we ignore any checkpoint


@sgugger Do you want me to make an attempt at giving access to the `Trainer` from callbacks or is it a pattern you want to avoid?

Hi @borisdayma, sorry I took a bit of time to reply on this, we were waiting for the new version of the model hub to materialize before moving forward on this.

So! The callbacks aren't 2-way in Transformers because then you have to be very careful about the order of their execution. Here the design was to just allow for callbacks that can read the state, not write, and for any piece of code that needs the write access, users should subclass the `Trainer`. The circular reference is also problematic for memory management so we leave 2-way callbacks for libraries focused on training models, and keep our simple reporting callbacks as they are :-)

Like you said, you have access to the state with `best_model_checkpoint`. You can also unpack the model from the kwargs and access it. What is in the `Trainer.save_model` method that you need? Worst case scenario, you can even isntantiate an empty Trainer with just the model and the training arguments, and use its `save_model` method.

The issue with `best_model_checkpoint` is that it does not exist if there's no measurement metric set.
It could make sense to define it as the last checkpoint in that case.

The next issues would then be:
* sometimes no model has been saved yet (maybe not enough epochs) while we want to log the model -> we could accept that it's an issue on the user side and give a warning
* sometimes we may log every 100 steps and run for 180 steps. The last checkpoint is a bit old -> on this aspect I feel like the `Trainer` should automatically save the final step as a checkpoint

What do you think?

The alternative would be to completely ignore that logic, let wandb save a model somewhere and upload it. I had not realized we could have access to `model` from the callback (though saving from `Trainer` is better as it handles TPU, save tokenizer, args and may also change in the future).

I think the most logical is to save the final model, the intermediate checkpoints are there to resume training is something went wrong, or load the best model at the end (which is done before the `on_train_end` event). That's also why we don't always save the model at the end of training, leaving that part to the user in a script.

If you use the logic of unpacking the model from the kwargs, you can simply create a new `Trainer` with it which can then save it easily with the `Trainer.save_model` method. Normally the model you unpack is the reference to the real model, so you won't have a `DistributedDataParallel` or something like that, and everything should work smoothly.


Finally getting closer!

Few notes:
* I import Trainer inside my function to avoid circular reference
* I need to find a way to see if I need `Trainer` or `TfTrainer`, should I infer it through `type(model)`
* I use `Trainer.state` as model metadata but maybe it's not that useful. The artifact is associated to a run which already has all the config parameters but it could be useful to relog it, or maybe I should just log the final metrics instead (that I can get through wandb)

I think it's becoming pretty cool. Here is [an artifact](https://wandb.ai/borisd13/huggingface/artifacts/model/run-test-clm/2fc1855753d610244974) logged with this method.

The current limitation is that it only works with Pytorch for now.
Are there any plan for more synergy between `Trainer` or `TfTrainer` or should they be considered independently?

`TFTrainer` will be reworked in the near future and be a simple wrap around the Keras fit method (and the callbacks will be regular Keras callbacks).

I adjusted the metadata when we use `load_best_model_at_end`.
In that case we don't want to log the last metrics but only flos and best metric.

Small change:
* force commit of last step
* more robust way to get metadata, it will consider any data that has been logged and is a number

I'm now ready on my side. Feel free to ping me!

@LysandreJik let me know if you have any comments

Happy new year everybody!
Since it has already been a while since this PR was made, am I supposed to merge master and verify the tests are passing?
Let me know if I need to do anything on my side.