fix: address Copilot review on OpenAI-compatible providers#5043
fix: address Copilot review on OpenAI-compatible providers#5043alex-clawd wants to merge 1 commit intocrewAIInc:mainfrom
Conversation
…#5042) - Delegate supports_function_calling() to parent (handles o1 models via OpenRouter) - Guard empty env vars in base_url resolution - Fix misleading comment about model validation rules - Remove unused MagicMock import - Use 'is not None' for env var restoration in tests
There was a problem hiding this comment.
Pull request overview
Follow-up to #5042 to refine OpenAI-compatible provider behavior (capability checks, base URL resolution) and clean up related tests/comments; additionally, it removes two GitHub workflow checks.
Changes:
- Delegate
OpenAICompatibleCompletion.supports_function_calling()to the OpenAI implementation and refine base URL env-var resolution to ignore empty values. - Minor test cleanup/fix (remove unused import; correct env restoration when original env var is an empty string).
- Remove PR title and PR size GitHub Actions workflows.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
lib/crewai/src/crewai/llms/providers/openai_compatible/completion.py |
Adjusts base URL env override logic and delegates function-calling support to OpenAI logic. |
lib/crewai/tests/llms/openai_compatible/test_openai_compatible.py |
Removes unused import and fixes env var restoration condition in a test. |
lib/crewai/src/crewai/llm.py |
Updates comment clarifying model-prefix restrictions for certain OpenAI-compatible providers. |
.github/workflows/pr-title.yml |
Removes semantic PR title check workflow. |
.github/workflows/pr-size.yml |
Removes PR size labeling workflow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| elif config.base_url_env: | ||
| resolved = os.getenv(config.base_url_env, config.base_url) | ||
| env_value = os.getenv(config.base_url_env) | ||
| resolved = env_value if env_value else config.base_url |
There was a problem hiding this comment.
The new base_url_env handling treats an empty env var as "unset" (falls back to config.base_url), but there’s no test covering the empty-string case. Please add a test that sets e.g. DEEPSEEK_BASE_URL="" (or OLLAMA_HOST="") and asserts the default base URL is used.
| def supports_function_calling(self) -> bool: | ||
| """Check if the provider supports function calling. | ||
|
|
||
| All modern OpenAI-compatible providers support function calling. | ||
| Delegates to the parent OpenAI implementation which handles | ||
| edge cases like o1 models (which may be routed through | ||
| OpenRouter or other compatible providers). | ||
|
|
||
| Returns: | ||
| True, as all supported providers have function calling support. | ||
| Whether the model supports function calling. | ||
| """ | ||
| return True | ||
| return super().supports_function_calling() |
There was a problem hiding this comment.
supports_function_calling() now delegates to OpenAICompletion, which can return False for o1 models. Since this is a behavior change from the previous unconditional True, please add a test exercising an o1* model routed through an OpenAI-compatible provider (e.g., provider="openrouter", model="o1-mini") to lock in the intended behavior.
| # OpenAI-compatible providers - most accept any model name, but some | ||
| # (DeepSeek, Dashscope) restrict to their own model prefixes |
There was a problem hiding this comment.
The PR description says this change is limited to addressing Copilot review comments in 3 files, but this PR also deletes two GitHub workflows (.github/workflows/pr-title.yml and pr-size.yml). Please either revert those workflow removals or update the PR description/scope to explicitly include (and justify) removing these repo checks.
| # OpenAI-compatible providers - most accept any model name, but some | ||
| # (DeepSeek, Dashscope) restrict to their own model prefixes |
There was a problem hiding this comment.
Removing pr-title.yml and pr-size.yml eliminates semantic PR title validation and PR size labeling, and there doesn’t appear to be any replacement workflow/config in the repo. If this is intentional, consider moving the workflow removals into a separate PR (or document the replacement mechanism) to avoid weakening CI/triage as part of an unrelated provider fix.
|
Reopening as #new from the org repo to fix the workflow file deletions (OAuth scope issue with fork pushes). Same changes, clean diff. |
Follow-up to #5042 (merged). Addresses 5 Copilot review comments:
is not Noneinstead of truthy check for proper test cleanup3 files, +11/-8