Skip to content

Conversation

@sam-hey
Copy link
Contributor

@sam-hey sam-hey commented Feb 28, 2025

Allowing users to pass the Path to a template to create the model card from it:

SentenceTransformerModelCardData( model_card_template_path=...)

This eliminates the need to override _create_model_card(), making it easier for PyLate, and enables users to provide their own model template.

cc @NohTow

@tomaarsen
Copy link
Member

Hello!

So I understand correctly, what do you intend to override exactly for PyLate? the model.model_card_data.model_card_template_path = "..."? (I'm not really sure what the current problem is that you're having on PyLate.)

If so, we might not have to introduce the template_path argument in generate_model_card at all, it can just grab it from model.model_card_data.model_card_template_path.

Also, as a nit: the model_card_ part of model_card_template_path is already kind of implied by that attribute living on SentenceTransformerModelCardData, so just template_path might make more sense.

  • Tom Aarsen

@sam-hey
Copy link
Contributor Author

sam-hey commented Feb 28, 2025

Hey @tomaarsen,

Thanks a lot for reaching out so quickly!

The problem is with this line:
https://github.com/UKPLab/sentence-transformers/blob/309a9accb784e0931636ad97be7806e37c57dfb4/sentence_transformers/SentenceTransformer.py#L1269

https://github.com/UKPLab/sentence-transformers/blob/309a9accb784e0931636ad97be7806e37c57dfb4/sentence_transformers/model_card.py#L1018-L1021

PyLate has its own template that should be applied. Unfortunately, the position is relative to model_card.py, so the path to the template needs to be updated accordingly.

Overriding _create_model_card() does not seem like a nice approach to solving this.

Thx for the hint!

@NohTow
Copy link
Contributor

NohTow commented Feb 28, 2025

Basically, IIUC, right now the main issue is that we have to overwrite _create_model_card in PyLate to be able to call the PyLate generate_model_card that is basically the same as ST, up to the ModelCard.from_template that points towards our local .md (and the HF emoji of the dog, but I am not even sure where it is used ahah).

It was working before as I was overwriting _create_model_card but removed it because you change it and I was not recalling why I did overwrite it. Honestly ; I am fine with overwriting it if you do not like adding a path, we already overwrite other stuff for the card. PyLate was mostly build by extending ST without asking for modifications (except when they is no way around it), but obviously sometimes the extension could be prettier with some modifications, so up to you @tomaarsen.

Edit: thanks @sam-hey for looking into all of this btw!

@tomaarsen
Copy link
Member

(and the HF emoji of the dog, but I am not even sure where it is used ahah).

In ST it's (only) used in the inference snippet:
image

I couldn't get it working in Jinja itself, so I used this trick instead:
https://github.com/UKPLab/sentence-transformers/blob/5c362dc1f7b069ebad20c44380566f40bfedb4e5/sentence_transformers/model_card_template.md?plain=1#L81

I think this implementation is quite neat, so I'm open to moving forward with this if @NohTow agrees that it would be valuable for simplifying PyLate.

Also, apologies for the delay. I've been trying to focus on #3222 in preparation for a big v4.0 release.

  • Tom Aarsen

@tomaarsen
Copy link
Member

Also, @NohTow, I'm guessing that you can just override template_path in your SentenceTransformerModelCardData subclass?
In that case, I'll move template_path around a bit and take it out of the __init__, as normal users don't need to update it.

  • Tom Aarsen

@NohTow
Copy link
Contributor

NohTow commented Mar 21, 2025

if @NohTow agrees that it would be valuable for simplifying PyLate.

Well it allows us to not override a function just for this, so it is always good, as we saw it broke PyLate once!

Also, @NohTow, I'm guessing that you can just override template_path in your SentenceTransformerModelCardData subclass?

As discussed, we can indeed just set it in the subclass and it should work nicely!

@tomaarsen tomaarsen merged commit 3fd59c3 into huggingface:master Mar 21, 2025
8 of 9 checks passed
@tomaarsen
Copy link
Member

Thanks for this! Once you move PyLate to ST v4.0+, you'll be able to start simplifying your code quite a bit :)

  • Tom Aarsen

@sam-hey sam-hey deleted the feat/allow_path_card_template branch March 21, 2025 18:11
@sam-hey
Copy link
Contributor Author

sam-hey commented Mar 21, 2025

Hey Tom,

Thanks a lot for merging! Looking forward to seeing more awesome work on ST :)

Best
Sam

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