Skip to content
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

feat: Support for setting top_k for HybridRouteLayer #197

Merged
merged 9 commits into from
Mar 14, 2024

Conversation

andreped
Copy link
Contributor

@andreped andreped commented Mar 13, 2024

fixes #198.

Also did the following:

  • Added unit test for top_k and alpha for the HybridRouteLayer
  • Updated CONTRIBUTIONS.md to include all-extras during installation setup relevant for unit tests
  • Performed linting (following the CONTRIUBTIONS.md)

@andreped
Copy link
Contributor Author

andreped commented Mar 13, 2024

I have tested the solution in one of my projects, and seen a change in performance.
To use this new feature, simply do:

from semantic_router.encoders import AzureOpenAIEncoder, TfidfEncoder
from semantic_router.hybrid_layer import HybridRouteLayer

model = HybridRouteLayer(
    encoder=AzureOpenAIEncoder(...),
    sparse_encoder=TfidfEncoder(),
    routes=routes,
    alpha=0.3,  # default = 0.3
    top_k=3,  # default = 5
)

@jamescalam jamescalam self-requested a review March 14, 2024 03:46
@jamescalam
Copy link
Member

hi @andreped — this looks great, we have not focused on the HybridRouteLayer so far so I'm curious as to whether you're seeing improved performance when using it? In any case the hybrid approach should in theory unlock better performance so it's something we'd like to spend more time on soon

Will complete the review once tests have finished running

Copy link

codecov bot commented Mar 14, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 77.72%. Comparing base (1e753d9) to head (f0ffac5).

Files Patch % Lines
semantic_router/hybrid_layer.py 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #197      +/-   ##
==========================================
- Coverage   77.74%   77.72%   -0.02%     
==========================================
  Files          42       42              
  Lines        2089     2092       +3     
==========================================
+ Hits         1624     1626       +2     
- Misses        465      466       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@andreped
Copy link
Contributor Author

andreped commented Mar 14, 2024

Hello, @jamescalam! Hybrid search stuff has worked great for me t in the past, so I would think that the HybridRouteLayer would offer similar benefits.

By using the base HybridRouteLayer without any modifications, I achieved 4% higher macro F1-score on our application. I further gained a few percentages by lowering topk to 2 or 3 from 5. Runtime was twice as slow with Hybrid compared to without, but with a runtime per prompt of 0.16s this was fine. Now I basically just need to refine the utterances which were labelled wrongly occasionally or were suboptimal to describe some of the routes.

I will be exploring it more today, and may make a new PR, if I find one technique to work better for my application. For instance, changing the aggregation method from SUM to MEAN or MAX, would also be of interest. I can make an issue and PR about this separately :]

Copy link
Member

@jamescalam jamescalam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@jamescalam
Copy link
Member

hey @andreped, that's awesome — yes I think the HybridRouteLayer could definitely do with some optimization, feel free to submit changes it would help a lot! For now I'm merging this, thanks for the PR :)

@jamescalam jamescalam merged commit 19cf5d2 into aurelio-labs:main Mar 14, 2024
8 checks passed
@andreped
Copy link
Contributor Author

andreped commented Mar 14, 2024

hey @andreped, that's awesome — yes I think the HybridRouteLayer could definitely do with some optimization, feel free to submit changes it would help a lot! For now I'm merging this, thanks for the PR :)

Made an issue #201 and I will draft a PR very soon.


EDIT: 🚀 PR #202 is ready to review now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for setting top_k for HybridRouteLayer?
2 participants