Skip to content

Consider vectorizer the owner of dtype #267

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

Merged
merged 6 commits into from
Jan 31, 2025
Merged

Conversation

abrookins
Copy link
Collaborator

@abrookins abrookins commented Jan 29, 2025

The dtype argument to SemanticCache, SemanticRouter, and SemanticSessionManager is intended to specify the vectorizer's data type. However, the constructors for these classes allow passing in both a vectorizer instance (with dtype already set) and dtype, in which case, the vectorizer may have a different dtype than the dtype used in the index schema.

This PR works on eliminating this possibility by adding validation and treating the vectorizer as the true "owner" of dtype. We also begin the deprecation process for standalone dtype arguments, guiding users to pass in vectorizer instances instead if they require customizing the vectorizer's dtype. Finally, we make vectorizer validation consistent across these classes.

@tylerhutcherson tylerhutcherson marked this pull request as ready for review January 30, 2025 20:24
@tylerhutcherson
Copy link
Collaborator

@abrookins looks like we'd still need to do a similar treatment to the SemanticSessionManager class :)

@abrookins
Copy link
Collaborator Author

@abrookins looks like we'd still need to do a similar treatment to the SemanticSessionManager class :)

Ah, yes! Thank you, I'll get on that.

@tylerhutcherson tylerhutcherson self-requested a review January 31, 2025 14:31
@tylerhutcherson tylerhutcherson merged commit e96d739 into main Jan 31, 2025
20 checks passed
@tylerhutcherson tylerhutcherson deleted the RAE-568/dtype-tweaks branch January 31, 2025 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants