chore(llm-providers): Add env OPENAI_BASE_URL for openai ; unify llm provider configs#1577
Conversation
Signed-off-by: Will Killian <wkillian@nvidia.com>
WalkthroughMultiple LLM plugin modules now derive OpenAI Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: Will Killian <wkillian@nvidia.com>
mnajafian-nv
left a comment
mnajafian-nv
left a comment
There was a problem hiding this comment.
Great work!
You mentioned you're open to suggestions for Semantic Kernel. Here's how I'd extend the same pattern there.
Based on my understanding the main difference is that unlike LangChain/LlamaIndex which accept base_url directly, Semantic Kernel's OpenAIChatCompletion needs a custom AsyncOpenAI client to handle non-default endpoints.
Suggestions:
File: packages/nvidia_nat_semantic_kernel/src/nat/plugins/semantic_kernel/llm.py
Add these imports at the top:
import os
from typing import TypeVar
from openai import AsyncOpenAI
from nat.builder.builder import Builder
Update the openai_semantic_kernel function:
@register_llm_client(config_type=OpenAIModelConfig, wrapper_type=LLMFrameworkEnum.SEMANTIC_KERNEL)
async def openai_semantic_kernel(llm_config: OpenAIModelConfig, _builder: Builder):
from semantic_kernel.connectors.ai.open_ai import OpenAIChatCompletion
validate_no_responses_api(llm_config, LLMFrameworkEnum.SEMANTIC_KERNEL)
# Determine base_url (config takes precedence, env var as fallback)
base_url = llm_config.base_url
if not base_url:
base_url = os.getenv("OPENAI_BASE_URL")
# Create custom AsyncOpenAI client if base_url is specified
# This follows the official Semantic Kernel pattern for custom endpoints
async_client = None
if base_url:
api_key = get_secret_value(llm_config.api_key) or os.getenv("OPENAI_API_KEY")
async_client = AsyncOpenAI(
api_key=api_key,
base_url=base_url,
)
llm = OpenAIChatCompletion(
ai_model_id=llm_config.model_name,
async_client=async_client,
)
yield _patch_llm_based_on_config(llm, llm_config)
This approach:
• Matches Semantic Kernel's official pattern (https://github.com/microsoft/semantic-kernel/blob/main/python/samples/concepts/setup/chat_completion_services.py) used for DeepSeek/OpenRouter
• Uses the same fallback logic as your Strands implementation, but cleaner with a single os.getenv() call
• Non-breaking when base_url isn't set, it passes None and uses default behavior
Happy to open a PR with this if that's easier, or feel free to just copy it. Either way works!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/nvidia_nat_semantic_kernel/src/nat/plugins/semantic_kernel/llm.py`:
- Around line 115-120: The AsyncOpenAI instance (async_client) is not closed,
risking an httpx AsyncClient leak; wrap the generator's yield in a try/finally:
create async_client = AsyncOpenAI(...), construct llm =
OpenAIChatCompletion(...), then do try: yield llm finally: await
async_client.aclose() (or call the appropriate close method on AsyncOpenAI if
named differently) to ensure the underlying httpx.AsyncClient is closed when the
generator finishes.
🧹 Nitpick comments (2)
packages/nvidia_nat_llama_index/src/nat/plugins/llama_index/llm.py (1)
134-144:base_urlnot excluded frommodel_dump— inconsistent with sibling plugins.In the agno plugin,
base_urlis excluded frommodel_dump(line 110 of agno/llm.py) and then conditionally re-added. Here,base_urlis left in the dump and potentially overwritten on line 144. This works but is inconsistent and could silently double-set the key.Consider excluding
base_urlfrom themodel_dumpcall to match the pattern used in the other plugins changed in this PR:Proposed fix
config_dict = llm_config.model_dump( - exclude={"type", "thinking", "api_type", "api_key"}, + exclude={"type", "thinking", "api_type", "api_key", "base_url"}, by_alias=True, exclude_none=True, exclude_unset=True, )packages/nvidia_nat_semantic_kernel/src/nat/plugins/semantic_kernel/llm.py (1)
115-116: Use importedget_secret_value()utility for consistency and null safety.Line 115 calls
.get_secret_value()directly on theSecretStrfield, but the same file imports theget_secret_valueutility fromnat.data_models.common(line 22) and uses it elsewhere in the file. The utility safely handlesNonevalues, whereas direct method calls riskAttributeErrorif the field isNone. Use the imported helper for consistency:- api_key = llm_config.api_key.get_secret_value() or os.getenv("OPENAI_API_KEY") + api_key = get_secret_value(llm_config.api_key) or os.getenv("OPENAI_API_KEY")This also applies to the agno and llama_index plugins, which exhibit the same pattern.
packages/nvidia_nat_semantic_kernel/src/nat/plugins/semantic_kernel/llm.py
Outdated
Show resolved
Hide resolved
willkill07
force-pushed
the
wkk_add-env-configurable-oai-base_url
branch
2 times, most recently
from
dc387e4 to
d87d40a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/nvidia_nat_adk/src/nat/plugins/adk/llm.py (1)
118-128:⚠️ Potential issue | 🟡 Minor
api_keyshould be excluded frommodel_dumpfor consistency with other plugins.The other plugin files in this PR (llama_index, agno, etc.) explicitly exclude
"api_key"frommodel_dumpbefore conditionally re-adding it. Here,api_keyis not excluded, so when it's set on the config, it will appear inconfig_dictas aSecretStrobject from the dump and then be overwritten by line 126 with the plain string. While the overwrite masks the issue, passing aSecretStrtoLiteLlm(if the overwrite branch isn't reached) could cause problems.Proposed fix
config_dict = config.model_dump( - exclude={"type", "max_retries", "thinking", "model_name", "model", "base_url", "api_type"}, + exclude={"type", "max_retries", "thinking", "model_name", "model", "base_url", "api_key", "api_type"}, by_alias=True, exclude_none=True, exclude_unset=True, )
🤖 Fix all issues with AI agents
In `@packages/nvidia_nat_llama_index/src/nat/plugins/llama_index/llm.py`:
- Around line 134-149: The config_dict created by llm_config.model_dump
currently does not exclude "base_url", which can lead to duplicate/stale values;
update the call to llm_config.model_dump (the model_dump call that builds
config_dict) to include "base_url" in its exclude set so base_url is omitted
from the dump, then keep the existing conditional that sets
config_dict["base_url"] from llm_config.base_url or
os.getenv("OPENAI_BASE_URL"); this change affects the symbols config_dict,
llm_config.model_dump, and the subsequent conditional that assigns
config_dict["base_url"] before instantiating OpenAI or OpenAIResponses.
In `@packages/nvidia_nat_semantic_kernel/src/nat/plugins/semantic_kernel/llm.py`:
- Around line 115-118: The code calls llm_config.api_key.get_secret_value()
unconditionally which can raise AttributeError if llm_config.api_key is None;
update the initialization for api_key to first check if llm_config.api_key is
truthy (e.g., if llm_config.api_key) and only then call get_secret_value(),
otherwise fall back to os.getenv("OPENAI_API_KEY"); keep base_url logic the same
and pass the resolved api_key into AsyncOpenAI(api_key=api_key,
base_url=base_url).
packages/nvidia_nat_semantic_kernel/src/nat/plugins/semantic_kernel/llm.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Will Killian <wkillian@nvidia.com>
willkill07
force-pushed
the
wkk_add-env-configurable-oai-base_url
branch
from
d87d40a to
ddbef53
Compare
Signed-off-by: Will Killian <wkillian@nvidia.com>
willkill07
changed the title
openaiOPENAI_BASE_URL for openai ; align llm provider expansions
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/nvidia_nat_llama_index/src/nat/plugins/llama_index/llm.py`:
- Around line 142-145: Update the env-var lookups to prefer NAT_-prefixed names
while retaining OPENAI_* as fallback: when building config_dict use
get_secret_value(llm_config.api_key) or os.getenv("NAT_OPENAI_API_KEY") (fall
back to os.getenv("OPENAI_API_KEY") if needed) to set "api_key", and use
llm_config.base_url or os.getenv("NAT_OPENAI_BASE_URL") (fall back to
os.getenv("OPENAI_BASE_URL")) to set "base_url"; keep the same keys in
config_dict and the same conditional structure around get_secret_value and
llm_config to minimize changes.
In `@packages/nvidia_nat_semantic_kernel/src/nat/plugins/semantic_kernel/llm.py`:
- Around line 115-116: Update the environment variable lookups in the LLM
initialization so they use the NAT_ prefix first and fall back to the existing
OPENAI_* names: when resolving the API key
(get_secret_value(llm_config.api_key)) and base URL (llm_config.base_url) prefer
NAT_OPENAI_API_KEY and NAT_OPENAI_BASE_URL, respectively, and only if those are
not set fall back to OPENAI_API_KEY / OPENAI_BASE_URL; adjust the calls around
get_secret_value(llm_config.api_key) and the base_url assignment to reflect this
lookup order.
packages/nvidia_nat_semantic_kernel/src/nat/plugins/semantic_kernel/llm.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.py (1)
322-335: 🛠️ Refactor suggestion | 🟠 Major
api_key/base_urlnot excluded frommodel_dump— inconsistent withopenai_autogen.In
openai_autogen(line 150),api_keyandbase_urlare explicitly excluded frommodel_dumpbefore being conditionally re-added from config/env. Here,nim_autogenrelies onexclude_unset=Trueto keep them out when not explicitly set, but if they are set, the raw Pydantic field values (potentiallySecretStrobjects) will land inconfig_objalongside (or before) the overwrite on lines 334–335.When
get_secret_value(llm_config.api_key)and the env var are both falsy (e.g., key isSecretStr("")), theifon line 334 is false, and theSecretStrobject frommodel_dumpremains inconfig_obj— which is unlikely to be accepted as a plain string byOpenAIChatCompletionClient.Consider aligning with the
openai_autogenpattern:Proposed fix
config_obj = { **llm_config.model_dump( - exclude={"type", "model_name", "thinking"}, + exclude={"type", "model_name", "thinking", "api_key", "base_url"}, by_alias=True, exclude_none=True, exclude_unset=True, ), } - if llm_config.base_url is None: - config_obj["base_url"] = "https://integrate.api.nvidia.com/v1" + config_obj["base_url"] = llm_config.base_url or "https://integrate.api.nvidia.com/v1" if (api_key := get_secret_value(llm_config.api_key) or os.getenv("NVIDIA_API_KEY")): config_obj["api_key"] = api_key
🧹 Nitpick comments (1)
packages/nvidia_nat_autogen/src/nat/plugins/autogen/llm.py (1)
199-211: Azure path passes rawllm_config.api_key(likelySecretStr) — pre-existing but worth noting.Line 201 passes
llm_config.api_keydirectly (aSecretStr/OptionalSecretStr) rather than resolving it viaget_secret_value(). This is not a regression from this PR, but it's inconsistent with the newopenai_autogenpattern and could cause issues ifAzureOpenAIChatCompletionClientdoesn't handleSecretStr. Consider harmonizing in a follow-up.
willkill07
changed the title
OPENAI_BASE_URL for openai ; align llm provider expansionsOPENAI_BASE_URL for openai ; unify llm provider configs
Signed-off-by: Will Killian <wkillian@nvidia.com>
|
/merge |
…llm provider configs (NVIDIA#1577) Add this so we can override `OPENAI_BASE_URL` and `OPENAI_API_KEY` when testing in nightly CI. Also update the logic for passthrough of `OPENAI_API_KEY` Closes ## By Submitting this PR I confirm: - I am familiar with the [Contributing Guidelines](https://github.com/NVIDIA/NeMo-Agent-Toolkit/blob/develop/docs/source/resources/contributing/index.md). - We require that all contributors "sign-off" on their commits. This certifies that the contribution is your original work, or you have rights to submit it under the same license, or a compatible license. - Any contribution which contains commits that are not Signed-Off will not be accepted. - When the PR is ready for review, new or existing tests cover these changes. - When the PR is ready for review, the documentation is up to date with these changes. ## Summary by CodeRabbit * **New Features** * LLM plugins now accept API key and base URL from environment variables as fallbacks across providers. * Semantic Kernel integration now uses an asynchronous OpenAI client for improved async handling. * **Refactor** * Unified config construction and cleanup across multiple LLM integrations for more consistent instantiation. * **Tests** * Test suite updated to ensure API-key-related test setup is applied. Authors: - Will Killian (https://github.com/willkill07) Approvers: - https://github.com/mnajafian-nv URL: NVIDIA#1577 Signed-off-by: Sam Pastoriza <spastoriza@nvidia.com>
2 participants
Description
Add this so we can override
OPENAI_BASE_URLandOPENAI_API_KEYwhen testing in nightly CI.Also update the logic for passthrough of
OPENAI_API_KEYCloses
By Submitting this PR I confirm:
Summary by CodeRabbit
New Features
Refactor
Tests