bitflicker64 commented on code in PR #372: URL: https://github.com/apache/hugegraph-ai/pull/372#discussion_r3952041389
########## hugegraph-llm/src/hugegraph_llm/extraction_runtime/__init__.py: ########## @@ -0,0 +1,18 @@ +# 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. + +"""Dormant experimental extraction runtime. Review Comment: ‼️ Nothing outside this package's own tests calls it. The docstring right here says "Dormant", `docs/extraction-runtime.md` says "The package has no production caller", and `test_packaging_compatibility.py::test_production_modules_do_not_import_the_dormant_runtime` enforces it. That is 2,612 lines landing inside the distributed package `src/hugegraph_llm/`, so `pip install hugegraph-llm` ships a dormant subsystem plus a 300-line fixture domain (`conformance/inventory.py`) and a replay test double. The tightest example of the pattern is the packaged descriptor: `resources/runtime-contract-v1.json` (25 lines), `resources/__init__.py`, `v1/resources.py` with its schema-version validation, and a new `MANIFEST.in` recursive-include. That is roughly 71 lines and a packaging rule whose only consumer is a test asserting the JSON still equals the `RUNTIME_CONTRACT` literal in `v1/fingerprint.py` that it was copied from. Requested change: land the slice a caller exercises. Pick the one integration you actually want (`GraphExtractFlow` routing a single extraction through the review/repair loop), and ship the engine, graph state, budget accounting, and terminal resolution that path uses. Fingerprint layers, semantic manifests, adaptation records, the packaged descriptor, and the provider dialect can follow the first code that reads them. If the prototype must land whole, put it in a top-level `examples/` rather than `src/hugegraph_llm/`, so users don't install a subsystem the project itself does not call. ########## hugegraph-llm/src/hugegraph_llm/extraction_runtime/provider/dialect.py: ########## @@ -0,0 +1,171 @@ +# 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. + +"""Provider capability planning without transport or credentials.""" + +from __future__ import annotations + +from hugegraph_llm.extraction_runtime.provider.contracts import ( + AdaptationAction, + AdaptationDecisionV1, + AdaptationRecordV1, + EffectiveRequestV1, + ProviderCapabilitiesV1, + ProviderNeutralRequestV1, + UnsupportedProviderParameterError, +) +from hugegraph_llm.extraction_runtime.v1.json_value import JsonObject, digest_json, freeze_json_object + + +class ProviderDialectV1: + """Plan a credential-free effective request from explicit capabilities.""" + + contract = "provider-dialect/v1" + + def plan( Review Comment: ⚠️ litellm already does this, and it is already a dependency. `hugegraph-llm/pyproject.toml` declares `litellm`, and `models/llms/litellm.py` already wraps it. `litellm.get_supported_openai_params(model=...)` reports which parameters a provider accepts and `litellm.drop_params = True` drops the rest: the same kept / dropped / downgraded decision this 171-line dialect and the `ProviderCapabilitiesV1` / `AdaptationDecisionV1` / `AdaptationRecordV1` dataclasses hand-roll. The difference cuts against the hand-rolled version, because litellm knows each provider's real capabilities, whereas `ProviderCapabilitiesV1` makes every caller declare them by hand and get the optional ones quietly wrong. Related: `retry_policy` and `timeout_seconds` are validated, copied into the effective payload, and folded into the digest, but no transport reads either. Live transports being application work is the stated design, so the simplification is to add these two fields alongside the transport that honours them. Retry in this repo is `tenacity`, already a dependency and already used in `models/llms/litellm.py`. Requested change: drop `provider/dialect.py` and the capability/adaptation dataclasses, and build the request through the existing `LiteLLMClient` when a live transport lands. If a record of dropped parameters is genuinely needed, it is a log line next to that call, not a contract layer. ########## hugegraph-llm/src/tests/extraction_runtime/test_packaging_compatibility.py: ########## @@ -0,0 +1,75 @@ +# 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 ast +from pathlib import Path + +import pytest + +from hugegraph_llm.api.models.graph_extract_requests import GraphExtractRequest +from hugegraph_llm.extraction_runtime.v1 import canonical_json +from hugegraph_llm.extraction_runtime.v1.fingerprint import RUNTIME_CONTRACT +from hugegraph_llm.extraction_runtime.v1.resources import load_runtime_contract_resource + +pytestmark = pytest.mark.contract + + +def test_runtime_contract_resource_matches_implementation() -> None: + resource = load_runtime_contract_resource() + assert canonical_json(resource["runtime_contract"]) == canonical_json(RUNTIME_CONTRACT) + assert resource["public_integration"] == "none" + assert tuple(resource["terminal_kinds"]) == ("final", "candidate", "blocked", "failed") + + +def test_graph_extract_route_and_defaults_remain_unchanged() -> None: Review Comment: ⚠️ These tests pin the source text of files this PR does not touch. `test_graph_extract_route_and_defaults_remain_unchanged` string-matches an exact decorator line from `api/graph_extract_api.py`, and `test_existing_scheduler_still_owns_graph_extract_flow` (line 49) string-matches an import line and two expressions from `flows/scheduler.py`. Neither file imports the runtime, so nothing here can regress it, but wrapping a long line or reordering a decorator argument in either file breaks this PR's tests for reasons unrelated to extraction. `test_production_modules_do_not_import_the_dormant_runtime` (line 57) has the mirror-image problem: it is a test asserting the feature is unused, so it has to be deleted the day the feature is used. Requested change: drop the string-matching assertions and delete that last test. Keep lines 42-46, the four `GraphExtractRequest` default checks, which are behavioural and survive reformatting. `test_dependency_guard.py` already enforces the direction that matters (the runtime not reaching into `api`, `flows`, `nodes`, `operators` or `pyhugegraph`), and it does so by walking imports rather than matching source strings. Keep that one. ########## hugegraph-llm/src/hugegraph_llm/extraction_runtime/v1/batch.py: ########## @@ -0,0 +1,58 @@ +# 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. + +"""Concurrent execution of independent chunks using the single-chunk engine.""" + +from collections.abc import Callable, Iterable +from concurrent.futures import ThreadPoolExecutor +from dataclasses import dataclass + +from hugegraph_llm.extraction_runtime.v1.contracts import NormalizedChunkV1 +from hugegraph_llm.extraction_runtime.v1.engine import ( + ExtractionBundleV1, + ExtractionEngineV1, + ExtractionRunResultV1, + RunControlV1, +) + + +@dataclass(frozen=True) +class ChunkRunResultV1: + chunk: NormalizedChunkV1 + result: ExtractionRunResultV1 + + +def run_chunks_v1( + *, + chunks: Iterable[NormalizedChunkV1], + prepare: Callable[[NormalizedChunkV1], tuple[ExtractionBundleV1, RunControlV1]], + max_workers: int = 4, +) -> tuple[ChunkRunResultV1, ...]: + """Run a finite batch with at most ``max_workers`` chunks executing at once. + + ``prepare`` runs in worker threads and must create a separate Bundle and + stateful Provider for each chunk. It returns that Bundle and its control. + Results follow input order, regardless of completion order or chunk ordinal. + Engine failure terminals are collected normally; preparation and input + iteration errors propagate to the caller. The pool closes after running + tasks finish; preparation errors may cancel tasks that have not started. + The full batch is submitted and retained in memory. + """ + + def run_chunk(chunk: NormalizedChunkV1) -> ChunkRunResultV1: + bundle, control = prepare(chunk) + result = ExtractionEngineV1().run(bundle=bundle, chunk=chunk, control=control) + return ChunkRunResultV1(chunk=chunk, result=result) + + with ThreadPoolExecutor(max_workers=max_workers) as executor: Review Comment: ⚠️ This module is `executor.map`. Take away the license header, the `ChunkRunResultV1` dataclass and the 10-line docstring, and what is left is a four-line `run_chunk` closure over `prepare` and `ExtractionEngineV1().run`, then: ```python with ThreadPoolExecutor(max_workers=max_workers) as executor: return tuple(executor.map(run_chunk, chunks)) ``` The docstring, plus roughly twenty more lines in `docs/extraction-runtime.md`, partly restates `ThreadPoolExecutor`'s own documented behaviour: results in input order, running tasks finish before the pool closes, the whole batch held in memory. `ChunkRunResultV1` pairs a chunk with the result of that chunk, which the caller already has. Requested change: delete `v1/batch.py` along with its `v1/__init__.py` re-exports and the 195-line `test_batch.py`, and put those two lines in the docs instead: a caller who wants a batch writes them and keeps its own result type. If a batch helper does earn a place later, note the repo already runs every extraction flow through `pycgraph.GPipeline` (`flows/graph_extract.py`), which owns element ordering and parallel execution. ########## hugegraph-llm/src/hugegraph_llm/extraction_runtime/provider/contracts.py: ########## @@ -0,0 +1,240 @@ +# 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. + +"""Credential-free provider-neutral execution contracts.""" + +from __future__ import annotations + +import math +from dataclasses import dataclass, field +from enum import Enum +from typing import Literal, Protocol + +from hugegraph_llm.extraction_runtime.v1.json_value import JsonObject, digest_json, freeze_json_object + +_CREDENTIAL_PARAMETER_NAMES = { + "api_key", + "authorization", + "cookie", + "cookies", + "password", + "secret", + "token", +} +_RESERVED_PARAMETER_NAMES = { + "max_output_tokens", + "messages", + "model", + "parallel_tool_calls", + "reasoning_effort", + "response_schema", + "retry_policy", + "strict_schema", + "thinking", + "timeout_seconds", + "tools", +} + + +class UnsupportedProviderParameterError(ValueError): + """Raised when removing a parameter would change required semantics.""" + + +class AdaptationAction(str, Enum): + KEPT = "kept" + DROPPED = "dropped" + DOWNGRADED = "downgraded" + + +@dataclass(frozen=True) +class ProviderMessageV1: + role: Literal["system", "user", "assistant", "tool"] + content: str + name: str | None = None + + def __post_init__(self) -> None: + if not self.content: + raise ValueError("provider message content must not be empty") + + +@dataclass(frozen=True) +class RetryPolicyV1: + max_attempts: int = 1 + backoff_seconds: float = 0.0 + + def __post_init__(self) -> None: + if self.max_attempts < 1: + raise ValueError("max_attempts must be at least one") + if not math.isfinite(self.backoff_seconds) or self.backoff_seconds < 0: + raise ValueError("backoff_seconds must be finite and non-negative") + + +@dataclass(frozen=True) +class ProviderNeutralRequestV1: + stage: str + model: str + messages: tuple[ProviderMessageV1, ...] + max_output_tokens: int = 1024 + temperature: float = 0.0 + reasoning_effort: str | None = None + thinking: JsonObject | None = None + tools: tuple[JsonObject, ...] = () + response_schema: JsonObject | None = None + strict_schema: bool = False + parallel_tool_calls: bool | None = None + optional_parameters: JsonObject = field(default_factory=dict) + timeout_seconds: float = 30.0 + retry_policy: RetryPolicyV1 = field(default_factory=RetryPolicyV1) + contract: Literal["provider-neutral-request/v1"] = "provider-neutral-request/v1" + + def __post_init__(self) -> None: + if not self.stage: + raise ValueError("provider request stage must not be empty") + if not self.model: + raise ValueError("provider request model must not be empty") + if not self.messages: + raise ValueError("provider request must contain at least one message") + if self.max_output_tokens < 1: + raise ValueError("max_output_tokens must be positive") + if not math.isfinite(self.temperature) or self.temperature < 0: + raise ValueError("temperature must be finite and non-negative") + if not math.isfinite(self.timeout_seconds) or self.timeout_seconds <= 0: + raise ValueError("timeout_seconds must be finite and positive") + if self.strict_schema and not (self.tools or self.response_schema): + raise ValueError("strict_schema requires tools or a response schema") + if self.parallel_tool_calls is not None and not self.tools: + raise ValueError("parallel_tool_calls requires tools") + frozen_optional = freeze_json_object(self.optional_parameters) + for name in frozen_optional: + normalized = name.lower().replace("-", "_") + if normalized in _CREDENTIAL_PARAMETER_NAMES or normalized.endswith("_token"): + raise ValueError(f"credential parameter {name!r} is forbidden") + if normalized in _RESERVED_PARAMETER_NAMES: + raise ValueError(f"optional parameter {name!r} collides with a typed field") + object.__setattr__(self, "optional_parameters", frozen_optional) + object.__setattr__(self, "tools", tuple(freeze_json_object(tool) for tool in self.tools)) + if self.thinking is not None: + object.__setattr__(self, "thinking", freeze_json_object(self.thinking)) + if self.response_schema is not None: + object.__setattr__(self, "response_schema", freeze_json_object(self.response_schema)) + + def as_evidence_payload(self) -> JsonObject: + return freeze_json_object( + { + "contract": self.contract, + "stage": self.stage, + "model": self.model, + "messages": [ + {"role": message.role, "content": message.content, "name": message.name} + for message in self.messages + ], + "max_output_tokens": self.max_output_tokens, + "temperature": self.temperature, + "reasoning_effort": self.reasoning_effort, + "thinking": self.thinking, + "tools": list(self.tools), + "response_schema": self.response_schema, + "strict_schema": self.strict_schema, + "parallel_tool_calls": self.parallel_tool_calls, + "optional_parameters": self.optional_parameters, + "timeout_seconds": self.timeout_seconds, + "retry_policy": { + "max_attempts": self.retry_policy.max_attempts, + "backoff_seconds": self.retry_policy.backoff_seconds, + }, + } + ) + + +@dataclass(frozen=True) +class ProviderCapabilitiesV1: + reasoning_effort: bool = False + thinking: bool = False + structured_tools: bool = False + strict_schema: bool = False + parallel_tool_calls: bool = False + optional_parameters: tuple[str, ...] = () + contract: Literal["provider-capabilities/v1"] = "provider-capabilities/v1" + + def __post_init__(self) -> None: + if len(set(self.optional_parameters)) != len(self.optional_parameters): + raise ValueError("provider optional capability names must be unique") + + +@dataclass(frozen=True) +class AdaptationDecisionV1: + parameter: str + action: AdaptationAction + reason_code: str + requested_category: str + effective_category: str + + +@dataclass(frozen=True) +class AdaptationRecordV1: + adapter_contract: str + requested_digest: str + effective_digest: str + decisions: tuple[AdaptationDecisionV1, ...] + + +@dataclass(frozen=True) +class EffectiveRequestV1: + payload: JsonObject + adaptation: AdaptationRecordV1 + contract: Literal["provider-effective-request/v1"] = "provider-effective-request/v1" + + def __post_init__(self) -> None: + frozen = freeze_json_object(self.payload) + if digest_json(frozen) != self.adaptation.effective_digest: + raise ValueError("adaptation effective_digest does not bind the effective payload") + object.__setattr__(self, "payload", frozen) + + +@dataclass(frozen=True) +class ProviderResponseV1: + output: JsonObject + model: str + model_revision: str | None = None + usage: JsonObject = field(default_factory=dict) + contract: Literal["provider-response/v1"] = "provider-response/v1" + + def __post_init__(self) -> None: + if not self.model: + raise ValueError("provider response model must not be empty") + object.__setattr__(self, "output", freeze_json_object(self.output)) + object.__setattr__(self, "usage", freeze_json_object(self.usage)) + + @property + def response_digest(self) -> str: + return digest_json( + { + "contract": self.contract, + "output": self.output, + "model": self.model, + "model_revision": self.model_revision, + "usage": self.usage, + } + ) + + +class ProviderAdapterV1(Protocol): Review Comment: ‼️ Nothing is typed against `ProviderAdapterV1`. It is declared here and re-exported in `provider/__init__.py`, and that is every reference to the name in the tree: no parameter is annotated with it, nothing accepts it, and `InventoryBundleV1.__init__` builds a concrete `ProviderDialectV1` directly. `ProviderDialectV1.plan` does match it structurally, which is rather the point: the satisfying class is one file over, so declaring the Protocol buys nothing today. The other two are barely better. `ProviderTransportV1` (line 239) is satisfied only by `ReplayProvider` and the scripted doubles in the tests; `ExtractionBundleV1` (`v1/engine.py:53`) only by the inventory fixture and `test_engine.py`'s `ScriptedInventoryBundle`. Requested change: delete `ProviderAdapterV1` and its export, and let `InventoryBundleV1` name `ProviderDialectV1` directly. Add a Protocol back the day a second implementation exists. (Leaving `ExtractionEngineV1` alone here on purpose: your own `test_dependency_guard.py` forbids `v1/` importing `conformance`, so the engine cannot name the concrete bundle without that guard going too.) -- 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]
