imbajin commented on code in PR #358:
URL: https://github.com/apache/hugegraph-ai/pull/358#discussion_r3346728623
##########
hugegraph-python-client/src/tests/api/test_gremlin.py:
##########
@@ -139,8 +143,35 @@ def test_set_up_class_skips_when_env_var_set(self):
def test_gremlin_error_surface_is_explicit(client_utils):
- with pytest.raises(NotFoundError) as exc_info:
+ with pytest.raises(ServerError) as exc_info:
Review Comment:
SKIP_GREMLIN_TESTS=true does not skip this module-level Gremlin test. The
TestGremlin class is skipped, but this function still requests the client_utils
fixture, connects to the local HugeGraph service, and fails during graph
cleanup when the service is unavailable or has different credentials. Please
apply the same explicit skip gate to this test or move the skip into the shared
hugegraph/client fixture so the opt-out is reliable.
##########
hugegraph-llm/src/hugegraph_llm/api/admin_api.py:
##########
@@ -15,29 +15,56 @@
# specific language governing permissions and limitations
# under the License.
+import ntpath
import os
-from fastapi import APIRouter, status
+from fastapi import APIRouter, HTTPException, status
from fastapi.responses import StreamingResponse
from hugegraph_llm.api.exceptions.rag_exceptions import generate_response
from hugegraph_llm.api.models.rag_requests import LogStreamRequest
from hugegraph_llm.api.models.rag_response import RAGResponse
from hugegraph_llm.config import admin_settings
+LOG_DIR = "logs"
+INSECURE_ADMIN_TOKENS = {"", "xxxx"}
+
+
+def _is_configured_admin_token(admin_token: str | None) -> bool:
+ return admin_token is not None and admin_token not in INSECURE_ADMIN_TOKENS
+
+
+def _resolve_log_path(log_file: str | None) -> str:
+ if not log_file:
+ raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST,
detail="Invalid log file name.")
+ if (
+ os.path.isabs(log_file)
+ or ntpath.isabs(log_file)
+ or ntpath.splitdrive(log_file)[0]
+ or "/" in log_file
+ or "\\" in log_file
+ or os.path.normpath(log_file) in {"", ".", ".."}
+ ):
+ raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST,
detail="Invalid log file name.")
+ return os.path.join(LOG_DIR, log_file)
+
-# FIXME: line 31: E0702: Raising dict while only classes or instances are
allowed (raising-bad-type)
def admin_http_api(router: APIRouter, log_stream):
@router.post("/logs", status_code=status.HTTP_200_OK)
async def log_stream_api(req: LogStreamRequest):
+ if not _is_configured_admin_token(admin_settings.admin_token):
Review Comment:
This hardening only covers the FastAPI /logs route. The Gradio Admin Tools
path still authenticates via admin_block.check_password() with a direct
password == admin_token comparison and then reads logs through
read_llm_server_log(), so deployments that keep the default/empty admin token
can still expose logs through the UI even though POST /logs is now rejected.
Please share the same configured-token check with the UI path, or disable Admin
Tools log access when the admin token is insecure.
##########
hugegraph-llm/src/hugegraph_llm/api/rag_api.py:
##########
@@ -161,68 +189,82 @@ def graph_config_api(req: GraphConfigRequest):
# TODO: restructure the implement of llm to three types, like
"/config/chat_llm"
@router.post("/config/llm", status_code=status.HTTP_201_CREATED)
def llm_config_api(req: LLMConfigRequest):
- llm_settings.chat_llm_type = req.llm_type
- llm_settings.extract_llm_type = req.llm_type
- llm_settings.text2gql_llm_type = req.llm_type
-
- if req.llm_type == "openai":
- res = apply_llm_conf(
- req.api_key,
- req.api_base,
- req.language_model,
- req.max_tokens,
- origin_call="http",
- )
- else:
- res = apply_llm_conf(req.host, req.port, req.language_model, None,
origin_call="http")
- return generate_response(RAGResponse(status_code=res, message="Missing
Value"))
+ original_values = _snapshot_settings(llm_settings, _LLM_TYPE_FIELDS)
Review Comment:
The rollback here only restores the provider-type fields. The production
callbacks mutate concrete provider settings before returning a non-2xx status,
so a failed /config/llm, /config/embedding, or /config/rerank request can leave
API keys, base URLs, model names, or token limits half-applied even though the
endpoint returns an error. Please snapshot and restore the concrete provider
config fields as well, not just *_type, and extend the rollback tests to cover
those fields.
##########
hugegraph-llm/src/tests/integration/test_graph_rag_pipeline.py:
##########
@@ -15,275 +15,27 @@
# specific language governing permissions and limitations
# under the License.
-
-import shutil
-import tempfile
-import unittest
-from unittest.mock import MagicMock
-
import pytest
-from tests.utils.mock import MockEmbedding
-
-pytestmark = [pytest.mark.external, pytest.mark.slow]
-
-
-class BaseLLM:
- def generate(self, prompt, **kwargs):
- pass
-
- async def async_generate(self, prompt, **kwargs):
- pass
-
- def get_llm_type(self):
- pass
-
-
-# 模拟RAGPipeline类
-class RAGPipeline:
- def __init__(self, llm=None, embedding=None):
- self.llm = llm
- self.embedding = embedding
- self.operators = {}
-
- def extract_word(self, text=None, language="english"):
- if "word_extract" in self.operators:
- return self.operators["word_extract"]({"query": text})
- return {"words": []}
-
- def extract_keywords(self, text=None, max_keywords=5, language="english",
extract_template=None):
- if "keyword_extract" in self.operators:
- return self.operators["keyword_extract"]({"query": text})
- return {"keywords": []}
-
- def keywords_to_vid(self, by="keywords", topk_per_keyword=5,
topk_per_query=10):
- if "semantic_id_query" in self.operators:
- return self.operators["semantic_id_query"]({"keywords": []})
- return {"match_vids": []}
-
- def query_graphdb(
- self,
- max_deep=2,
- max_graph_items=10,
- max_v_prop_len=2048,
- max_e_prop_len=256,
- prop_to_match=None,
- num_gremlin_generate_example=1,
- gremlin_prompt=None,
- ):
- if "graph_rag_query" in self.operators:
- return self.operators["graph_rag_query"]({"match_vids": []})
- return {"graph_result": []}
-
- def query_vector_index(self, max_items=3):
- if "vector_index_query" in self.operators:
- return self.operators["vector_index_query"]({"query": ""})
- return {"vector_result": []}
-
- def merge_dedup_rerank(
- self, graph_ratio=0.5, rerank_method="bleu",
near_neighbor_first=False, custom_related_information=""
- ):
- if "merge_dedup_rerank" in self.operators:
- return self.operators["merge_dedup_rerank"]({"graph_result": [],
"vector_result": []})
- return {"merged_result": []}
-
- def synthesize_answer(
- self,
- raw_answer=False,
- vector_only_answer=True,
- graph_only_answer=False,
- graph_vector_answer=False,
- answer_prompt=None,
- ):
- if "answer_synthesize" in self.operators:
- return self.operators["answer_synthesize"]({"merged_result": []})
- return {"answer": ""}
-
- def run(self, **kwargs):
- context = {"query": kwargs.get("query", "")}
-
- # 执行各个步骤
- if not kwargs.get("skip_extract_word", False):
- context.update(self.extract_word(text=context["query"]))
-
- if not kwargs.get("skip_extract_keywords", False):
- context.update(self.extract_keywords(text=context["query"]))
-
- if not kwargs.get("skip_keywords_to_vid", False):
- context.update(self.keywords_to_vid())
-
- if not kwargs.get("skip_query_graphdb", False):
- context.update(self.query_graphdb())
-
- if not kwargs.get("skip_query_vector_index", False):
- context.update(self.query_vector_index())
-
- if not kwargs.get("skip_merge_dedup_rerank", False):
- context.update(self.merge_dedup_rerank())
-
- if not kwargs.get("skip_synthesize_answer", False):
- context.update(
- self.synthesize_answer(
- vector_only_answer=kwargs.get("vector_only_answer", False),
- graph_only_answer=kwargs.get("graph_only_answer", False),
- graph_vector_answer=kwargs.get("graph_vector_answer",
False),
- )
- )
-
- return context
-
-
-class MockLLM(BaseLLM):
- """Mock LLM class for testing"""
-
- def __init__(self):
- self.model = "mock_llm"
-
- def generate(self, prompt, **kwargs):
- # Return a simple mock response based on the prompt
- if "person" in prompt.lower():
- return "This is information about a person."
- if "movie" in prompt.lower():
- return "This is information about a movie."
- return "I don't have specific information about that."
-
- async def async_generate(self, prompt, **kwargs):
- # Async version returns the same as the sync version
- return self.generate(prompt, **kwargs)
-
- def get_llm_type(self):
- return "mock"
-
-
-class TestGraphRAGPipeline(unittest.TestCase):
- def setUp(self):
- # Create a temporary directory for testing
- self.test_dir = tempfile.mkdtemp()
-
- # Create mock models
- self.embedding = MockEmbedding()
- self.llm = MockLLM()
-
- # Create mock operators
- self.mock_word_extract = MagicMock()
- self.mock_word_extract.return_value = {"words": ["person", "movie"]}
-
- self.mock_keyword_extract = MagicMock()
- self.mock_keyword_extract.return_value = {"keywords": ["person",
"movie"]}
-
- self.mock_semantic_id_query = MagicMock()
- self.mock_semantic_id_query.return_value = {"match_vids": ["1:person",
"2:movie"]}
-
- self.mock_graph_rag_query = MagicMock()
- self.mock_graph_rag_query.return_value = {
- "graph_result": ["Person: John Doe, Age: 30", "Movie: The Matrix,
Year: 1999"]
- }
-
- self.mock_vector_index_query = MagicMock()
- self.mock_vector_index_query.return_value = {
- "vector_result": ["John Doe is a software engineer.", "The Matrix
is a science fiction movie."]
- }
-
- self.mock_merge_dedup_rerank = MagicMock()
- self.mock_merge_dedup_rerank.return_value = {
- "merged_result": [
- "Person: John Doe, Age: 30",
- "Movie: The Matrix, Year: 1999",
- "John Doe is a software engineer.",
- "The Matrix is a science fiction movie.",
- ]
- }
-
- self.mock_answer_synthesize = MagicMock()
- self.mock_answer_synthesize.return_value = {
- "answer": (
- "John Doe is a 30-year-old software engineer. The Matrix is a
science fiction movie released in 1999."
- )
- }
-
- # 创建RAGPipeline实例
- self.pipeline = RAGPipeline(llm=self.llm, embedding=self.embedding)
- self.pipeline.operators = {
- "word_extract": self.mock_word_extract,
- "keyword_extract": self.mock_keyword_extract,
- "semantic_id_query": self.mock_semantic_id_query,
- "graph_rag_query": self.mock_graph_rag_query,
- "vector_index_query": self.mock_vector_index_query,
- "merge_dedup_rerank": self.mock_merge_dedup_rerank,
- "answer_synthesize": self.mock_answer_synthesize,
- }
-
- def tearDown(self):
- # Clean up the temporary directory
- shutil.rmtree(self.test_dir)
-
- def test_rag_pipeline_end_to_end(self):
- # Run the pipeline with a query
- query = "Tell me about John Doe and The Matrix movie"
- result = self.pipeline.run(query=query)
-
- # Verify the result
- self.assertIn("answer", result)
- self.assertEqual(
- result["answer"],
- "John Doe is a 30-year-old software engineer. The Matrix is a
science fiction movie released in 1999.",
- )
-
- # Verify that all operators were called
- self.mock_word_extract.assert_called_once()
- self.mock_keyword_extract.assert_called_once()
- self.mock_semantic_id_query.assert_called_once()
- self.mock_graph_rag_query.assert_called_once()
- self.mock_vector_index_query.assert_called_once()
- self.mock_merge_dedup_rerank.assert_called_once()
- self.mock_answer_synthesize.assert_called_once()
-
- def test_rag_pipeline_vector_only(self):
- # Run the pipeline with a query, skipping graph-related steps
- query = "Tell me about John Doe and The Matrix movie"
- result = self.pipeline.run(
- query=query,
- skip_keywords_to_vid=True,
- skip_query_graphdb=True,
- skip_merge_dedup_rerank=True,
- vector_only_answer=True,
- )
-
- # Verify the result
- self.assertIn("answer", result)
- self.assertEqual(
- result["answer"],
- "John Doe is a 30-year-old software engineer. The Matrix is a
science fiction movie released in 1999.",
- )
+pytestmark = [pytest.mark.smoke, pytest.mark.integration]
- # Verify that only vector-related operators were called
- self.mock_word_extract.assert_called_once()
- self.mock_keyword_extract.assert_called_once()
- self.mock_semantic_id_query.assert_not_called()
- self.mock_graph_rag_query.assert_not_called()
- self.mock_vector_index_query.assert_called_once()
- self.mock_merge_dedup_rerank.assert_not_called()
- self.mock_answer_synthesize.assert_called_once()
- def test_rag_pipeline_graph_only(self):
- # Run the pipeline with a query, skipping vector-related steps
- query = "Tell me about John Doe and The Matrix movie"
- result = self.pipeline.run(
- query=query, skip_query_vector_index=True,
skip_merge_dedup_rerank=True, graph_only_answer=True
- )
+def test_vector_only_rag_flow_builds_production_pipeline():
Review Comment:
These integration tests now use more production classes, but most of the
coverage stops at build-time wiring or isolated operators. They do not execute
the flow-level contract or assert post_deal/final response behavior, so
regressions in node ordering, state propagation, or output shaping can still
pass. Could we keep at least one deterministic end-to-end smoke per production
flow instead of only build-smoke coverage?
##########
hugegraph-python-client/src/pyhugegraph/api/gremlin.py:
##########
@@ -45,10 +46,10 @@ def exec(self, gremlin):
"g": f"__g_{self._sess.cfg.graph_name}",
}
- try:
- if response := self._invoke_request(data=gremlin_data.to_json()):
- return ResponseData(response).result
- log.error("Gremlin can't get results: %s", str(response))
- return None
- except Exception as e:
- raise NotFoundError(f"Gremlin can't get results: {e}") from e
+ response = self._invoke_request(data=gremlin_data.to_json())
+ if response is not None:
+ if not isinstance(response, dict) or not
_REQUIRED_RESPONSE_FIELDS.issubset(response):
+ raise ValueError(f"Invalid Gremlin response payload:
{response}")
Review Comment:
This malformed-success path still raises a raw ValueError, while
ResponseValidation now standardizes malformed successful HTTP bodies as typed
client exceptions. That leaves Gremlin callers with a special-case parse/error
surface. Could we normalize this to the same typed exception contract and
update the regression test to assert that public contract?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
