Add multi-provider LLM support (OpenAI + MiniMax)#11
Open
octo-patch wants to merge 1 commit intoNirDiamant:mainfrom
Open
Add multi-provider LLM support (OpenAI + MiniMax)#11octo-patch wants to merge 1 commit intoNirDiamant:mainfrom
octo-patch wants to merge 1 commit intoNirDiamant:mainfrom
Conversation
Add utils/llm_provider.py providing a get_llm() factory function that supports multiple LLM providers (OpenAI, MiniMax) through LangChain's ChatOpenAI. MiniMax M2.5/M2.7 models are available via their OpenAI-compatible API at https://api.minimax.io/v1. Includes a tutorial notebook (multi-provider-prompting.ipynb) demonstrating side-by-side provider comparison for zero-shot, few-shot, chain-of-thought, and role prompting techniques. - utils/llm_provider.py: Multi-provider factory with auto-detection via LLM_PROVIDER env var, temperature clamping, and configurable defaults - all_prompt_engineering_techniques/multi-provider-prompting.ipynb: Tutorial notebook with 7 sections covering provider usage and comparison - tests/test_llm_provider.py: 28 unit tests covering provider detection, LLM creation, configuration, and edge cases - tests/test_integration_minimax.py: 3 integration tests with live MiniMax API - README.md: Added technique #23 (Multi-Provider Prompting) to table and detailed section
📝 WalkthroughWalkthroughThis pull request introduces a multi-provider LLM abstraction layer enabling seamless switching between OpenAI and MiniMax providers through a unified interface, complete with configuration auto-detection, comprehensive tests, and a tutorial notebook demonstrating usage patterns across multiple prompt engineering techniques. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Detect as Provider Detector
participant OpenAI as OpenAI Provider
participant MiniMax as MiniMax Provider
App->>Detect: get_provider_name()
alt LLM_PROVIDER env var set
Detect->>Detect: validate against SUPPORTED_PROVIDERS
Detect-->>App: return specified provider
else MINIMAX_API_KEY present<br/>without OPENAI_API_KEY
Detect-->>App: return "minimax"
else default
Detect-->>App: return "openai"
end
App->>Detect: get_llm(provider, model, temperature, ...)
activate Detect
Detect->>Detect: resolve provider (auto-detect if None)
Detect->>Detect: validate API key env var exists
Detect->>Detect: apply provider defaults & clamp temperature
alt provider == "openai"
Detect->>OpenAI: ChatOpenAI(model, api_key, temperature, ...)
OpenAI-->>Detect: initialized instance
else provider == "minimax"
Detect->>MiniMax: ChatOpenAI(model, api_key, base_url, temperature, ...)
MiniMax-->>Detect: initialized instance
end
deactivate Detect
Detect-->>App: return configured ChatOpenAI instance
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
all_prompt_engineering_techniques/multi-provider-prompting.ipynb (1)
327-339: RestoreLLM_PROVIDERusingtry/finallyto prevent leaked state.If an exception occurs inside the loop, the environment may remain mutated for later cells.
Exception-safe restore pattern
- "original_provider = os.getenv(\"LLM_PROVIDER\", \"\")\n", - "\n", - "for provider_name in SUPPORTED_PROVIDERS:\n", - " os.environ[\"LLM_PROVIDER\"] = provider_name\n", - " detected = get_provider_name()\n", - " print(f\"LLM_PROVIDER={provider_name!r} -> get_provider_name() = {detected!r}\")\n", - "\n", - "# Restore original\n", - "if original_provider:\n", - " os.environ[\"LLM_PROVIDER\"] = original_provider\n", - "else:\n", - " os.environ.pop(\"LLM_PROVIDER\", None)\n", + "original_provider = os.getenv(\"LLM_PROVIDER\", \"\")\n", + "try:\n", + " for provider_name in SUPPORTED_PROVIDERS:\n", + " os.environ[\"LLM_PROVIDER\"] = provider_name\n", + " detected = get_provider_name()\n", + " print(f\"LLM_PROVIDER={provider_name!r} -> get_provider_name() = {detected!r}\")\n", + "finally:\n", + " if original_provider:\n", + " os.environ[\"LLM_PROVIDER\"] = original_provider\n", + " else:\n", + " os.environ.pop(\"LLM_PROVIDER\", None)\n",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@all_prompt_engineering_techniques/multi-provider-prompting.ipynb` around lines 327 - 339, The cell mutates os.environ["LLM_PROVIDER"] in a loop and restores it only after, which can leak state if an exception occurs; wrap the loop in a try/finally so the original_provider (captured from os.getenv) is always restored in the finally block, using the same logic that sets os.environ["LLM_PROVIDER"] for each provider_name from SUPPORTED_PROVIDERS and calling get_provider_name() inside the try, then in finally restore original_provider or pop "LLM_PROVIDER" if original_provider is falsy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@all_prompt_engineering_techniques/multi-provider-prompting.ipynb`:
- Around line 64-66: The current bootstrap uses sys.path.insert(0,
os.path.join(os.getcwd(), "..")) which relies on os.getcwd() and can point
outside the repo; replace this with logic that deterministically locates the
repository root (e.g., walk up from the notebook/file location when available or
from Path.cwd() otherwise) by searching for repo marker files like .git,
pyproject.toml, setup.cfg or a known package folder, then insert that discovered
repo_root into sys.path (update the call site using sys.path.insert and remove
the os.getcwd() usage) so the subsequent from utils... imports resolve reliably
regardless of kernel start location.
In `@README.md`:
- Around line 323-327: The README uses duplicate subheadings "#### Overview 🔎"
and "#### Implementation 🛠️" which triggers markdownlint MD024; replace those
heading lines with non-heading labels (e.g., bold text like "**Overview 🔎**"
and "**Implementation 🛠️**") so the content and style remain but
duplicate-heading lint errors are avoided—update the instances that currently
use "#### Overview 🔎" and "#### Implementation 🛠️".
In `@tests/test_integration_minimax.py`:
- Around line 56-57: The assertion checking the capital should be made
case-insensitive to avoid flakiness: normalize the response from chain.invoke
(the variable result returned by chain.invoke({"country":
"France"}).content.strip()) and assert that a lowercase form contains "paris"
(or strip surrounding punctuation before checking) instead of asserting "Paris"
exactly; update the test near the chain.invoke call to perform the
case-insensitive containment check on result.
- Around line 20-22: The pytest skip condition uses os.getenv("MINIMAX_API_KEY")
which treats whitespace-only values as present; update the pytestmark expression
to treat values that are empty or whitespace-only as absent by retrieving the
env var (via os.getenv("MINIMAX_API_KEY") or os.environ.get) and applying
.strip() (e.g., not api_key or not api_key.strip()) so tests are skipped when
MINIMAX_API_KEY is missing or only contains whitespace; update the pytestmark
assignment that references os.getenv("MINIMAX_API_KEY") accordingly.
In `@utils/llm_provider.py`:
- Around line 107-110: Validate inputs early: ensure `provider` is a string
before calling `strip().lower()` (if not a string, either set `provider =
get_provider_name()` or raise a clear ValueError) and ensure the stripped
`provider` is non-empty; also validate `max_tokens` is an integer > 0 before use
(coerce with int() if appropriate or raise ValueError with a descriptive
message). Place these checks at the start of the function that uses
`provider`/`max_tokens` (look for references to `provider`,
`get_provider_name()`, and `max_tokens` in this module) so downstream code no
longer fails with ambiguous errors.
---
Nitpick comments:
In `@all_prompt_engineering_techniques/multi-provider-prompting.ipynb`:
- Around line 327-339: The cell mutates os.environ["LLM_PROVIDER"] in a loop and
restores it only after, which can leak state if an exception occurs; wrap the
loop in a try/finally so the original_provider (captured from os.getenv) is
always restored in the finally block, using the same logic that sets
os.environ["LLM_PROVIDER"] for each provider_name from SUPPORTED_PROVIDERS and
calling get_provider_name() inside the try, then in finally restore
original_provider or pop "LLM_PROVIDER" if original_provider is falsy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b9403a9f-eaa6-45a7-8e9f-e86d69e6b9da
📒 Files selected for processing (7)
README.mdall_prompt_engineering_techniques/multi-provider-prompting.ipynbtests/__init__.pytests/test_integration_minimax.pytests/test_llm_provider.pyutils/__init__.pyutils/llm_provider.py
Comment on lines
+64
to
+66
| "# Add the project root to sys.path so we can import utils\n", | ||
| "sys.path.insert(0, os.path.join(os.getcwd(), \"..\"))\n", | ||
| "\n", |
There was a problem hiding this comment.
CWD-dependent import bootstrap can break notebook execution.
Line 65 assumes os.getcwd() is the notebook directory. If the kernel starts from repo root (common), .. points outside the repo and from utils... fails.
Path bootstrap that is safer across launch locations
- "import os\n",
- "import sys\n",
+ "import os\n",
+ "import sys\n",
+ "from pathlib import Path\n",
@@
- "# Add the project root to sys.path so we can import utils\n",
- "sys.path.insert(0, os.path.join(os.getcwd(), \"..\"))\n",
+ "# Add project root to sys.path so we can import utils (works from repo root or notebook dir)\n",
+ "repo_root = Path.cwd().resolve()\n",
+ "if not (repo_root / \"utils\").exists():\n",
+ " repo_root = repo_root.parent\n",
+ "sys.path.insert(0, str(repo_root))\n",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@all_prompt_engineering_techniques/multi-provider-prompting.ipynb` around
lines 64 - 66, The current bootstrap uses sys.path.insert(0,
os.path.join(os.getcwd(), "..")) which relies on os.getcwd() and can point
outside the repo; replace this with logic that deterministically locates the
repository root (e.g., walk up from the notebook/file location when available or
from Path.cwd() otherwise) by searching for repo marker files like .git,
pyproject.toml, setup.cfg or a known package folder, then insert that discovered
repo_root into sys.path (update the call site using sys.path.insert and remove
the os.getcwd() usage) so the subsequent from utils... imports resolve reliably
regardless of kernel start location.
Comment on lines
+323
to
+327
| #### Overview 🔎 | ||
| Demonstrates how to use multiple LLM providers (OpenAI, MiniMax) with the same prompt engineering techniques, enabling provider comparison and avoiding vendor lock-in. | ||
|
|
||
| #### Implementation 🛠️ | ||
| Covers the `utils/llm_provider.py` helper for multi-provider support, side-by-side provider comparison for zero-shot/few-shot/CoT prompts, auto-detection via environment variables, and using MiniMax M2.5/M2.7 models through an OpenAI-compatible API. |
There was a problem hiding this comment.
Duplicate subheadings trigger markdownlint MD024.
Lines 323 and 326 reuse heading text already used elsewhere (#### Overview 🔎, #### Implementation 🛠️). Consider converting these to bold labels to keep style but avoid duplicate-heading lint failures.
✅ Lint-safe wording example
- #### Overview 🔎
- Demonstrates how to use multiple LLM providers (OpenAI, MiniMax) with the same prompt engineering techniques, enabling provider comparison and avoiding vendor lock-in.
+ **Overview 🔎**
+ Demonstrates how to use multiple LLM providers (OpenAI, MiniMax) with the same prompt engineering techniques, enabling provider comparison and avoiding vendor lock-in.
- #### Implementation 🛠️
- Covers the `utils/llm_provider.py` helper for multi-provider support, side-by-side provider comparison for zero-shot/few-shot/CoT prompts, auto-detection via environment variables, and using MiniMax M2.5/M2.7 models through an OpenAI-compatible API.
+ **Implementation 🛠️**
+ Covers the `utils/llm_provider.py` helper for multi-provider support, side-by-side provider comparison for zero-shot/few-shot/CoT prompts, auto-detection via environment variables, and using MiniMax M2.5/M2.7 models through an OpenAI-compatible API.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #### Overview 🔎 | |
| Demonstrates how to use multiple LLM providers (OpenAI, MiniMax) with the same prompt engineering techniques, enabling provider comparison and avoiding vendor lock-in. | |
| #### Implementation 🛠️ | |
| Covers the `utils/llm_provider.py` helper for multi-provider support, side-by-side provider comparison for zero-shot/few-shot/CoT prompts, auto-detection via environment variables, and using MiniMax M2.5/M2.7 models through an OpenAI-compatible API. | |
| **Overview 🔎** | |
| Demonstrates how to use multiple LLM providers (OpenAI, MiniMax) with the same prompt engineering techniques, enabling provider comparison and avoiding vendor lock-in. | |
| **Implementation 🛠️** | |
| Covers the `utils/llm_provider.py` helper for multi-provider support, side-by-side provider comparison for zero-shot/few-shot/CoT prompts, auto-detection via environment variables, and using MiniMax M2.5/M2.7 models through an OpenAI-compatible API. |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 323-323: Multiple headings with the same content
(MD024, no-duplicate-heading)
[warning] 326-326: Multiple headings with the same content
(MD024, no-duplicate-heading)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 323 - 327, The README uses duplicate subheadings
"#### Overview 🔎" and "#### Implementation 🛠️" which triggers markdownlint
MD024; replace those heading lines with non-heading labels (e.g., bold text like
"**Overview 🔎**" and "**Implementation 🛠️**") so the content and style remain
but duplicate-heading lint errors are avoided—update the instances that
currently use "#### Overview 🔎" and "#### Implementation 🛠️".
Comment on lines
+20
to
+22
| pytestmark = pytest.mark.skipif( | ||
| not os.getenv("MINIMAX_API_KEY"), | ||
| reason="MINIMAX_API_KEY not set", |
There was a problem hiding this comment.
Strengthen API-key presence check for whitespace-only values.
If MINIMAX_API_KEY is set to only spaces, tests won’t be skipped but auth will still fail.
Small guard improvement
pytestmark = pytest.mark.skipif(
- not os.getenv("MINIMAX_API_KEY"),
+ not os.getenv("MINIMAX_API_KEY", "").strip(),
reason="MINIMAX_API_KEY not set",
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_integration_minimax.py` around lines 20 - 22, The pytest skip
condition uses os.getenv("MINIMAX_API_KEY") which treats whitespace-only values
as present; update the pytestmark expression to treat values that are empty or
whitespace-only as absent by retrieving the env var (via
os.getenv("MINIMAX_API_KEY") or os.environ.get) and applying .strip() (e.g., not
api_key or not api_key.strip()) so tests are skipped when MINIMAX_API_KEY is
missing or only contains whitespace; update the pytestmark assignment that
references os.getenv("MINIMAX_API_KEY") accordingly.
Comment on lines
+56
to
+57
| result = chain.invoke({"country": "France"}).content.strip() | ||
| assert "Paris" in result |
There was a problem hiding this comment.
Make the capital-city assertion case-insensitive to reduce flakiness.
Some models return paris or include punctuation/casing variations.
More robust assertion
- result = chain.invoke({"country": "France"}).content.strip()
- assert "Paris" in result
+ result = chain.invoke({"country": "France"}).content.strip().lower()
+ assert "paris" in result🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_integration_minimax.py` around lines 56 - 57, The assertion
checking the capital should be made case-insensitive to avoid flakiness:
normalize the response from chain.invoke (the variable result returned by
chain.invoke({"country": "France"}).content.strip()) and assert that a lowercase
form contains "paris" (or strip surrounding punctuation before checking) instead
of asserting "Paris" exactly; update the test near the chain.invoke call to
perform the case-insensitive containment check on result.
Comment on lines
+107
to
+110
| if provider is None: | ||
| provider = get_provider_name() | ||
| provider = provider.strip().lower() | ||
|
|
There was a problem hiding this comment.
Add upfront input validation for provider and max_tokens.
Right now, a non-string provider (Line 109) or non-positive max_tokens (Line 131) can fail later with less actionable errors.
🛠️ Suggested hardening
def get_llm(
provider: Optional[str] = None,
model: Optional[str] = None,
temperature: float = 0.0,
max_tokens: int = 1024,
**kwargs,
) -> ChatOpenAI:
@@
if provider is None:
provider = get_provider_name()
+ elif not isinstance(provider, str):
+ raise ValueError("provider must be a string")
provider = provider.strip().lower()
@@
+ if not isinstance(max_tokens, int) or max_tokens <= 0:
+ raise ValueError("max_tokens must be a positive integer")
+
init_kwargs = {
"model": model,
"temperature": temperature,
"max_tokens": max_tokens,
"api_key": api_key,
**kwargs,
}Also applies to: 128-132
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@utils/llm_provider.py` around lines 107 - 110, Validate inputs early: ensure
`provider` is a string before calling `strip().lower()` (if not a string, either
set `provider = get_provider_name()` or raise a clear ValueError) and ensure the
stripped `provider` is non-empty; also validate `max_tokens` is an integer > 0
before use (coerce with int() if appropriate or raise ValueError with a
descriptive message). Place these checks at the start of the function that uses
`provider`/`max_tokens` (look for references to `provider`,
`get_provider_name()`, and `max_tokens` in this module) so downstream code no
longer fails with ambiguous errors.
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
1 participant
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.
Summary
utils/llm_provider.py— a multi-provider LLM factory that creates LangChain-compatible chat model instances for OpenAI and MiniMax via a singleget_llm()functionhttps://api.minimax.io/v1MINIMAX_API_KEYis set (noOPENAI_API_KEY), the system automatically selects MiniMax as the default providerFiles changed (7 files, 826 additions)
utils/llm_provider.pyutils/__init__.pyget_llm,get_provider_name,SUPPORTED_PROVIDERSall_prompt_engineering_techniques/multi-provider-prompting.ipynbtests/test_llm_provider.pytests/test_integration_minimax.pyREADME.mdtests/__init__.pyTest plan
Summary by CodeRabbit
Release Notes
New Features
Documentation