feat: add Ollama provider support

[majiayu000]

Summary

• Add LMMEngineOllama class to engine.py for local Ollama model support
• Ollama uses OpenAI-compatible API with default endpoint http://localhost:11434/v1
• Supports OLLAMA_HOST environment variable for custom endpoint

Usage

agent_s \
    --provider ollama \
    --model llama3.2-vision \
    --ground_provider ollama \
    --ground_url http://localhost:11434 \
    --ground_model llama3.2-vision \
    --grounding_width 1920 \
    --grounding_height 1080

Closes #44

Summary by CodeRabbit

• New Features

  • Added two new selectable LLM engines (DeepSeek and Qwen) with configurable endpoints and API key support.
  • Ollama engine added as a selectable option with configurable endpoint and API key.

• Tests

  • Added provider configuration tests to validate engine selection and endpoint/key handling across engines.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

Added two new concrete LLM engines (DeepSeek, Qwen) and extended LMMAgent to support engine_type values for ollama, deepseek, and qwen. Tests validating provider selection and env-var resolution were added. No existing engines were removed.

Changes

Cohort / File(s)	Summary
New LLM engine implementations gui_agents/s3/core/engine.py	Added LMMEngineDeepSeek and LMMEngineQwen: each resolves API key and base URL from parameters or env vars, lazily initializes an OpenAI-compatible client, forwards generate to chat completions with max_tokens default 4096, and uses backoff retries on API errors.
LMMAgent dispatch and provider handling gui_agents/s3/core/mllm.py	Added handling for engine_type == "ollama" (maps to OpenAI-style engine using base_url from param or OLLAMA_HOST, normalizing to end with /v1) and added support for deepseek and qwen engine types in initialization/dispatch paths.
Tests for provider selection tests/test_providers.py	New tests to validate LMMAgent initialization for ollama, deepseek, and qwen, including env-var cleanup, error when Ollama endpoint missing, and base_url normalization behavior.
Manifest / requirements manifest_file, requirements.txt	Minor additions noted in diff (dependency/manifest adjustments related to new engines/tests).

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant LMMAgent
    participant Engine (DeepSeek/Qwen/Ollama)
    participant ExternalAPI

    User->>LMMAgent: instantiate (engine_type="deepseek"/"qwen"/"ollama", params)
    LMMAgent->>LMMAgent: resolve engine_type, env vars, engine_params
    LMMAgent->>Engine: instantiate engine with engine_params
    User->>LMMAgent: add_message / request
    LMMAgent->>Engine: forward messages to generate(...)
    Engine->>ExternalAPI: HTTP/SDK call to provider endpoint (with api_key)
    ExternalAPI-->>Engine: response
    Engine-->>LMMAgent: generated reply
    LMMAgent-->>User: deliver output

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I bound through envs and endpoints wide,
New engines nested by my side.
DeepSeek, Qwen, and Ollama too,
I stitch the keys and routes for you.
A hop, a test, the messages flew. 🥕

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	The PR introduces DeepSeek and Qwen engine support in addition to Ollama, which extends beyond the scope of the linked issue #44 that requests only Ollama support.	Consider separating DeepSeek and Qwen support into a separate PR to keep changes focused on the linked issue objective, or update the issue scope to explicitly include these new providers.
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title 'feat: add Ollama provider support' accurately reflects the primary objective of adding Ollama support, though the changes also include DeepSeek and Qwen support not mentioned in the title.
Linked Issues check	✅ Passed	The code changes successfully implement Ollama provider support as requested in issue #44, including configuration via OLLAMA_HOST environment variable and base_url parameter, with proper error handling.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

🧹 Recent nitpick comments

gui_agents/s3/core/mllm.py (1)

41-57: Move import os to module level.

The os module is imported inside the conditional block, but it's a standard library module that should be imported at the top of the file for consistency and to avoid repeated imports if this code path is executed multiple times.

Additionally, there's an inconsistency: when base_url is provided via the parameter, it's used as-is, but when sourced from OLLAMA_HOST, it gets /v1 appended. Consider normalizing both paths consistently.

♻️ Suggested improvement

Add import os at the top of the file (it may already be imported elsewhere), and consider normalizing base_url consistently:

                elif engine_type == "ollama":
                    # Reuse LMMEngineOpenAI for Ollama
-                   if "base_url" not in engine_params:
-                       import os
-                       base_url = os.getenv("OLLAMA_HOST")
-                       if base_url:
-                           if not base_url.endswith("/v1"):
-                               base_url = base_url.rstrip("/") + "/v1"
-                           engine_params["base_url"] = base_url
-                       else:
-                           # RAISE ERROR instead of default
-                           raise ValueError(
-                               "Ollama endpoint must be provided via 'base_url' parameter or 'OLLAMA_HOST' environment variable."
-                           )
+                   base_url = engine_params.get("base_url") or os.getenv("OLLAMA_HOST")
+                   if not base_url:
+                       raise ValueError(
+                           "Ollama endpoint must be provided via 'base_url' parameter or 'OLLAMA_HOST' environment variable."
+                       )
+                   if not base_url.endswith("/v1"):
+                       base_url = base_url.rstrip("/") + "/v1"
+                   engine_params["base_url"] = base_url
                    if "api_key" not in engine_params:
                        engine_params["api_key"] = "ollama"
                    self.engine = LMMEngineOpenAI(**engine_params)

tests/test_providers.py (3)

4-4: Remove unused import.

MagicMock is imported but never used in the test file.

-from unittest.mock import patch, MagicMock
+from unittest.mock import patch

---

9-16: Consider using addCleanup for better test isolation.

The setUp method clears environment variables, but there's no tearDown to restore them. While the current approach works, tests could leak state if they set these variables outside of patch.dict context managers.

♻️ Alternative approach using addCleanup

def setUp(self):
    # Save and clear env vars, restore on cleanup
    for var in ["OLLAMA_HOST", "DEEPSEEK_API_KEY", "QWEN_API_KEY"]:
        original = os.environ.pop(var, None)
        if original is not None:
            self.addCleanup(os.environ.__setitem__, var, original)

---

24-32: Consider adding a test case for base_url without /v1 suffix.

This test provides base_url with /v1 already appended. Given the current implementation, a base_url parameter without /v1 would be used as-is (unlike the env var path which normalizes it). Consider adding a test to verify expected behavior in that scenario.

gui_agents/s3/core/engine.py (3)

448-458: Missing temperature instance variable for consistency.

Other engines like LMMEngineOpenAI, LMMEngineGemini, LMMEngineOpenRouter, etc. accept a temperature parameter in __init__ and store it as self.temperature. This allows forcing a specific temperature across all calls. LMMEngineDeepSeek lacks this feature, which could lead to inconsistent behavior when used interchangeably with other engines.

♻️ Suggested fix

 class LMMEngineDeepSeek(LMMEngine):
     def __init__(
-        self, base_url=None, api_key=None, model=None, rate_limit=-1, **kwargs
+        self, base_url=None, api_key=None, model=None, rate_limit=-1, temperature=None, **kwargs
     ):
         assert model is not None, "DeepSeek model id must be provided"
         self.base_url = base_url
         self.model = model
         self.api_key = api_key
         self.request_interval = 0 if rate_limit == -1 else 60.0 / rate_limit
         self.llm_client = None
+        self.temperature = temperature

Then in generate():

+        temp = self.temperature if self.temperature is not None else temperature
         return (
             self.llm_client.chat.completions.create(
                 model=self.model,
                 messages=messages,
                 max_tokens=max_new_tokens if max_new_tokens else 4096,
-                temperature=temperature,
+                temperature=temp,
                 **kwargs,
             )

---

490-500: Missing temperature instance variable (same as DeepSeek).

For consistency with other engines, consider adding temperature parameter support to LMMEngineQwen as well.

♻️ Suggested fix

 class LMMEngineQwen(LMMEngine):
     def __init__(
-        self, base_url=None, api_key=None, model=None, rate_limit=-1, **kwargs
+        self, base_url=None, api_key=None, model=None, rate_limit=-1, temperature=None, **kwargs
     ):
         assert model is not None, "Qwen model id must be provided"
         self.base_url = base_url
         self.model = model
         self.api_key = api_key
         self.request_interval = 0 if rate_limit == -1 else 60.0 / rate_limit
         self.llm_client = None
+        self.temperature = temperature

---

511-521: Consider condensing the inline comment.

The comment block explaining the default URL decision is quite verbose. A shorter comment would improve readability.

♻️ Suggested condensed comment

         base_url = self.base_url or os.getenv("QWEN_ENDPOINT_URL")
         if base_url is None:
-             # Alibaba Qwen often uses DashScope, but for compatible APIs let's assume standard or user provided
-             # If strictly Qwen (via DashScope compatible), valid URL is needed.
-             # defaulting to DashScope compatible endpoint as placeholder or rely on user.
-             # For this strict implementation, ensuring we have a URL is better if known,
-             # but generic "Qwen" usually implies usage via compatible interface (like vLLM serving Qwen or DashScope).
-             # Let's require it or default to a common one if reasonable.
-             # Given the other engines, let's enforce user providing it or env var if we don't have a single canonical one (DashScope is common).
-             # Let's default to DashScope's openai compatible endpoint if none provided?
-             # https://dashscope.aliyuncs.com/compatible-mode/v1
-             base_url = "https://dashscope.aliyuncs.com/compatible-mode/v1"
+             # Default to DashScope's OpenAI-compatible endpoint
+             base_url = "https://dashscope.aliyuncs.com/compatible-mode/v1"

---

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 234450b and d2271de.

📒 Files selected for processing (3)

• gui_agents/s3/core/engine.py
• gui_agents/s3/core/mllm.py
• tests/test_providers.py

🧰 Additional context used

🧬 Code graph analysis (1)

gui_agents/s3/core/mllm.py (1)

gui_agents/s3/core/engine.py (7)

• LMMEngineOpenAI (19-68)
• LMMEngineOpenRouter (204-250)
• LMMEngineParasail (405-445)
• LMMEnginevLLM (314-366)
• LMMEngineGemini (155-201)
• LMMEngineDeepSeek (448-487)
• LMMEngineQwen (490-538)

🪛 Ruff (0.14.11)

gui_agents/s3/core/mllm.py

52-54: Avoid specifying long messages outside the exception class

(TRY003)

gui_agents/s3/core/engine.py

450-450: Unused method argument: kwargs

(ARG002)

---

465-467: Avoid specifying long messages outside the exception class

(TRY003)

---

492-492: Unused method argument: kwargs

(ARG002)

---

507-509: Avoid specifying long messages outside the exception class

(TRY003)

🔇 Additional comments (6)

gui_agents/s3/core/mllm.py (2)

58-61: LGTM!

The DeepSeek and Qwen engine dispatch follows the established pattern used by other engines in this file.

---

156-157: LGTM!

Adding DeepSeek and Qwen to the isinstance check correctly enables API-style message handling for these OpenAI-compatible engines.

tests/test_providers.py (2)

18-22: LGTM!

Good coverage for the error case when Ollama endpoint is missing.

---

34-65: LGTM!

Good test coverage for the happy paths of Ollama (via env var), DeepSeek, and Qwen initialization. The comment on line 55-56 correctly notes that the default URL resolution happens at generate() time.

gui_agents/s3/core/engine.py (2)

459-487: LGTM!

The generate() method follows the established pattern with proper API key validation, lazy client initialization, backoff retry handling, and **kwargs forwarding.

---

523-538: LGTM!

The generate() method correctly implements lazy client initialization and properly forwards **kwargs to the API call.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

gui_agents/s3/core/engine.py (1)

448-464: Initialization looks correct.

The __init__ method follows the pattern of other engine implementations. Note that request_interval is calculated but not used in the generate method—this is consistent with several other engine classes in the file (e.g., LMMEngineGemini, LMMEngineOpenRouter), suggesting it may be used externally or reserved for future rate limiting.

The **kwargs parameter is unused but matches the pattern across all engine classes for forward compatibility.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2cb57fb and 432a9f4.

📒 Files selected for processing (2)

• gui_agents/s3/core/engine.py
• gui_agents/s3/core/mllm.py

🧰 Additional context used

🧬 Code graph analysis (1)

gui_agents/s3/core/mllm.py (1)

gui_agents/s3/core/engine.py (1)

• LMMEngineOllama (448-491)

🪛 Ruff (0.14.10)

gui_agents/s3/core/engine.py

456-456: Unused method argument: kwargs

(ARG002)

---

474-474: Unused method argument: kwargs

(ARG002)

🔇 Additional comments (4)

gui_agents/s3/core/mllm.py (3)

9-9: LGTM!

The import is correctly added and follows the existing pattern.

---

39-40: LGTM!

The engine initialization follows the established pattern and is correctly integrated into the engine selection logic.

---

135-135: LGTM!

Correctly includes LMMEngineOllama in the API-style inference type check, consistent with Ollama's OpenAI-compatible API.

gui_agents/s3/core/engine.py (1)

476-480: Base URL handling is well-designed.

The fallback chain (instance attribute → OLLAMA_HOST env var → localhost default) and automatic /v1 suffix normalization provide good flexibility and user convenience.

[Richard-Simular]

Hi, thanks for the contribution!

I believe for any OpenAI-compatible provider you can just init a LMMEngineOpenAI with a custom base_url:

engine = LMMEngineOpenAI(
    base_url="http://localhost:11434/v1",
    api_key="apikey",
    model="llama3.2"
)

Can you try if this works for your use case? If so, I'd prefer to avoid adding additional boilerplate code

[Richard-Simular]

Thanks!
Instead of defaulting to localhost, raising an actionable error is better and easier for devs to debug.

Would you mind including DeepSeek and Qwen into this PR and close #164?

I think we can check in after the change and some test