add new mode 'openai_compatible' #278
Conversation
Summary of ChangesHello @grandgen, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the system's flexibility by introducing an 'openai_compatible' mode for Large Language Model (LLM) integration. This new mode allows the application to seamlessly connect with various LLM providers, including Google's Gemini, that expose an API compatible with OpenAI's specification. The changes involve updating LLM instantiation logic, default configurations, CLI options, and documentation, making it easier for users to switch between different LLM backends using environment variables. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for OpenAI-compatible endpoints like Google Gemini, which is a valuable enhancement. The implementation correctly introduces the new provider option across the configuration and CLI. However, there are a few significant issues that need to be addressed. The configuration for the new provider is not handled consistently, leading to a critical bug where settings from the config object are ignored in favor of direct environment variable access. There are also inconsistencies in environment variable naming in .env.example and a misleading hardcoded URL in the CLI utility. Additionally, several large, non-essential files have been added to the repository, including AI-generated analysis documents and a package-lock.json for a Python project, which should be removed to maintain repository health. I've left specific comments with suggestions for each of these points.
| elif self.config["llm_provider"].lower() == "openai_compatible": | ||
| self.deep_thinking_llm = ChatOpenAI( | ||
| model=self.config["deep_think_llm"], | ||
| api_key=os.environ.get("OPENAI_COMPATIBLE_API_KEY"), | ||
| base_url=os.environ.get("OPENAI_COMPATIBLE_BASE_URL"), | ||
| ) | ||
| self.quick_thinking_llm = ChatOpenAI( | ||
| model=self.config["quick_think_llm"], | ||
| api_key=os.environ.get("OPENAI_COMPATIBLE_API_KEY"), | ||
| base_url=os.environ.get("OPENAI_COMPATIBLE_BASE_URL"), | ||
| ) |
There was a problem hiding this comment.
The implementation for the openai_compatible provider reads the API key and base URL directly from environment variables using os.environ.get(). This bypasses the configuration provided in self.config, which is intended to be the single source of truth for all providers. This approach breaks the ability to configure the openai_compatible provider via the config dictionary passed to the TradingAgentsGraph constructor, making the configuration system inconsistent and buggy. Any configuration overrides for this provider will be ignored.
| elif self.config["llm_provider"].lower() == "openai_compatible": | |
| self.deep_thinking_llm = ChatOpenAI( | |
| model=self.config["deep_think_llm"], | |
| api_key=os.environ.get("OPENAI_COMPATIBLE_API_KEY"), | |
| base_url=os.environ.get("OPENAI_COMPATIBLE_BASE_URL"), | |
| ) | |
| self.quick_thinking_llm = ChatOpenAI( | |
| model=self.config["quick_think_llm"], | |
| api_key=os.environ.get("OPENAI_COMPATIBLE_API_KEY"), | |
| base_url=os.environ.get("OPENAI_COMPATIBLE_BASE_URL"), | |
| ) | |
| elif self.config["llm_provider"].lower() == "openai_compatible": | |
| self.deep_thinking_llm = ChatOpenAI( | |
| model=self.config["deep_think_llm"], | |
| api_key=self.config.get("openai_compatible_api_key"), | |
| base_url=self.config.get("openai_compatible_base_url"), | |
| ) | |
| self.quick_thinking_llm = ChatOpenAI( | |
| model=self.config["quick_think_llm"], | |
| api_key=self.config.get("openai_compatible_api_key"), | |
| base_url=self.config.get("openai_compatible_base_url"), | |
| ) |
| ("OpenAI", "https://api.openai.com/v1"), | ||
| ("Anthropic", "https://api.anthropic.com/"), | ||
| ("Google", "https://generativelanguage.googleapis.com/v1"), | ||
| ("OpenAI Compatible", "http://127.0.0.1:8080/v1"), |
There was a problem hiding this comment.
The base URL for "OpenAI Compatible" is hardcoded to a local address (http://127.0.0.1:8080/v1). This is misleading for users who want to use remote services like Google Vertex AI, as mentioned in the documentation. Furthermore, this hardcoded URL is passed to the configuration but is ultimately ignored by the openai_compatible provider logic in trading_graph.py, which reads the base URL from an environment variable instead. This creates an inconsistent and confusing configuration flow.
The configuration should be handled consistently, preferably through the config object, which should be the single source of truth.
| ALPHA_VANTAGE_API_KEY=alpha_vantage_api_key_placeholder | ||
| OPENAI_API_KEY=openai_api_key_placeholder | ||
| OPENAI_API_KEY=openai_api_key_placeholder | ||
| GOOGLE_API_KEY=google_api_key_placeholder |
There was a problem hiding this comment.
The new environment variable GOOGLE_API_KEY is inconsistent with the rest of the changes, which use OPENAI_COMPATIBLE_API_KEY for the new OpenAI-compatible provider. This can be confusing for users setting up their environment.
To maintain consistency, please use OPENAI_COMPATIBLE_API_KEY and also add OPENAI_COMPATIBLE_BASE_URL on a new line, as documented in README.md.
OPENAI_COMPATIBLE_API_KEY=your_api_key_placeholder
| # SPEC-1-TradingAgents-Codebase-Review | ||
|
|
||
| ## Background | ||
|
|
||
| A careful review was performed of the `tradingAgents` project provided in the uploaded ZIP. The codebase implements a multi-agent trading research framework that composes specialized analyst, researcher, risk-manager and trader components into a directed-graph execution pipeline built on top of `langgraph` and `langchain` (OpenAI connectors are used). The purpose is research and analysis (not financial advice) and the system designs an opinionated, debatable research process combining news, social sentiment, fundamentals, and technical analysis for investment recommendations. | ||
|
|
||
| ## Files & repository layout (high level) | ||
|
|
||
| Key top-level entries: | ||
|
|
||
| * `README.md` — long project README with architecture overview, CLI screenshots, and examples. | ||
| * `main.py` — example runner that constructs a graph using `DEFAULT_CONFIG` and the `TradingAgentsGraph` abstraction. | ||
| * `tradingagents/` — main package with subpackages: | ||
|
|
||
| * `agents/` — agent factories and utilities (analysts, researchers, risk_mgmt, managers, trader, utils) | ||
| * `dataflows/` — vendor adapters (yfinance, alpha_vantage, google news, openai backends, reddit utils, etc.) | ||
| * `graph/` — the orchestration pieces built on `langgraph` (setup, conditional logic, propagation, reflection, trading_graph) | ||
| * `default_config.py` — default configuration and vendor selections | ||
| * `docs/` — `InitAnalysis.md` and `Prompts.md` containing higher-level analysis and prompt templates. | ||
| * `requirements.txt` / `pyproject.toml` — dependency lists (langchain-openai, langgraph, chromadb, yfinance, etc.) | ||
|
|
||
| ## High-level architecture & execution model | ||
|
|
||
| 1. **Graph-First Orchestration**: The core runtime is a stateful directed graph implemented with `langgraph`. The graph contains *tool nodes* (agents, data tools, wrappers) and *message states* which pass agent outputs forward. There are clear `START` / `END` states and per-node routing decisions. | ||
|
|
||
| 2. **Agents-as-ToolNodes**: Agents are implemented as factory functions that accept an LLM (ChatOpenAI wrappers are used) and return functions that operate on the shared `state` dictionary. These are wired into `ToolNode`s in `GraphSetup`. | ||
|
|
||
| 3. **Two-Tier LLM Strategy**: The code distinguishes a `quick_think_llm` for fast, less expensive calls and a `deep_think_llm` for longer, more contemplative tasks (e.g., judge/reflector). Configurable through `DEFAULT_CONFIG`. | ||
|
|
||
| 4. **Specialized Agent Roles**: | ||
|
|
||
| * Analysts: `market`, `news`, `social_media`, `fundamentals` — collect & synthesize different modalities. | ||
| * Researchers: `bull` and `bear` researchers that debate investment theses. | ||
| * Risk Debators: `aggressive`, `conservative`, `neutral` — produce risk-framed arguments. | ||
| * Managers: `research_manager` (coordinates debate/judge flow) and `risk_manager` (integrates risk decisions). | ||
| * Trader: a `trader` node that translates recommendations into actions/instructions. | ||
|
|
||
| 5. **Dataflow Layer**: `tradingagents/dataflows` contains vendor adapters and wrappers: yfinance, alpha_vantage, Google/GoogleNews scraping utilities, Reddit scrapers, an OpenAI-based news summarizer, and several helpers for indicators. The design allows swapping vendors via configuration. | ||
|
|
||
| 6. **Memory / Long-Term Storage**: `agents/utils/memory.py` implements `FinancialSituationMemory` backed by `chromadb` and OpenAI embeddings. Memories can be queried to retrieve past situations and recommendations. | ||
|
|
||
| 7. **Reflection & Propagation**: There are distinct components that manage state propagation across nodes (`Propagator`), reflection for learning from outcomes (`Reflector`), and signal processing for derived signals (`SignalProcessor`). These are orchestrated by `TradingAgentsGraph`. | ||
|
|
||
| 8. **Prompt Templates**: A large number of prompt templates and instructions are baked into manager and agent node implementations (docs/Prompts.md contains many of them). The system depends heavily on prompting to shape agent behavior. | ||
|
|
||
| ## Data flow (simplified) | ||
|
|
||
| 1. Input (ticker + trade_date + config) -> graph `START`. | ||
| 2. Analyst nodes call dataflow adapters to fetch: OHLC history (yfinance/alpha_vantage), indicators, Reddit posts, news articles, macro data. | ||
| 3. Each analyst returns a report string placed into `state` (e.g., `market_report`, `news_report`, `sentiment_report`, `fundamentals_report`). | ||
| 4. Researcher agents (bull/bear) read these reports and start a debate sequence coordinated by `research_manager` — repeated message passing through the graph with conditional branching. | ||
| 5. Risk debators augment the debate with risk framing; `research_manager` uses `judge` logic to decide whether to continue debate or finalize. | ||
| 6. `invest_judge` / `invest_manager` produce a recommendation and a trader plan; `trader` node converts that into final actions. | ||
| 7. `Reflector` runs post-hoc analyses and stores reflections/memories in `chromadb`. | ||
|
|
||
| ## Integrations & External Dependencies | ||
|
|
||
| * **LLM**: `langchain_openai.ChatOpenAI` is used (wrapper for OpenAI). The code also contains references to different model names (e.g., `gpt-4o-mini`). The `DEFAULT_CONFIG` keeps model names configurable. | ||
| * **langgraph**: Graph orchestration is built on `langgraph` and its `ToolNode`, `StateGraph` primitives. | ||
| * **Data vendors**: `yfinance`, `alpha_vantage` (adapter), `finnhub` wrappers (present in requirements), Google News scrapers, Reddit (`praw`) utilities, local offline adapters for reproducible runs. | ||
| * **Vector DB**: `chromadb` for memory; embeddings are fetched via the OpenAI client (abstracted in `memory.py`) — uses `text-embedding-3-small` or `nomic-embed-text` depending on backend. | ||
| * **Misc**: `stockstats` for indicators, `pandas`, `backtrader` listed as dependency (not deeply wired in core flow but present), `questionary` & CLI tools. | ||
|
|
||
| ## Observability, Telemetry & Logging | ||
|
|
||
| * There is **no centralized telemetry or metrics system** in the codebase (no Prometheus, no Sentry, and no logging imports were found). Internal state is tracked in Python structures (`TradingAgentsGraph.log_states_dict`) and the final state is serializable to JSON. | ||
| * Short-term print/console oriented flows are used (CLI screenshots showing progress). This implies limited production-grade observability and minimal error/tracing capabilities. | ||
|
|
||
| ## Security & Secrets | ||
|
|
||
| * The project relies on environment variables (an `.env.example` is present). Secrets are expected to be loaded via `dotenv` in `main.py`. | ||
| * Several vendor adapters will require API keys (Alpha Vantage, Finnhub, OpenAI, etc.). There is no central secrets manager integration or explicit advice for safely rotating keys. | ||
|
|
||
| ## Design patterns & code organization | ||
|
|
||
| * **Graph / Node pattern**: Orchestration is expressed as ToolNode nodes; nodes operate on a shared mutable `state` dict. This is an event/state propagation pattern. | ||
| * **Adapter pattern**: The `dataflows` directory acts as vendor adapters with a unified interface (`get_YFin_data_online`, `get_stock`, `get_news`, etc.). This enables vendor swapping through `DEFAULT_CONFIG`. | ||
| * **Factory functions**: Agents are provided through `create_*` functions that accept an LLM and return node behavior; this makes injection of different LLM wrappers straightforward. | ||
| * **Strategy / Debate**: The debate mechanism between bull/bear researchers plus judge is implemented as a state machine using Graph conditional transitions. | ||
| * **Memory as a service**: Memory is encapsulated in `FinancialSituationMemory` with query & add methods, following a simple repository pattern. | ||
|
|
||
| ## Strengths & Good choices | ||
|
|
||
| * Clear separation between **data adapters**, **agent logic**, and **orchestration** (graph) — makes the codebase understandable and swappable. | ||
| * Two-LLM tiering for cost/performance tradeoffs is pragmatic. | ||
| * Use of `chromadb` + embeddings to record and retrieve prior situations is a sensible approach for agent memory and long-term learning. | ||
| * `langgraph` + `langchain` give a native architectural fit for agentic message-passing workflows. | ||
| * Extensive prompt engineering (templates + docs) already present. | ||
|
|
||
| ## Risks, gaps & weaknesses (observations) | ||
|
|
||
| 1. **Observability & Logging**: No structured logging or metrics. Hard to debug runs in production or trace LLM costs and latencies. | ||
| 2. **Error handling & retries**: Vendor adapters appear to be synchronous and may lack robust retry/backoff logic for network/API failures. | ||
| 3. **Testing**: There is little evidence of unit or integration tests for critical components (no tests besides `test.py` placeholder). | ||
| 4. **Concurrency & throughput**: The design uses synchronous LLM calls and blocking data fetches — scaling to many tickers or parallel research runs will be slow. | ||
| 5. **Secrets management**: Keys fetched from `.env` — no mention of vault integration or per-environment configurations. | ||
| 6. **Vector DB persistence**: `chromadb` client is created without persistent configuration. Depending on how chroma is set up, memory may be ephemeral. Also the embedding provider selection is hard-coded per `backend_url`. | ||
| 7. **Resource usage & cost controls**: No built-in quota or estimation of LLM tokens, cost accounting, or budgets per run. | ||
| 8. **Tight coupling to `langgraph`**: Heavy use of `langgraph` primitives makes migration to another orchestrator non-trivial. | ||
| 9. **Missing telemetry for LLM calls**: No per-prompt logging of response latency, token usage, or errors. | ||
| 10. **Limited type enforcement**: Project mixes typed dicts with untyped state dict usage which can lead to runtime errors. | ||
|
|
||
| ## Concrete recommendations (quick wins) | ||
|
|
||
| ### Short-term (low-effort, high-impact) | ||
|
|
||
| * Add structured logging (Python `logging`) at entrypoints: agent invocations, external vendor calls, and LLM call wrappers. Log context (ticker, date, node name). | ||
| * Add simple metrics counters (e.g., via Prometheus client or expose a `/metrics` endpoint) for counts of LLM calls, failures, vendor call latencies. | ||
| * Centralize LLM invocation through a single wrapper that records token usage, latency, and catches common API errors — makes adding retries and fallback models easier. | ||
| * Implement retry/backoff for external HTTP/API calls using `tenacity` or `httpx` with retry. | ||
| * Persist chromadb collection to disk or configure a persistent deployment so memories survive across runs. | ||
| * Add basic unit tests for `dataflows` adapters (mock vendor responses) and `agents`’ deterministic pieces. | ||
|
|
||
| ### Medium-term | ||
|
|
||
| * Introduce asynchronous execution (async/await) and batch operations when requesting market data for multiple tickers. | ||
| * Add a plugin-style adapter registry for vendor integrations so new providers can be registered without touching internal imports. | ||
| * Introduce a pluggable telemetry backend with traces for each graph run (OpenTelemetry) and LLM spans. | ||
| * Add a configuration profile system for environments (dev/staging/prod) and secret management guidance (Vault, AWS Secrets Manager, or similar). | ||
| * Improve type coverage with `mypy` and stricter typed data models for the shared `state` (pydantic models could help). | ||
|
|
||
| ### Refactors & larger changes | ||
|
|
||
| * Decouple orchestration from `langgraph` with an internal thin interface, so other orchestrators could be used in future. | ||
| * Abstract vector DB usage behind an interface with multiple implementations (Chroma, Pinecone, etc.) and configuration-driven provider selection. | ||
| * Add a cost-control and LLM budgeter that estimates tokens and optionally refuses expensive calls beyond configured budgets. | ||
| * Create end-to-end integration/infrastructure tests that run a short scenario with mocked LLMs to validate the graph transitions and state outputs. | ||
|
|
||
| ## Suggested file-by-file highlights (non-exhaustive) | ||
|
|
||
| * `tradingagents/graph/setup.py` — central wiring that constructs `ToolNode`s; good place to centralize LLM wrapper. | ||
| * `tradingagents/agents/*` — agent factories: prefer to make these pure functions that only transform inputs and call an injected `llm_api` helper. Avoid reading global config inside the node body. | ||
| * `tradingagents/dataflows/*` — vendor adapters: add standardized error classes, timeouts and retry. | ||
| * `tradingagents/agents/utils/memory.py` — ensure Chroma collection initialization accepts persistence directory and does not create ephemeral in-memory collections silently. | ||
| * `tradingagents/graph/trading_graph.py` — the Graph runtime; add hooks for metrics, logging and structured state snapshots. | ||
|
|
||
| ## PlantUML: Simplified component diagram | ||
|
|
||
| ```plantuml | ||
| @startuml | ||
| package "Client / CLI" { | ||
| [User Input] | ||
| } | ||
| package "Orchestrator" { | ||
| [TradingAgentsGraph] --> [GraphSetup] | ||
| [TradingAgentsGraph] --> [Propagator] | ||
| [TradingAgentsGraph] --> [Reflector] | ||
| } | ||
| package "Agents" { | ||
| [Analysts] --> [Researcher(s)] | ||
| [Researcher(s)] --> [Research Manager] | ||
| [Risk Debators] --> [Risk Manager] | ||
| [Research Manager] --> [Trader] | ||
| } | ||
| package "Data & Services" { | ||
| [YFinance / AlphaVantage / Finnhub] | ||
| [Google News / Reddit] | ||
| [OpenAI (LLM & Embeddings)] | ||
| [ChromaDB] | ||
| } | ||
| [Analysts] --> [YFinance / News / Reddit] | ||
| [Research Manager] --> [OpenAI (deep_llm)] | ||
| [Agents] --> [ChromaDB] | ||
| @enduml | ||
| ``` No newline at end of file |
There was a problem hiding this comment.
This file, along with InitAnalysis.md, Prompts.md, and claude-sonnet-45.md, appears to be an AI-generated analysis of the codebase. Committing these large, non-user-facing documents adds unnecessary clutter and size to the repository. It's best to keep the repository focused on source code and essential user documentation.
Please consider removing these files from the pull request. If they are needed for internal reference, they should be stored outside the project's repository.
| { | ||
| "name": "TradingAgents", | ||
| "lockfileVersion": 3, | ||
| "requires": true, | ||
| "packages": {} | ||
| } |
There was a problem hiding this comment.
This package-lock.json file is used for Node.js projects to lock dependency versions. Since this is a Python project that uses requirements.txt, this file is unnecessary and seems to have been added by mistake. It should be removed.
1 participant
Completed all the necessary code changes to support Google's Gemini model via an OpenAI-compatible endpoint. Here is a summary of the changes:
trading_graph.py: Updated the LLM instantiation logic to include a new "openai_compatible" provider. This new provider uses the ChatOpenAI class and is configured using the OPENAI_COMPATIBLE_API_KEY and OPENAI_COMPATIBLE_BASE_URL environment variables.
default_config.py: Updated the default configuration to include the new "openai_compatible" provider and the associated settings.
utils.py: Added "OpenAI Compatible" to the list of LLM providers in the CLI and added a placeholder for the Gemini Pro model.
README.md: Updated the documentation to instruct users on how to configure the system for Vertex AI and other OpenAI-compatible endpoints.
You can now configure the project to use your Gemini API key by setting the appropriate environment variables in your .env file and selecting the "OpenAI Compatible" provider when running the CLI.