-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[CHORE] Remove validation on ef config update for model path #4571
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
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
Please tag your PR title with one of: [ENH | BUG | DOC | TST | BLD | PERF | TYP | CLN | CHORE]. See https://docs.trychroma.com/contributing#contributing-code-and-ideas |
258ea35
to
d7a22a9
Compare
This PR removes the validation that previously prevented users from updating the 'model_name' (used as model path) in the configuration of instructor, sentence transformer, and text2vec embedding functions. Now, users can change the model path after initialization, enabling scenarios such as updating the path to a local or upgraded model file. This summary was automatically generated by @propel-code-bot |
What is an example of a valid use here? |
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.
Would be good to comment a tangible example
came from a need of a user I can see a need where users move the paths of their models around, maybe for cleanup, maybe because they are updating apis from v1 to v2, and so need to update the file path for that. Will add a comment in the code too |
d7a22a9
to
08a1047
Compare
Description of changes
Some embedding functions allow the user to pass a path for their embedding function. We should not be validating that the user is trying to update the model path, as that could be too restrictive.
Test plan
How are these changes tested?
pytest
for python,yarn test
for js,cargo test
for rustDocumentation Changes
Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs section?