Skip to content

fix: exit the goose and show the error message when provider environment variable is not set #103

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.

Sign up for GitHub

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

Merged
lifeizhou-ap merged 13 commits into from
Oct 4, 2024
Merged

fix: exit the goose and show the error message when provider environment variable is not set #103

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
c321c8c
exit the goose and show the error message when provider environment i…
lifeizhou-ap Oct 1, 2024
6cb7cfc
fixed the message content
lifeizhou-ap Oct 1, 2024
dfb4522
show error message when configuration is incorrect
lifeizhou-ap Oct 2, 2024
a710080
Merge branch 'main' into lifei/show-readable-error-message-when-api-k…
lifeizhou-ap Oct 2, 2024
b52f3a3
created custom errors when env var for providers does not exist or pr…
lifeizhou-ap Oct 3, 2024
40fab9b
fixed format issues
lifeizhou-ap Oct 3, 2024
1b725f7
fixed the errror message
lifeizhou-ap Oct 3, 2024
451cda4
add a new line in message
lifeizhou-ap Oct 3, 2024
7829262
format the code
lifeizhou-ap Oct 3, 2024
03af0c6
renamed to invalidChoiceError
lifeizhou-ap Oct 4, 2024
4e97f0a
inline methods
lifeizhou-ap Oct 4, 2024
36b5d2c
set default provider
lifeizhou-ap Oct 4, 2024
208b265
rename variable
lifeizhou-ap Oct 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions src/goose/cli/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,6 @@ def default_model_configuration() -> Tuple[str, str, str]:
break
except Exception:
pass
else:

@lifeizhou-ap lifeizhou-ap Oct 1, 2024 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

we don't need this message. The error message (env var is not set) will be thrown when the provider is loaded while creating the session.

@baxen baxen Oct 3, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

so in this case, the provider value that is returned below will be whatever is last in the list of providers, and then be used to set the default profile. It will later error like you say.

Should we do

else:
    provider = RECOMMENDED_DEFAULT

so that we don't set it to something niche?

raise ValueError(
"Could not detect an available provider,"
+ " make sure to plugin a provider or set an env var such as OPENAI_API_KEY"
)

recommended = {
"ollama": (OLLAMA_MODEL, OLLAMA_MODEL),
Expand Down
17 changes: 14 additions & 3 deletions src/goose/cli/session.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import sys
import traceback
from pathlib import Path
from typing import Any, Dict, List, Optional

from exchange import Message, ToolResult, ToolUse, Text
from exchange import Message, ToolResult, ToolUse, Text, Exchange
from exchange.providers.base import MissingProviderEnvVariableError
from rich import print
from rich.console import RenderableType
from rich.live import Live
Expand Down Expand Up @@ -89,8 +91,7 @@ def __init__(
self.profile = profile
self.status_indicator = Status("", spinner="dots")
self.notifier = SessionNotifier(self.status_indicator)

self.exchange = build_exchange(profile=load_profile(profile), notifier=self.notifier)
self.exchange = self._create_exchange()
setup_logging(log_file_directory=LOG_PATH, log_level=log_level)

self.exchange.messages.extend(self._get_initial_messages())
Expand All @@ -100,6 +101,16 @@ def __init__(

self.prompt_session = GoosePromptSession()

def _create_exchange(self) -> Exchange:
try:
return build_exchange(profile=load_profile(self.profile), notifier=self.notifier)
except MissingProviderEnvVariableError as e:
error_message = (
f"Missing environment variable: {e.message}. Please set the required environment variable to continue."
)
print(Panel(error_message, style="red"))
sys.exit(1)

def _get_initial_messages(self) -> List[Message]:
messages = self.load_session()

Expand Down
15 changes: 15 additions & 0 deletions tests/cli/test_session.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import pytest
from exchange import Exchange, Message, ToolUse, ToolResult
from exchange.providers.base import MissingProviderEnvVariableError
from goose.cli.prompt.goose_prompt_session import GoosePromptSession
from goose.cli.prompt.user_input import PromptAction, UserInput
from goose.cli.session import Session
Expand Down Expand Up @@ -151,3 +152,17 @@ def test_set_generated_session_name(create_session_with_mock_configs, mock_sessi
with patch("goose.cli.session.droid", return_value=generated_session_name):
session = create_session_with_mock_configs({"name": None})
assert session.name == generated_session_name


def test_create_exchange_exit_when_env_var_does_not_exist(create_session_with_mock_configs, mock_sessions_path):
session = create_session_with_mock_configs()
expected_error = MissingProviderEnvVariableError(env_variable="OPENAI_API_KEY", provider="openai")
with patch("goose.cli.session.build_exchange", side_effect=expected_error), patch(

@lamchau lamchau Oct 1, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

:nit: we can make this easier to read by forcing ruff format to newline with trailing command and parenthesis

with (
  patch("goose.cli.session.build_exchange", side_effect=expected_error), 
  patch("goose.cli.session.print") as mock_print, 
  patch("sys.exit") as mock_exit,
):

@lifeizhou-ap lifeizhou-ap Oct 3, 2024

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I prefer the separated lines too. However, I had a look, it seems that ruff format is unable to do it as it only supports some light formatting rules.

@lamchau lamchau Oct 3, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what do you mean? i refactored one of our tests and it worked for me - how did ruff format not work/fail?

image

@lifeizhou-ap lifeizhou-ap Oct 3, 2024

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

oh, sorry I missed the extra brackets. I've updated and it works now!

"goose.cli.session.print"
) as mock_print, patch("sys.exit") as mock_exit:
session._create_exchange()
mock_print.call_args_list[0][0][0].renderable == (
"Missing environment variable OPENAI_API_KEY for provider openai. ",
"Please set the required environment variable to continue.",
)
mock_exit.assert_called_once_with(1)