bitflicker64 commented on code in PR #372:
URL: https://github.com/apache/hugegraph-ai/pull/372#discussion_r4270946251
##########
hugegraph-llm/src/hugegraph_llm/models/llms/openai.py:
##########
@@ -67,13 +67,15 @@ def generate(
completions = self.client.chat.completions.create(
model=self.model,
temperature=self.temperature,
- max_tokens=self.max_tokens,
+ **({"max_tokens": self.max_tokens} if self.max_tokens is not
None else {}),
messages=messages,
)
if not completions.choices:
raise RuntimeError(f"Empty choices in LLM response:
{str(completions)[:200]}")
if completions.usage:
log.info("Token usage: %s",
completions.usage.model_dump_json())
+ if completions.choices[0].finish_reason == "length":
+ raise LLMOutputTruncatedError()
Review Comment:
Important: this raises `LLMOutputTruncatedError` on `finish_reason=length`
for every caller of `OpenAIClient` and `LiteLLMClient`, not only the reviewed
path. The PR says requests without `review_options` "retain the existing
extraction path and response shape", but legacy `PropertyGraphExtract`,
`KeywordExtract` (256-token `get_extract_llm` default), `AnswerSynthesize` and
Text2Gremlin share these clients.
Reproduced with the SDK mocked to return `finish_reason="length"`, this head
versus `openai.py` from merge-base `5ee21cc`:
- Legacy `PropertyGraphExtract.run` with two chunks, the second truncated:
base returns the first chunk's vertex; this head raises, and `extract_sync`
turns that into HTTP 500 for the whole request.
- `KeywordExtract` with a 256-token response cut mid-list: base returns
`['graph', 'database', 'hugegraph']`; this head raises and the RAG query fails.
Requested change: make it opt-in, e.g. a `raise_on_truncation=False`
constructor argument set only for the reviewed path, plus a test that a default
client still returns truncated content. Same for the other three checks here
and the four in `litellm.py`.
##########
hugegraph-llm/src/hugegraph_llm/extraction_runtime/v1/contracts.py:
##########
@@ -0,0 +1,60 @@
+# 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.
+
+"""Inputs and per-chunk results for reviewed extraction."""
+
+from dataclasses import asdict, dataclass, field
+from typing import Literal
+
+from pydantic import BaseModel, Field
+
+
+class ExtractionOptions(BaseModel):
+ max_reviews: int = Field(default=2, ge=0, strict=True)
+ max_fixes: int = Field(default=1, ge=0, strict=True)
+ max_workers: int = Field(default=4, ge=1, strict=True)
Review Comment:
Important: these budgets come straight from the `POST /graph/extract` body
with only a lower bound, so `{"max_reviews": 1000000, "max_fixes": 1000000,
"max_workers": 100000}` validates. With `split_type: "sentence"`, one request
runs up to `min(max_workers, chunk count)` threads at once, each with its own
client from `prepare`, and up to `1 + max_reviews + max_fixes` model calls per
chunk while validation keeps failing or the reviewer keeps answering `fix`. All
of it is billed to the server's API key, and login is off by default
(`enable_login = "False"` in `config/admin_config.py`). The legacy path makes
one sequential call per chunk.
Requested change: add an upper bound (`le=`) to all three fields, as fixed
caps or a server-side setting a request can only lower, and a request-model
test that rejects values above it.
##########
hugegraph-llm/src/hugegraph_llm/extraction_runtime/property_graph.py:
##########
@@ -0,0 +1,288 @@
+# 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.
+
+"""Property-graph extraction and review using the configured model client."""
+
+import json
+import math
+import re
+from copy import deepcopy
+from dataclasses import asdict
+from datetime import date
+
+from hugegraph_llm.config import prompt
+from hugegraph_llm.extraction_runtime.v1.contracts import ChunkInput, Issue,
ReviewResult
+from hugegraph_llm.models.llms.base import BaseLLM
+from hugegraph_llm.operators.llm_op.property_graph_extract import (
+ PropertyGraphExtract,
+ generate_extract_property_graph_prompt,
+)
+
+
+def _invalid_constant(value: str):
+ raise ValueError(f"Invalid JSON constant: {value}")
+
+
+def _parse_float(value: str) -> float:
+ number = float(value)
+ if not math.isfinite(number):
+ raise ValueError(f"Non-finite JSON number: {value}")
+ return number
+
+
+def _parse_json(response: str):
+ text = response.strip()
+ fenced = re.fullmatch(r"```(?:json)?\s*\n?(.*?)\s*```", text, re.DOTALL |
re.IGNORECASE)
+ if fenced:
+ text = fenced.group(1)
+ return json.loads(text, parse_constant=_invalid_constant,
parse_float=_parse_float)
+
+
+def _parse_graph(response: str) -> dict:
+ graph = _parse_json(response)
+ if isinstance(graph, list):
+ grouped = {"vertices": [], "edges": []}
+ for item in graph:
+ if not isinstance(item, dict) or item.get("type") not in
("vertex", "edge"):
+ raise ValueError("Flat graph entries must declare type vertex
or edge")
+ grouped["vertices" if item["type"] == "vertex" else
"edges"].append(item)
+ graph = grouped
+ if not isinstance(graph, dict) or any(not isinstance(graph.get(key), list)
for key in ("vertices", "edges")):
+ raise ValueError("Graph response must contain vertices and edges
arrays")
+ for key, kind in (("vertices", "vertex"), ("edges", "edge")):
+ for item in graph[key]:
+ if isinstance(item, dict):
+ item.setdefault("type", kind)
+ return graph
+
+
+def _valid_id(value) -> bool:
+ return (isinstance(value, str) and bool(value)) or (isinstance(value, int)
and not isinstance(value, bool))
+
+
+def _valid_scalar(value, data_type: str) -> bool:
+ if data_type in ("TEXT", "UUID"):
+ return isinstance(value, str)
+ if data_type == "BOOLEAN":
+ return isinstance(value, bool)
+ if data_type in ("BYTE", "INT", "LONG"):
+ bits = {"BYTE": 8, "INT": 32, "LONG": 64}[data_type]
+ return isinstance(value, int) and not isinstance(value, bool) and -(2
** (bits - 1)) <= value < 2 ** (bits - 1)
+ if data_type in ("FLOAT", "DOUBLE"):
+ if not isinstance(value, (int, float)) or isinstance(value, bool):
+ return False
+ try:
+ return math.isfinite(value)
+ except OverflowError:
+ return False
+ if data_type == "DATE":
+ if not isinstance(value, str):
+ return False
+ try:
+ date.fromisoformat(value)
+ return True
+ except ValueError:
+ return False
+ return False
Review Comment:
Important: `_valid_scalar` rejects values HugeGraph accepts.
- `DATE` only passes `date.fromisoformat`, which rejects `"2024-01-01
10:00:00"` and `"2024-01-01 10:00:00.000"` on Python 3.10 and 3.11 (checked on
this head) and any epoch-millisecond number. HugeGraph's `DateUtil` parses
`yyyy-MM-dd`, `yyyy-MM-dd HH:mm:ss` and `yyyy-MM-dd HH:mm:ss.SSS`.
- `OBJECT` and `BLOB` reach the final `return False` for every value.
Each such value is a validation issue, so the chunk ends `blocked:
validation_failed` once `max_fixes` is spent unless repair drops the value; for
an `OBJECT` or `BLOB` primary or non-nullable key no repair can pass. The
legacy path does not type-check values, so text that extracts there is dropped
once `review_options` is set.
Requested change: accept the forms HugeGraph parses, skip the check for
types the validator does not model, and add `DATE`-with-time, `OBJECT` and
`BLOB` cases to `test_property_graph.py`.
--
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]
