Skip to content

Conversation

SamanehSaadat
Copy link
Member

@SamanehSaadat SamanehSaadat commented Apr 17, 2024

This PR adds a basic model card for Hugging Face upload, which was discussed in #1529.

Text classification example:

Text generation model example:

@SamanehSaadat SamanehSaadat requested a review from fchollet April 17, 2024 20:36
@SamanehSaadat
Copy link
Member Author

Hi @Wauplin !

I'm not sure why I can't add you as a reviewer here but please let me know if you have any feedback on this PR.

if config["class_name"].endswith("Backbone")
else config["class_name"]
)
markdown_content += f"This is a `{model_name}` model and has been uploaded using the KerasNLP library.\n"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove "and has been"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

if config["class_name"].endswith("Backbone")
else config["class_name"]
)
markdown_content += f"This is a `{model_name}` model and has been uploaded using the KerasNLP library.\n"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may want to add a link to the relevant keras.io docs page for the model.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a link to model pages like this.

markdown_content += "\n"
markdown_content += (
"This model card has been generated automatically and should be completed "
"by the model author. See https://huggingface.co/docs/hub/model-cards.\n"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a markdown link instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed it to markdown link.

if config["class_name"].endswith("Backbone")
else config["class_name"]
)
model_link = f"https://keras.io/api/keras_nlp/models/{model_name.lower()}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is model_name.lower() always correct? Can you try it on all Model instances found in the namespace?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right! It doesn't work for some models!
I was trying to figure out if I can programatically convert the model name to model path, e.g. converting CamelCase to snake_case, but things like XLMRoberta -> xlm_roberta makes it complicated! I can create a manual mapping in a dictionary from model to path based on this which seems to be our source of truth! But then kerasNLP and keras.io won't be automatically synced if we change one!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed this issue. I believe it should now work for all models.

Copy link
Contributor

@Wauplin Wauplin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @SamanehSaadat! Left a couple comments otherwise the model card already looks great!

else task_config["class_name"]
)
markdown_content += (
f"This model is related to a `{task_type}` task.\n\n"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any chance we can bind this task_type to a task from https://huggingface.co/tasks?

If yes, that would be awesome to also set it as a pipeline_tag in the yaml part of the model card. This way the models would be recognized as such and therefore searchable on the Hub (for instance on https://huggingface.co/models?pipeline_tag=text-classification). Even if we can't assign a pipeline_tag in every cases (because of uncertainty), having it for a subset of the models would already be nice. If some models support multiple tasks, you can set the main one as pipeline_tag and then secondary ones listed as tags.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can list all supported tasks like this:

curl -s https://huggingface.co/api/tasks | jq -r 'keys[]'

which outputs:

audio-classification
audio-to-audio
automatic-speech-recognition
depth-estimation
document-question-answering
feature-extraction
fill-mask
image-classification
image-feature-extraction
image-segmentation
image-to-3d
image-to-image
image-to-text
mask-generation
object-detection
question-answering
reinforcement-learning
sentence-similarity
summarization
table-question-answering
tabular-classification
tabular-regression
text-classification
text-generation
text-to-3d
text-to-image
text-to-speech
text-to-video
token-classification
translation
unconditional-image-generation
video-classification
visual-question-answering
zero-shot-classification
zero-shot-image-classification
zero-shot-object-detection

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not possible, then let's keep it as it is now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only capture the high-level task type in our configs, i.e. classification vs. generation. If a user picks a text generation model and trains it on a text summarization dataset to make it a text summarization model, I'm not sure if they can record that anywhere. If the high-level task type sounds good to you, I can add that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added high-level task as pipeline_tag.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Having text-generation or text-classification is already a very nice thing. If the user retrains on a summarization dataset, then it is their responsibility to update the model card correctly (in my opinion).

"Please install with `pip install huggingface_hub`."
)
hf_handle = uri.removeprefix(HF_PREFIX)
create_model_card(preset)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With current implementation, if the create_repo call fails (e.g. if user is not logged in) then the local model card is not deleted. To be sure the model card is correctly deleted, I would move the create_model_card call just before upload_folder and encapsulate it in a try/except.

So doing something like this:

try:
    huggingface_hub.upload_folder(
        repo_id=repo_url.repo_id, folder_path=preset
    )
finally:
    # Clean up the preset directory in case user attempts to upload the
    # preset directory into Kaggle hub as well.
    delete_model_card(preset)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right! Thanks for catching that! Done!

markdown_content += "\n"
markdown_content += (
"This model card has been generated automatically and should be completed "
"by the model author. See [Model Cards documentation]"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one nudging the author to complete the card :)

Copy link
Member Author

@SamanehSaadat SamanehSaadat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review, @Wauplin!

"Please install with `pip install huggingface_hub`."
)
hf_handle = uri.removeprefix(HF_PREFIX)
create_model_card(preset)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right! Thanks for catching that! Done!

Copy link
Collaborator

@fchollet fchollet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

Comment on lines 461 to 468
try:
huggingface_hub.upload_folder(
repo_id=repo_url.repo_id, folder_path=preset
)
finally:
# Clean up the preset directory in case user attempts to upload the
# preset directory into Kaggle hub as well.
delete_model_card(preset)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, last thing I forgot to mention. If the user push a model to an existing repo, we don't want to generate a model card as it might overwrite content that the user has manually written already. So I would implement a logic like that:

  1. create the repo with exists_ok=True
  2. check if file_exists
  3. generate model card (if doesn't exist)
  4. upload folder
  5. delete local model card (if generated in 3.)
Suggested change
try:
huggingface_hub.upload_folder(
repo_id=repo_url.repo_id, folder_path=preset
)
finally:
# Clean up the preset directory in case user attempts to upload the
# preset directory into Kaggle hub as well.
delete_model_card(preset)
has_model_card = huggingface_hub.file_exists(repo_id=repo_url.repo_id, filename=README_FILE)
if not has_model_card:
# Remote repo do not contain a model card => create one
create_model_card(preset)
try:
huggingface_hub.upload_folder(
repo_id=repo_url.repo_id, folder_path=preset
)
finally:
if not requires_model_card:
# Clean up the preset directory in case user attempts to upload the
# preset directory into Kaggle hub as well.
delete_model_card(preset)

(and do not forget to remove the create_model_card call above L449)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! Updated the code to only create mode card if it doesn't exist.

Copy link
Contributor

@Wauplin Wauplin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Thanks for iterating on it @SamanehSaadat 🤗

@SamanehSaadat
Copy link
Member Author

Thanks for the review and feedback @Wauplin!

@SamanehSaadat SamanehSaadat merged commit aec862a into keras-team:master Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants