Skip to content

chore: Add goose providers list command#116

Merged
elenazherdeva merged 8 commits intomainfrom
ez/list_providers_cli
Oct 9, 2024
Merged

chore: Add goose providers list command#116
elenazherdeva merged 8 commits intomainfrom
ez/list_providers_cli

Conversation

@elenazherdeva
Copy link
Contributor

image

Copy link
Collaborator

@baxen baxen left a comment

Choose a reason for hiding this comment

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

LGTM!



def test_lint_providers():
lint_providers()
Copy link
Collaborator

@lifeizhou-ap lifeizhou-ap Oct 4, 2024

Choose a reason for hiding this comment

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

I guess you followed the existing implementation pattern.

I am not sure whether lint_providers function is used anywhere else. If it does not, we could move the implementation of lint_providers into this test_linting.py as the implementation is just to check the doc string is correct. I feel it will be easier for people to read and understand

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lukealvoeiro can describe it better, but the reason behind this is that lint_providers could be used in tests for the goose-plugins-block internal library. Currently, we only have one internal provider, but if there are more in the future, they should follow the same structure. Do you think we should reconsider this logic?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, we want this function to be exported so we can lint this in any open source or internal plugin repos as well.

required_envs = []
env_block = False
for line in lines:
if "Required env vars:" in line:
Copy link
Collaborator

@lifeizhou-ap lifeizhou-ap Oct 4, 2024

Choose a reason for hiding this comment

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

just thinking whether it would be helpful if we list the env vars in the code as a variable or function instead of parsing doc string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tbh, I don’t really like this parsing either, but I couldn’t come up with a better approach. Could you elaborate, please? How can we list it as “a variable or function”?

The only idea I have is to add get_required_env function to each provider and call it in main.py

Copy link
Contributor

@lamchau lamchau Oct 5, 2024

Choose a reason for hiding this comment

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

what do you think if we extend the base provider? we could do the check but the documenting (via mkdocs) would need some playing around with for formatting

here's an example in case i'm not explaining very well 😅

class Provider(ABC):
    REQUIRED_ENV_VARS: list[str] = []

    @classmethod
    def check_env_vars(cls):
       ...

    def __init__(self) -> None:
        self.check_env_vars()

class BedrockProvider(Provider):
    REQUIRED_ENV_VARS = [
        "AWS_ACCESS_KEY_ID",
        "AWS_SECRET_ACCESS_KEY",
        "AWS_SESSION_TOKEN", 
    ]

then to get it into the docs, we could selectively generate it for one constant

::: example.BedrockProvider
    options:
      members:
        - REQUIRED_ENV_VARS

but the formatting isn't the most readable and i'm not sure mkdocstrings gives us control of how to render members. which leaves us with potentially preprocessing to generate the docstring (maybe too complicated)

image

@lifeizhou-ap
Copy link
Collaborator

lifeizhou-ap commented Oct 4, 2024

Hey @elenazherdeva

Thanks for the PR! Looks good! I asked a couple of questions for consideration.



@providers.command(name="list")
def list_providers() -> None:
Copy link
Collaborator

@lifeizhou-ap lifeizhou-ap Oct 4, 2024

Choose a reason for hiding this comment

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

maybe write a test for this function?

Copy link
Contributor

Choose a reason for hiding this comment

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

kinda hacky but if the exchange/providers project is visible via the goose distro but might be possible to render this with an os.listdirs()

@lily-de
Copy link
Contributor

lily-de commented Oct 4, 2024

Could be a separate PR but could we add functionality to list which of the env vars a user has set or missing? No need to print them, just if they're stored already

return cls()

@classmethod
def check_env_vars(cls: Type["Provider"], instructions_url: Optional[str] = None) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!

@elenazherdeva elenazherdeva merged commit 8276e9b into main Oct 9, 2024
envs = provider.REQUIRED_ENV_VARS
if envs:
env_required_str = ", ".join(envs)
print(f" [dim]env vars required: {env_required_str}")
Copy link
Contributor

Choose a reason for hiding this comment

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

:nit: since these are required i think they should be emphasized instead of dim

@lamchau
Copy link
Contributor

lamchau commented Oct 9, 2024

looks great! added a couple of minor thoughts :)

lukealvoeiro added a commit that referenced this pull request Oct 9, 2024
* main: (41 commits)
  chore: Add goose providers list command (#116)
  docs: working ollama for desktop (#125)
  docs: format and clean up warnings/errors (#120)
  docs: update deploy workflow (#124)
  feat: Implement a goose run command (#121)
  feat: saved api_key to keychain for user (#104)
  docs: add callout plugin (#119)
  chore: add a page to docs for Goose application examples (#117)
  fix: exit the goose and show the error message when provider environment variable is not set (#103)
  fix: Update OpenAI pricing per https://openai.com/api/pricing/ (#110)
  fix: update developer tool prompts to use plan task status to match allowable statuses update_plan tool call (#107)
  fix: removed the panel in the output so that the user won't have unnecessary pane borders in the copied content (#109)
  docs: update links to exchange to the new location (#108)
  chore: setup workspace for exchange (#105)
  fix: resolve uvx when using a git client or IDE (#98)
  ci: add include-markdown for mkdocs (#100)
  chore: fix broken badge on readme (#102)
  feat: add global optional user goosehints file (#73)
  docs: update docs (#99)
  chore(release): release 0.9.3 (#97)
  ...
ahau-square pushed a commit that referenced this pull request Oct 10, 2024
* main:
  feat: add groq provider (#134)
  feat: add a deep thinking reasoner model (o1-preview/mini) (#68)
  fix: use concrete SessionNotifier (#135)
  feat: add guards to session management (#101)
  fix: Set default model configuration for the Google provider. (#131)
  test: convert Google Gemini tests to VCR (#118)
  chore: Add goose providers list command (#116)
  docs: working ollama for desktop (#125)
  docs: format and clean up warnings/errors (#120)
  docs: update deploy workflow (#124)
  feat: Implement a goose run command (#121)
@lamchau lamchau deleted the ez/list_providers_cli branch October 24, 2024 11:45
ahau-square pushed a commit that referenced this pull request May 2, 2025
Co-authored-by: Bradley Axen <baxen@squareup.com>
cbruyndoncx pushed a commit to cbruyndoncx/goose that referenced this pull request Jul 20, 2025
Co-authored-by: Bradley Axen <baxen@squareup.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

Comments