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.

Sign up for GitHub

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

Jump to bottom

refactor: organize the import and export of SparseModel #141

Merged
merged 2 commits into from
Jan 14, 2025
Merged

refactor: organize the import and export of SparseModel #141

merged 2 commits into from
Jan 14, 2025

Conversation

honsunrise
Copy link
Contributor

Hi @Anush008,

Additionally, I'm not quite clear on your thoughts. Perhaps we could move the RerankerModelInfo from crate::models::reranking to crate::models::model_info.
If you think this aligns with your design, then I can add a commit into this PR or open a new PR.

@Anush008
Copy link
Owner

Hey @honsunrise. Thanks for the contribution.

Additionally, I'm not quite clear on your thoughts. Perhaps we could move the RerankerModelInfo from crate::models::reranking to crate::models::model_info.
If you think this aligns with your design, then I can add a commit into this PR or open a new PR.

Fine by me.

@honsunrise honsunrise force-pushed the feat/pub-sparse-models branch from 7e4ebbd to c1415cd Compare January 13, 2025 03:30
@Anush008
Copy link
Owner

Hey @honsunrise. Thank you.
I see there have been multiple changes, which could potentially be breaking(We can't do that)
Could you please describe your changes?

@honsunrise
Copy link
Contributor Author

honsunrise commented Jan 13, 2025 •
edited
Loading

Hey @honsunrise. Thank you. I see there have been multiple changes, which could potentially be breaking(We can't do that) Could you please describe your changes?

Hey @Anush008. Thank for your review.

Yes, There has two breaking changes:

  1. Added the ModelEmbeddingDim trait for models that have a fixed output dimension. So if we need to obtain the model's output dimension, the following changes should be made: change model_info.dim to model_info.dim()
  2. Due to changes in features made in hf-hub 0.4, the following adjustments have been made accordingly: the online feature has been changed to hf-hub-native-tls and hf-hub-rustls-tls

For the first breaking change, if we want to unify ModelInfo to achieve a consistent and clean API, this might be the best approach I can think of.
For the second change, perhaps we can add online = ["hf-hub-native-tls"] to fix it, if you think it's absolutely necessary.

@Anush008
Copy link
Owner

We cannot have breaking changes unless itss a major release though.

@honsunrise
Copy link
Contributor Author

We cannot have breaking changes unless itss a major release though.

Can we consider this PR as part of a major release?

@honsunrise
Copy link
Contributor Author

We cannot have breaking changes unless itss a major release though.

I rolled back all the breaking changes and moved them to a new PR.

@honsunrise honsunrise force-pushed the feat/pub-sparse-models branch from c1415cd to 4497fae Compare January 13, 2025 06:59
@honsunrise honsunrise force-pushed the feat/pub-sparse-models branch from 4497fae to dff7f02 Compare January 13, 2025 07:00
@Anush008
Copy link
Owner

Can we consider this PR as part of a major release?
There isn't anything planned for a major release.

Anush008
Anush008 approved these changes Jan 14, 2025
View reviewed changes
Copy link
Owner

@Anush008 Anush008 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 contributing @honsunrise. LGTM!

@Anush008 Anush008 changed the title feat: organize the import and export of SparseModel refactor: organize the import and export of SparseModel Jan 14, 2025
@Anush008 Anush008 merged commit 2f982f1 into Anush008:main Jan 14, 2025
@honsunrise honsunrise deleted the feat/pub-sparse-models branch January 15, 2025 02:52
github-actions bot pushed a commit that referenced this pull request Feb 18, 2025
@laulauland @github-actions
chore(release): 4.5.0 [skip ci]
6990869 
# [4.5.0](v4.4.0...v4.5.0) (2025-02-18)

### Features

* Add modernbert-embed-large ([#146](#146)) ([8184799](8184799))

## [4.5.0](v4.4.0...v4.5.0) (2025-02-18)

### 🍕 Features

* Add modernbert-embed-large ([#146](#146)) ([8184799](8184799))

### 🧑‍💻 Code Refactoring

* organize the import and export of SparseModel ([#141](#141)) ([2f982f1](2f982f1))

### 🧹 Chores

* Bump hf-hub to 0.4.1 ([#142](#142)) ([b5911b4](b5911b4))
@github-actions GitHub Actions
Copy link

🎉 This PR is included in version 4.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Anush008 Anush008 Anush008 approved these changes

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@honsunrise @Anush008