-
-
Notifications
You must be signed in to change notification settings - Fork 6.9k
[CI][Bugfix] Fix bad tolerance for test_batch_base64_embedding #16221
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
[CI][Bugfix] Fix bad tolerance for test_batch_base64_embedding #16221
Conversation
Signed-off-by: mgoin <[email protected]>
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add 🚀 |
Signed-off-by: mgoin <[email protected]>
There was a problem hiding this 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 fix 🙏 !
Seems that the default tolerance isn't quite enough. Does this issue appear when we use V0? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I think embedding doesn't work on V1 yet, so this should be V0 |
Signed-off-by: mgoin <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I faced the same problem in my PR #16188
I used the check_embeddings_close
function from models.embedding.utils
. That function uses cosine similarity, but I think allclose should be fine as well.
Signed-off-by: mgoin <[email protected]>
…project#16221) Signed-off-by: mgoin <[email protected]>
…project#16221) Signed-off-by: mgoin <[email protected]>
Previously the test was doing equality on float values, which is always a bad idea. See an example of flaky test https://buildkite.com/vllm/fastcheck/builds/19776/steps?jid=0196126c-0cdf-4b29-9363-d925adaf681a#0196126c-0cdf-4b29-9363-d925adaf681a/6-13621
This PR changes it to use allclose