Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ds model card #2094

Closed
wants to merge 16 commits into from
Closed

Add ds model card #2094

wants to merge 16 commits into from

Conversation

bursteratom
Copy link
Collaborator

Continuation of PR#1015 with rebasing

Description

Add dataset tagging to model card for both local and push to huggingface hub.

https://github.com/huggingface/hub-docs/blob/main/modelcard.md

Motivation and Context

How has this been tested?

Screenshots (if appropriate)

Types of changes

Social Handles (Optional)

@bursteratom
Copy link
Collaborator Author

@winglian this is a temporary WIP patch, the proper way will probably be actually parsing dataset_tags into the trainer for proper kwarg sanitising, I think. Do you think it's necessary?

@bursteratom bursteratom requested a review from winglian November 21, 2024 14:01
@bursteratom bursteratom marked this pull request as draft November 21, 2024 20:06
@winglian
Copy link
Collaborator

Why skip for dpo? Let's make sure to properly handle that case too.

@bursteratom
Copy link
Collaborator Author

@winglian trl trainer's def create_model_card() has different arguments that don't seem to directly support dataset tags (it only allows you to parse the name of a single dataset),
https://github.com/huggingface/trl/blob/main/trl/trainer/dpo_trainer.py
so thinking of how to get around that properly

@bursteratom bursteratom marked this pull request as ready for review November 23, 2024 01:45
This reverts commit 816181c, reversing
changes made to a349efd.
Copy link
Collaborator

@winglian winglian left a comment

Choose a reason for hiding this comment

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

there are too many unrelated changes in this PR from doing a merge commit. Please resolve and re-request a review.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This file should have no edits, you have some errors in your merge commit. I personally prefer to rebase branches rather than merge commits to avoid issues like this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

saem as above.

Copy link
Collaborator

Choose a reason for hiding this comment

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

saem as above.

Copy link
Collaborator

Choose a reason for hiding this comment

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

saem as above.

Copy link
Collaborator

Choose a reason for hiding this comment

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

saem as above.

Copy link
Collaborator

Choose a reason for hiding this comment

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

saem as above.

@bursteratom
Copy link
Collaborator Author

to be continued with rebasing here: #2101

@bursteratom bursteratom deleted the add_ds_model_card branch November 24, 2024 19:48
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.

2 participants