fix(hgraph): relax Tune source coverage - #2719
Conversation
Signed-off-by: jc543239 <jc543239@antgroup.com> Assisted-by: Codex:GPT-5
|
/label status/waiting-for-review |
Merge Protections🟢 All 3 merge protections satisfied — ready to merge. Show 3 satisfied protections🟢 Require kind label
🟢 Require version label
🟢 Require linked issue for feature/bug PRs
|
There was a problem hiding this comment.
Pull request overview
This PR addresses an HGraph Tune failure mode where total_count_ (and ID metadata) can be ahead of the available FP32/raw codes after a failed Add, and Tune should still proceed by rebuilding only the readable prefix.
Changes:
- Adjust Tune’s source selection to pick the available raw/FP32 source with the largest available prefix.
- Bound Tune training and code rebuilding by the selected source’s available count (rather than the published HGraph count).
- Add a regression test covering Add submission failure leaving HGraph metadata ahead of codes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/algorithm/hgraph/hgraph.cpp | Relaxes Tune’s source selection and limits rebuild/training to the selected source’s available count. |
| src/algorithm/hgraph/hgraph_add_test.cpp | Adds a regression test using a rejecting thread pool to reproduce Add failure followed by a successful Tune. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
/retest |
Prefer complete raw or FP32 Tune sources when available, then fall back without rejecting incomplete sources. Preserve the pre-coverage training and full published-ID rebuild ranges. Signed-off-by: jc543239 <jc543239@antgroup.com> Assisted-by: Codex:GPT-5
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/algorithm/hgraph/hgraph_add_test.cpp:60
ArmableRejectingThreadPool::Enqueuereturns a default-constructedstd::future<void>, which is invalid. If any code path callsEnqueuedirectly and later waits/gets on the returned future, this will throwstd::future_error. Return a ready future instead to satisfy the ThreadPool contract.
if (reject_submissions_.load(std::memory_order_acquire)) {
throw std::bad_alloc();
}
task();
return {};
src/algorithm/hgraph/hgraph.cpp:214
- The fallback tune-source selection (second pass with require_coverage=false) always picks the first available source (raw/high_precise/basic) rather than the one with the largest readable prefix. This contradicts the PR/issue requirement to choose the available raw/FP32 source with the largest readable prefix, and can also pick a shorter source even when a longer FP32 source is available.
auto select_tune_source = [&](bool require_coverage) {
auto select = [&](const FlattenInterfacePtr& codes, bool require_fp32) {
if (tune_source != nullptr or codes == nullptr or
(require_fp32 and codes->GetQuantizerName() != QUANTIZATION_TYPE_VALUE_FP32) or
(require_coverage and not covers_active_ids(codes))) {
src/algorithm/hgraph/hgraph.cpp:274
current_countis loaded fromtotal_count_(the published HGraph count), but when Tune falls back to an incomplete source,decode_tune_source(i, ...)will still be called fori >= tune_source->TotalCount()during both training (for i < train_count) and rebuilding (for i < current_count). In the Add-failure scenario this PR targets,GetCodesByIdcan return nullptr and Tune will throw an INTERNAL_ERROR instead of rebuilding the readable prefix. Tune should bound both the training sample loop and the rebuild loop bymin(published_count, tune_source->TotalCount()).
for (int64_t i = 0; i < current_count; ++i) {
decode_tune_source(i, insert_buffer.data());
new_code->InsertVector(static_cast<const void*>(insert_buffer.data()), i);
}
Keep raw-first source selection explicit, falling back to precise or base FP32 codes only when the earlier source is unavailable or empty. Assisted-by: Codex:GPT-5 Signed-off-by: jc543239 <jc543239@antgroup.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
src/algorithm/hgraph/hgraph.cpp:270
- Rebuild currently iterates [0, current_count) even when the chosen tune_source is a fallback that may only have a shorter readable prefix. For FlattenDataCell + MemoryIO, GetCodesById reads from the resized buffer without validating writes, so this can decode uninitialized memory and silently rebuild corrupted vectors.
When using an incomplete source, cap the rebuild loop to the source’s readable count (e.g., TotalCount()).
Vector<float> insert_buffer(dim_, 0, allocator_);
for (int64_t i = 0; i < current_count; ++i) {
decode_tune_source(i, insert_buffer.data());
new_code->InsertVector(static_cast<const void*>(insert_buffer.data()), i);
}
| if (is_tune_base_code or is_tune_precise_code or is_tune_raw_code) { | ||
| if (covers_active_ids(raw_vector_)) { | ||
| tune_source = raw_vector_; | ||
| } else if (covers_active_ids(high_precise_codes_) and | ||
| high_precise_codes_->GetQuantizerName() == QUANTIZATION_TYPE_VALUE_FP32) { | ||
| tune_source = raw_vector_; | ||
| if ((tune_source == nullptr or tune_source->TotalCount() == 0) and | ||
| high_precise_codes_ != nullptr and | ||
| high_precise_codes_->GetQuantizerName() == QUANTIZATION_TYPE_VALUE_FP32) { |
Change Type
Linked Issue
What Changed
Test Evidence
Test details:
Results: 8 regression assertions, 7 deduplicated-Tune assertions, and 8040 Tune-source assertions passed.
Compatibility Impact
Performance and Concurrency Impact
Documentation Impact
README.mdDEVELOPMENT.mdCONTRIBUTING.mdRisk and Rollback
Checklist