Conversation
…ce, Brave and Perplexity Add BaseSearchService base class with factory pattern routing by engine config. Add BraveSearchService (httpx, native offset) and PerplexitySearchService (httpx, over-fetch+slice). Extend SearchConfig with engine, brave_*, perplexity_* fields. Refactor TavilySearchService to inherit BaseSearchService.
… with shared BaseSearchTool base Add _BaseSearchTool base class with shared fields and __call__ logic Refactor WebSearchTool to inherit from _BaseSearchTool Add TavilySearchTool, BraveSearchTool, PerplexitySearchTool standalone tools Fix description ClassVar inheritance in __init_subclass__ Add Tavily key guard to ExtractPageContentTool Update exports and config examples with multi-provider settings
…ti-provider support Add test_search_services.py with factory, conversion and missing API key tests Update test_tools.py: patch BaseSearchService instead of TavilySearchService, add init/execution tests for BraveSearchTool, PerplexitySearchTool, TavilySearchTool
…Service and adjust tool exports for consistency
sgr_agent_core/base_tool.py
Outdated
| def __init_subclass__(cls, **kwargs) -> None: | ||
| super().__init_subclass__(**kwargs) | ||
| if cls.__name__ not in ("BaseTool", "MCPBaseTool"): | ||
| if cls.__name__ not in ("BaseTool", "MCPBaseTool", "_BaseSearchTool"): |
There was a problem hiding this comment.
Думаю _BaseSearchTool тут лишне, потому что _BaseSearchTool и так наследует BaseTool.
There was a problem hiding this comment.
_BaseSearchTool тут нужен - без него он зарегается в ToolRegistry как полноценный тул с именем _basesearchtool а это абстрактный базовый класс, его нельзя вызвать
сам подход с хардкодом имён конечно так себе, в идеале заменить на маркер типа _abstract = True, но это уже отдельная задача
EvilFreelancer
left a comment
There was a problem hiding this comment.
В целом очень хорошо, плюс пару предложений по допилке фичей.
…ider logic into tools with unified registry and tests
…rch-providers # Conflicts: # sgr_agent_core/base_tool.py
| provider_cls = _search_registry.get(search_config.engine) | ||
| if provider_cls is None: | ||
| raise ValueError(f"Unsupported search engine: {search_config.engine}") |
There was a problem hiding this comment.
Не понятен этот момент:
Мы делаем выбор провайдера в base search tool, хотя их конкретная логика есть в дочерних тулах.
- Какая именно тула предполагается к использованию пользователями фреймворка?
- Если _BaseЫуфксрTool - внутренняя абстракция, то зачем тут эта логика?
There was a problem hiding this comment.
выбор провайдера в базе потому что все 4 тула шарят один и тот же флоу в call: config merging, выбор провайдера, вызов _search(), форматирование, обновление context. если вынести эту логику в WebSearchTool то придется либо дублировать весь call, либо городить промежуточный метод-прокси типа _do_search в каждом туле
по вопросам:
- пользователи используют WebSearchTool, это основной инструмент, движок выбирается через SearchConfig.engine. standalone тулы (tavily, brave, perplexity) это альтернатива когда нужно чтобы LLM сам выбирал конкретный движок
- _BaseSearchTool внутренняя абстракция (поэтому _ префикс, исключен из ToolRegistry), но выбор провайдера это часть общего флоу который все тулы наследуют. по сути template method, база задает скелет, конкретные тулы реализуют _search()
|
|
||
|
|
||
| class WebSearchTool(BaseTool): | ||
| class WebSearchTool(_BaseSearchTool): |
There was a problem hiding this comment.
Как я понимаю, эт теперь legacy. Надо подписать что это именно оно. Возможно повесить депрекейтед тег.
В том числе в docstring.
There was a problem hiding this comment.
не, WebSearchTool не legacy. это основной тул для пользователей, все примеры в examples/ и agents.yaml.example используют именно web_search_tool. standalone тулы (tavily, brave, perplexity) нигде в примерах не фигурируют
смысл WebSearchTool в том что он не форсит конкретный движок, а берет engine из конфига. пользователь может переключить провайдера одной строкой в config.yaml без изменения агента. standalone тулы это альтернатива для кейса когда нужно чтобы LLM сам выбирал между движками
что он пустой это не баг, а результат рефакторинга, вся общая логика в _BaseSearchTool. но согласен что это неочевидно, добавлю пояснение в докстринг
| web_search_tool: | ||
| engine: "tavily" # Search engine: "tavily" (default), "brave", or "perplexity" | ||
| tavily_api_key: "your-tavily-api-key-here" # Tavily API key (get at tavily.com) | ||
| tavily_api_base_url: "https://api.tavily.com" # Tavily API URL | ||
| max_results: 12 | ||
| max_searches: 6 | ||
| extract_page_content_tool: | ||
| tavily_api_key: "your-tavily-api-key-here" # Same Tavily API key | ||
| tavily_api_key: "your-tavily-api-key-here" # Same Tavily API key (Tavily-only feature) | ||
| tavily_api_base_url: "https://api.tavily.com" | ||
| content_limit: 2000 | ||
| # Standalone search tools (for multi-engine setups where LLM picks the engine) | ||
| brave_search_tool: | ||
| brave_api_key: "your-brave-api-key-here" # Brave Search API key | ||
| brave_api_base_url: "https://api.search.brave.com/res/v1/web/search" | ||
| perplexity_search_tool: | ||
| perplexity_api_key: "your-perplexity-api-key-here" # Perplexity API key | ||
| perplexity_api_base_url: "https://api.perplexity.ai/search" | ||
| tavily_search_tool: | ||
| tavily_api_key: "your-tavily-api-key-here" | ||
| tavily_api_base_url: "https://api.tavily.com" |
There was a problem hiding this comment.
Описать бы более детальное разделение тулов. Зачем столько много в примере.
Мы уверены, что прям так надо показывать пользователю?
There was a problem hiding this comment.
тут паттерн проекта такой что целые опциональные секции (prompts, proxy, доп mcp серверы) комментируются, а тулы с конфигами всегда раскомментированы. пользователь просто не добавляет ненужный тул в agent tools список. standalone тулы следуют этому же паттерну, плюс комментарий-разделитель уже есть на строке 72
но я согласен что пояснение можно сделать лучше, сейчас одна строчка и всё. можно расписать два сценария подробнее
как по твоему лучше оформить - закоментировать блоки или оставить с более подробным описанием?
…y for search tools to centralize and standardize search provider registration and lookup
…avily_search_tool to improve separation of concerns and update tests accordingly
…r and add section comments for clarity
…tion and usage options
|
Чтобы подбить клинья:
Идеи:
|
Adds Brave and Perplexity as standalone search providers alongside existing Tavily
BaseSearchService— abstract base with factory method and provider routing byengineconfigBraveSearchService/PerplexitySearchService— concrete implementations with httpx.AsyncClient_BaseSearchTool— shared base for all search tools (fields,__call__logic,__init_subclass__auto-registration)BraveSearchTool/TavilySearchTool/PerplexitySearchTool— standalone tools that force their engineWebSearchTooluses engine from config (default: tavily)config.yaml(API keys, base URLs per provider)Closes #176