-
Notifications
You must be signed in to change notification settings - Fork 36
feat(runtime): update to v1 API for leanvec ood #262
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
Conversation
| virtual ~LeanVecTrainingData(); | ||
|
|
||
| /* Build LeanVec training data (compression matrices) from the provided | ||
| * data. |
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.
Should we call "Leanvec transformation matrices" instead of training data?
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.
LeanVecTrainingData was purposefully chosen to be generic. In hindsight, I think we could have been more specific. But changing this would require an API update and therefore conflicts with your suggestion in the other comment.
| size_t n, | ||
| const float* x, | ||
| size_t n_train, | ||
| const float* q, |
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.
Can we keep these two new arguments in the end and default initialize to 0 and nullptr? In that case, the older v0 calls to build will still work without any modifications. Also "n_train" is kind of confusing as both data/queries are used for training? How about we explicitly say "n_queries"?
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.
That's exactly the discussion I wanted to have. Your suggestion would indeed allow us to stick with v0 (only that we'd need a copy of the function and we can't use default values, due to ABI compability, but that's just a detail).
In my opinion, the order (n_data, const float* data, n_query, const float* queries, size_t leanvec_dims) just makes more sense than (n_data, const float* data, size_t leanvec_dims, n_query, const float* queries). But does it justify bumping to v0?
Your preference would be to stick with v0 for as long as possible?
Increments runtime API version to
v1, becauseLeanVecTrainingData::build()changed significantly for OOD suport.Unchanged entities from
v0are aliased inv1.Successful runtime lib build pending changes from svs prebuilt binaries.