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

Fix re-rankings payload schema for NIMs #220

Merged
merged 2 commits into from
Dec 16, 2024
Merged

Conversation

dyastremsky
Copy link
Contributor

@dyastremsky dyastremsky commented Dec 16, 2024

During the inputs modularization work, the payload for benchmarking re-ranking NIMs was broken. Fix the payload schema.

After changes:
image

@dyastremsky dyastremsky self-assigned this Dec 16, 2024
@dyastremsky dyastremsky marked this pull request as ready for review December 16, 2024 22:05
@dyastremsky dyastremsky changed the title Fix re-rankings payload for benchmarking NIM Fix re-rankings payload schema for NIMs Dec 16, 2024
Copy link
Contributor

@nv-hwoo nv-hwoo left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix! 🚀

Copy link
Contributor

@nv-braf nv-braf left a comment

Choose a reason for hiding this comment

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

This looks fine. I like that there is unit testing for this, but I'm curious as to who is the "owner" of the formatting for this? How did you discover it was broken (as the unit test would not have failed)?
Is this something that should be tested live (L0)?

@dyastremsky
Copy link
Contributor Author

This looks fine. I like that there is unit testing for this, but I'm curious as to who is the "owner" of the formatting for this? How did you discover it was broken (as the unit test would not have failed)? Is this something that should be tested live (L0)?

We had a customer report this as broken. We 100% do need testing for this.

In regards to CI: TPA-509 tracks adding a HuggingFace TEI CI test, which had dependency issues and was deprioritized. The testing you're describing is something that we have outlined as essential for GA: adding CI testing for customers (e.g. CI for NIMs). We do not currently test NIMs in CI.

This unit test breaking should have been identified and noticed sooner. We were racing to get the modularization out, which required a lot of unit testing updates and this failure got lost among them. I was testing against HuggingFace TEI at that point and had not realized that the ranking NIM folks were using a pinned version of GAP for testing, which is why they didn't see or report this failure immediately.

@dyastremsky dyastremsky merged commit bc5220e into main Dec 16, 2024
6 of 7 checks passed
@dyastremsky dyastremsky deleted the dyas-rankings-payload-fix branch December 16, 2024 22:39
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.

3 participants