From a4b58d9749fd5a3920a5d0aa4bd3ff0b0bd1a642 Mon Sep 17 00:00:00 2001 From: Eric Gustin <34000337+EricGustin@users.noreply.github.com> Date: Thu, 19 Dec 2024 10:13:58 -0800 Subject: [PATCH] Fix `/show` for Cloud Engine (#177) # PR Description ### The following bug was observed: * When connected to the cloud engine for `arcade chat`, and the user types `/show`, then the local environment tools are displayed. Instead, the cloud engine's tools should be displayed. ### Why was this bug happening?: * When a user entered the `/show` command, the CLI Command `show` was being called directly. Since the function was a CLI command, the `local` parameter was not being processed and resolved to its intended value because the Typer CLI interface was being bypassed. So, the conditional `if local:` would always evaluate to `True`. ### How this was fixed: * I created a wrapper function for the `show` CLI Command. Now, when the user types `/show`, then the wrapper function is called instead of the `show` CLI command. This ensures that all input parameters are resolved to their intended values. --- arcade/arcade/cli/main.py | 42 ++++----------------------- arcade/arcade/cli/show.py | 54 +++++++++++++++++++++++++++++++++++ arcade/arcade/cli/utils.py | 1 + arcade/tests/cli/test_show.py | 40 ++++++++++++++++++++++++++ 4 files changed, 100 insertions(+), 37 deletions(-) create mode 100644 arcade/arcade/cli/show.py create mode 100644 arcade/tests/cli/test_show.py diff --git a/arcade/arcade/cli/main.py b/arcade/arcade/cli/main.py index 885c66f8..812fb4cf 100644 --- a/arcade/arcade/cli/main.py +++ b/arcade/arcade/cli/main.py @@ -18,19 +18,16 @@ from arcade.cli.constants import DEFAULT_CLOUD_HOST, DEFAULT_ENGINE_HOST, LOCALH from arcade.cli.display import ( display_arcade_chat_header, display_eval_results, - display_tool_details, display_tool_messages, - display_tools_table, ) from arcade.cli.launcher import start_servers +from arcade.cli.show import show_logic from arcade.cli.utils import ( OrderCommands, compute_engine_base_url, compute_login_url, - create_cli_catalog, delete_deprecated_config_file, get_eval_files, - get_tools_from_engine, get_user_input, handle_chat_interaction, handle_tool_authorization, @@ -177,38 +174,7 @@ def show( """ Show the available toolkits or detailed information about a specific tool. """ - try: - if local: - catalog = create_cli_catalog(toolkit=toolkit) - tools = [t.definition for t in list(catalog)] - else: - tools = get_tools_from_engine(host, port, force_tls, force_no_tls, toolkit) - - if tool: - # Display detailed information for the specified tool - tool_def = next( - ( - t - for t in tools - if t.get_fully_qualified_name().name.lower() == tool.lower() - or str(t.get_fully_qualified_name()).lower() == tool.lower() - ), - None, - ) - if not tool_def: - console.print(f"❌ Tool '{tool}' not found.", style="bold red") - typer.Exit(code=1) - else: - display_tool_details(tool_def) - else: - # Display the list of tools as a table - display_tools_table(tools) - - except Exception as e: - if debug: - raise - error_message = f"❌ Failed to list tools: {escape(str(e))}" - console.print(error_message, style="bold red") + show_logic(toolkit, tool, host, local, port, force_tls, force_no_tls, debug) @cli.command(help="Start Arcade Chat in the terminal", rich_help_panel="Launch") @@ -282,7 +248,9 @@ def chat( # Add the input to history readline.add_history(user_input) - if handle_user_command(user_input, history, host, port, force_tls, force_no_tls, show): + if handle_user_command( + user_input, history, host, port, force_tls, force_no_tls, show_logic + ): continue history.append({"role": "user", "content": user_input}) diff --git a/arcade/arcade/cli/show.py b/arcade/arcade/cli/show.py new file mode 100644 index 00000000..2f3331a8 --- /dev/null +++ b/arcade/arcade/cli/show.py @@ -0,0 +1,54 @@ +import typer +from rich.console import Console +from rich.markup import escape + +from arcade.cli.display import display_tool_details, display_tools_table +from arcade.cli.utils import create_cli_catalog, get_tools_from_engine + + +def show_logic( + toolkit: str | None, + tool: str | None, + host: str, + local: bool, + port: int | None, + force_tls: bool, + force_no_tls: bool, + debug: bool, +) -> None: + """Wrapper function for the `arcade show` CLI command + Handles the logic for showing tools/toolkits. + """ + console = Console() + try: + if local: + catalog = create_cli_catalog(toolkit=toolkit) + tools = [t.definition for t in list(catalog)] + else: + tools = get_tools_from_engine(host, port, force_tls, force_no_tls, toolkit) + + if tool: + # Display detailed information for the specified tool + tool_def = next( + ( + t + for t in tools + if t.get_fully_qualified_name().name.lower() == tool.lower() + or str(t.get_fully_qualified_name()).lower() == tool.lower() + ), + None, + ) + if not tool_def: + console.print(f"❌ Tool '{tool}' not found.", style="bold red") + typer.Exit(code=1) + else: + display_tool_details(tool_def) + else: + # Display the list of tools as a table + display_tools_table(tools) + + except Exception as e: + if debug: + raise + error_message = f"❌ Failed to list tools: {escape(str(e))}" + console.print(error_message, style="bold red") diff --git a/arcade/arcade/cli/utils.py b/arcade/arcade/cli/utils.py index 6ea4c666..6bc8aa17 100644 --- a/arcade/arcade/cli/utils.py +++ b/arcade/arcade/cli/utils.py @@ -671,6 +671,7 @@ def handle_user_command( toolkit=None, tool=None, host=host, + local=False, port=port, force_tls=force_tls, force_no_tls=force_no_tls, diff --git a/arcade/tests/cli/test_show.py b/arcade/tests/cli/test_show.py new file mode 100644 index 00000000..ee0848b0 --- /dev/null +++ b/arcade/tests/cli/test_show.py @@ -0,0 +1,40 @@ +from unittest.mock import patch + +from arcade.cli.show import show_logic + + +def test_show_logic_local_false(): + with patch("arcade.cli.show.get_tools_from_engine") as mock_get_tools: + mock_get_tools.return_value = [] + show_logic( + toolkit=None, + tool=None, + host="localhost", + local=False, + port=None, + force_tls=False, + force_no_tls=False, + debug=False, + ) + + # get_tools_from_engine should be called when local=False + mock_get_tools.assert_called_once() + + +def test_show_logic_local_true(): + with patch("arcade.cli.show.create_cli_catalog") as mock_create_catalog: + mock_create_catalog.return_value = [] + + show_logic( + toolkit=None, + tool=None, + host="localhost", + local=True, + port=None, + force_tls=False, + force_no_tls=False, + debug=False, + ) + + # create_cli_catalog should be called when local=True + mock_create_catalog.assert_called_once()