Use default model_name in metadata_update

[lvwerra]

As reported by a user, the metadata_update does not work anymore (likely introduced in #940) if no "name" field in the "model-index" is provided. This breaks for example evaluate.push_to_hub where evaluation results are pushed to the hub.

With this PR the following behaviour is supported:

• if "model-index" of new metadata has a "name" it is used (1)
• if there is no "name"
  • if the existing repo card has one use this one (2)
  • else use the repo name as default (3)

Since the "name" is I think a little known feature of the PwC integration and providing a model name when pushing the results to a model repo seems a bit redundant I feel like having good defaults and not bother the user with it is a good idea. If you think this is a bad idea, we can throw an error in case (3).

cc @julien-c

[HuggingFaceDocBuilderDev]

The documentation is not available anymore as the PR was closed or merged.

[Wauplin]

I think a little known feature (...) I feel like having good defaults and not bother the user with it is a good idea.

To be honest, I don't know enough about metadata to really have an opinion on the "name" under "model-index". But I agree with you that in general we don't want to bother users about very specific features. I ping @nateraw would worked on that and might be more opinionated.

---

Otherwise, I noticed that for (3) you are talking about the repo name (e.g. "bart-base") and not the repo id (e.g. "facebook/bart-base"). I guess that's intended but wanted to highlight it for others in the discussion.

And also if we move forward on this PR, @lvwerra could you please add at least one test that triggers case (2) or (3) (e.g. a case that is currently broken but that your PR solves). Thanks in advance !

[lvwerra]

Yes, will add tests if people are happy with the suggested change, thanks @Wauplin!

[julien-c]

personally I would use the full repo_id, but otherwise, lgtm!

[lvwerra]

Sounds good, also added some tests :)

[Wauplin]

Looks good to me, thanks for adding the test !
Please wait for @nateraw 's review before merging :)

[nateraw]

Just getting to his ( was off on Friday ). Having a look!

[nateraw]

personally I would use the full repo_id, but otherwise, lgtm!

Agree with repo ID but I'm not too opinionated

[nateraw]

LGTM thanks for the update. One concern is that we do mention this required name in the docs for model card.

Do we need to update the docstring around this? Mentioned in eval_result kwarg docstring as well.

[nateraw]

Thanks for docstring update. ❤️

once quality tests/other tests pass this is good to go I think :)

[codecov]

Codecov Report

Base: 84.54% // Head: 84.70% // Increases project coverage by +0.16% 🎉

Coverage data is based on head (4d46b7e) compared to base (e646412).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1157      +/-   ##
==========================================
+ Coverage   84.54%   84.70%   +0.16%     
==========================================
  Files          42       42              
  Lines        4147     4184      +37     
==========================================
+ Hits         3506     3544      +38     
+ Misses        641      640       -1     

Impacted Files	Coverage Δ
src/huggingface_hub/repocard_data.py	98.41% <ø> (+0.02%)	⬆️
src/huggingface_hub/repocard.py	95.72% <100.00%> (+1.73%)	⬆️
src/huggingface_hub/utils/_deprecation.py	95.55% <0.00%> (-4.45%)	⬇️
src/huggingface_hub/hf_api.py	87.56% <0.00%> (-0.04%)	⬇️
src/huggingface_hub/file_download.py	88.48% <0.00%> (+0.60%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[Wauplin]

I'm finally merging the PR. Thanks @lvwerra for all the modifications ! :)