Improve Azure example configuration handling#430
Improve Azure example configuration handling#430guming3d wants to merge 1 commit intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request enhances Azure OpenAI example scripts by improving configuration management through environment variable loading, updating client initialization for proper Azure compatibility, and refactoring imports for better code organization.
Key changes:
- Added
dotenvintegration with.envfile support for simplified local development configuration - Updated OpenAI client instantiation in
capital_agent.pyto useAzureOpenAIwith required API version validation - Refactored imports to use explicit module paths and replaced deprecated
setup_logging()withconfigure_logger()
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
examples/azure/train_capital_agent.py |
Refactored imports to explicit module paths and updated logging function from setup_logging() to configure_logger() |
examples/azure/capital_agent.py |
Added dotenv support, updated OpenAI client to use AzureOpenAI with API version validation |
examples/azure/aoai_finetune.py |
Added dotenv support, updated import path for batch_iter_over_dataset, and initialized OpenAI client within the training loop |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.openai_client = OpenAI( | ||
| api_key=self.azure_openai_api_key, | ||
| base_url=self.azure_openai_endpoint + "/openai/v1/", | ||
| ) |
There was a problem hiding this comment.
The OpenAI client is being re-initialized inside the run loop, which overrides the client already created in __init__ at lines 134-137. This re-initialization changes the base_url by appending "/openai/v1/" to the endpoint. If this path modification is necessary for the fine-tuning API, it should be done in __init__ instead to avoid confusion and ensure consistency. If both URLs are needed for different purposes, consider using separate client instances with descriptive names.
| self.openai_client = OpenAI( | |
| api_key=self.azure_openai_api_key, | |
| base_url=self.azure_openai_endpoint + "/openai/v1/", | |
| ) |
| openai_client = openai.OpenAI(base_url=llm.endpoint, api_key=os.getenv("AZURE_OPENAI_API_KEY", "")) | ||
| api_version = os.getenv("AZURE_OPENAI_API_VERSION", "") | ||
| if not api_version: | ||
| raise ValueError("Set AZURE_OPENAI_API_VERSION to match your Azure OpenAI resource.") |
There was a problem hiding this comment.
The error message states "Set AZURE_OPENAI_API_VERSION to match your Azure OpenAI resource" but doesn't provide guidance on what a valid value looks like. Consider enhancing the error message to include an example of a valid API version format (e.g., "2024-08-01-preview") to help users configure this correctly.
| raise ValueError("Set AZURE_OPENAI_API_VERSION to match your Azure OpenAI resource.") | |
| raise ValueError( | |
| "Set AZURE_OPENAI_API_VERSION (for example, '2024-08-01-preview') to match your Azure OpenAI resource's API version." | |
| ) |
| load_dotenv(override=True) | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
There are extra blank lines after the dotenv loading code. According to PEP 8 style guidelines, there should only be two blank lines between top-level code blocks. Remove one of the blank lines to maintain consistency.
|
|
||
| def main(): | ||
| setup_logging() | ||
| configure_logger() |
There was a problem hiding this comment.
The function configure_logger() is deprecated in favor of setup_logging(). According to the documentation in agentlightning/logging.py, this function emits a deprecation warning. Consider using setup_logging() instead, which is imported as from agentlightning import setup_logging (or use the setup function from agentlightning.logging).
| from agentlightning.adapter.messages import OpenAIMessages, TraceToMessages | ||
| from agentlightning.algorithm import Algorithm | ||
| from agentlightning.algorithm.utils import batch_iter_over_dataset | ||
| from agentlightning.algorithm.apo.apo import batch_iter_over_dataset |
There was a problem hiding this comment.
The import path change is incorrect. The function batch_iter_over_dataset is defined in agentlightning.algorithm.utils (not in agentlightning.algorithm.apo.apo). While apo.py imports it from utils, importing directly from apo.py creates an indirect dependency. Please import from the original location: from agentlightning.algorithm.utils import batch_iter_over_dataset.
| from agentlightning.algorithm.apo.apo import batch_iter_over_dataset | |
| from agentlightning.algorithm.utils import batch_iter_over_dataset |
ultmaster
left a comment
ultmaster
left a comment
There was a problem hiding this comment.
please see whether the copilot comments make sense.
| training_data = await self.prepare_data_for_training(messages_group, reward_group, "train") | ||
| self._log_info(f"[Stage 4] Prepared {len(training_data)} training examples after filtering.") | ||
|
|
||
| self.openai_client = OpenAI( |
There was a problem hiding this comment.
Maybe we should encourage users to use the correct endpoint ending with /openai/v1. In other words, the documentation should be updated instead of the code.
| from dotenv import load_dotenv | ||
|
|
||
| # Load environment variables from .env file (override existing) | ||
| load_dotenv(override=True) |
There was a problem hiding this comment.
dotenv run is a better idea than load_dotenv.
| raise ValueError("Set AZURE_OPENAI_API_VERSION to match your Azure OpenAI resource.") | ||
|
|
||
| # Use AzureOpenAI client for Azure endpoints | ||
| openai_client = openai.AzureOpenAI( |
There was a problem hiding this comment.
Why AzureOpenAI is a must here?
2 participants
This pull request improves Azure OpenAI example scripts by enhancing environment variable management, updating OpenAI client usage for Azure compatibility, and refactoring imports for clarity and maintainability. The most important changes are grouped below.
Environment Variable Handling:
dotenvsupport and automatic loading of environment variables from a.envfile (with override) in bothaoai_finetune.pyandcapital_agent.pyto simplify local development and configuration. [1] [2]OpenAI Client Usage and Azure Compatibility:
capital_agent.pyto useopenai.AzureOpenAI, requiring explicitapi_versionand proper Azure endpoint configuration, improving compatibility with Azure OpenAI deployments.aoai_finetune.py, set up the OpenAI client with Azure-specific API key and endpoint before fine-tuning, ensuring correct authentication and endpoint usage.Import and Logging Refactoring:
train_capital_agent.pyto use explicit module paths forTraceToMessages,Trainer, and logging configuration, improving code clarity and maintainability.setup_logging()withconfigure_logger()intrain_capital_agent.pyfor more explicit logging setup.Internal Utility Import Update:
batch_iter_over_datasetinaoai_finetune.pyto reference its new location inagentlightning.algorithm.apo.apo, reflecting internal codebase restructuring.