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

Add ST annotation to evaluators #2586

Merged
merged 2 commits into from
Apr 11, 2024
Merged

Add ST annotation to evaluators #2586

merged 2 commits into from
Apr 11, 2024

Conversation

kddubey
Copy link
Contributor

@kddubey kddubey commented Apr 10, 2024

Hello,

Small follow-up. There isn't a circular import (in fact there's already an import in main).

Another small thing is that the imports aren't in isort-like order. In case you want to apply that order, run—

ruff check sentence_transformers/evaluation --fix --select I

—and add extend-select = ["I"] to ruff.toml.

But I'm not sure if you want this style enforced. I'll leave that up to you to commit :-)

@kddubey
Copy link
Contributor Author

kddubey commented Apr 11, 2024

Resolving the conflicts now

@tomaarsen
Copy link
Collaborator

Thank you!

@kddubey
Copy link
Contributor Author

kddubey commented Apr 11, 2024

Resolved

@tomaarsen tomaarsen merged commit d105ec8 into UKPLab:master Apr 11, 2024
9 checks passed
@kddubey kddubey deleted the evaluators-model-type-annotation branch April 11, 2024 07:50
@tomaarsen
Copy link
Collaborator

Thanks a bunch! PR looks good :)

  • Tom Aarsen

@tomaarsen
Copy link
Collaborator

As for ruff check sentence_transformers/evaluation --fix --select I - yes, I think in the long run we do want to do proper import sorting. However, I think it's likely to result in a lot of merge conflicts, so I'd like to hold off on until most PRs for the v3 release are merged.

  • Tom Aarsen

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.

2 participants