-
Notifications
You must be signed in to change notification settings - Fork 105
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
Add support for namespaces #243
Conversation
Add namespace optional path param to /chat/completion
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See a few suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more comments
There was a syntax error in our `urljoin()` call, which caused for the `/v1/` prefix being dropped. Up until now we also supported bare prefix without `/v1/`, so it worked.
Still urljoin() problems
…to feature/add_namespace
This simplifies the code a bit, and also allows nicer documentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see a few more comments
src/canopy_cli/cli.py
Outdated
@@ -488,7 +499,11 @@ def _chat( | |||
@click.option("--chat-server-url", default=DEFAULT_SERVER_URL, | |||
help=("URL of the Canopy server to use." | |||
f" Defaults to {DEFAULT_SERVER_URL}")) | |||
def chat(chat_server_url, rag, debug, stream): | |||
@click.option("--namespace", "-n", default=None, envvar="INDEX_NAMESPACE", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit shooting ourselves in the leg, isn't it?
The point of using an API prefix is to tell users "to use namespaces, you simply change the base_url
". If we're giving users a dedicated --namespace
param - they might miss message.
Maybe we should give clear examples in the documentation (App API doc, CLI help messages, README) of using namespace by changing the API route?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, maybe it is a bit weird to enforce a namespace let's keep it dynamic. I am updating the docs also
Refactor the api route prefix mechanism
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@izellevy approved, but please see one comment about the test
Problem
We don't have a chat endpoint for specific namespaces of an index.
Solution
Added namespace support to ChatEngine, ContextEngine & KnowledgeBase
Dropped support for endpoints not containing v1 urls.
Type of Change
Test Plan
Added relevant tests.
Closes #219