chore(cleanup): make llm openai compatible; clean up - #16
Conversation
This comment has been minimized.
This comment has been minimized.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| deployment = os.environ.get("AZURE_OPENAI_DEPLOYMENT", "").strip() | ||
| endpoint = os.environ.get("OPENAI_BASE_URL", "").strip() | ||
| deployment = os.environ.get("OPENAI_MODEL", "").strip() | ||
| api_key = os.environ.get("OPENAI_API_KEY", "").strip() |
There was a problem hiding this comment.
Correctness — api_key stored as "" instead of None when env var is absent.
os.environ.get("OPENAI_API_KEY", "").strip() yields "" when unset, but Config.api_key is typed str | None with default None. This works today because not "" is True, but downstream code checking config.api_key is None would behave differently.
Suggested fix:
api_key = os.environ.get("OPENAI_API_KEY", "").strip() or None| # OPENAI_API_KEY="" optional; omit if using Entra ID | ||
| # LANGSMITH_API_KEY="" optional; set to enable tracing | ||
| # LANGSMITH_PROJECT=""......optional; default "vimai" | ||
| # VIMAI_EXTERNAL_AGENTS_DIR="" optiona; default "/path/to/vimai/.agents" |
There was a problem hiding this comment.
Documentation — Typo: "optiona" → "optional"
✅ Looks GoodAll issues from the previous review have been resolved. The migration from Azure-specific env vars to generic OpenAI-compatible config is clean, correct, and well-tested. Findings
Verification
Cross-cutting observations
|
There was a problem hiding this comment.
Generated by Agentic PR Review for issue #16 · opus46 3.7M
Comments that could not be inline-anchored
src/vimai/llm.py:1
Documentation — Module docstring is stale after this PR.
It still reads "Azure OpenAI LLM setup using Entra ID authentication" but the module now supports direct API key auth for any OpenAI-compatible endpoint. Consider updating to reflect both paths, e.g.:
"""OpenAI-compatible LLM setup with optional Entra ID authentication (F05)."""
1 participant
No description provided.