Deprecate global distance_threshold with semantic router #275
Conversation
rbs333
commented
Feb 11, 2025
rbs333
commented
- change ownership of distance_threshold
- move filtering to happen as part of db call
- make code more DRY
- rename user_guide because it's really been bothering me
rbs333
requested review from
abrookins,
tylerhutcherson and
justin-cechmanek
rbs333
changed the title
rbs333
force-pushed
the
feat/RAAE-615/dist-thresh-ownership
branch
from
84bdaee to
e565a57
Compare
| @@ -16,7 +16,7 @@ class Route(BaseModel): | |||
| """List of reference phrases for the route.""" | |||
| metadata: Dict[str, str] = Field(default={}) | |||
| """Metadata associated with the route.""" | |||
| distance_threshold: Optional[float] = Field(default=None) | |||
| distance_threshold: float = Field(default=0.5) | |||
There was a problem hiding this comment.
I saw you deleted another validator, so maybe this isn't important, but with Pydantic 2, you can validate that a number is within a range using Annotated:
from typing_extensions import Annotated
from pydantic.v1 import BaseModel, Field, ValidationError
class Foo(BaseModel):
bar: Annotated[float, Field(strict=True, gt=0, lte=1)] # or gt, lt
for bad_number in (-1, 2):
try:
Foo(bar=bad_number)
except ValidationError:
print("worked")
else:
raise RuntimeError("Didn't work!")
print(Foo(bar=0.5))
This should still work with a v1 BaseModel.
There was a problem hiding this comment.
Yeah I deleted a validator for the now depreciated field since it has no impact. I think we should update to the new style of validators but I think that should be included in the pr for pydantic versioning stuff because it fits more with that theme: https://redislabs.atlassian.net/browse/RAAE-607
| """The maximum number of top matches to return.""" | ||
| distance_threshold: float = Field( | ||
| default=0.5, | ||
| deprecated=True, |
There was a problem hiding this comment.
On a v1 BaseModel, this only adds "deprecated" to the JSON schema for this field, since what's really happening is any extra keyword args pass through to the schema. That's not ideal, but I'm ok with it. In v2, this would log a deprecation warning if a user used the field. We can leave it in place like this and then if it's still around when we cut over to using v2 BaseModel, it'll work better.
| ) -> List[RouteMatch]: | ||
| """Get the route matches for a given vector and aggregation method.""" | ||
|
|
||
| thresholds: List[float] = [route.distance_threshold for route in self.routes] |
There was a problem hiding this comment.
Why the variable type annotation here? Did mypy not pick up that route.distance_threshold is a float?
There was a problem hiding this comment.
Good catch it failed mypy originally because the pydantic model had it as optional so I update there but forgot to delete this. Pushed a fix
rbs333
dismissed stale reviews from abrookins and tylerhutcherson
via
fa7cf0b
