plan for PYDANTIC_AI migration and model selection refactorings#201
Closed
bmerkle wants to merge 1 commit intomicrosoft:agnosticfrom
Closed
plan for PYDANTIC_AI migration and model selection refactorings#201bmerkle wants to merge 1 commit intomicrosoft:agnosticfrom
bmerkle wants to merge 1 commit intomicrosoft:agnosticfrom
Conversation
- enhance implementation and test to use either openai or azure - refactored provider selection into one place (instead several ones)
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request refactors provider selection and API key detection into centralized helper functions, adds environment variable fallback support for model configuration, and introduces a comprehensive migration plan for transitioning from TypeChat to pydantic_ai. The changes build on PR #200 and consolidate provider selection logic that was previously scattered across multiple functions.
Changes:
- Centralized provider detection into
infer_provider_prefix(),has_api_key(), andprefers_azure()helper functions inutils.py - Made
create_chat_model()andcreate_embedding_model()accept optional model specs with fallback to environment variables (PYDANTIC_AI_MODEL,PYDANTIC_AI_EMBEDDING_MODEL) - Added comprehensive test coverage for provider auto-detection and environment variable fallback behavior
- Documented migration plan from TypeChat to pydantic_ai in
PYDANTIC_AI.md
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/typeagent/aitools/utils.py | Added three provider detection helper functions (infer_provider_prefix, has_api_key, prefers_azure) and refactored create_async_openai_client and make_agent to use centralized provider selection |
| src/typeagent/aitools/model_adapters.py | Made model factory functions accept optional model specs with environment variable and auto-detection fallbacks; re-exported infer_provider_prefix for convenience |
| src/typeagent/aitools/embeddings.py | Updated AsyncEmbeddingModel to use the new prefers_azure() helper instead of inline provider detection logic |
| tests/test_model_adapters.py | Added comprehensive tests for provider auto-detection, environment variable fallbacks, and default model constants; introduced provider fixture for existing tests |
| tests/test_utils.py | Simplified test to use has_api_key() helper instead of direct environment variable checks |
| tests/conftest.py | Updated really_needs_auth fixture to use has_api_key() helper |
| PYDANTIC_AI.md | Added detailed 6-phase migration plan documenting the transition from TypeChat to pydantic_ai |
| .env-example | Added example configuration for pydantic_ai environment variables and Azure OpenAI setup |
Comments suppressed due to low confidence (3)
src/typeagent/aitools/model_adapters.py:172
- This import violates the "Import Architecture Rules" stated in AGENTS.md: "Never import a symbol from a module that just re-exports it" and "Always import directly from the module that defines the symbol". The comment says "Re-export from utils for backward compatibility and convenience", but this should follow the pattern
from .utils import infer_provider_prefix as infer_provider_prefixto make it an explicit re-export. However, since the noqa comment is present and the docstring mentions backward compatibility, this appears to be intentional. Consider adding this to all if this module is meant to re-export this function.
from .utils import infer_provider_prefix as infer_provider_prefix # noqa: E402
tests/test_model_adapters.py:25
- The provider fixture calls infer_provider_prefix() which raises RuntimeError if no API keys are available. Tests using this fixture will fail with an unhandled RuntimeError instead of being skipped when API keys are not present. Consider modifying the fixture to handle the missing key case gracefully, for example by calling pytest.skip() when has_api_key() returns False, or by making tests that use this fixture depend on the really_needs_auth fixture. This would make the test behavior more consistent with other tests in the codebase that check for API keys (see conftest.py:92-93).
@pytest.fixture
def provider() -> str:
"""Current provider prefix based on available API keys."""
return infer_provider_prefix()
.env-example:8
- Typo in comment: "auzre" should be "azure"
# auzre openAI
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
this PR is based on #200 and contributes the above points