Introducing list of tags to Keras model card

[merveenoyan]

This PR closes #800
Resulting model card looks like this when given the tags list ["audio", "speech"].

[julien-c]

BTW (commenting here because i just glanced at the code): maybe more future proof to create the yaml part of the model card using pyyaml (which is already a dependency of this lib anyways)

I remember that's what we did in transformers

[osanseviero]

Actually we already have repocard.py which could make things easier for this

https://github.com/huggingface/huggingface_hub/blob/main/src/huggingface_hub/repocard.py

[merveenoyan]

@julien-c thanks for the tip, it looks much cleaner!

[merveenoyan]

@osanseviero I didn't see your comment and added with yaml.dump(), it was two lines of code, do you want me to change it?

[osanseviero]

I think using pyyaml like this works, I will add some additional suggestions to this PR a bit later

[merveenoyan]

tags can be passed as string or a list, brought back task_name after tags, added warning only for save_pretrained_keras since it's already called from push_to_hub_keras and others.

[HuggingFaceDocBuilderDev]

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

[merveenoyan]

Merged main into this branch for other docs related changes and aligned docstring with main.

[merveenoyan]

is there any formatter or style checker that can indicate if a docstring is fit? @osanseviero

[osanseviero]

is there any formatter or style checker that can indicate if a docstring is fit? @osanseviero

Not sure, but as you can see at #806 (comment), the docs are actually built for you 🔥

Maybe @LysandreJik knows

[osanseviero]

Thanks!

[merveenoyan]

Sorry I kinda rushed it, missed the nits. I added a test. @osanseviero I'll be waiting for @adrinjalali's comment on deprecation.

[merveenoyan]

I left metadata part as dictionary for convenience.
So if the task_name is seen, user receives warning, it's popped from kwargs and passed to metadata. I've also put a test to see if the warning is raised.

save_pretrained_keras(
                model,
                f"{WORKING_REPO_DIR}/{REPO_NAME}",
                tags=["test"],
                task_name="test",
                save_traces=True,
            )

[osanseviero]

Cool! This is looking safer and very nice! I left a couple of cleanup nits but almost ready to go 🚀

[osanseviero]

In lines 194 to 199 you can maybe do

task_name = model_save_kwargs.pop("task_name", None) # This means the default will be None
if task_name is not None:
   warnings.warn(
            "`task_name` input argument is removed. Pass `tags` instead.",
            FutureWarning,
        )

and similarly, here you won't need to check locals(), which seems a bit hacky, and you can just do
if task_name is not None.

[merveenoyan]

it doesn't work, it says "task_name local variable mentioned before assignment" that's why I had to do it. I'll check if I mention it anywhere else.

[osanseviero]

Yes, you get the error because task_name is created within an if. My suggestion is to have this outside all the ifs.

os.makedirs(save_directory, exist_ok=True)

task_name = model_save_kwargs.pop("task_name", None) # This means the default will be None
if task_name is not None:
   warnings.warn(
            "`task_name` input argument is removed. Pass `tags` instead.",
            FutureWarning,
        )
...

[merveenoyan]

I thought it wouldn't be able to pop or return KeyError. thanks!

[merveenoyan]

@osanseviero my stupidity, I did fix them after I did a git add and I didn't add changes again apparently 😅😂 I was like "I could swear I changed them previously!!! am I hallucinating? 🥲" very sorry, I do listen to your comments before I resolve them, just saying.

[osanseviero]

Very nice! Thank you so much for this PR and iterating on it! 🚀

[osanseviero]

Please don't merge PRs that are in red like this one, all tests from last 4 days show in red https://github.com/huggingface/huggingface_hub/actions/workflows/python-tests.yml due to a (very minor though) mismatch in a test warning. I sent #855 to fix this

[adrinjalali]

Also, note that we're trying to merge other people's PRs instead of our own on this repo :)

(I should get on writing our policies and their variations for our repos)