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

[128] - Toggle /voice-search endpoint on/off #436

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

MustafaAkolawala
Copy link
Collaborator

Reviewer: @suzinyou
Estimate: 10 min


Ticket

Fixes: JIRA_TICKET_LINK

Description

This PR implements a feature to toggle the /voice-search endpoint on or off based on the TOGGLE_VOICE environment variable.

Goal

To enhance endpoint design and user experience by making the /voice-search endpoint optional. This change also improves code readability and facilitates easier integration of external and custom models.

Changes

  • Added TOGGLE_VOICE environment variable to control /voice-search endpoint availability

How has this been tested?

  • Verified functionality using docker-compose
  • Checked endpoint visibility in Swagger UI
  • Updated and ran unit tests

How to test this?

  1. Set or unset the TOGGLE_VOICE environment variable
  2. Observe changes in Swagger UI and endpoint accessibility

Checklist

Fill with x for completed.

  • My code follows the style guidelines of this project
  • I have reviewed my own code to ensure good quality
  • I have tested the functionality of my code to ensure it works as intended
  • I have resolved merge conflicts
  • I have updated the automated tests
  • I have updated the CI/CD scripts in .github/workflows/

@@ -34,6 +34,11 @@ LITELLM_ENDPOINT="http://localhost:4000"
#PGVECTOR_VECTOR_SIZE=1024

#### Speech APIs ###############################################################
# Set this variable to 'external' or 'custom' accordingly to toggle the /voice-search endpoint
# By default it is not set so it defaults to None
# TOGGLE_VOICE=external
Copy link
Collaborator

@amiraliemami amiraliemami Sep 17, 2024

Choose a reason for hiding this comment

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

change to boolean ENABLE_VOICE_SEARCH for clarity? We only use it as a boolean check anyway. I'd suggest changing the description to:

# This variable controls whether the voice search endpoint is active (set to true) or inactive (set to false). Default is false.

# If enabled, we default to using external services unless `CUSTOM_SPEECH_ENDPOINT` is set, in which case the custom hosted APIs will be used.

@amiraliemami amiraliemami self-requested a review September 17, 2024 09:42
Copy link
Collaborator

@amiraliemami amiraliemami left a comment

Choose a reason for hiding this comment

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

Thanks for this - exactly what was needed. Just a comment on what the variable should be called and that it should be a boolean for clarity. Also update the docs with this new info, thanks!

# If enabled, we default to using external services unless `CUSTOM_SPEECH_ENDPOINT` is set, in which case the custom hosted APIs will be used.
# ENABLE_VOICE_SEARCH=True

# if TOGGLE_VOICE is set to 'Custom' then make sure to also set the Environment variables mentioned below
Copy link
Contributor

Choose a reason for hiding this comment

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

What is TOGGLE_VOICE, I don't see anywhere else. Probably that's what turned into ENABLE_VOICE_SEARCH

Copy link
Contributor

Choose a reason for hiding this comment

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

You should probably change that in the docs. I saw it in docs/components/voice-service/inded.md

Copy link
Contributor

@lickem22 lickem22 left a comment

Choose a reason for hiding this comment

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

Hello Mustafa, I approved, just a minor change to make.

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.

3 participants