Skip to content

Conversation

@mandelbro
Copy link
Owner

@mandelbro mandelbro commented Oct 7, 2025

Summary

This change extends the MCP server configuration to fully support separate configuration for LLMs and embedding models so you can, for example, use OpenAI or Bedrock for LLM support and use nomic-embed-text locally via Ollama for embedding.

llm:
  # LLM Configuration - Using OpenAI API
  model: "gpt-5-mini"
  small_model: "gpt-5-nano"
  base_url: "https://api.openai.com/v1"
  temperature: 0.1
  max_tokens: 25000

  # LLM-specific parameters
  model_parameters:
    presence_penalty: 0.0
    frequency_penalty: 0.0
    top_p: 1.0
    n: 1
    stream: false

embedder:
  # Embedder Configuration - Using Local Ollama
  model: "nomic-embed-text"
  base_url: "http://localhost:11434/v1"
  dimension: 768

  # Embedder-specific parameters
  model_parameters:
    num_ctx: 4096

In addition, this PR includes several scripts for testing and verifying your local settings to support AWS Bedrock connections

@mandelbro mandelbro requested a review from Copilot October 7, 2025 20:12
cursor[bot]

This comment was marked as outdated.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds dual support for local and remote models by extending the MCP server configuration to fully support separate configuration for LLMs and embedding models. The key change enables mixed provider setups where you can use different providers for LLM and embeddings (e.g., OpenAI/Bedrock for LLM and local Ollama for embeddings).

  • Unified configuration system allowing mixed provider configurations in a single file
  • Enhanced provider detection logic that automatically determines the provider type based on base_url patterns
  • Comprehensive testing and validation scripts for AWS Bedrock endpoint connections and configurations

Reviewed Changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
tests/test_bedrock_endpoint.py Comprehensive test suite for validating Bedrock endpoint configurations and connections
src/config_loader.py Enhanced config loader with unified config support and provider fallback logic
src/config/llm_config.py Updated LLM configuration with unified config loading and automatic provider detection
src/config/embedder_config.py Updated embedder configuration with unified config loading and base_url/dimension support
scripts/test_openai_compatibility.py Script to test OpenAI-compatible API endpoints with SSL verification
scripts/test_ollama_embeddings.sh Shell script for validating local Ollama embedding model setup
scripts/test_embedding_models.sh Shell script for testing multiple embedding model naming conventions on LLM Gateway
scripts/test_auth_methods.sh Shell script for testing different authentication methods and endpoints
scripts/setup_ssl_env.sh Automated SSL certificate environment setup script
scripts/debug_bedrock_connection.sh Comprehensive debugging script for Bedrock gateway connectivity
docs/UNIFIED_CONFIG_GUIDE.md Complete guide for the new unified configuration system
docs/SSL_CERTIFICATE_SETUP.md Documentation for SSL certificate configuration for Salesforce endpoints
docs/QUICK_START_SSL_SETUP.md Quick start guide for SSL certificate setup
docs/MIXED_PROVIDER_SETUP.md Guide for configuring mixed provider setups
docs/BEDROCK_QUICK_START.md Quick start guide for Bedrock endpoint testing
config/config.yml Example unified configuration file with mixed provider setup

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +104 to +106
This method is deprecated in favor of load_unified_config() but maintained
for backwards compatibility.
Copy link

Copilot AI Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The deprecation notice should specify a timeline or version when the method will be removed, and provide clear migration instructions.

Suggested change
This method is deprecated in favor of load_unified_config() but maintained
for backwards compatibility.
DEPRECATED: This method is deprecated in favor of load_unified_config() and will be removed in version 2.0.0.
Migration instructions:
- Consolidate your provider-specific configuration files into a single `config.local.yml` file.
- Use the `load_unified_config()` method to load configuration.
- Refer to the documentation for details on the unified configuration format.

Copilot uses AI. Check for mistakes.
Comment on lines 76 to 77
# Detect if this is Ollama based on base_url
is_ollama = "localhost:11434" in base_url or "ollama" in base_url.lower()
Copy link

Copilot AI Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hardcoded localhost:11434 port and string matching for provider detection could be fragile. Consider using a more robust URL parsing approach with configurable port detection.

Copilot uses AI. Check for mistakes.
Comment on lines 170 to 171
import httpx
llm_client.client._client = httpx.AsyncClient(verify=ssl_verify, timeout=30.0)
Copy link

Copilot AI Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Directly accessing private attribute _client is fragile and could break with library updates. Consider using the public API or dependency injection for SSL configuration.

Copilot uses AI. Check for mistakes.
Comment on lines 246 to 250
# Use base_url and dimension from config (already loaded from YAML)
embedder_config = OpenAIEmbedderConfig(
api_key=self.api_key, embedding_model=self.model
api_key=self.api_key,
embedding_model=self.model,
base_url=self.base_url,
Copy link

Copilot AI Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential issue if self.base_url or self.dimension are None when creating OpenAIEmbedderConfig. Add validation to ensure required fields are present.

Copilot uses AI. Check for mistakes.
@@ -0,0 +1,223 @@
#!/usr/bin/env zsh
Copy link

Copilot AI Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using zsh as the shebang reduces portability. Consider using #!/bin/bash or #!/bin/sh for better compatibility across different systems.

Suggested change
#!/usr/bin/env zsh
#!/usr/bin/env bash

Copilot uses AI. Check for mistakes.
Comment on lines 178 to 186
messages = cast(list[ChatCompletionMessageParam], [
{
"role": "system",
"content": "You are a helpful assistant. Respond concisely.",
},
{
"role": "user",
"content": "Say 'Hello, Bedrock endpoint is working!' and nothing else.",
},
Copy link

Copilot AI Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The hardcoded test message could be extracted to a constant or configuration to make it easier to maintain and modify across different test scenarios.

Copilot uses AI. Check for mistakes.
Introduces unified configuration via config/config.yml for flexible provider selection for LLM and embedder.

Refactors config loading logic to prioritize unified config, adds SSL/TLS certificate utilities for secure connections, and patches OpenAI/Azure clients to support custom SSL certificates using httpx. Updates related modules to use new SSL utilities and improves provider detection logic.
Introduces scripts/test_config.sh for validating Graphiti MCP Server configuration, environment variables, dependencies, and service connectivity. Adds scripts/test_openai_compatibility.py to verify LLM Gateway support for OpenAI-compatible and native API formats.
@mandelbro mandelbro force-pushed the feat/dual-support-for-local-and-remote-models branch from 77f7575 to 5084580 Compare October 7, 2025 22:34
Introduces scripts/debug_bedrock_connection.sh to test Bedrock gateway connectivity, including SSL certificate validation, environment variable checks, endpoint accessibility, and model availability. The script provides detailed output and troubleshooting steps for connection and authentication issues.
Introduces docs/UNIFIED_CONFIG_GUIDE.md, detailing the unified config system for Graphiti MCP Server. The guide covers setup, provider mixing, migration from provider-specific configs, troubleshooting, and best practices for configuration management.
@mandelbro mandelbro force-pushed the feat/dual-support-for-local-and-remote-models branch from 5084580 to e1f1acc Compare October 7, 2025 22:35
cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

@mandelbro mandelbro force-pushed the feat/dual-support-for-local-and-remote-models branch from d611236 to d31f1d4 Compare October 10, 2025 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants