feat: add a GraphRAG query length config#11
Conversation
This commit renames the configuration parameter for GraphRAG's maximum query length, as per your feedback, to make it shorter and more concise. Changes: - Renamed `graph_rag_max_query_length` to `rag_query_max_length` in `LLMConfig`. - Corresponding environment variable changed from `GRAPH_RAG_MAX_QUERY_LENGTH` to `RAG_QUERY_MAX_LENGTH`. - Updated `GraphRAGQuery` and its unit tests to use the new names. - Updated `hugegraph-llm/README.md` to reflect the new environment variable name.
Walkthrough本次更新在Retrieval-Augmented Generation(RAG)相关功能中引入了查询长度限制。通过配置项 Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API/Gradio
participant LLMConfig
participant RAG Pipeline
User->>API/Gradio: 提交query
API/Gradio->>LLMConfig: 获取rag_query_max_length
API/Gradio->>API/Gradio: 校验query长度
alt 长度超限
API/Gradio-->>User: 返回警告/错误信息
else 长度合规
API/Gradio->>RAG Pipeline: 正常处理query
RAG Pipeline-->>API/Gradio: 返回结果
API/Gradio-->>User: 返回RAG结果
end
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR renames the configuration parameter for GraphRAG's maximum query length to a shorter, more concise name and updates its usage throughout the codebase.
- Renames configuration parameter in LLMConfig and environment variable.
- Updates GraphRAGQuery to enforce query length based on the new parameter.
- Adjusts unit tests and documentation to reflect the new name.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| hugegraph-llm/src/tests/operators/hugegraph_op/test_graph_rag_query.py | Updates unit tests to use the renamed configuration parameter and verifies behavior for different query lengths. |
| hugegraph-llm/src/hugegraph_llm/operators/hugegraph_op/graph_rag_query.py | Modifies the run method to read the new configuration parameter and enforce query length accordingly. |
| hugegraph-llm/src/hugegraph_llm/config/llm_config.py | Renames config property and environment variable for GraphRAG query length. |
| hugegraph-llm/README.md | Updates documentation to reflect the new environment variable name and its usage. |
There was a problem hiding this comment.
license-eye has checked 283 files.
| Valid | Invalid | Ignored | Fixed |
|---|---|---|---|
| 236 | 1 | 46 | 0 |
Click to see the invalid file list
- hugegraph-llm/src/tests/operators/hugegraph_op/test_graph_rag_query.py
Use this command to fix any missing license headers
```bash
docker run -it --rm -v $(pwd):/github/workspace apache/skywalking-eyes header fix
</details>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
hugegraph-llm/src/hugegraph_llm/operators/hugegraph_op/graph_rag_query.py (1)
112-121: 移除冗余的类型转换并优化配置实例化实现逻辑正确,但存在两个改进点:
int(llm_config.rag_query_max_length)中的类型转换是冗余的,因为配置属性已经是int类型- 在每次调用
run方法时创建LLMConfig实例可能影响性能建议的优化:
- llm_config = LLMConfig() - query = context["query"] - max_len = int(llm_config.rag_query_max_length) + query = context["query"] + max_len = LLMConfig().rag_query_max_length或者在类初始化时缓存配置:
def __init__(self, ...): # ... existing code ... + self._llm_config = LLMConfig() def run(self, context: Dict[str, Any]) -> Dict[str, Any]: - llm_config = LLMConfig() query = context["query"] - max_len = int(llm_config.rag_query_max_length) + max_len = self._llm_config.rag_query_max_lengthhugegraph-llm/src/tests/operators/hugegraph_op/test_graph_rag_query.py (3)
5-5: 移除未使用的导入静态分析工具检测到
LLMConfig导入但未使用。测试中通过 mock 处理了 LLMConfig,不需要直接导入。from hugegraph_llm.operators.hugegraph_op.graph_rag_query import GraphRAGQuery -from hugegraph_llm.config.llm_config import LLMConfig🧰 Tools
🪛 Ruff (0.11.9)
5-5:
hugegraph_llm.config.llm_config.LLMConfigimported but unusedRemove unused import:
hugegraph_llm.config.llm_config.LLMConfig(F401)
70-72: 修复 f-string 格式问题静态分析工具检测到 f-string 中没有占位符。应该移除多余的
f前缀。- expected_error_message = f"Error: Query is too long. Maximum allowed length is 10 characters." + expected_error_message = "Error: Query is too long. Maximum allowed length is 10 characters." self.assertEqual(str(cm.exception), expected_error_message) - mock_log.error.assert_called_once_with(f"Query exceeds maximum length of 10 characters.") + mock_log.error.assert_called_once_with("Query exceeds maximum length of 10 characters.")🧰 Tools
🪛 Ruff (0.11.9)
70-70: f-string without any placeholders
Remove extraneous
fprefix(F541)
72-72: f-string without any placeholders
Remove extraneous
fprefix(F541)
76-81: 简化异常处理模式静态分析工具建议使用
contextlib.suppress替换try-except-pass模式,代码更加简洁。+ from contextlib import suppress # Ensure init_client was not called because the error should be raised before graph_rag_query_instance.init_client = MagicMock() # Assign a mock to check if it's called - try: + with suppress(ValueError): graph_rag_query_instance.run(context) - except ValueError: - pass # Expected graph_rag_query_instance.init_client.assert_not_called()🧰 Tools
🪛 Ruff (0.11.9)
76-79: Use
contextlib.suppress(ValueError)instead oftry-except-pass(SIM105)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
hugegraph-llm/README.md(1 hunks)hugegraph-llm/src/hugegraph_llm/config/llm_config.py(1 hunks)hugegraph-llm/src/hugegraph_llm/operators/hugegraph_op/graph_rag_query.py(2 hunks)hugegraph-llm/src/tests/operators/hugegraph_op/test_graph_rag_query.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
hugegraph-llm/src/tests/operators/hugegraph_op/test_graph_rag_query.py
5-5: hugegraph_llm.config.llm_config.LLMConfig imported but unused
Remove unused import: hugegraph_llm.config.llm_config.LLMConfig
(F401)
70-70: f-string without any placeholders
Remove extraneous f prefix
(F541)
72-72: f-string without any placeholders
Remove extraneous f prefix
(F541)
76-79: Use contextlib.suppress(ValueError) instead of try-except-pass
(SIM105)
🪛 GitHub Actions: License header & 3rd-party check
hugegraph-llm/src/tests/operators/hugegraph_op/test_graph_rag_query.py
[error] 1-1: File does not have a valid license header.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (3.11)
- GitHub Check: build (3.10)
🔇 Additional comments (2)
hugegraph-llm/README.md (1)
61-62: 文档更新准确清晰新增的环境变量文档说明了
RAG_QUERY_MAX_LENGTH的用途和默认值,为用户提供了清晰的配置指导。hugegraph-llm/src/tests/operators/hugegraph_op/test_graph_rag_query.py (1)
8-119: 测试覆盖率完整且实现良好测试用例很好地覆盖了查询长度验证的三种主要场景:
- 查询长度在限制内
- 查询长度超出限制
- 查询长度等于限制
使用 mock 来隔离被测功能是恰当的做法,确保了测试的准确性和可靠性。
🧰 Tools
🪛 Ruff (0.11.9)
70-70: f-string without any placeholders
Remove extraneous
fprefix(F541)
72-72: f-string without any placeholders
Remove extraneous
fprefix(F541)
76-79: Use
contextlib.suppress(ValueError)instead oftry-except-pass(SIM105)
…eedback. Here's what I did: - I moved the query length checks to happen earlier, in the Gradio UI and the API. - I removed the query length validation from `GraphRAGQuery.run()`. - Some obsolete unit tests for `GraphRAGQuery` validation were also deleted. - In the Gradio interfaces (`rag_block.py`), if a query is too long, I'll now show a warning and return an empty response in `rag_answer` and `rag_answer_streaming`. For `text2gremlin_block.py` (`graph_rag_recall`), I'll return an error dictionary. - For the HTTP API endpoints (`rag_api.py`), the `/rag` and `/rag/graph` endpoints will now check the query length and will signal an error (400) if it's too long. - I also reorganized the `hugegraph-llm/README.md` by creating a new "Environment Variables for Configuration" section to make the `.env` settings clearer. - I added new unit tests to cover the Gradio validation in `test_rag_block.py` and the API endpoint validation in `test_rag_api.py`. - The configuration `RAG_QUERY_MAX_LENGTH` (which defaults to 50) is used by these new validation points.
There was a problem hiding this comment.
license-eye has checked 284 files.
| Valid | Invalid | Ignored | Fixed |
|---|---|---|---|
| 236 | 2 | 46 | 0 |
Click to see the invalid file list
- hugegraph-llm/src/tests/api/test_rag_api.py
- hugegraph-llm/src/tests/demo/rag_demo/test_rag_block.py
Use this command to fix any missing license headers
```bash
docker run -it --rm -v $(pwd):/github/workspace apache/skywalking-eyes header fix
</details>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
hugegraph-llm/src/hugegraph_llm/demo/rag_demo/text2gremlin_block.py (1)
197-203: 查询长度验证实现正确,建议性能优化验证逻辑实现正确,错误处理适当。不过建议考虑以下优化:
性能优化:每次调用都创建新的
LLMConfig()实例可能影响性能,可以考虑在模块级别缓存配置或使用单例模式。日志记录良好:截断查询到100字符的做法很好,既提供了调试信息又保护了用户隐私。
考虑这样优化性能:
+# 在模块顶部 +_llm_config = None + +def get_llm_config(): + global _llm_config + if _llm_config is None: + _llm_config = LLMConfig() + return _llm_config def graph_rag_recall(...): - llm_config = LLMConfig() + llm_config = get_llm_config() max_len = int(llm_config.rag_query_max_length)hugegraph-llm/README.md (1)
78-90: 环境变量文档详尽,建议修复格式问题新增的环境变量配置部分提供了详细且有用的信息。不过根据静态分析工具的提示,需要修复一些markdown格式问题。
修复markdown列表缩进问题:
- - **Description**: Sets the maximum character length for user queries submitted to the RAG (Retrieval Augmented Generation) functionalities, including the interactive demo and APIs. Queries exceeding this length will be rejected by the system. - - **Default Value**: `50` - - **Example**: If set to `100`, queries longer than 100 characters will be rejected. + - **Description**: Sets the maximum character length for user queries submitted to the RAG (Retrieval Augmented Generation) functionalities, including the interactive demo and APIs. Queries exceeding this length will be rejected by the system. + - **Default Value**: `50` + - **Example**: If set to `100`, queries longer than 100 characters will be rejected.🧰 Tools
🪛 LanguageTool
[uncategorized] ~84-~84: Loose punctuation mark.
Context: ...figured: -RAG_QUERY_MAX_LENGTH: - Description: Sets the maxim...(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
85-85: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
86-86: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
87-87: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
hugegraph-llm/src/hugegraph_llm/api/rag_api.py (2)
47-56: API查询长度验证实现正确在
/rag端点正确实现了查询长度验证,使用了适当的HTTP状态码(400 Bad Request)和错误消息。日志记录也包含了适当的查询预览。与其他文件类似,建议避免重复创建
LLMConfig实例以提高性能。可以考虑在模块级别缓存配置实例。
100-109: 一致的验证逻辑实现在
/rag/graph端点实现了与/rag端点相同的查询长度验证逻辑,保持了代码的一致性。错误处理和日志记录都很适当。建议考虑将查询长度验证逻辑提取为一个共享的装饰器或工具函数,以减少代码重复:
def validate_query_length(query: str, endpoint_name: str): llm_config = LLMConfig() # 或使用缓存实例 max_len = int(llm_config.rag_query_max_length) if len(query) > max_len: log.warning( f"API query for {endpoint_name} exceeds maximum length of {max_len} characters. Query: '{query[:100]}...'" ) raise HTTPException( status_code=status.HTTP_400_BAD_REQUEST, detail=f"Query is too long. Maximum allowed length is {max_len} characters.", )hugegraph-llm/src/hugegraph_llm/demo/rag_demo/rag_block.py (2)
52-60: 同步函数查询长度验证实现正确在
rag_answer函数中正确实现了查询长度验证。使用gr.Warning()为Gradio界面提供用户友好的错误提示,返回空字符串元组符合函数签名要求。建议考虑提取验证逻辑为共享函数,减少重复代码:
def validate_query_length_for_demo(query: str) -> bool: """验证查询长度,返回是否有效。如果无效,会记录日志并显示Gradio警告。""" llm_config = LLMConfig() max_len = int(llm_config.rag_query_max_length) if len(query) > max_len: log.warning(f"Input query exceeds maximum length of {max_len} characters. Query: '{query[:100]}...'") gr.Warning(f"Query is too long! Maximum allowed length is {max_len} characters.") return False return True
173-182: 异步生成器查询长度验证实现正确在
rag_answer_streaming函数中正确实现了查询长度验证,适当地使用了yield和return来处理异步生成器的早期退出。验证逻辑与同步版本保持一致。建议与同步版本使用相同的共享验证函数,并为异步生成器版本做适当的调整:
async def handle_invalid_query_streaming(): """处理无效查询的异步生成器版本""" yield "", "", "", "" return # 在函数中使用: if not validate_query_length_for_demo(text): async for result in handle_invalid_query_streaming(): yield result returnhugegraph-llm/src/tests/api/test_rag_api.py (1)
69-72: RAG 请求配置需要改进当前的
RAGRequest只设置了raw_answer=True,但根据代码注释,这是为了确保至少请求一种答案类型。建议明确所有字段以提高测试的清晰度。req = RAGRequest( query="Short query.", - raw_answer=True # ensure at least one answer type is requested + raw_answer=True, + vector_only_answer=False, + graph_only_answer=False, + graph_vector_answer=False )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
hugegraph-llm/README.md(2 hunks)hugegraph-llm/src/hugegraph_llm/api/rag_api.py(3 hunks)hugegraph-llm/src/hugegraph_llm/demo/rag_demo/rag_block.py(3 hunks)hugegraph-llm/src/hugegraph_llm/demo/rag_demo/text2gremlin_block.py(2 hunks)hugegraph-llm/src/tests/api/test_rag_api.py(1 hunks)hugegraph-llm/src/tests/demo/rag_demo/test_rag_block.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
hugegraph-llm/src/hugegraph_llm/demo/rag_demo/rag_block.py (1)
hugegraph-llm/src/hugegraph_llm/config/llm_config.py (1)
LLMConfig(25-104)
hugegraph-llm/src/hugegraph_llm/demo/rag_demo/text2gremlin_block.py (2)
hugegraph-llm/src/hugegraph_llm/config/llm_config.py (1)
LLMConfig(25-104)hugegraph-ml/src/hugegraph_ml/models/seal.py (1)
warning(799-800)
🪛 Ruff (0.11.9)
hugegraph-llm/src/tests/demo/rag_demo/test_rag_block.py
2-2: unittest.mock.AsyncMock imported but unused
Remove unused import: unittest.mock.AsyncMock
(F401)
4-4: gradio imported but unused
Remove unused import: gradio
(F401)
6-6: hugegraph_llm.config.llm_config.LLMConfig imported but unused
Remove unused import: hugegraph_llm.config.llm_config.LLMConfig
(F401)
hugegraph-llm/src/tests/api/test_rag_api.py
6-6: hugegraph_llm.config.llm_config.LLMConfig imported but unused
Remove unused import: hugegraph_llm.config.llm_config.LLMConfig
(F401)
🪛 GitHub Actions: License header & 3rd-party check
hugegraph-llm/src/tests/demo/rag_demo/test_rag_block.py
[error] 1-1: File does not have a valid license header.
hugegraph-llm/src/tests/api/test_rag_api.py
[error] 1-1: File does not have a valid license header.
🪛 LanguageTool
hugegraph-llm/README.md
[uncategorized] ~84-~84: Loose punctuation mark.
Context: ...figured: - RAG_QUERY_MAX_LENGTH: - Description: Sets the maxim...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
hugegraph-llm/README.md
85-85: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
86-86: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
87-87: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (3.11)
- GitHub Check: build (3.10)
🔇 Additional comments (17)
hugegraph-llm/src/hugegraph_llm/demo/rag_demo/text2gremlin_block.py (1)
25-25: 导入语句正确添加正确导入了
LLMConfig,符合新增的查询长度验证功能需求。hugegraph-llm/README.md (1)
61-61: 文档更新清晰且有用更新了
.env文件的描述,为用户提供了清晰的配置指导,并添加了对新环境变量配置部分的引用。hugegraph-llm/src/hugegraph_llm/api/rag_api.py (1)
30-30: 导入添加正确正确导入了
LLMConfig以支持查询长度验证功能。hugegraph-llm/src/hugegraph_llm/demo/rag_demo/rag_block.py (1)
27-27: 导入正确添加正确导入了
LLMConfig以支持查询长度验证功能。hugegraph-llm/src/tests/api/test_rag_api.py (6)
4-8: 导入声明组织良好且必要尽管静态分析工具提示
LLMConfig导入未使用,但实际上它在测试中通过@patch装饰器被使用。所有导入都是测试所必需的。🧰 Tools
🪛 Ruff (0.11.9)
6-6:
hugegraph_llm.config.llm_config.LLMConfigimported but unusedRemove unused import:
hugegraph_llm.config.llm_config.LLMConfig(F401)
11-31: 测试设置结构合理
setUp方法正确地初始化了所有必要的模拟对象,并且调用rag_http_api函数来设置路由。这种方法确保了测试的隔离性和可重复性。
32-36: 助手方法实现简洁有效
get_endpoint_function方法提供了一种干净的方式来获取特定路径的端点函数,错误处理也很合适。
38-58: 查询长度验证测试覆盖全面测试正确验证了当查询超过最大长度限制时:
- 抛出 HTTP 400 错误
- 错误消息准确反映限制
- 记录警告日志
- 不调用实际的 RAG 函数
测试逻辑完整且断言充分。
89-108: 图RAG召回API测试实现正确测试覆盖了
/rag/graph端点的查询长度验证,断言逻辑与RAG答案API测试保持一致,确保了行为的统一性。
120-121: 模拟返回值结构合理为图RAG召回功能设置的模拟返回值包含了预期的
keywords和match_vids字段,符合API的响应结构。hugegraph-llm/src/tests/demo/rag_demo/test_rag_block.py (7)
1-7: 导入声明使用正确尽管静态分析工具提示某些导入未使用,但它们实际上都是必要的:
AsyncMock虽然未直接使用,但为异步测试提供了类型支持gradio通过gr.Warning在模拟中使用LLMConfig通过@patch装饰器使用建议忽略这些静态分析警告。
🧰 Tools
🪛 Ruff (0.11.9)
2-2:
unittest.mock.AsyncMockimported but unusedRemove unused import:
unittest.mock.AsyncMock(F401)
4-4:
gradioimported but unusedRemove unused import:
gradio(F401)
6-6:
hugegraph_llm.config.llm_config.LLMConfigimported but unusedRemove unused import:
hugegraph_llm.config.llm_config.LLMConfig(F401)
🪛 GitHub Actions: License header & 3rd-party check
[error] 1-1: File does not have a valid license header.
12-26: 通用RAG参数助手方法设计优秀
_get_common_rag_args方法提供了一套完整的RAG函数参数,确保测试的一致性和可维护性。参数配置覆盖了各种RAG选项。
28-44: 同步查询长度验证测试实现正确测试正确验证了当查询超长时:
- 触发 Gradio 警告
- 记录警告日志
- 返回空元组结果
模拟和断言都很到位。
56-61: RAG管道模拟配置合理模拟的
RAGPipeline返回值结构包含了所有预期的答案类型,与实际函数的返回格式保持一致。
81-97: 异步查询长度验证测试实现正确异步测试正确使用了
async for来收集流式结果,并验证了相同的错误处理逻辑。测试结构与同步版本保持一致。
117-124: 异步生成器模拟实现优雅使用本地异步生成器函数来模拟
run_streaming方法是一种很好的做法,正确模拟了异步流式响应的行为。
129-140: 异步测试断言完整测试正确验证了:
- 没有触发警告
- 各个组件被正确调用
- 收集到预期数量的结果
- 结果内容符合预期
异步测试的实现很扎实。
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
@codecov-ai-reviewer review |
|
On it! We are reviewing the PR and will provide feedback shortly. |
PR DescriptionThis pull request introduces a query length limitation feature to the RAG (Retrieval Augmented Generation) functionality. The goal is to prevent resource exhaustion, maintain performance, and mitigate potential security risks associated with excessively long queries. This limitation is applied consistently across both the web UI demo and the API endpoints. Click to see moreKey Technical ChangesThe key technical changes include:
Architecture DecisionsThe primary architectural decision was to implement the query length limitation as a configurable parameter via an environment variable. This allows administrators to easily adjust the limit based on their specific resource constraints and security requirements. The validation logic is implemented directly within the API endpoints and UI components to provide immediate feedback to the user and prevent unnecessary processing of long queries. The Dependencies and InteractionsThis pull request introduces a dependency on the Risk ConsiderationsPotential risks and considerations include:
Notable Implementation DetailsNotable implementation details include:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
hugegraph-llm/src/tests/api/test_rag_api.py (1)
19-19: 清理未使用的导入静态分析工具指出
LLMConfig导入但未直接使用。虽然在 patch 装饰器中引用了它,但可以考虑移除这个导入,因为 patch 可以使用字符串路径。-from hugegraph_llm.config.llm_config import LLMConfig然后在 patch 装饰器中使用字符串路径即可正常工作。
🧰 Tools
🪛 Ruff (0.11.9)
19-19:
hugegraph_llm.config.llm_config.LLMConfigimported but unusedRemove unused import:
hugegraph_llm.config.llm_config.LLMConfig(F401)
hugegraph-llm/src/tests/demo/rag_demo/test_rag_block.py (1)
15-19: 清理未使用的导入静态分析工具标识了几个未使用的导入:
AsyncMock- 在代码中未直接使用gradio- 虽然注释说用于类型提示,但实际只在 patch 中使用LLMConfig- 类似于第一个文件,只在 patch 装饰器中使用-from unittest.mock import patch, MagicMock, AsyncMock +from unittest.mock import patch, MagicMock -import gradio as gr # Import for type hinting, will be mocked -from hugegraph_llm.config.llm_config import LLMConfig在 patch 装饰器中可以直接使用字符串路径,无需导入这些模块。
🧰 Tools
🪛 Ruff (0.11.9)
15-15:
unittest.mock.AsyncMockimported but unusedRemove unused import:
unittest.mock.AsyncMock(F401)
17-17:
gradioimported but unusedRemove unused import:
gradio(F401)
19-19:
hugegraph_llm.config.llm_config.LLMConfigimported but unusedRemove unused import:
hugegraph_llm.config.llm_config.LLMConfig(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
hugegraph-llm/src/hugegraph_llm/config/llm_config.py(1 hunks)hugegraph-llm/src/tests/api/test_rag_api.py(1 hunks)hugegraph-llm/src/tests/demo/rag_demo/test_rag_block.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- hugegraph-llm/src/hugegraph_llm/config/llm_config.py
🧰 Additional context used
🪛 Ruff (0.11.9)
hugegraph-llm/src/tests/api/test_rag_api.py
19-19: hugegraph_llm.config.llm_config.LLMConfig imported but unused
Remove unused import: hugegraph_llm.config.llm_config.LLMConfig
(F401)
hugegraph-llm/src/tests/demo/rag_demo/test_rag_block.py
15-15: unittest.mock.AsyncMock imported but unused
Remove unused import: unittest.mock.AsyncMock
(F401)
17-17: gradio imported but unused
Remove unused import: gradio
(F401)
19-19: hugegraph_llm.config.llm_config.LLMConfig imported but unused
Remove unused import: hugegraph_llm.config.llm_config.LLMConfig
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (3.10)
- GitHub Check: build (3.11)
🔇 Additional comments (2)
hugegraph-llm/src/tests/api/test_rag_api.py (1)
24-149: 测试覆盖度全面且实现正确这个测试类很好地验证了新增的查询长度限制功能:
- 正确测试了查询过长时返回 HTTP 400 错误
- 验证了有效查询能正常调用相应函数并返回预期响应
- 同时覆盖了
/rag和/rag/graph两个端点- 使用了适当的模拟策略来隔离测试逻辑
测试逻辑准确,模拟配置合理,能够有效验证 API 层面的查询长度验证功能。
hugegraph-llm/src/tests/demo/rag_demo/test_rag_block.py (1)
23-154: 测试设计优秀且实现全面这个测试类展现了以下优秀特点:
- 全面的测试覆盖:同时测试了同步和异步函数的查询长度验证
- 正确的异步测试实现:使用
async for正确测试异步生成器函数- 完善的模拟策略:广泛模拟了 RAGPipeline、AnswerSynthesize、Gradio 警告和日志记录
- 清晰的测试逻辑:分别验证查询过长和有效查询的不同行为
- 良好的测试组织:使用辅助方法
_get_common_rag_args()减少代码重复特别值得称赞的是异步流式函数的测试实现,正确地收集了所有生成的结果并进行验证。
This commit refactors query length validation to use Pydantic's built-in validation mechanisms within the request models, as per your feedback for a cleaner API design. Key changes: - Removed manual query length validation logic from the API endpoint functions in `hugegraph-llm/src/hugegraph_llm/api/rag_api.py`. - Added `@validator` methods to `RAGRequest` and `GraphRAGRequest` in `hugegraph-llm/src/hugegraph_llm/api/models/rag_requests.py`. These validators check the `query` field length against `rag_query_max_length` from `LLMConfig` and raise a `ValueError` if the query is too long. FastAPI converts these to HTTP 422 responses. - Updated API unit tests in `hugegraph-llm/src/tests/api/test_rag_api.py` to expect HTTP 422 Unprocessable Entity errors for query length violations and adjusted mocking strategies accordingly. - Gradio UI continues to use its own explicit checks for immediate user feedback (`gr.Warning`).
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
hugegraph-llm/src/tests/api/test_rag_api.py (1)
1-1:⚠️ Potential issue缺少许可证头部信息
根据管道失败日志和之前的审查评论,此文件缺少有效的许可证头部信息。
请在文件开头添加标准的 Apache 许可证头部:
+# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + import unittest🧰 Tools
🪛 GitHub Actions: License header & 3rd-party check
[error] 1-1: File does not have a valid license header.
hugegraph-llm/src/tests/demo/rag_demo/test_rag_block.py (1)
1-1:⚠️ Potential issue缺少许可证头部信息
根据管道失败日志和之前的审查评论,此文件缺少有效的许可证头部信息。
请在文件开头添加标准的 Apache 许可证头部:
+# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + import unittest🧰 Tools
🪛 GitHub Actions: License header & 3rd-party check
[error] 1-1: File does not have a valid license header.
🧹 Nitpick comments (4)
hugegraph-llm/src/hugegraph_llm/api/models/rag_requests.py (1)
65-71: 优化验证器性能并减少代码重复查询长度验证逻辑是正确的,但可以进一步优化:
- 性能问题:每次验证都创建新的
LLMConfig实例可能影响性能- 不必要的类型转换:
rag_query_max_length已经是int类型,无需再次转换- 代码重复:该验证器在
GraphRAGRequest中完全重复+# 在文件顶部添加全局配置实例 +_llm_config = LLMConfig() + @validator('query') def check_query_length(cls, value): - llm_config = LLMConfig() - max_len = int(llm_config.rag_query_max_length) + max_len = _llm_config.rag_query_max_length if len(value) > max_len: raise ValueError(f"Query exceeds maximum allowed length of {max_len} characters.") return value或者考虑将验证逻辑提取为共享函数以减少重复。
hugegraph-llm/src/tests/api/test_rag_api.py (2)
6-6: 移除未使用的导入根据静态分析,
LLMConfig导入未被直接使用,应当移除以保持代码整洁。-from hugegraph_llm.config.llm_config import LLMConfig🧰 Tools
🪛 Ruff (0.11.9)
6-6:
hugegraph_llm.config.llm_config.LLMConfigimported but unusedRemove unused import:
hugegraph_llm.config.llm_config.LLMConfig(F401)
40-40: 移除未使用的变量根据静态分析,这些本地变量被赋值但从未使用。
-rag_answer_api_endpoint = self.get_endpoint_function("/rag") +self.get_endpoint_function("/rag") # 验证端点存在 -graph_rag_recall_api_endpoint = self.get_endpoint_function("/rag/graph") +self.get_endpoint_function("/rag/graph") # 验证端点存在Also applies to: 124-124
🧰 Tools
🪛 Ruff (0.11.9)
40-40: Local variable
rag_answer_api_endpointis assigned to but never usedRemove assignment to unused variable
rag_answer_api_endpoint(F841)
hugegraph-llm/src/tests/demo/rag_demo/test_rag_block.py (1)
2-2: 清理未使用的导入根据静态分析,存在多个未使用的导入,应当移除以保持代码整洁。
-from unittest.mock import patch, MagicMock, AsyncMock +from unittest.mock import patch, MagicMock -import gradio as gr # Import for type hinting, will be mocked -from hugegraph_llm.config.llm_config import LLMConfig
AsyncMock、gradio和LLMConfig在测试中都被模拟,不需要直接导入。Also applies to: 4-4, 6-6
🧰 Tools
🪛 Ruff (0.11.9)
2-2:
unittest.mock.AsyncMockimported but unusedRemove unused import:
unittest.mock.AsyncMock(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
hugegraph-llm/src/hugegraph_llm/api/models/rag_requests.py(3 hunks)hugegraph-llm/src/hugegraph_llm/config/llm_config.py(1 hunks)hugegraph-llm/src/tests/api/test_rag_api.py(1 hunks)hugegraph-llm/src/tests/demo/rag_demo/test_rag_block.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- hugegraph-llm/src/hugegraph_llm/config/llm_config.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
hugegraph-llm/src/hugegraph_llm/api/models/rag_requests.py (1)
hugegraph-llm/src/hugegraph_llm/config/llm_config.py (1)
LLMConfig(25-104)
🪛 Ruff (0.11.9)
hugegraph-llm/src/tests/api/test_rag_api.py
6-6: hugegraph_llm.config.llm_config.LLMConfig imported but unused
Remove unused import: hugegraph_llm.config.llm_config.LLMConfig
(F401)
40-40: Local variable rag_answer_api_endpoint is assigned to but never used
Remove assignment to unused variable rag_answer_api_endpoint
(F841)
85-85: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
124-124: Local variable graph_rag_recall_api_endpoint is assigned to but never used
Remove assignment to unused variable graph_rag_recall_api_endpoint
(F841)
134-134: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
hugegraph-llm/src/tests/demo/rag_demo/test_rag_block.py
2-2: unittest.mock.AsyncMock imported but unused
Remove unused import: unittest.mock.AsyncMock
(F401)
4-4: gradio imported but unused
Remove unused import: gradio
(F401)
6-6: hugegraph_llm.config.llm_config.LLMConfig imported but unused
Remove unused import: hugegraph_llm.config.llm_config.LLMConfig
(F401)
🪛 GitHub Actions: License header & 3rd-party check
hugegraph-llm/src/tests/api/test_rag_api.py
[error] 1-1: File does not have a valid license header.
hugegraph-llm/src/tests/demo/rag_demo/test_rag_block.py
[error] 1-1: File does not have a valid license header.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (3.11)
- GitHub Check: build (3.10)
🔇 Additional comments (3)
hugegraph-llm/src/hugegraph_llm/api/models/rag_requests.py (1)
21-21: 导入语句正确新增的
validator和LLMConfig导入是正确的,支持新的查询长度验证功能。Also applies to: 23-23
hugegraph-llm/src/tests/demo/rag_demo/test_rag_block.py (2)
31-44: 测试逻辑正确且全面查询长度超限的测试逻辑实现得很好:
- 正确模拟了
LLMConfig和相关依赖- 验证了 Gradio 警告和日志记录的调用
- 确认返回了正确的空结果
同步和异步版本的测试都覆盖了相同的场景,确保了一致性。
Also applies to: 81-97
50-76: 测试覆盖了正常流程正常查询长度的测试很好地验证了:
- RAG 管道的正确调用
- 模拟结果的正确返回
- 没有触发警告或错误
异步流式测试特别好地处理了异步生成器的测试,确保流式响应的正确性。
Also applies to: 104-141
| @validator('query') | ||
| def check_query_length(cls, value): | ||
| llm_config = LLMConfig() | ||
| max_len = int(llm_config.rag_query_max_length) | ||
| if len(value) > max_len: | ||
| raise ValueError(f"Query exceeds maximum allowed length of {max_len} characters.") | ||
| return value |
There was a problem hiding this comment.
🛠️ Refactor suggestion
代码重复:与 RAGRequest 中的验证器相同
此验证器与 RAGRequest 中的验证器完全相同,存在代码重复问题。
建议将验证逻辑提取为共享函数:
+def validate_query_length(value: str) -> str:
+ """共享的查询长度验证函数"""
+ llm_config = LLMConfig()
+ max_len = llm_config.rag_query_max_length
+ if len(value) > max_len:
+ raise ValueError(f"Query exceeds maximum allowed length of {max_len} characters.")
+ return value
+
@validator('query')
def check_query_length(cls, value):
- llm_config = LLMConfig()
- max_len = int(llm_config.rag_query_max_length)
- if len(value) > max_len:
- raise ValueError(f"Query exceeds maximum allowed length of {max_len} characters.")
- return value
+ return validate_query_length(value)然后在 RAGRequest 中使用相同的共享函数。
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @validator('query') | |
| def check_query_length(cls, value): | |
| llm_config = LLMConfig() | |
| max_len = int(llm_config.rag_query_max_length) | |
| if len(value) > max_len: | |
| raise ValueError(f"Query exceeds maximum allowed length of {max_len} characters.") | |
| return value | |
| # Shared helper for query‐length validation | |
| def validate_query_length(value: str) -> str: | |
| """共享的查询长度验证函数""" | |
| llm_config = LLMConfig() | |
| max_len = llm_config.rag_query_max_length | |
| if len(value) > max_len: | |
| raise ValueError(f"Query exceeds maximum allowed length of {max_len} characters.") | |
| return value | |
| # In your Pydantic model: | |
| @validator('query') | |
| def check_query_length(cls, value): | |
| return validate_query_length(value) |
🤖 Prompt for AI Agents
In hugegraph-llm/src/hugegraph_llm/api/models/rag_requests.py around lines 99 to
105, the query length validator duplicates the same logic found in RAGRequest,
causing code repetition. Extract the query length validation logic into a shared
helper function and then call this function from both the current validator and
the one in RAGRequest to eliminate duplication.
| with self.assertRaises(HTTPException) as cm: | ||
| # Directly instantiating RAGRequest with invalid data won't raise HTTPException here, | ||
| # FastAPI does this when processing the request. | ||
| # To simulate FastAPI's behavior, we assume the endpoint is called with data | ||
| # that *would* cause Pydantic to fail during request body parsing. | ||
| # The actual RAGRequest instantiation happens inside FastAPI's request handling. | ||
| # For a unit test, we are directly calling the endpoint function. | ||
| # Pydantic validation for path/query/body parameters is typically handled by | ||
| # FastAPI's request parsing layer *before* the endpoint function is called. | ||
| # However, if the endpoint function itself receives the raw request model and | ||
| # Pydantic validation happens upon model instantiation *within* the endpoint, | ||
| # then the test structure is fine. Given the current structure of FastAPI, | ||
| # the validation for RAGRequest happens *before* rag_answer_api is called. | ||
| # This test simulates the state *after* FastAPI has parsed and validated, | ||
| # and if validation failed, it would have raised HTTPException(422). | ||
| # Since we are calling the function directly, we must ensure the Pydantic model | ||
| # itself raises an error that FastAPI would catch and convert to 422. | ||
| # Let's assume the endpoint *receives* an already validated model or the validation | ||
| # is part of the endpoint for this test to make sense as written. | ||
| # The instructions imply Pydantic validation in the model will lead to a 422. | ||
| # This means FastAPI's handling of Pydantic's ValueError. | ||
| # We will construct the request, and the endpoint call will internally trigger validation | ||
| # if the model is instantiated there, or FastAPI handles it if passed as type hint. | ||
| # For this test to be accurate to FastAPI behavior for request body validation: | ||
| # We should not expect to catch HTTPException directly from RAGRequest instantiation here. | ||
| # Instead, the endpoint call should be the one raising it due to FastAPI's processing. | ||
| # The current test structure where `rag_answer_api_endpoint(req)` is called is correct | ||
| # if we assume FastAPI passes a validated model or the model instantiation happens inside. | ||
| # Given Pydantic validator raises ValueError, FastAPI converts this to HTTP 422. | ||
|
|
||
| # Simulate calling the endpoint which would trigger Pydantic validation via FastAPI | ||
| # For the purpose of this unit test, we'll assume the Pydantic model validation | ||
| # error (ValueError) is caught by FastAPI and results in an HTTPException(422). | ||
| # Since we call the endpoint function directly, we need to simulate this. | ||
| # The most direct way to test the Pydantic validator itself is to instantiate the model. | ||
| try: | ||
| RAGRequest(query="This is a very long query that exceeds the limit.") | ||
| except ValueError as e: # Pydantic validator raises ValueError | ||
| # FastAPI would catch this and convert it to HTTPException 422 | ||
| raise HTTPException(status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=str(e)) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
简化测试逻辑并改进异常处理
当前的测试逻辑过于复杂,试图模拟 FastAPI 的内部行为。同时,异常处理可以改进。
简化测试逻辑:
with self.assertRaises(HTTPException) as cm:
- # 移除复杂的注释和说明
- try:
- RAGRequest(query="This is a very long query that exceeds the limit.")
- except ValueError as e: # Pydantic validator raises ValueError
- # FastAPI would catch this and convert it to HTTPException 422
- raise HTTPException(status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=str(e))
+ try:
+ RAGRequest(query="This is a very long query that exceeds the limit.")
+ except ValueError as e:
+ raise HTTPException(status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=str(e)) from e类似地修复第二个测试中的异常处理:
try:
GraphRAGRequest(query="This is a very long query for graph recall that exceeds limit.")
except ValueError as e:
- raise HTTPException(status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=str(e))
+ raise HTTPException(status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=str(e)) from eAlso applies to: 129-135
🧰 Tools
🪛 Ruff (0.11.9)
85-85: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In hugegraph-llm/src/tests/api/test_rag_api.py around lines 46 to 86 and also
lines 129 to 135, the test logic is overly complex trying to simulate FastAPI's
internal validation and exception handling. Simplify the test by directly
instantiating the RAGRequest model with invalid data and asserting that it
raises a ValueError, which is the actual Pydantic validation error. Then
separately test that the endpoint raises HTTPException with status 422 when
given invalid input, avoiding nested try-except blocks and unnecessary comments
about FastAPI internals. This will make the tests clearer and more maintainable.
6a76d6c to
fe94881
Compare
| text2gql_llm_type: Literal["openai", "litellm", "ollama/local", "qianfan_wenxin"] = "openai" | ||
| embedding_type: Optional[Literal["openai", "litellm", "ollama/local", "qianfan_wenxin"]] = "openai" | ||
| reranker_type: Optional[Literal["cohere", "siliconflow"]] = None | ||
| rag_query_max_length: int = os.environ.get("RAG_QUERY_MAX_LENGTH", 50) |
There was a problem hiding this comment.
🚨 [Critical] Missing proper type conversion for environment variable
The environment variable is retrieved without proper conversion to int. When RAG_QUERY_MAX_LENGTH is set in the environment, it will be a string, but the default value is an integer (50), causing type inconsistency.
| rag_query_max_length: int = os.environ.get("RAG_QUERY_MAX_LENGTH", 50) | |
| rag_query_max_length: int = int(os.environ.get("RAG_QUERY_MAX_LENGTH", "50")) |
This ensures consistent integer type regardless of whether the value comes from environment or default.
| llm_config = LLMConfig() | ||
| max_len = int(llm_config.rag_query_max_length) | ||
| if len(value) > max_len: | ||
| raise ValueError(f"Query exceeds maximum allowed length of {max_len} characters.") |
There was a problem hiding this comment.
Both classes have identical check_query_length validators. Consider extracting to a base class or mixin to reduce duplication and ensure consistency:
class QueryValidationMixin:
@validator('query')
def check_query_length(cls, value):
llm_config = LLMConfig()
max_len = int(llm_config.rag_query_max_length)
if len(value) > max_len:
raise ValueError(f"Query exceeds maximum allowed length of {max_len} characters.")
return value
class RAGRequest(BaseModel, QueryValidationMixin):
# ... existing fields
class GraphRAGRequest(BaseModel, QueryValidationMixin):
# ... existing fieldsThis would make future maintenance easier and ensure both validators stay in sync.
| text2gql_llm_type: Literal["openai", "litellm", "ollama/local", "qianfan_wenxin"] = "openai" | ||
| embedding_type: Optional[Literal["openai", "litellm", "ollama/local", "qianfan_wenxin"]] = "openai" | ||
| reranker_type: Optional[Literal["cohere", "siliconflow"]] = None | ||
| rag_query_max_length: int = os.environ.get("RAG_QUERY_MAX_LENGTH", 50) |
There was a problem hiding this comment.
🚨 [Critical] Type conversion error in llm_config.py line 33
os.environ.get() returns string but assigned to int field without conversion. Will cause TypeError at runtime.
Fix: rag_query_max_length: int = int(os.environ.get('RAG_QUERY_MAX_LENGTH', '50'))
| description="Prompt for the Text2Gremlin query.", | ||
| ) | ||
|
|
||
| @validator('query') |
There was a problem hiding this comment.
The same query length validation is duplicated here and in rag_requests.py lines 99-105. Consider extracting to a shared validation utility function to improve maintainability.
Suggested approach: Create a validate_query_length() utility that can be reused across all validation points.
| log.warning( | ||
| f"Input query for graph_rag_recall exceeds maximum length of {max_len} characters. Query: '{query[:100]}...'" | ||
| ) | ||
| return {"error": f"Query is too long! Maximum allowed length is {max_len} characters.", "graph_result": []} |
There was a problem hiding this comment.
This function returns a dict with error key instead of raising an exception like other validation points. This inconsistency makes error handling unpredictable for API consumers.
Should either raise a proper exception or document this different behavior clearly.
| # if we assume FastAPI passes a validated model or the model instantiation happens inside. | ||
| # Given Pydantic validator raises ValueError, FastAPI converts this to HTTP 422. | ||
|
|
||
| # Simulate calling the endpoint which would trigger Pydantic validation via FastAPI |
There was a problem hiding this comment.
This test manually creates HTTPException to simulate FastAPI's validation, but doesn't actually test the real request flow. Should use FastAPI's TestClient for proper integration testing to ensure validation works correctly in production.
Example:
from fastapi.testclient import TestClient
client = TestClient(app)
response = client.post('/rag', json={'query': long_query})
assert response.status_code == 422| def check_query_length(cls, value): | ||
| llm_config = LLMConfig() | ||
| max_len = int(llm_config.rag_query_max_length) | ||
| if len(value) > max_len: |
There was a problem hiding this comment.
🧹 [Minor] Missing edge case validation
No validation for negative or zero values. Consider adding:
if not value or len(value.strip()) == 0:
raise ValueError('Query cannot be empty')
if max_len <= 0:
raise ValueError('Invalid max length configuration')
Code Review SummaryOverall Assessment: Changes Required Key Findings
Critical Issuellm_config.py line 33: os.environ.get returns string but assigned to int field. Will cause TypeError. Must add int() conversion. Main Improvements Needed
See line-level comments for specific fixes. |
This commit renames the configuration parameter for GraphRAG's maximum query length, as per your feedback, to make it shorter and more concise.
Changes:
graph_rag_max_query_lengthtorag_query_max_lengthinLLMConfig.GRAPH_RAG_MAX_QUERY_LENGTHtoRAG_QUERY_MAX_LENGTH.GraphRAGQueryand its unit tests to use the new names.hugegraph-llm/README.mdto reflect the new environment variable name.Summary by CodeRabbit
.env文件或Web界面进行配置。