fix(label): keep labels already named when a nested retry fails to parse - #3956
Vikram-Lex wants to merge 1 commit into
Conversation
_label_batch_with_retry retried the ids a truncated reply left out (Graphify-Labs#3671) inside the try that guards the batch's own parse. A retry that still would not parse therefore landed in this batch's except branch, which re-split the whole batch, asking the backend again for ids it had already named, and when that failed the same way the error discarded every name of the batch, the ones already parsed included. Likewise, once a parse failure split a batch, one half failing at the base case discarded the names the other half got. Parse the batch's own reply inside the try, then retry the missing ids (all of them after a parse failure, the plain Graphify-Labs#1278 split) outside it, one half at a time: a half that still won't parse is recorded and the other half still runs. A batch re-raises only when none of it could be named, so label_communities still skips a batch that yields nothing. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
Thanks for the pull request, @Vikram-Lex. A maintainer will review it soon. Want to talk it through while it is in review? Come join us on our Discord server. For longer-form discussion there is also GitHub Discussions. A couple of things that speed up review: make sure the test suite passes on Python 3.10 and 3.13, and that the change keeps extraction deterministic. |
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Formal verification. No changes could be formally verified in this run.
Graphify review — findings
Fixes _label_batch_with_retry so a sub-batch that still won't parse no longer wipes out labels already obtained. The recursive retries now run outside the parse try, so a failing half can't re-split the whole batch and re-request ids that are already named. Each half's parse error is caught and whatever the batch and its other half named is kept; the error is re-raised only when nothing in the batch got labeled, so label_communities still skips fully failed batches.
No blocking issues surfaced. 1 lower-confidence candidate did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 876 functions depend on the 195 functions this change touches.
Health — this change adds coupling hotspots:
- new:
deduplicate_entities()— 79 callers, 24 callees - new:
build_merge()— 76 callers, 14 callees - new:
extract_files_direct()— 17 callers, 20 callees - new:
build()— 52 callers, 6 callees - new:
_call_claude_cli()— 33 callers, 9 callees - new:
main()— 98 callers, 3 callees - new:
extract_corpus_parallel()— 26 callers, 11 callees - new:
dispatch_command()— 2 callers, 126 callees - …and 19 more — each is listed as a finding
Verification — 876 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 534 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
41 of 308 test file(s) selected (13%) via static blast radius.
tests/test_backend_env_isolation.py— impacttests/test_backend_extras.py— impacttests/test_build.py— impacttests/test_build_merge_dedup_scope.py— impacttests/test_build_merge_hyperedges_and_prune.py— impacttests/test_build_merge_shrink_guard.py— impacttests/test_carried_hyperedge_remap.py— impacttests/test_charmap_encoding.py— impacttests/test_chunking.py— impacttests/test_claude_cli_backend.py— impacttests/test_corrupt_graph_json.py— impacttests/test_cross_extension_reexport_self_cycle.py— impacttests/test_dedup.py— impacttests/test_dedup_remaps_hyperedges.py— impacttests/test_dedup_survivor_richness.py— impacttests/test_evidence_binding.py— impacttests/test_file_slice.py— impacttests/test_global_graph.py— impacttests/test_go_qualified_resolution.py— impacttests/test_hyperedge_member_shapes.py— impacttests/test_image_vision.py— impacttests/test_injection_sentinel_coverage.py— impacttests/test_issue_3472_source_file_collision.py— impacttests/test_label_retry.py— impact, changed-testtests/test_labeling.py— impacttests/test_llm_backends.py— impacttests/test_llm_parser.py— impacttests/test_llm_parser_reasoning.py— impacttests/test_no_dedup_flag.py— impacttests/test_non_string_node_ids.py— impacttests/test_ollama.py— impacttests/test_ollama_retry_cap.py— impacttests/test_oversized_document_slicing.py— impacttests/test_partial_cache.py— impacttests/test_pdf_slicing.py— impacttests/test_pdf_token_estimate.py— impacttests/test_provider_registry.py— impacttests/test_prs.py— impacttests/test_prune_sweeps_orphans.py— impacttests/test_semantic_fragment_sanitize.py— impacttests/test_unverified_semantic_shrink.py— impact
Selection is safe under the controlled-regression assumption; always-run tests + a periodic full run are the backstops. Advisory — it never changes the check verdict.
Formal verification
Could not verify: Could not verify \_label\_batch\_with\_retry.
The verifier did not have enough to check \_label\_batch\_with\_retry, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: no capturable inputs from the test suite; property tier: not verifiable: all 9 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly TypeError — names the real obstacle, not a sampling gap)
· 1 grounded finding(s) anchored inline below; 26 more finding(s) on lines outside this diff (see the check run).
| return parsed | ||
|
|
||
|
|
||
| def label_communities( |
There was a problem hiding this comment.
label_communities()
20 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
Solidity free functions (#3906), VB.NET qualified calls (#3909), Astro frontmatter-only AST pass (#3902), surprise bonus/reason alignment (#3934), exclude-hubs stranded-neighbour (#3933), graph-DB push index (#3957), label retry keep-named (#3956), stale-hook status (#3951), virtual-workspace Cargo.toml skip (#3930), and symlinked-instructions install/uninstall handling (#3950/#3953). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
|
Shipped in v0.9.73 (now on PyPI) via an authorship-preserving cherry-pick, so your commit keeps contributor-graph credit. Thanks @Vikram-Lex! Release: https://github.com/Graphify-Labs/graphify/releases/tag/v0.9.73 |
3 participants
Problem
_label_batch_with_retryretries the ids a truncated reply left out (#3671) from inside thetrythat guards the batch's own parse. When one of those retries still won't parse, its error is caught by this batch'sexcept (json.JSONDecodeError, ValueError)branch, which then:label_communitiesskips the batch and loses every name in it, including the ones already parsed.The split after a parse failure (#1278) has a related problem. If one half fails at the base case, the error escapes before the other half's names are returned.
Example: a batch of 4 whose reply names only 2 ids before it is truncated, where one of the 2 missing ids never parses. On
v8the batch makes 5 backend calls, asks again for the 2 ids it already had, and still loses all 4 names. With this change it makes 3 calls and keeps 3 names.Change
try.cluster-onlyskips labeling batch on JSON parse error without retry or chunk split (inconsistent withextract) #1278 split.label_communitiesstill skips a batch that yields nothing.Verification
New tests in
tests/test_label_retry.py:test_a_missing_id_that_never_parses_keeps_the_labels_already_hadchecks the partial reply case. It also asserts the backend is not asked again for ids it already named.test_a_half_that_never_parses_does_not_discard_the_other_halfchecks the split case.test_a_batch_that_never_parses_still_raiseschecks that the error still reaches the caller when nothing could be named.Before the fix, the first two fail with
ValueError: label response is not parseable JSON, which drops every name. The third passes before and after.Commands run:
uv run pytest tests/test_label_retry.py tests/test_labeling.py -q: 41 passed.uv run pytest tests/ -q: 6009 passed, 102 skipped, 24 failed.v8in this environment. They are the Erlang, R, Solidity and VB.NET extractor tests (their grammar packages are not installed by a plainuv sync) andtest_ollama_retry_cap.py(openaiis not installed).test_incremental_mtime_collision.py::test_same_size_rewrite_in_one_tick_is_requeued, is timing-sensitive. It failed only in the full run on a heavily loaded machine and passes when run on its own.uv run ruff check .: all checks passed.uv run pyright graphify/llm.py tests/test_label_retry.py: same 25 errors as unmodifiedv8, none new.Scope and limits
The change is limited to
_label_batch_with_retry. It does not changemax_tokensbudgeting; #3755 addresses that separately and touches the same function, so one of the two may need a small rebase. I did not add a CHANGELOG entry because release batches appear to be written by maintainers.