Trainer's create_model_card creates an invalid yaml metadata `datasets: - null`
Author: elishowk
Pull request: no
Labels: name: model card

name: Should Fix

## Environment info

- any env

### Who can help

- discussed with @julien-c @sgugger and @LysandreJik 

## Information

- The hub will soon reject push with invalid model card metadata,
- **only when `datasets`, `model-index` or `license` are present**, their content need to follow the specification cf. https://github.com/huggingface/huggingface_hub/pull/342

## To reproduce

Steps to reproduce the behavior:

1. Train a model
2. Do not association any datasets
3. The trained model and the model card are rejected by the server

## Expected behavior

trainer.py git push should be successfull, even with the coming patch https://github.com/huggingface/transformers/pull/13514


Comments:
I think there are more fixes we can add to properly validate metadata:
- check the licence is correct
- check all the required fields are present before adding a `result` (which is going to also error out).

I will suggest a PR later today with all of that.

(note that this cannot really be hardcoded in `transformers` as rules can and will change on the server side)

I thought the goal was to fix the faulty model cards the Trainer was generating? If we don't want to hardcode anything in Transformers, we can just close the issue then. Why only fix the missing datasets and not the missing metrics for instance?

Hmm, the `datasets: - null` issue is not about missing datasets, it's about invalid data.

in YAML, this is parsed as `datasets = [None]` (python-syntax) whereas it should be an array of string.

In my opinion, we will not enforce rejections for missing data any time soon (especially for automatically generated model cards).

> I will suggest a PR later today with all of that.

Thanks @sgugger 
Ok if you take care of `license`.
But more simply here I wanted to avoid transformer code to insert `null` as dataset name.


(That being said, by all means, if you want to improve validation in transformers in the meantime, please do so. I'm just pointing out that writing validation code in transformers might in some cases be a little redundant)

To reformulate, we also have some problems if there is a `model_index` passed with no metrics in some result, which can be generated if the Trainer makes a push to hub with no evaluation done but can still be assigned a task and a dataset, which is a bit similar to the issue of passing a `datsasets: - null`. The license is different so we should definitely drop it for now if the list is expected to move.

The code for that won't be redundant in the sense the Trainer should not try to pass an incomplete result if we want the push to succeed. Even if we move the validation to the hf_hub library, we will still get an error. It's just the check will happen at a different level.

Yes I see. I think what you propose sounds good. Thoughts @elishowk?

IMHO the central point is to ensure transformers and the huggingface_hub CLI to display really well the server's warning and errors, that are meant to be clear and provide advices and documentation about validation failures.

> The license is different so we should definitely drop it for now if the list is expected to move.

The license enumeration is very simple, I don't think it's gonna change soon (is it ?) and the hub git server will return [the docs anyways](https://huggingface.co/docs/hub/model-repos#list-of-license-identifiers)

> Trainer should not try to pass an incomplete result if we want the push to succeed.

OK for me it's you call @sgugger and @julien-c, really, but if any checking is gonna be done on transformers' side before pushing a model's README.md metadata changes, IMHO we should use the same schema, in order to create as little maintenance as possible..

- The hub server the schema we use for `model-indel` is (for the moment) the following: [model-index-set.jtd.zip](https://github.com/huggingface/transformers/files/7153383/model-index-set.jtd.zip) (sorry github doesn't allow json uploads)
- we can generate a validation function for python using https://jsontypedef.com/docs/python-codegen/
- finally, if the JTD schema changes on the hub, we'll need to open a PR here first to update the schema, and may be later to version the schema server-side so that we don't break the older clients users have. for example by introducing a hub metadata schema version marked in the yaml itself. The server could respond (without breaking) that the pushed version is deprecated, and also later if needed implement schema conversion code.

(personally i don't see the point of this duplication if we can just try to push i.e. perform validation on the server side and report warnings/errors if push is not successful for validation reasons)

I might be missing something though

In the meantime I've suggested a fix for the problems for which this issue was created and for the incomplete results I mentioned. Both have their origin in the code of the `TrainingSummary`, so fixing them is not duplicate code :-)

We can think more about what validation we want to do where, personally I would see this more in the hf_hub side, in the function that adds metadata (which we will use in the Trainer once it's merged and in a release of hf_hub).

> In the meantime I've suggested a fix for the problems for which this issue was created and for the incomplete results I mentioned. Both have their origin in the code of the `TrainingSummary`, so fixing them is not duplicate code :-)

OK I think we understood each other now, it's about fixing the code that generates bad yaml, so also OK with @julien-c 
Let's just fix the yaml generation errors we know at this time. The hub server will tell us future validation errors and how to fix then.