bitflicker64 commented on code in PR #372: URL: https://github.com/apache/hugegraph-ai/pull/372#discussion_r4303162198
########## hugegraph-llm/src/hugegraph_llm/extraction_runtime/v1/engine.py: ########## @@ -0,0 +1,104 @@ +# 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. + +"""A bounded extract, validate, review and repair loop for one chunk.""" + +import logging +from typing import Literal, Protocol + +from hugegraph_llm.extraction_runtime.v1.contracts import ( + ChunkInput, + ChunkResult, + ExtractionOptions, + Issue, + ReviewResult, +) +from hugegraph_llm.models.llms.base import LLMOutputTruncatedError + +log = logging.getLogger(__name__) + + +class ExtractionBundle(Protocol): + def extract(self, chunk: ChunkInput) -> dict: ... + + def validate(self, graph: dict, chunk: ChunkInput) -> list[Issue]: ... + + def review(self, graph: dict, chunk: ChunkInput) -> ReviewResult: ... + + def repair(self, graph: dict, chunk: ChunkInput, issues: list[Issue]) -> dict: ... + + +class ExtractionEngineV1: Review Comment: Important: should not exist. `v1/` is a version namespace for code that has no v0. It costs five files and 268 lines: `extraction_runtime/__init__.py` (a docstring), `v1/__init__.py` (re-exports), `contracts.py`, `engine.py` and `batch.py`, each with its own license header. `ExtractionEngineV1` holds no state. Every caller writes `ExtractionEngineV1().run(...)`, and `batch.py:50` builds a new one per chunk only to call that one method. `ExtractionBundle` is a Protocol with one production implementation, `PropertyGraphBundle`. The test `ScriptedBundle` is duck-typed and never imports it. Merge it all into one module, for example `extraction_runtime/review.py`, holding the dataclasses and two functions: ```python def run_chunk(bundle, chunk: ChunkInput, options: ExtractionOptions) -> ChunkResult: ... # the body of ExtractionEngineV1.run, unchanged def run_chunks(chunks, prepare, options) -> tuple[ChunkResult, ...]: ... # the prepare-then-run_chunk closure and executor.map from batch.py ``` Drop the Protocol and the `V1`/`_v1` suffixes. The imports change in `extract_info.py`, `graph_extract_requests.py`, `flows/graph_extract.py`, `state/ai_state.py`, the docs example and the tests. Name a second version when one exists. ########## hugegraph-llm/src/hugegraph_llm/models/llms/litellm.py: ########## @@ -68,14 +70,16 @@ def generate( model=self.model, messages=messages, temperature=self.temperature, - max_tokens=self.max_tokens, + **({"max_tokens": self.max_tokens} if self.max_tokens is not None else {}), Review Comment: Minor: local tidy. In this file the wrapper has no effect. `litellm.completion` and `acompletion` already default to `max_tokens=None`. `get_optional_params` drops every argument that is still at its default before it builds the provider request (`_get_non_default_params` in `litellm/utils.py`, 1.61.20). So `max_tokens=self.max_tokens` already sends no limit for `None`. Revert lines 73, 105, 132 and 168 to the original `max_tokens=self.max_tokens`. Then change `test_output_limits.py:116` to `assert sdk_call.call_args.kwargs.get("max_tokens") is None`, which holds for both clients. Keep the `**` form in `openai.py`, because that SDK strips only `NOT_GIVEN`, not `None`. ########## hugegraph-llm/src/hugegraph_llm/extraction_runtime/property_graph.py: ########## @@ -0,0 +1,397 @@ +# 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 datetime +from decimal import Decimal + +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, +) + +_JAVA_HEX_LETTERS = str.maketrans("ＡＢＣＤＥＦａｂｃｄｅｆ", "ABCDEFabcdef") +# The midpoint above the largest finite float32 rounds to infinity, not to FLOAT_MAX. +_FLOAT32_OVERFLOW_MIDPOINT = Decimal(2**128 - 2**103) + + +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_uuid(value) -> bool: + """Match HugeGraph StringEncoding.uuid without rewriting the supplied value.""" + if not isinstance(value, str): + return False + if len(value) == 36 and "-" in value: + # Java UUID.fromString also accepts nonstandard widths and leading '+'. + parts, bits = value.split("-"), 63 + if len(parts) != 5: + return False + elif len(value) == 32: + parts, bits = [value[:16], value[16:]], 64 + else: + return False + for part in parts: + # Java Character.digit(char, 16) includes BMP decimal digits and fullwidth + # A-F. Python int also accepts underscores and non-BMP digits; exclude them. + part = part.translate(_JAVA_HEX_LETTERS) + if re.fullmatch(r"\+?[\da-fA-F]+", part) is None or any(ord(char) > 0xFFFF for char in part): + return False + if int(part, 16) >= 2**bits: + return False + return True + + +def _uuid_identity(value: str) -> int: + """Compare accepted spellings as StringEncoding.uuid/UUID.fromString does.""" + if len(value) == 32: + return (int(value[:16].translate(_JAVA_HEX_LETTERS), 16) << 64) | int( + value[16:].translate(_JAVA_HEX_LETTERS), 16 + ) + parts = [int(part.translate(_JAVA_HEX_LETTERS), 16) for part in value.split("-")] + identity = 0 + for part, bits in zip(parts, (32, 16, 16, 16, 48)): + identity = (identity << bits) | (part & (2**bits - 1)) + return identity + + +def _float_identity(value) -> tuple[bool, int]: Review Comment: Important: replace or collapse. `struct` already rounds to IEEE float32 ties-to-even, and it raises `OverflowError` when the result is infinity. That replaces this 20-line function, `_FLOAT32_OVERFLOW_MIDPOINT` and the `Decimal` comparison on line 157: ```python # _valid_scalar, in place of line 157 if data_type == "FLOAT": try: struct.pack("<f", value) except OverflowError: return False return True ``` and in `_primary_identity`, `return data_type, struct.pack("<f", value)`. I compared this function with `struct.pack("<f", v)` on 300,000 random doubles: no mismatches. The range checks agree on 1e100, -1e100, 3.4028235e38, 3.4028236e38, 1e-46 and -0.0. They disagree only on a double that sits exactly on a float32 midpoint, such as `2**128 - 2**103`. Java rounds the decimal string while `struct` rounds the binary value, so this value is rejected and costs one repair. No model writes that number. Name the limit in a `# ponytail:` comment rather than keeping 20 lines of bit arithmetic for it. ########## hugegraph-llm/src/hugegraph_llm/extraction_runtime/property_graph.py: ########## @@ -0,0 +1,397 @@ +# 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 datetime +from decimal import Decimal + +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, +) + +_JAVA_HEX_LETTERS = str.maketrans("ＡＢＣＤＥＦａｂｃｄｅｆ", "ABCDEFabcdef") +# The midpoint above the largest finite float32 rounds to infinity, not to FLOAT_MAX. +_FLOAT32_OVERFLOW_MIDPOINT = Decimal(2**128 - 2**103) + + +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_uuid(value) -> bool: Review Comment: Important: replace or collapse. `_JAVA_HEX_LETTERS`, this function and `_uuid_identity` add about 40 lines that copy every quirk of Java's `UUID.fromString`: fullwidth `Ａ-Ｆ`, a leading `+`, BMP decimal digits and hex groups of any width. A model asked for a UUID writes 8-4-4-4-12 or 32 hex characters. Accept those two forms and let the stdlib do the arithmetic: ```python _UUID = re.compile(r"[0-9a-f]{8}(-[0-9a-f]{4}){3}-[0-9a-f]{12}|[0-9a-f]{32}", re.ASCII | re.IGNORECASE) def _valid_uuid(value) -> bool: return isinstance(value, str) and _UUID.fullmatch(value) is not None ``` Use `uuid.UUID(value).int` as the identity. It equals `_uuid_identity` for both forms. Do not use bare `uuid.UUID(value)` as the check: it accepts `{...}` and `urn:uuid:`, which HugeGraph rejects. A quirky spelling that Java would accept goes to repair, and nothing invalid reaches `final`. The quirk cases in `test_hugegraph_compatible_uuids_reach_final_without_normalization` can then be deleted. -- 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]
