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

Relax sentencepiece version #74

Merged
merged 3 commits into from
Mar 1, 2024
Merged

Relax sentencepiece version #74

merged 3 commits into from
Mar 1, 2024

Conversation

lewtun
Copy link
Member

@lewtun lewtun commented Feb 28, 2024

This PR puts a lower bound on the sentencepiece version to be minimally 0.1.99 so lighteval can be more easily pip installed in other environments that also have transformers installed. If this is OK, I will do something similar with other deps like pytest which ideally shouldn't be pinned (at most upper bounded)

@clefourrier
Copy link
Member

Don't you mean at most lower bounded?

@lewtun
Copy link
Member Author

lewtun commented Feb 28, 2024

Don't you mean at most lower bounded?

Ah for things like pytest where one doesn't expect breaking changes across minor versions, it can be helpful to put an upper bound on the next major version. But for sentencepiece I used just a lower bound since I'm not sure if they use proper semantic versioning.

Btw if we don't use sentencepiece anywhere outside of transformers, a better approach would be to link it to the transformers dependency via transformers[sentencepiece]

Happy to amend this PR like that if you agree!

@clefourrier
Copy link
Member

Thanks for the explanation, makes sense!
I think @NathanHB should take the decision on this, as it's likely he'll become the lead maintainer of lighteval in the next few months.
And I agree on making sentencepiece a dep of transformers, cool idea!

@NathanHB
Copy link
Member

NathanHB commented Feb 28, 2024

Btw if we don't use sentencepiece anywhere outside of transformers, a better approach would be to link it to the transformers dependency via transformers[sentencepiece]

This sounds good ! I see no reason for not doing that :)

@clefourrier
Copy link
Member

@NathanHB what about using requirement bounds instead of fixed version? :)

@NathanHB
Copy link
Member

I tend to prefer using fixed versions, as I had issues with breaking changes more than once. However, using bounds for minor version seems better for compatibility with other lib. So Lewis proposition is good for me :)

@clefourrier clefourrier requested a review from NathanHB February 28, 2024 18:19
@NathanHB NathanHB merged commit b9d0277 into main Mar 1, 2024
2 checks passed
@lewtun lewtun deleted the lewtun-patch-1 branch March 2, 2024 10:50
@farzadab
Copy link

farzadab commented Jul 4, 2024

Maybe a dumb question, but where is sentencepiece even used? I don't see it imported directly anywhere in the code.

@clefourrier
Copy link
Member

clefourrier commented Jul 5, 2024

If I remember properly, we had to fix the lower bound of the sentencepiece version because it will be needed/used when loading some tokenizers and prior versions had issues with our codebase.

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.

4 participants