graphiti icon indicating copy to clipboard operation
graphiti copied to clipboard

Refactor recommendation: no internal ghost variables

Open zeroasterisk opened this issue 5 months ago • 7 comments

There are many ghost params which are not configurable. For example in the gemini provider, you switch between models, large and small:

DEFAULT_MODEL = 'gemini-2.5-flash'
DEFAULT_SMALL_MODEL = 'gemini-2.5-flash-lite-preview-06-17'

This is problematic because the flash lite model has location constraints (global only) in Vertex AI, which differ from the flash model.

I recommend you allow a configuration of all clients + models, similar to how I did on PR #679, and not swap them out in code.

The (model + client + config) choice is really a provider, so you could simply collect all the provider variations you need up front, and swap between them as needed.

It will be functionally about the same, but simpler to customize and not be surprised by internal variables not changeable without editing the package.

zeroasterisk avatar Jul 06 '25 05:07 zeroasterisk

Thanks for the suggestion. @claude created a PR with a proposed solution based on this suggestion. Think hard about a generalized solution while limiting breaking changes.

danielchalef avatar Jul 09 '25 14:07 danielchalef

Claude finished @danielchalef's task —— View jobclaude/issue-681-20250709_144821Create PR ➔


Ghost Variables Issue Resolution ✅

Successfully implemented a generalized solution for eliminating internal ghost variables while maintaining backward compatibility.

Todo List:

  • [x] Set up development environment and analyze codebase
  • [x] Examine current LLM client implementations for ghost variables
  • [x] Review Gemini provider specific issue with model switching
  • [x] Check PR #679 for the desired configuration pattern
  • [x] Design generalized configurable provider system
  • [x] Implement the solution
  • [x] Run tests and linting
  • [x] Create PR with documentation

Solution Summary:

✅ Complete Provider Configuration System Implemented

Key Features:

  • Centralized Configuration: All provider defaults moved to provider_defaults.py
  • Environment Variable Support: Easy customization without code changes
  • Backward Compatibility: Existing configurations continue to work unchanged
  • Provider-Specific Defaults: Each provider has optimized model selections

Ghost Variables Eliminated:

  • gemini_client.py: Replaced hardcoded model constants
  • openai_base_client.py: Replaced hardcoded model constants
  • openai_generic_client.py: Updated to use provider defaults
  • anthropic_client.py: Updated to use provider defaults
  • groq_client.py: Updated to use provider defaults
  • edge_operations.py: Made extract_edges_max_tokens configurable
  • gemini_reranker_client.py: Updated reranker to use provider defaults
  • openai_reranker_client.py: Updated reranker to use provider defaults

Environment Variables Added:

# Example: Solve Gemini Vertex AI location constraints
export GEMINI_DEFAULT_MODEL="gemini-2.5-flash"
export GEMINI_DEFAULT_SMALL_MODEL="gemini-2.5-flash"  # Use flash instead of flash-lite

# Other providers
export OPENAI_DEFAULT_MODEL="gpt-4o"
export ANTHROPIC_DEFAULT_MODEL="claude-3-5-sonnet-latest"
export GROQ_DEFAULT_MODEL="llama-3.1-70b-versatile"

Files Modified:

  • Created provider_defaults.py - Core configuration system
  • Updated 5 LLM clients to use configurable defaults
  • Updated 2 cross-encoder reranker clients
  • Updated edge operations for configurable token limits
  • Added comprehensive documentation in PROVIDER_CONFIGURATION.md

Benefits Achieved:

  • ✅ No more hardcoded "ghost variables"
  • ✅ Easy model updates via environment variables
  • ✅ Addresses Gemini Vertex AI location constraint issues
  • ✅ Maintains full backward compatibility
  • ✅ Supports all existing and future providers
  • ✅ Reduces maintenance burden for model updates

claude[bot] avatar Jul 09 '25 14:07 claude[bot]

@zeroasterisk Is this still relevant? Please confirm within 14 days or this issue will be closed.

claude[bot] avatar Oct 05 '25 00:10 claude[bot]

Yes, I think it's still a good design principle to let users control the config.

I think it looks better. But this is your project and not mine.

zeroasterisk avatar Oct 05 '25 16:10 zeroasterisk

@zeroasterisk Is this still an issue? Please confirm within 14 days or this issue will be closed.

claude[bot] avatar Oct 22 '25 00:10 claude[bot]

@zeroasterisk Is this still an issue? Please confirm within 14 days or this issue will be closed.

claude[bot] avatar Oct 29 '25 00:10 claude[bot]

@zeroasterisk Is this still relevant? Please confirm within 14 days or this issue will be closed.

claude[bot] avatar Nov 17 '25 00:11 claude[bot]