Skip to content

fix: type api_key as optional on LiteLLMConversation._get_litellm_object - #337

Closed
AmirF194 wants to merge 1 commit into
biocypher:mainfrom
AmirF194:fix/306-litellm-api-key-optional-annotation
Closed

AmirF194 wants to merge 1 commit into
biocypher:mainfrom
AmirF194:fix/306-litellm-api-key-optional-annotation

Conversation

@AmirF194

Copy link
Copy Markdown

Root cause

LiteLLMConversation.get_litellm_object() declared api_key: str, but the method's
own docstring and body document and implement a ValueError for api_key is None.
That branch is real, not hypothetical: set_api_key() forwards its api_key
argument here unchanged, and biochatter/podcast.py calls
set_api_key(api_key=os.getenv("OPENAI_API_KEY"), ...), which is None whenever
that environment variable is unset. The declared type contradicted the implemented
and documented behavior.

Fix

Widened the annotation to api_key: str | None, matching the sibling
implementations in openrouter.py and langchain.py, which already type this
parameter the same way. Also renamed the method to _get_litellm_object, per
@slobentanzer's comment on #306: it is not part of the public API.

Verification

  • New tests in test/test_llm_connect/test_litellm_conversation.py: one asserts
    the annotation now includes NoneType (fails on main, passes on this branch),
    one asserts the documented ValueError fires for api_key=None.
  • pytest test/test_llm_connect/: 119 passed. 3 pre-existing failures are
    unrelated to this change (the xinference optional dependency group, not
    installed in my local run) and reproduce identically on unmodified main.
  • ruff check / ruff format on both changed files: no new violations beyond
    the four SLF001 hits from the rename itself, consistent with how this file
    already tests other private methods directly.
  • Not verified: the repo's xinference extra and the full CI matrix, since
    installing torch locally exceeded available disk on my machine.

Fixes #306

api_key was typed as str, but the method already implements and documents
an explicit ValueError for api_key is None. That branch is real: podcast.py
sources the key via os.getenv, which returns None when the env var is
unset, and set_api_key() forwards it here unchanged. Widen the annotation
to str | None so it matches the reachable, implemented behavior, and
rename the method to _get_litellm_object per the maintainer's comment on
biocypher#306 since it is not part of the public API.

Fixes biocypher#306
@AmirF194

Copy link
Copy Markdown
Author

No rush, just checking in about a week after opening. CI on this fork hasn't run yet since it needs a maintainer to approve the workflow run for a first-time contributor, so let me know if there's anything else you'd like changed in the meantime.

@AmirF194

AmirF194 commented Sep 9, 2026

Copy link
Copy Markdown
Author

Closing to keep the queue clean; happy to reopen if there's interest.

@AmirF194 AmirF194 closed this Sep 9, 2026
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.

[Bug] LiteLLMConversation unused exception

1 participant