From 71d889a75781ddccf41be87e1d55bf94424630ee Mon Sep 17 00:00:00 2001 From: ilai Date: Mon, 13 Nov 2023 13:43:34 +0200 Subject: [PATCH 1/7] [cli] Bug fix - used `os.path.join()` instead of 'urljoin()' That created an error on Windows --- src/canopy_cli/cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/canopy_cli/cli.py b/src/canopy_cli/cli.py index 17f31fea..cf60af95 100644 --- a/src/canopy_cli/cli.py +++ b/src/canopy_cli/cli.py @@ -488,7 +488,7 @@ def chat(chat_server_url, rag, debug, stream): history=history_with_pinecone, message=message, stream=stream, - api_base=os.path.join(chat_server_url, "context"), + api_base=os.path.join(urljoin(url, "/context")), print_debug_info=debug, ) From 81c8fa75cd63b626d508439590db5edd8a4172aa Mon Sep 17 00:00:00 2001 From: ilai Date: Mon, 13 Nov 2023 13:43:56 +0200 Subject: [PATCH 2/7] [server] Support `canopy stop` on Windows Two bugs: 1. Trying to `pgrep gunicorn` on Windows, even though windows doesn't have `pgrep` and Gunicorn isn't supported on windows. 2. In windows we need to send the `CTRL_C` signal, which has a totally different implmentation behind it than GITINT --- src/canopy_cli/cli.py | 43 ++++++++++++++++++++-------------------- src/canopy_server/app.py | 3 ++- 2 files changed, 24 insertions(+), 22 deletions(-) diff --git a/src/canopy_cli/cli.py b/src/canopy_cli/cli.py index cf60af95..3a78fe82 100644 --- a/src/canopy_cli/cli.py +++ b/src/canopy_cli/cli.py @@ -577,28 +577,29 @@ def start(host: str, port: str, reload: bool, @click.option("url", "--url", default="http://0.0.0.0:8000", help="URL of the Canopy server to use. Defaults to http://0.0.0.0:8000") def stop(url): - # Check if the server was started using Gunicorn - res = subprocess.run(["pgrep", "-f", "gunicorn canopy_server.app:app"], - capture_output=True) - output = res.stdout.decode("utf-8").split() - - # If Gunicorn was used, kill all Gunicorn processes - if output: - msg = ("It seems that Canopy server was launched using Gunicorn.\n" - "Do you want to kill all Gunicorn processes?") - click.confirm(click.style(msg, fg="red"), abort=True) - try: - subprocess.run(["pkill", "-f", "gunicorn canopy_server.app:app"], - check=True) - except subprocess.CalledProcessError: + if os.name != "nt": + # Check if the server was started using Gunicorn + res = subprocess.run(["pgrep", "-f", "gunicorn canopy_server.app:app"], + capture_output=True) + output = res.stdout.decode("utf-8").split() + + # If Gunicorn was used, kill all Gunicorn processes + if output: + msg = ("It seems that Canopy server was launched using Gunicorn.\n" + "Do you want to kill all Gunicorn processes?") + click.confirm(click.style(msg, fg="red"), abort=True) try: - [os.kill(int(pid), signal.SIGINT) for pid in output] - except OSError: - msg = ( - "Could not kill Gunicorn processes. Please kill them manually." - f"Found process ids: {output}" - ) - raise CLIError(msg) + subprocess.run(["pkill", "-f", "gunicorn canopy_server.app:app"], + check=True) + except subprocess.CalledProcessError: + try: + [os.kill(int(pid), signal.SIGINT) for pid in output] + except OSError: + msg = ( + "Could not kill Gunicorn processes. Please kill them manually." + f"Found process ids: {output}" + ) + raise CLIError(msg) try: res = requests.get(urljoin(url, "/shutdown")) diff --git a/src/canopy_server/app.py b/src/canopy_server/app.py index 6c67494a..26b4b48a 100644 --- a/src/canopy_server/app.py +++ b/src/canopy_server/app.py @@ -255,7 +255,8 @@ async def shutdown() -> ShutdownResponse: status_code=500, detail="Failed to locate parent process. Cannot shutdown server.", ) - os.kill(pid, signal.SIGINT) + kill_signal = signal.CTRL_C_EVENT if os.name == 'nt' else signal.SIGINT + os.kill(pid, kill_signal) return ShutdownResponse() From b7131808b2a2e2fd09af053ac3d5d61142011048 Mon Sep 17 00:00:00 2001 From: ilai Date: Mon, 13 Nov 2023 13:50:52 +0200 Subject: [PATCH 3/7] [cli] Improve chat warnning message Since there is no Gunicorn on windows, it doesn't make sense to tell the users to run it --- src/canopy_cli/cli.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/canopy_cli/cli.py b/src/canopy_cli/cli.py index 3a78fe82..f2c575f0 100644 --- a/src/canopy_cli/cli.py +++ b/src/canopy_cli/cli.py @@ -541,12 +541,17 @@ def start(host: str, port: str, reload: bool, config: Optional[str], index_name: Optional[str]): note_msg = ( "🚨 Note 🚨\n" - "For debugging only. To run the Canopy server in production, run the command:" + "For debugging only. To run the Canopy server in production " + ) + msg_suffix = ( + "run the command:" "\n" "gunicorn canopy_server.app:app --worker-class uvicorn.workers.UvicornWorker " f"--bind {host}:{port} --workers " + ) if os.name != "nt" else ( + "please use Docker" # TODO: Add Docker instructions once we have a Dockerfile ) - for c in note_msg: + for c in note_msg + msg_suffix: click.echo(click.style(c, fg="red"), nl=False) time.sleep(0.01) click.echo() From dd07a76f295db14df067901c361a61810ffa1143 Mon Sep 17 00:00:00 2001 From: ilai Date: Mon, 13 Nov 2023 13:53:53 +0200 Subject: [PATCH 4/7] [cli] Bug fix - CLI couldn't open server routes on Windows The CLI used the url `http://0.0.0.0:8000` by default, which is not supported on windows. I replaced it with `http://localhost:8000`. --- src/canopy_cli/cli.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/canopy_cli/cli.py b/src/canopy_cli/cli.py index f2c575f0..ae9e7ddf 100644 --- a/src/canopy_cli/cli.py +++ b/src/canopy_cli/cli.py @@ -171,8 +171,8 @@ def cli(ctx): @cli.command(help="Check if canopy server is running and healthy.") -@click.option("--url", default="http://0.0.0.0:8000", - help="Canopy's server url. Defaults to http://0.0.0.0:8000") +@click.option("--url", default="http://localhost:8000", + help="Canopy's server url. Defaults to http://localhost:8000") def health(url): check_server_health(url) click.echo(click.style("Canopy server is healthy!", fg="green")) @@ -432,8 +432,8 @@ def _chat( help="Print additional debugging information") @click.option("--rag/--no-rag", default=True, help="Compare RAG-infused Chatbot with vanilla LLM",) -@click.option("--chat-server-url", default="http://0.0.0.0:8000", - help="URL of the Canopy server to use. Defaults to http://0.0.0.0:8000") +@click.option("--chat-server-url", default="http://localhost:8000", + help="URL of the Canopy server to use. Defaults to http://localhost:8000") def chat(chat_server_url, rag, debug, stream): check_server_health(chat_server_url) note_msg = ( @@ -579,8 +579,8 @@ def start(host: str, port: str, reload: bool, """ ) ) -@click.option("url", "--url", default="http://0.0.0.0:8000", - help="URL of the Canopy server to use. Defaults to http://0.0.0.0:8000") +@click.option("url", "--url", default="http://localhost:8000", + help="URL of the Canopy server to use. Defaults to http://localhost:8000") def stop(url): if os.name != "nt": # Check if the server was started using Gunicorn @@ -625,8 +625,8 @@ def stop(url): """ ) ) -@click.option("--url", default="http://0.0.0.0:8000", - help="Canopy's server url. Defaults to http://0.0.0.0:8000") +@click.option("--url", default="http://localhost:8000", + help="Canopy's server url. Defaults to http://localhost:8000") def api_docs(url): import webbrowser From feac58b3f8d32e090c96b9a836acf80f0715157b Mon Sep 17 00:00:00 2001 From: ilai Date: Mon, 13 Nov 2023 14:32:05 +0200 Subject: [PATCH 5/7] Don't use tenerary if, so mypy will be happy What can you do... --- src/canopy_cli/cli.py | 4 ++-- src/canopy_server/app.py | 5 ++++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/canopy_cli/cli.py b/src/canopy_cli/cli.py index ae9e7ddf..79a08cfb 100644 --- a/src/canopy_cli/cli.py +++ b/src/canopy_cli/cli.py @@ -488,7 +488,7 @@ def chat(chat_server_url, rag, debug, stream): history=history_with_pinecone, message=message, stream=stream, - api_base=os.path.join(urljoin(url, "/context")), + api_base=os.path.join(urljoin(chat_server_url, "/context")), print_debug_info=debug, ) @@ -549,7 +549,7 @@ def start(host: str, port: str, reload: bool, "gunicorn canopy_server.app:app --worker-class uvicorn.workers.UvicornWorker " f"--bind {host}:{port} --workers " ) if os.name != "nt" else ( - "please use Docker" # TODO: Add Docker instructions once we have a Dockerfile + "please use Docker" # TODO: Add Docker instructions once we have a Dockerfile ) for c in note_msg + msg_suffix: click.echo(click.style(c, fg="red"), nl=False) diff --git a/src/canopy_server/app.py b/src/canopy_server/app.py index 26b4b48a..490dca55 100644 --- a/src/canopy_server/app.py +++ b/src/canopy_server/app.py @@ -255,7 +255,10 @@ async def shutdown() -> ShutdownResponse: status_code=500, detail="Failed to locate parent process. Cannot shutdown server.", ) - kill_signal = signal.CTRL_C_EVENT if os.name == 'nt' else signal.SIGINT + if sys.platform == 'win32': + kill_signal = signal.CTRL_C_EVENT + else: + kill_signal = signal.SIGINT os.kill(pid, kill_signal) return ShutdownResponse() From 602a485931c8eb1c097194b349967efc2787a308 Mon Sep 17 00:00:00 2001 From: ilai Date: Mon, 13 Nov 2023 23:08:19 +0200 Subject: [PATCH 6/7] [cli] Bug fix - still using os.path.join() for url Caught in PR #170 review. --- src/canopy_cli/cli.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/canopy_cli/cli.py b/src/canopy_cli/cli.py index 79a08cfb..59022e18 100644 --- a/src/canopy_cli/cli.py +++ b/src/canopy_cli/cli.py @@ -488,7 +488,7 @@ def chat(chat_server_url, rag, debug, stream): history=history_with_pinecone, message=message, stream=stream, - api_base=os.path.join(urljoin(chat_server_url, "/context")), + api_base=urljoin(chat_server_url, "/context"), print_debug_info=debug, ) @@ -549,7 +549,7 @@ def start(host: str, port: str, reload: bool, "gunicorn canopy_server.app:app --worker-class uvicorn.workers.UvicornWorker " f"--bind {host}:{port} --workers " ) if os.name != "nt" else ( - "please use Docker" # TODO: Add Docker instructions once we have a Dockerfile + "please use Docker with a Gunicorn server." # TODO: Add Docker instructions once we have a Dockerfile ) for c in note_msg + msg_suffix: click.echo(click.style(c, fg="red"), nl=False) From dbff3875e10382b7a202a5a52d42bfa42522b6a6 Mon Sep 17 00:00:00 2001 From: ilai Date: Tue, 14 Nov 2023 10:09:51 +0200 Subject: [PATCH 7/7] linter --- src/canopy_cli/cli.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/canopy_cli/cli.py b/src/canopy_cli/cli.py index 59022e18..5699af60 100644 --- a/src/canopy_cli/cli.py +++ b/src/canopy_cli/cli.py @@ -549,7 +549,8 @@ def start(host: str, port: str, reload: bool, "gunicorn canopy_server.app:app --worker-class uvicorn.workers.UvicornWorker " f"--bind {host}:{port} --workers " ) if os.name != "nt" else ( - "please use Docker with a Gunicorn server." # TODO: Add Docker instructions once we have a Dockerfile + # TODO: Replace with proper instructions once we have a Dockerfile + "please use Docker with a Gunicorn server." ) for c in note_msg + msg_suffix: click.echo(click.style(c, fg="red"), nl=False)