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

[Sentence Transformers] Complete the update of model_type #1645

Closed
wants to merge 3 commits into from

Conversation

JingyaHuang
Copy link
Collaborator

@JingyaHuang JingyaHuang commented Jan 12, 2024

What does this PR do?

  • Improve the update of model_type, with the previous update, the model_type remains as the previous one (eg. bert) when displaying the config (which is relatively ok):
    image

But if we apply config.to_dict(), the model_type is not changed, so I would suggest to complete the change of model_type thoroughly.
image

  • The clip models in sentence transformers is not fully supported nor tested (the one in the test set actually goes for sentence-transformers-transformer instead of sentence-transformers-clip)

Copy link
Collaborator

@fxmarty fxmarty left a comment

Choose a reason for hiding this comment

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

Thank you for the notice, would this suggestion work?

Comment on lines 1683 to +1685
model.config = model[0].auto_model.config
model.config.model_type = "sentence-transformers-transformer"
model.config.__class__.model_type = "sentence-transformers-transformer"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
model.config = model[0].auto_model.config
model.config.model_type = "sentence-transformers-transformer"
model.config.__class__.model_type = "sentence-transformers-transformer"
model.config = PretrainedConfig.from_dict(model[0].auto_model.config.to_dict())
model.config.model_type = "sentence-transformers-transformer"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nope:
image

Comment on lines 1687 to +1689
model.config = model[0].model.config
model.config.model_type = "sentence-transformers-clip"
model.config.__class__.model_type = "sentence-transformers-clip"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
model.config = model[0].model.config
model.config.model_type = "sentence-transformers-clip"
model.config.__class__.model_type = "sentence-transformers-clip"
model.config = PretrainedConfig.from_dict(model[0].model.config.to_dict())
model.config.model_type = "sentence-transformers-clip"

@fxmarty
Copy link
Collaborator

fxmarty commented Jan 15, 2024

Right, the issue comes from https://github.com/huggingface/transformers/blob/881e966aced6f0f208f43d7b7e7e55bc680f8fa5/src/transformers/configuration_utils.py#L901-L902. This sounds like a bug in Transformers to me, self.__class__ is for some reason taking priority over self.

In the meantime we rather fix with:

class SentenceTransformersTransformerPretrainedConfig(PretrainedConfig):
    model_type = "sentence-transformers-transformer"

class SentenceTransformersClipPretrainedConfig(PretrainedConfig):
    model_type = "sentence-transformers-clip"

to avoid modifying class attributes?

@JingyaHuang
Copy link
Collaborator Author

close as this will be tackled in #1647

@JingyaHuang JingyaHuang deleted the fix-config-display branch January 18, 2024 14:41
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.

None yet

2 participants