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 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
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.
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.
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.
@izellevy approved, but please see one comment about the test
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
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