-
Notifications
You must be signed in to change notification settings - Fork 571
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
ModelHubMixin: Fix attributes lost in inheritance #2305
Conversation
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
@Wauplin thanks for a quick response and fix! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great fix, just nit comments
if info.model_card_data.tags is not None: | ||
info.model_card_data.tags.extend(tags) | ||
else: | ||
info.model_card_data.tags = tags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we extend tags instead of overriding them? In that case, it would be impossible to drop base model tags and set new ones. Probably there is no the best solution, but I would expect them to be replaced rather than extended 🙂 WDYT?
if info.model_card_data.tags is not None: | |
info.model_card_data.tags.extend(tags) | |
else: | |
info.model_card_data.tags = tags | |
info.model_card_data.tags = tags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to extend them actually, specifically so that we don't loose tags defined by base models. Worst case, users can overwrite the internal attribute 😕
TBH I don't think this will ever be a problem, multiple inheritance when using ModelHubMixin is already not common so cases where nested classes define tags and where these tags should not be inherited might just never happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense, thank you!
Co-authored-by: Pavel Iakubovskii <qubvel@gmail.com>
There was a problem hiding this 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! Why are we switching from languages
to language
if we expect it to still be a list?
EDIT: Ah, to be consistent with ModelCardData
Yes, and moreover to be consistent with the existing repos on the Hub (and typically to filter by Thanks for the review :) |
* ModelHubMixn: Fix attributes lost in inhericance * make style * deprecate * style * Update src/huggingface_hub/hub_mixin.py Co-authored-by: Pavel Iakubovskii <qubvel@gmail.com> --------- Co-authored-by: Pavel Iakubovskii <qubvel@gmail.com>
Fix #2300.
Instead of recreating the
MixinInfo
object, we should complete its information as mentioned by @qubvel. Thanks for reporting this!EDIT: made some minor edits to
ModelCardData
as well but mostly setting alphabetical order + adding license_name/license_link officially.EDIT 2: deprecated
languages
in favor oflanguage
to be consistent withModelCardData
.