imbajin commented on code in PR #240:
URL: https://github.com/apache/hugegraph-ai/pull/240#discussion_r3384835172
##########
hugegraph-llm/src/hugegraph_llm/operators/index_op/build_semantic_index.py:
##########
@@ -86,8 +163,69 @@ def run(self, context: Dict[str, Any]) -> Dict[str, Any]:
self.vid_index.to_index_file(self.index_dir)
else:
log.debug("No update vertices to build vector index.")
+ return removed_num, len(added_vids)
+
+ def _update_property_index(self, context: dict[str, Any]) -> tuple[int,
int | str]:
+ removed_props_num = 0
+ added_props_vector_num: int | str = 0
+
+ present_prop_value_to_propset = self.get_present_props(context)
+ # log.debug("present_prop_value_to_propset: %s",
present_prop_value_to_propset)
+ past_prop_value_to_propset = self.get_past_props()
+ # log.debug("past_prop_value_to_propset: %s",
past_prop_value_to_propset)
+ to_add, to_update, to_remove, to_update_remove =
self.diff_property_sets(
+ present_prop_value_to_propset,
+ past_prop_value_to_propset
+ )
+ log.debug("to_add: %s", to_add)
+ log.debug("to_update: %s", to_update)
+ log.debug("to_remove: %s", to_remove)
+ log.debug("to_update_remove: %s", to_update_remove)
+ log.info("Removing %s outdated property value", len(to_remove))
+ removed_props_num = self.prop_index.remove(to_remove)
+ if removed_props_num:
+ self.prop_index.to_index_file(self.index_dir_prop)
+ all_to_add = to_add + to_update
+ add_propsets = []
+ add_prop_values = []
+ for prop_value, propset in all_to_add:
+ add_propsets.append(propset)
+ add_prop_values.append(prop_value)
+ if add_prop_values:
+ if len(add_prop_values) > 100000:
Review Comment:
‼️ **Check the update limit before mutating the index**
Evidence: `to_remove` is already removed and persisted at lines 184-187
before this `> 100000` guard returns early. That leaves the on-disk property
index half-updated: deletions are committed, but additions/updates are skipped.
Please compute and validate the full mutation set before any `remove()` /
`to_index_file()` call, or stage the new index and commit it atomically only
after the limit check passes.
##########
hugegraph-llm/src/hugegraph_llm/operators/index_op/build_semantic_index.py:
##########
@@ -27,14 +28,33 @@
from hugegraph_llm.models.embeddings.base import BaseEmbedding
from hugegraph_llm.operators.hugegraph_op.schema_manager import SchemaManager
from hugegraph_llm.utils.log import log
+from pyhugegraph.client import PyHugeClient
+INDEX_PROPERTY_GREMLIN = """
+g.V().hasLabel('{label}')
+ .limit(100000)
Review Comment:
‼️ **Avoid truncating the property snapshot before diffing**
Evidence: this query only reads the first 100000 vertices for each indexed
label, but `_update_property_index()` later treats every previously indexed
property value missing from this truncated snapshot as deleted and removes it
from `graph_props`. On graphs with more than 100000 matching vertices,
incremental rebuilds can silently delete valid property vectors and degrade
property recall. Please page through all matching vertices or avoid deleting
existing property vectors when the present snapshot is known to be capped.
##########
hugegraph-llm/src/hugegraph_llm/api/models/rag_requests.py:
##########
@@ -15,20 +15,21 @@
# specific language governing permissions and limitations
# under the License.
-from typing import Optional, Literal
-
+from typing import Optional, Literal, List
+from enum import Enum
from fastapi import Query
from pydantic import BaseModel
from hugegraph_llm.config import prompt
-
+from hugegraph_llm.config import huge_settings
Review Comment:
⚠️ **Remove the unused import before merging**
Evidence: `uv run ruff check` on the changed Python files fails with `F401
huge_settings imported but unused` on this line. Since the PR currently only
has the triage check on GitHub, this would be caught by the repository's normal
Python quality gate later. Please remove the import and run `uv run ruff check`
plus `uv run ruff format --check` for the touched Python files.
##########
hugegraph-llm/src/hugegraph_llm/operators/hugegraph_op/graph_rag_query.py:
##########
@@ -160,12 +162,14 @@ def _gremlin_generate_query(self, context: Dict[str,
Any]) -> Dict[str, Any]:
def _subgraph_query(self, context: Dict[str, Any]) -> Dict[str, Any]:
# 1. Extract params from context
matched_vids = context.get("match_vids")
+ matched_props = context.get("match_props")
if isinstance(context.get("max_deep"), int):
self._max_deep = context["max_deep"]
if isinstance(context.get("max_items"), int):
self._max_items = context["max_items"]
- if isinstance(context.get("prop_to_match"), str):
- self._prop_to_match = context["prop_to_match"]
+ if isinstance(context.get("match_props"), list):
Review Comment:
‼️ **Do not force property fallback when VID matches exist**
Evidence: whenever `match_props` is a list, this sets `_prop_to_match`, so
`use_id_to_match` becomes false even if `match_vids` also contains exact vertex
hits. A query that matches both a vertex id and an indexed property now skips
the more precise VID-neighbor query and is forced into the property fallback
path. Please prefer VID matching when `matched_vids` is non-empty, and use the
property path only when no VID recall is available.
##########
hugegraph-llm/src/hugegraph_llm/api/rag_api.py:
##########
@@ -139,7 +142,7 @@ def graph_rag_recall_api(req: GraphRAGRequest):
@router.post("/config/graph", status_code=status.HTTP_201_CREATED)
def graph_config_api(req: GraphConfigRequest):
# Accept status code
- res = apply_graph_conf(req.url, req.name, req.user, req.pwd, req.gs,
origin_call="http")
+ res = apply_graph_conf(req.url, req.graph, req.user, req.pwd, req.gs,
origin_call="http")
Review Comment:
‼️ **Thread token auth through the persistent graph config API**
Evidence: `GraphConfigRequest` now accepts `token`, and per-request
`client_config` copies it into `huge_settings`, but `/config/graph` still calls
`apply_graph_conf()` without `req.token`. Token-only users can submit the field
successfully, yet the saved graph configuration still validates and persists
only basic-auth credentials. Please pass and persist `token` here, and make the
connectivity check use bearer auth when a token is provided.
##########
hugegraph-llm/src/hugegraph_llm/operators/index_op/semantic_id_query.py:
##########
@@ -75,18 +79,57 @@ def _exact_match_vids(self, keywords: List[str]) ->
Tuple[List[str], List[str]]:
def _fuzzy_match_vids(self, keywords: List[str]) -> List[str]:
fuzzy_match_result = []
for keyword in keywords:
- keyword_vector = self.embedding.get_text_embedding(keyword)
- results = self.vector_index.search(keyword_vector,
top_k=self.topk_per_keyword,
+ keyword_vector = self.embedding.get_texts_embeddings([keyword])
+ results = self.vector_index.search(keyword_vector[0],
top_k=self.topk_per_keyword,
dis_threshold=float(self.vector_dis_threshold))
if results:
fuzzy_match_result.extend(results[:self.topk_per_keyword])
return fuzzy_match_result
+ def _exact_match_properties(self, keywords: List[str]) -> Tuple[List[str],
List[str]]:
+ property_keys = self.schema.getPropertyKeys()
+ log.debug("property_keys: %s", property_keys)
+ matched_properties = set()
+ unmatched_keywords = set(keywords)
+ for key in property_keys:
+ for keyword in list(unmatched_keywords):
+ gremlin_query = f"g.V().has('{key.name}',
'{keyword}').limit(1)"
+ log.debug("prop Gremlin query: %s", gremlin_query)
+ resp = self._client.gremlin().exec(gremlin_query)
+ if resp.get("data"):
+ matched_properties.add((key.name, keyword))
+ unmatched_keywords.remove(keyword)
+ return list(matched_properties), list(unmatched_keywords)
+
+ def _fuzzy_match_props(self, keywords: List[str]) -> List[str]:
+ fuzzy_match_result = []
+ for keyword in keywords:
+ keyword_vector = self.embedding.get_texts_embeddings([keyword])
+ results = self.prop_index.search(keyword_vector[0],
top_k=self.topk_per_keyword,
+
dis_threshold=float(self.vector_dis_threshold))
+ if results:
+ fuzzy_match_result.extend(results[:self.topk_per_keyword])
+ return fuzzy_match_result
+
+ def _reformat_mixed_list_to_unique_tuples(
+ self, mixed_data_list: List[Union[FrozenSet[Tuple[str, str]],
Tuple[str, str]]]
+ ) -> List[Tuple[str, str]]:
+ unique_tuples = set()
+ for item in mixed_data_list:
+ if isinstance(item, (frozenset, set)):
+ for prop_tuple in item:
+ if isinstance(prop_tuple, tuple) and len(prop_tuple) == 2:
+ unique_tuples.add(prop_tuple)
+ elif isinstance(item, tuple):
+ if len(item) == 2:
+ unique_tuples.add(item)
+ return list(unique_tuples)
+
def run(self, context: Dict[str, Any]) -> Dict[str, Any]:
graph_query_list = set()
if self.by == "query":
query = context["query"]
- query_vector = self.embedding.get_text_embedding(query)
+ query_vector = self.embedding.get_texts_embeddings([query])
results = self.vector_index.search(query_vector,
top_k=self.topk_per_query)
Review Comment:
‼️ **Pass a single query embedding to vector search**
Evidence: `get_texts_embeddings([query])` returns a list of embeddings, but
`VectorIndex.search()` expects one `List[float]` and checks `len(query_vector)
== index.d`. With an existing index this path passes a 2-D value and raises a
dimension mismatch, breaking `SemanticIdQuery(by="query")`. Please use the
first embedding, as the keyword/property branches already do, and add a
regression test for the whole-query path.
```suggestion
results = self.vector_index.search(query_vector[0],
top_k=self.topk_per_query)
```
##########
hugegraph-llm/src/hugegraph_llm/operators/hugegraph_op/graph_rag_query.py:
##########
@@ -207,31 +211,34 @@ def _subgraph_query(self, context: Dict[str, Any]) ->
Dict[str, Any]:
vertex_degree_list[0].update(vertex_knowledge)
else:
vertex_degree_list.append(vertex_knowledge)
- else:
+ elif matched_props:
# WARN: When will the query enter here?
- keywords = context.get("keywords")
- assert keywords, "No related property(keywords) for graph query."
- keywords_str = ",".join("'" + kw + "'" for kw in keywords)
- gremlin_query = PROPERTY_QUERY_NEIGHBOR_TPL.format(
- prop=self._prop_to_match,
- keywords=keywords_str,
- edge_labels=edge_labels_str,
- edge_limit=edge_limit_amount,
- max_deep=self._max_deep,
- max_items=self._max_items,
- )
- log.warning("Unable to find vid, downgraded to property query,
please confirm if it meets expectation.")
+ graph_chain_knowledge = set()
+ for prop_name, prop_value in matched_props:
+ self._prop_to_match = prop_name
+ gremlin_query = PROPERTY_QUERY_NEIGHBOR_TPL.format(
+ current_prop_name=prop_name,
+ current_prop_value=prop_value,
+ edge_labels=edge_labels_str,
+ edge_limit=edge_limit_amount,
+ max_deep=self._max_deep,
+ max_items=self._max_items
+ )
+ log.warning("Unable to find vid, downgraded to property query,
please confirm if it meets expectation.")
+ log.debug("property gremlin: %s", gremlin_query)
- paths: List[Any] =
self._client.gremlin().exec(gremlin=gremlin_query)["data"]
- graph_chain_knowledge, vertex_degree_list, knowledge_with_degree =
self._format_graph_query_result(
- query_paths=paths
- )
+ paths: List[Any] =
self._client.gremlin().exec(gremlin=gremlin_query)["data"]
Review Comment:
‼️ **Handle heterogeneous neighbors in property fallback**
Evidence: this traversal starts from vertices with `current_prop_name`, but
then walks arbitrary neighbors through `bothE().otherV()`. `_process_vertex()`
and `_process_edge()` later read `item["props"][self._prop_to_match]`; if a
neighbor label does not have the same property key, a normal heterogeneous
graph raises `KeyError` instead of returning partial graph context. Please
format property fallback paths using ids or safe `.get()` values for
non-matching neighbors.
##########
hugegraph-llm/src/hugegraph_llm/operators/hugegraph_op/graph_rag_query.py:
##########
@@ -207,31 +211,34 @@ def _subgraph_query(self, context: Dict[str, Any]) ->
Dict[str, Any]:
vertex_degree_list[0].update(vertex_knowledge)
else:
vertex_degree_list.append(vertex_knowledge)
- else:
+ elif matched_props:
# WARN: When will the query enter here?
- keywords = context.get("keywords")
- assert keywords, "No related property(keywords) for graph query."
- keywords_str = ",".join("'" + kw + "'" for kw in keywords)
- gremlin_query = PROPERTY_QUERY_NEIGHBOR_TPL.format(
- prop=self._prop_to_match,
- keywords=keywords_str,
- edge_labels=edge_labels_str,
- edge_limit=edge_limit_amount,
- max_deep=self._max_deep,
- max_items=self._max_items,
- )
- log.warning("Unable to find vid, downgraded to property query,
please confirm if it meets expectation.")
+ graph_chain_knowledge = set()
+ for prop_name, prop_value in matched_props:
+ self._prop_to_match = prop_name
+ gremlin_query = PROPERTY_QUERY_NEIGHBOR_TPL.format(
+ current_prop_name=prop_name,
+ current_prop_value=prop_value,
+ edge_labels=edge_labels_str,
+ edge_limit=edge_limit_amount,
+ max_deep=self._max_deep,
+ max_items=self._max_items
+ )
+ log.warning("Unable to find vid, downgraded to property query,
please confirm if it meets expectation.")
+ log.debug("property gremlin: %s", gremlin_query)
- paths: List[Any] =
self._client.gremlin().exec(gremlin=gremlin_query)["data"]
- graph_chain_knowledge, vertex_degree_list, knowledge_with_degree =
self._format_graph_query_result(
- query_paths=paths
- )
+ paths: List[Any] =
self._client.gremlin().exec(gremlin=gremlin_query)["data"]
+ log.debug("paths: %s", paths)
+ temp_graph_chain_knowledge, vertex_degree_list,
knowledge_with_degree = self._format_graph_query_result(
Review Comment:
⚠️ **Merge metadata for every matched property**
Evidence: `graph_chain_knowledge` is accumulated across all `matched_props`,
but `vertex_degree_list` and `knowledge_with_degree` are overwritten on every
loop iteration and only the last property's metadata is written to the context.
Callers can receive graph results from multiple property matches with
ranking/explainability metadata for only the final property. Please merge the
degree lists and `knowledge_with_degree` map alongside `graph_chain_knowledge`.
##########
hugegraph-llm/src/hugegraph_llm/api/vector_api.py:
##########
@@ -0,0 +1,65 @@
+# 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.
+
+
+from datetime import date
+from typing import Optional
+
+from fastapi import status, APIRouter, HTTPException, Body
+
+from hugegraph_llm.utils.log import log
+from hugegraph_llm.api.models.rag_requests import GraphConfigRequest
+from hugegraph_llm.config import huge_settings
+
+API_CALL_TRACKER = {}
+
+
+# pylint: disable=too-many-statements
+def vector_http_api(router: APIRouter, update_embedding_func):
+ @router.post("/vector/embedding", status_code=status.HTTP_200_OK)
+ def update_embedding_api(
+ daily_limit: int = 50,
Review Comment:
⚠️ **Do not let callers choose their own rate limit**
Evidence: the endpoint compares `call_count >= daily_limit`, but
`daily_limit` is a request parameter with a permissive default. Any
authenticated caller can bypass the protection by calling
`/vector/embedding?daily_limit=999999`, so the expensive embedding rebuild is
not actually rate-limited. Please move the limit to server-side config or clamp
user input to a configured maximum.
##########
hugegraph-llm/src/hugegraph_llm/operators/index_op/test.py:
##########
@@ -0,0 +1,242 @@
+# Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
⚠️ **Put regression coverage under the real test tree**
Evidence: this added `test.py` lives inside the production package and
duplicates operator code instead of adding tests under
`hugegraph-llm/src/tests/...`, which is where this module's CI-oriented tests
live. The new property-embedding, token config, property fallback, and vector
API behavior therefore has no runnable regression coverage. Please move this
into focused tests under `src/tests/api/` and `src/tests/operators/index_op/`
rather than shipping it as production package code.
##########
hugegraph-llm/src/hugegraph_llm/api/models/rag_requests.py:
##########
@@ -15,20 +15,21 @@
# specific language governing permissions and limitations
# under the License.
-from typing import Optional, Literal
-
+from typing import Optional, Literal, List
+from enum import Enum
from fastapi import Query
from pydantic import BaseModel
from hugegraph_llm.config import prompt
-
+from hugegraph_llm.config import huge_settings
class GraphConfigRequest(BaseModel):
url: str = Query('127.0.0.1:8080', description="hugegraph client url.")
- name: str = Query('hugegraph', description="hugegraph client name.")
- user: str = Query('', description="hugegraph client user.")
- pwd: str = Query('', description="hugegraph client pwd.")
+ graph: str = Query('hugegraph', description="hugegraph client name.")
Review Comment:
⚠️ **Keep the old `name` field compatible**
Evidence: the public graph config model changed `name` to `graph` and gives
`graph` a default of `"hugegraph"`. Existing clients that still send `{"name":
"custom_graph"}` will not get a validation error; the value is silently ignored
and requests fall back to the default graph. Please add an
alias/backward-compatible field mapping, or make old payloads fail loudly
instead of targeting the wrong graph.
--
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]
