[ENH] When embedding functions have defined default_space, use them if the user hasn't specified #4321
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
jairad26
force-pushed
the
jai/set-default-space-by-ef
branch
2 times, most recently
from
a131515 to
43a52d6
Compare
jairad26
force-pushed
the
jai/set-default-space-by-ef
branch
4 times, most recently
from
0b1b9ee to
d3baad8
Compare
…f the user hasn't specified
jairad26
force-pushed
the
jai/set-default-space-by-ef
branch
from
d3baad8 to
30d251e
Compare
| @@ -377,6 +377,24 @@ def create_collection_configuration_to_json( | |||
| "config": ef.get_config(), | |||
| } | |||
| register_embedding_function(type(ef)) # type: ignore | |||
| if hnsw_config is not None and hnsw_config.get("space") is None: | |||
| try: | |||
| hnsw_config["space"] = ef.default_space() | |||
There was a problem hiding this comment.
is there not some base impl that will always be used? I am bit a confused by this pattern. You could also check if its the base impl.
If default_space is not specified, what is the behavior we want?
There was a problem hiding this comment.
right, the base impl is l2, which is the space we define in our docs as the default. so when available, it’ll pull from the ef’s default otherwise the base one
There was a problem hiding this comment.
and if they’re running on an old version or are using a legacy custom ef (which won't have the base impl), the try will fail and issue a warning
Description of changes
This PR uses the default space defined for an embedding function if available, closes #2128
Test plan
How are these changes tested?
pytestfor python,yarn testfor js,cargo testfor rustDocumentation Changes
Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs repository?