Repository navigation
Conversation
hidb4ai
left a comment
There was a problem hiding this comment.
Found three cases where the emitted diagnostics identify the wrong failure cause or stage. Details are inline.
| cause_exception_type=cause_type, | ||
| ), | ||
| ) | ||
| if isinstance(error, (UnexpectedModelBehavior, ValidationError)): |
There was a problem hiding this comment.
[P2] Route embedding validation failures through this mapping
Through the public PydanticAIEmbeddingModel.embed() entry point, an injected embedding model raising pydantic.ValidationError produces InferenceUnavailableError with embedding/provider_unavailable. ValidationError inherits from ValueError, so _embed() catches it at lines 287-288 before _map_error() is called. This new mapping is therefore unreachable for that failure, although the direct helper test passes. Ensure the public embedding path reports embedding/invalid_output and preserves the original exception type in diagnostics; cover this through embed() rather than only _map_error().
There was a problem hiding this comment.
Rebased onto 0bc42d0. embed() routes ValidationError through the shared mapping, reporting embedding/invalid_output with cause_exception_type=ValidationError; a public-entry test covers it. The exception-type change and the FTS/text fallback impact are documented in the PR description.
| return attach_diagnostics( | ||
| InvalidInferenceOutputError(operation, "provider did not return a valid result"), | ||
| ProcessingDiagnostics( | ||
| stage=ProcessingStage.USAGE_ACCOUNTING, |
There was a problem hiding this comment.
[P2] Keep inference usage limits in the originating processing stage
With InferenceLimits(output_tokens_limit=10), a valid model response reporting 11 output tokens causes generate() to emit usage_accounting/usage_limit. Pydantic AI raises this exception while enforcing the generation budget, before the usage-reporting wrapper reaches accounting. Labeling it as an accounting failure misattributes a core processing failure and defeats #2126's requirement to distinguish the two. Keep generation/usage_limit for this case and reserve usage_accounting for failures in usage reporting or persistence.
There was a problem hiding this comment.
Rebased. Usage limits are reported in the originating stage (generation, or embedding when an adapter reports it) with failure_reason=usage_limit; usage_accounting is reserved for usage reporting or persistence. Public generate() and embed() tests cover both.
| reason=( | ||
| ProcessingFailureReason.RATE_LIMIT | ||
| if rate_limited | ||
| else ProcessingFailureReason.PROVIDER_UNAVAILABLE |
There was a problem hiding this comment.
[P2] Classify MiniMax HTTP failures from their status
Using MockTransport through the public MiniMaxEmbeddingModel.embed() entry point, HTTP 401, 408, and 504 all emit embedding/provider_unavailable: every non-429 status takes this branch. A 401 indicates an authentication/configuration problem, while 408/504 provide timeout evidence, which the Pydantic AI adapter already classifies separately. This gives operators different failure diagnoses for the same HTTP evidence. Derive the diagnostic reason from the status, or omit it when unsupported, while preserving the existing public exception contract if required.
There was a problem hiding this comment.
Rebased. MiniMax diagnostics follow the HTTP status: 401/403 configuration, 408/504 timeout, 409/425/5xx provider_unavailable, 429 rate_limit, and other 4xx omit the reason. The public exception remains InferenceUnavailableError; parametrized public embed() tests cover these cases.
| result = await self.extractor.generate(value) | ||
| output = AtomicMemoryExtractionOutput.model_validate(result.output) | ||
| try: | ||
| output = AtomicMemoryExtractionOutput.model_validate(result.output) |
There was a problem hiding this comment.
[P1] This branch no longer merges into master
master has moved past the PR base. The merge-base is c0c38dea, master is now 0bc42d0b (13 commits, including #2139 which rewrote input handling in this very function). git merge-tree --write-tree master refs/pr/2189 reports exactly one content conflict, and it is in this hunk: the new try: / except ValidationError wrapper around AtomicMemoryExtractionOutput.model_validate. GitHub agrees with mergeable: false / mergeable_state: dirty, so the PR cannot merge as submitted.
Two facts to fold into the rebase:
- The wrapper has to be re-applied on top of the new body from perf(atomic-memory): stop duplicating source text in extraction input to reduce token consumption #2139, not just carried over.
- feat(recall): add bounded atomic memory manifest #2192 is open at the same time and rewrites this same block to thread
descriptionthroughAtomicMemoryCandidate/AtomicMemoryContent. Whichever PR lands second must re-resolve this hunk, and the PR body's testing section should record how that reconciliation was verified.
Scope note: this is a branch-sync blocker, not a defect in the change. I re-ran the PR's own selection on the unmodified head (61df175a, Python 3.14.3): tests/builtin/test_processing_diagnostics.py tests/builtin/inference/test_pydantic_ai.py tests/builtin/inference/test_minimax_embedding.py tests/builtin/artifacts/atomic_memory tests/builtin/artifacts/memory tests/builtin/runtime/test_artifact_processing.py tests/e2e/test_worker_logging.py -> 213 passed, 5 skipped.
There was a problem hiding this comment.
Rebased onto 0bc42d0. The only conflict was the import block in atomic_memory/extraction.py; the try/except wrapper still covers model_validate and AtomicMemoryContent. git merge-tree --write-tree upstream/master HEAD exits 0 locally, a range-diff against the pre-rebase head preserved both commits, and the PR description records the #2192 overlap on the same hunk. Tests on the rebased tree: 276 passed, 5 skipped.
There was a problem hiding this comment.
@guamiao, GitHub currently reports merge conflicts with master (mergeable_state: dirty). Please update this branch against the latest master and resolve the conflicts, preserving the latest master changes and the new diagnostics. Then rerun the Memory/Atomic Memory, inference, and worker logging tests, and update the PR with the validation results.
There was a problem hiding this comment.
Conflict resolutions (70f6aa7e):
inference/pydantic_ai.py: kept master's_checked_rulehelper and the bounded_map_errornoqa.runtime/artifact_processing.py: kept master'sprocessing_error_code(type, error_code)call and re-applied the diagnostics classification.runtime/atomic_memory_processing.py: merged master's newSOURCES_TABLEimport with the diagnostics imports.tests/builtin/inference/test_pydantic_ai.py: kept master's expanded imports and addedUsageLimitExceeded/_map_error.
atomic_memory/extraction.py merged without a conflict on this head; the try/except ValidationError wrappers around AtomicMemoryExtractionOutput.model_validate and AtomicMemoryContent remain intact on top of the current body. #2192 is still open; if it lands first, this block will need the same re-resolution noted earlier.
Validation on 70f6aa7e (Python 3.14.3):
pytest tests/builtin/test_processing_diagnostics.py tests/builtin/inference tests/builtin/artifacts/atomic_memory tests/builtin/artifacts/memory tests/builtin/runtime/test_artifact_processing.py tests/e2e/test_worker_logging.py -q→ 272 passed, 5 skippedruff check,ruff format --check,ty checkon the changed files → cleangit diff --check upstream/master..HEAD→ clean- GitHub reports
mergeable: true.
The stacked Topic Memory branch was rebased onto this head (f7a06284); its suite is 555 passed, 5 skipped.
There was a problem hiding this comment.
复核新 head 70f6aa7e:mergeable=MERGEABLE,rebase 后与 master 无冲突,本线程的 P1 已解除。独立验证(exact head 树 + 隔离 PYTHONPATH):tests/builtin/test_processing_diagnostics.py 与 tests/builtin/inference/test_minimax_embedding.py 50 passed、tests/builtin/inference/test_pydantic_ai.py 39 passed;extraction.py 的 try/except ValidationError 包裹在 rebase 后仍覆盖 model_validate。按已修复记录,不再重复提出。
| | `failure_location` | Stable internal location or operation identifier | Bounded string | | ||
| | `cause_exception_type` | Original provider or library exception type before mapping | Bounded string | | ||
|
|
||
| `processing_stage` is distinct from the existing `stage` field, which names the Worker or |
There was a problem hiding this comment.
[P2] stage is not limited to Worker/supervisor boundaries
This sentence is the only basis for telling stage and processing_stage apart, but it understates the real value domain of stage. _log_failure emits error.failure.stage, which is copied from block.stage in the child Worker (src/powercontext/builtin/runtime/artifact_processing.py), and the only producer of a block sets stage="topic_memory" (src/powercontext/builtin/runtime/topic_memory_scope.py:85). The child also falls back to _safe_error_attribute(error, "stage", "worker"), so an exception-defined domain value can reach the field as well.
Impact: a real record such as stage=topic_memory next to processing_stage=generation reads, under this contract, as if stage were a boundary name, and the failure gets attributed to the wrong layer. The same wording is duplicated in docs/zh/development/processing-failures.md.
Suggest documenting the actual domain (boundary values, plus domain block names such as topic_memory, plus the worker fallback), or changing the field so that the documented distinction is accurate.
There was a problem hiding this comment.
Updated both locales. stage now documents Worker/supervisor values (worker_process, work_admission, supervisor, lease_renewal, scope_discovery, pending_observation, term_revocation, worker_termination), domain block names such as topic_memory, the child worker fallback, and the omitted-stage case. processing_stage remains the bounded domain stage, and usage_accounting is marked reserved.
There was a problem hiding this comment.
复核 70f6aa7e:上一条 P2 已闭合。docs/{en,zh}/development/processing-failures.md 现在把 stage 定义为"发出失败的位置或领域位置",并逐项列举 _log_failure 的全部实参——artifact_processing.py:678 supervisor、:736 lease_renewal、:779 scope_discovery、:807 pending_observation、:1141 work_admission、:1434 worker、:1469 term_revocation、:1499 worker_termination,加上 worker_process(:345/352/364)与领域 block topic_memory(topic_memory_scope.py:85),并说明 child Worker 的 worker 兜底与"没有领域 block 时可省略"。逐一比对源码与该表,取值域一致,无遗漏或臆造项;docs/{en,zh}/docs/operate/observability.md 的同一句也已同步。中英一致。按已修复记录,不再重复提出。
…tion (oceanbase#2189) Route a validation failure from an injected embedding model through the shared mapping so the public embed() path reports embedding/invalid_output and keeps the original exception type instead of classifying it as a transient outage. Report inference usage limits in their originating stage and reserve usage accounting for usage reporting or persistence. Derive MiniMax diagnostics from the HTTP status while keeping its public exception contract. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…tion (oceanbase#2189) Route a validation failure from an injected embedding model through the shared mapping so the public embed() path reports embedding/invalid_output and keeps the original exception type instead of classifying it as a transient outage. Report inference usage limits in their originating stage and reserve usage accounting for usage reporting or persistence. Derive MiniMax diagnostics from the HTTP status while keeping its public exception contract. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…ceanbase#2189) Describe every stage value the log can carry, mark usage accounting as reserved, and report usage limits in their originating generation or embedding stage. Add the public embedding usage-limit regression test and rebase the branch onto the current master. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
f334987 to
6e13c90
Compare
hireview001
left a comment
There was a problem hiding this comment.
- Resolve merge conflicts with the target branch.
…gnostics (oceanbase#2126) Add a shared failure contract for artifact processing and apply its first slice to Memory / Atomic Memory. Failures now carry a bounded processing stage and reason plus safe diagnostics (input item count, estimated input tokens and estimator identity, effective budget, provider request id and finish reason, original exception type, and failure location). The fields are allowlisted for JSON and console output and travel from a spawned Worker to the supervisor. Provider errors are classified only when the evidence supports it: timeout, rate limit, transient unavailability, provider-confirmed context overflow, schema failures, usage limits, and embedding validation. Budget exhaustion and extraction/reconciliation rules attach business-validation diagnostics. Diagnostics themselves degrade to no fields and never change the processing outcome. Publish the per-Family coverage matrix and the Memory / Atomic Memory operation-level table. Other Families remain Partial or Missing until their own slices land; this change is part of oceanbase#2126, not its completion. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…tion (oceanbase#2189) Route a validation failure from an injected embedding model through the shared mapping so the public embed() path reports embedding/invalid_output and keeps the original exception type instead of classifying it as a transient outage. Report inference usage limits in their originating stage and reserve usage accounting for usage reporting or persistence. Derive MiniMax diagnostics from the HTTP status while keeping its public exception contract. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…ceanbase#2189) Describe every stage value the log can carry, mark usage accounting as reserved, and report usage limits in their originating generation or embedding stage. Add the public embedding usage-limit regression test and rebase the branch onto the current master. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
6e13c90 to
70f6aa7
Compare
Part of #2126.
Rationale for this change
Issue #2126 asks for one failure-classification and safe-diagnostic contract across artifact processing. A PowerContext 1.4.0 POC saw 19 of 40 Memory processing invocations fail with
InvalidInferenceOutputError, and the available logs could not establish whether the failure came from generation, embeddings, output validation, or another dependency.This PR establishes the shared contract and lands the Memory / Atomic Memory slice. It is deliberately one slice: the coverage matrix leaves the other Families
PartialorMissing, and #2126 stays open until their own slices land.What changes are included in this PR?
ProcessingStage,ProcessingFailureReason, andProcessingDiagnostics. Every field is bounded and content-free: counts are non-negative, printable text is length-limited, and no field carries Source/Artifact bodies, prompts, model responses, credentials, or SQL parameters.InferenceUnavailableErrorsemantics.business_validation/invalid_outputorcontext_overflowwith safe accounting fields.ArtifactProcessingWorkerFailurecarries the diagnostics across the child process boundary; child and supervisorartifact_processing.failedlogs emit the same allowlisted fields, and JSON and console formatting use the same contract.stagefield's full value domain (Worker/supervisor boundaries, domain block names such astopic_memory, and the childworkerfallback) is documented next toprocessing_stage.No endpoint, status-code, or persisted-format changes. One internal exception type changes: a validation failure raised by an injected embedding model now surfaces as
InvalidInferenceOutputError(embedding/invalid_output) instead ofInferenceUnavailableError.Are there any user-facing changes?
New allowlisted log fields and documentation. The embedding exception-type change means the FTS/text fallback paths in
topic_memory_search,atomic_memory, andatomic_memory_processing, and the Dream retry branch, no longer treat malformed embedding output as a transient availability failure. This matches the existing malformed-output semantics. Other known limits, recorded in the coverage matrix:allow_continuations=Truepath, a provider finish reason is not always exposed, so some truncation paths are still reported as schema failures.PartialorMissinguntil their own slices.Partial.Rebase and sibling PRs
The branch was rebased onto
0bc42d0b(13 commits after the previous base). The only conflict was the import block inatomic_memory/extraction.py; the diagnostics wrapper is re-applied to the currentextract()body.git merge-tree --write-tree upstream/master HEADexits 0 locally, and a range-diff against the pre-rebase head preserved both commits. #2192 rewrites the sameAtomicMemoryContentblock, so whichever PR lands second must re-resolve that hunk; the tests listed below were run on this rebased tree.How was this change tested?
pytest tests/builtin/test_processing_diagnostics.py tests/builtin/inference tests/test_server_logging.py tests/builtin/runtime/test_artifact_processing.py tests/builtin/artifacts/atomic_memory tests/builtin/artifacts/memory tests/e2e/test_worker_logging.py -q→ 276 passed, 5 skipped.generate()andembed()usage-limit paths, and the MiniMax status mapping.ruff check,ruff format --check,ty checkon the changed files, andgit diff --checkpass.AI usage statement
AI-assisted: Claude Code (Anthropic, Claude Sonnet 4.6) was used to implement the change and write the tests. The diff was reviewed by two independent agent reviewers, and their findings were fixed before submission.