Enable semantic router reference updates #322
Conversation
rbs333
force-pushed
the
feat/RAAE-337/router-reference-crud
branch
from
56c9168 to
5f69f3b
Compare
| if reference_ids: | ||
| queries = cls._make_filter_queries(reference_ids) | ||
| else: | ||
| _, keys = index.client.scan( |
There was a problem hiding this comment.
Open to suggestions if there is a better way to get all keys within a prefix.
| @@ -41,13 +41,14 @@ def routes(): | |||
| @pytest.fixture | |||
| def semantic_router(client, routes): | |||
| router = SemanticRouter( | |||
| name="test-router", | |||
| name=f"test-router-{str(ULID())}", | |||
There was a problem hiding this comment.
Adding this here to help prevent collisions within test execution.
Generally to improve our test consistency I think it would be good practice to make all function level fixtures (the default) as idempotent as possible. Given we execute tests with multiple workers in an async fashion that might help.
tylerhutcherson
changed the title
| reference_vectors = vectorizer.embed_many(references, as_buffer=True) | ||
|
|
||
| # set route references | ||
| for i, reference in enumerate(references): |
There was a problem hiding this comment.
isn't this implementing the same or similar logic as _add_routes(...)?
There was a problem hiding this comment.
Yes. The gotcha here is the route reference one takes advantage of self. and this one is based on cls. I decided against "functionizing" since there wasn't a 3rd case. This is an example of where the code becomes cleaner if we drop the class method idea (like we had talked about at one point).
| # reference methods | ||
|
|
||
| @classmethod | ||
| def add_route_references( |
There was a problem hiding this comment.
Only question is an ergonomic one.. what would be the benefit of using add_route_references(...) on a non-instantiated router class, wherein you will still end up passing through the args here to essentially init the class? Isn't it effectively the same as calling router = SemanticRouter(...); router.add_route_references(...)?
I know this is/was one of the tricky and sticky points, so just curious to hear what you're thinking! :)
There was a problem hiding this comment.
The key difference here is that to you don't need to pass a list of routes when using the class method. You would have to provide the router name and redis_url but theoretically if you were unaware of the initial routes it wouldn't matter. I think there is some value in not having to know the previous routes.
That said, I'm open to removing because it makes the code more direct. A better way to handle could be to relax the requirement on passing routes in the init.
There was a problem hiding this comment.
Ahh yeah, I see what you mean!
Core issue is that class init is pretty rigid and expects certain inputs. It was probably a gap in the initial interface design for sure on my part.
This pr add methods so that you can easily get/add/delete route references.