perf(autotune): prune searches for memory-infeasible candidates

[jac0626]

Change Type

• Bug fix
• New feature
• Improvement/Refactor
• Documentation
• CI/Build/Infra

Linked Issue

• Fixes: [improve](eval_performance): prune AutoTune searches for memory-infeasible candidates #2688

What Changed

• Skip every search trial in a build group when the built index already exceeds an
  index_memory_mb constraint.
• Apply the same one-time memory check when tuning search parameters on an existing index.
• Report skipped trials as pruned, keep their measured shared metrics and constraint evidence,
  and prevent them from becoming recommendations.
• Return no_feasible_candidate with an explanatory best_effort when every candidate is
  pruned, while preferring fully evaluated trials whenever one is available.
• Document the behavior and report contract in the English and Chinese AutoTune documentation.

Test Evidence

• make fmt
• make lint
• make test
• make cov, run tests, and collect coverage
• Other (describe below)

Test details:

clang-format-15 --dry-run --Werror \
  tools/autotune/autotune.cpp \
  tools/autotune/autotune_evaluation.cpp \
  tools/autotune/autotune_test.cpp
git diff --check

cmake --build build-release --target autotune_test --parallel 96
./build-release/tools/autotune/autotune_test
All tests passed (308 assertions in 27 test cases)

cmake --build build-release --target autotune --parallel 96
./build-release/tools/autotune/autotune .autotune_memory_pruning_e2e.json

E2E dataset: /tmp/sift-50k-100.hdf5
Index: HGraph, two ef_search candidates
Constraint: index_memory_mb <= 0
Observed: one successful build, two pruned trials, no search/latency metrics,
          no_feasible_candidate, report persisted, generated artifact removed

Compatibility Impact

• API/ABI compatibility: none
• Behavior changes: trials known to violate the memory constraint now use status: "pruned" and
  skip search; an all-pruned request returns no_feasible_candidate instead of
  all_trials_failed.

Performance and Concurrency Impact

• Performance impact: improved for memory-infeasible candidates by avoiding all associated query
  evaluation work
• Concurrency/thread-safety impact: none; existing-index memory is read once before its trials

Documentation Impact

• No docs update needed
• Updated docs:
  • README.md
  • DEVELOPMENT.md
  • CONTRIBUTING.md
  • Other: English and Chinese AutoTune overview and V1 API contract

Risk and Rollback

• Risk level: low
• Rollback plan: revert this PR to restore unconditional search evaluation

Checklist

• I have linked the relevant issue (required for kind/bug and kind/feature; see "Linked Issue" above)
• I have added/updated tests for new behavior or bug fixes
• I have considered API compatibility impact
• I have updated docs if behavior/workflow changed
• My commit messages follow project conventions (Conventional Commits, optional [skip ci] prefix)

[vsag-bot]

/label status/waiting-for-review
/waiting-on reviewer
/request-review @jiaweizone
/request-review @wxyucs
/request-review @inabao

[mergify]

Merge Protections

🟢 All 2 merge protections satisfied — ready to merge.

Show 2 satisfied protections

🟢 Require kind label

• label~=^kind/

🟢 Require version label

• label~=^version/

[LHT129]

[suggestion] In autotune_evaluation.cpp:97, current_memory_metrics() catches std::exception broadly, which will also swallow std::bad_alloc and other unexpected errors that should propagate rather than being silently ignored. The intent is to handle cases where GetMemoryUsage() throws because the metric is unavailable, but the catch-all could mask real problems.

Consider narrowing the catch to the specific exception type(s) that GetMemoryUsage() is documented to throw when the metric is unavailable, or at minimum re-throw std::bad_alloc:

} catch (const std::bad_alloc&) {
    throw;
} catch (const std::exception&) {
    // An unavailable metric cannot prove that the candidate violates the constraint.
}

[LHT129]

Thank you for this well-structured PR. The memory-based pruning optimization is cleanly implemented with thorough test coverage.

Summary of review:

• autotune_evaluation.cpp: The current_memory_metrics() and exceeds_memory_constraint() helpers are well-encapsulated. The pruning logic in both EvaluateCandidates overloads is correctly placed — checking once per build group for build-and-search mode, and once upfront for search-only mode. The GetMemoryUsage() exception is properly caught and treated as "cannot prove violation", which is the safe default.

• autotune.cpp: Extracting violation_rank into a reusable lambda is a clean refactor. The best_pruned selection logic correctly prefers fully evaluated trials over pruned ones (via the !has_successful_trial guard), and only falls back to pruned trials when no successful trial exists. The violation_count > 0 guard in the pruned branch is correct — a pruned trial with zero violations would be an internal inconsistency.

• autotune_test.cpp: The new test cases cover the key scenarios well: all-pruned returns no_feasible_candidate, evaluated trials are preferred over pruned, end-to-end pruning with index_memory_mb <= 0, and search-only pruning on an existing index. The Pyramid test correctly verifies that GetMemoryUsage() == 0 does not trigger pruning.

• Documentation: Both English and Chinese docs accurately describe the new pruned status, the best_effort fallback behavior, and the zero-memory guard.

No blocking issues found.

[Copilot]

Pull request overview

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

This PR improves AutoTune performance by pruning search evaluations for candidates that already violate the index_memory_mb constraint, and updates selection/reporting semantics to surface pruned trials as explanatory best_effort.

Changes:

• Add memory-based pruning to skip search evaluation for memory-infeasible build groups and existing-index tuning.
• Update result selection to prefer evaluated trials, but fall back to the closest pruned trial when nothing was evaluated successfully.
• Expand tests and docs (EN/ZH) to cover/define pruned trial semantics and the no_feasible_candidate outcome when everything is pruned.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tools/autotune/autotune_evaluation.cpp	Implements memory metric sampling and pruning logic for build-and-search and search-only evaluation.
tools/autotune/autotune.cpp	Updates selection logic to rank pruned trials for best-effort explanations when no successes exist.
tools/autotune/autotune_test.cpp	Adds unit + e2e-style tests for pruning behavior, best-effort selection, and typed TuneSearch.
docs/docs/en/src/resources/autotune_api_v1.md	Documents pruned trial status, updated report contract, and pruning conditions.
docs/docs/en/src/resources/autotune.md	Notes boundary change: skipping search for memory-infeasible candidates.
docs/docs/zh/src/resources/autotune_api_v1.md	Chinese documentation updates mirroring EN contract/semantics for pruning and best-effort.
docs/docs/zh/src/resources/autotune.md	Chinese boundary update noting memory-infeasible candidates skip search.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[LHT129]

[suggestion] The search-only EvaluateCandidates drops shared_metrics on successful search, unlike the build-and-search path which merges them.

In the build-and-search path, shared_metrics entries are explicitly merged into the search metrics map via metrics.emplace(name, value). In the search-only path, the overwrite via trial["metrics"] = metrics_json(metrics) silently drops any index_memory_mb that was read from GetMemoryUsage() and stored in shared_metrics.

While search_metrics() also reads index_memory_mb from the raw eval result, the two sources can differ — GetMemoryUsage() is the authoritative index-level value, whereas the eval framework may report a different number. This inconsistency between the two evaluation paths could lead to subtle bugs if the values diverge.

Consider merging shared_metrics into the measured metrics (as done in the build-and-search path) instead of replacing.

[LHT129]

Thanks for this PR. The memory-aware pruning optimization is well-designed and the documentation updates are thorough.

I reviewed the changes across all seven files and the implementation is solid overall. The test coverage is comprehensive, covering both build-and-search and search-only paths, the all-pruned scenario, and the mixed pruned+evaluated scenario.

Two issues have already been flagged by other reviewers that I agree with:

1. search-only path drops shared_metrics on successful search (line 367 in autotune_evaluation.cpp): In the EvaluateCandidates(SearchTuningRequest) path, trial["metrics"] = metrics_json(metrics) overwrites the initial shared_metrics (which contains index_memory_mb from GetMemoryUsage()). The build-and-search path correctly merges them via metrics.emplace(name, value). The search-only path should do the same merge to avoid losing the authoritative index_memory_mb value.

2. Pyramid test assertion is brittle (line 1085 in autotune_test.cpp): REQUIRE(trial["status"] == "success") will break if Pyramid ever reports a positive GetMemoryUsage(). Consider relaxing to REQUIRE(trial["status"] != "failed").

Otherwise the PR looks good — the SelectResult refactoring to extract violation_rank is clean, the pruned status semantics are well-documented in both English and Chinese docs, and the E2E test validates the full lifecycle including artifact cleanup.

[LHT129]

Thanks for this PR. The memory-based pruning optimization is well-designed and the implementation is clean.

I reviewed the full diff across all 7 files. The core logic in SelectResult, EvaluateCandidates (both overloads), current_memory_metrics, and exceeds_memory_constraint is correct and handles edge cases properly:

• Pruned trials are correctly excluded from recommendation consideration
• best_effort correctly prefers evaluated trials over pruned ones
• The violation_rank lambda cleanly unifies the scoring logic
• Memory value of 0 or unavailable correctly does not trigger pruning
• The failed status correctly handles the case where all trials are pruned but successful trials exist without objective metrics

The test coverage is thorough, covering unit tests for SelectResult with pruned trials, E2E tests for build-and-search pruning, and search-only pruning on existing indices.

The documentation updates accurately describe the new behavior and contract changes.

Two existing review comments from Copilot and @LHT129 cover the remaining concerns:

1. The search-only path in EvaluateCandidates drops shared_metrics on successful search (unlike the build-and-search path which merges them)
2. The Pyramid test assertion at line 1085 is brittle against future behavior changes

Please address those two items. Otherwise LGTM.

[LHT129]

I have completed a thorough review of this PR. The overall design is sound and the implementation is clean.

Summary of findings:

The core logic — pruning search trials when the built/existing index already exceeds an index_memory_mb constraint — is correctly implemented across both build-and-search and search-only paths in autotune_evaluation.cpp. The SelectResult changes in autotune.cpp correctly handle pruned trials: they are tracked separately from successful trials, and only used as best_effort when no successful trial exists. The test coverage is comprehensive, covering pruned-only scenarios, mixed pruned+evaluated scenarios, end-to-end build-and-search pruning, search-only pruning, and the Pyramid GetMemoryUsage() == 0 non-pruning case.

Existing comments from other reviewers:

Two reviewers (Copilot and LHT129) have already identified the main issue: in the search-only EvaluateCandidates, trial["metrics"] = metrics_json(metrics) overwrites the shared_metrics (which contains index_memory_mb from GetMemoryUsage()) instead of merging them as the build-and-search path does via metrics.emplace(). While search_metrics() may also populate index_memory_mb from the eval framework, the two sources can differ, and if the eval framework does not report it, the constraint evaluation will see a missing metric rather than the authoritative value. I agree this should be addressed before merging.

The Copilot comment about the Pyramid test assertion (trial["status"] == "success") is a reasonable observation, though the test explicitly verifies GetMemoryUsage() == 0 immediately before, so the assertion is safe under current behavior.

No additional blocking issues found. The documentation updates are accurate and consistent with the code changes. The violation_rank lambda refactoring in SelectResult is a clean improvement that deduplicates the violation scoring logic.

[LHT129]

Overall this is a well-designed change. The pruning logic is clean, the fallback behavior in SelectResult is carefully handled, and the test coverage is thorough (including the all-pruned scenario, the prefer-evaluated-over-pruned scenario, and the search-only existing-index path).

A few observations beyond the existing inline comments:

Build-and-search path (autotune_evaluation.cpp:292-293): The shared_metrics merge via metrics.emplace(name, value) correctly preserves build-time index_memory_mb in successful trials. This is the right pattern.

Search-only path (autotune_evaluation.cpp:351): As noted by Copilot and LHT129, trial["metrics"] = metrics_json(metrics) overwrites the initial shared_metrics. While search_metrics() also reads index_memory_mb from the raw eval result, the two sources (GetMemoryUsage() vs the eval framework) can diverge. Consider applying the same emplace merge pattern used in the build-and-search path for consistency.

SelectResult status initialization (autotune.cpp:807): The initial status is set to no_feasible_candidate when best < 0, then overwritten to failed in the final else branch. This is correct but the double-write pattern is slightly subtle — consider restructuring to set status once at the end.

Test assertion (autotune_test.cpp:1085): The REQUIRE(trial["status"] == "success") assertion in the Pyramid existing-index test may become brittle if Pyramid ever reports a positive GetMemoryUsage(). Consider relaxing to != "failed" as Copilot suggested.

None of these are blocking — the core logic is sound and the tests pass.

[LHT129]

[suggestion] In SelectResult, the violation_rank lambda computes std::abs(actual - expected) / std::max(expected, 1e-12) for each violation. For pruned trials where the only metric is index_memory_mb, the denominator std::max(expected, 1e-12) will be expected (the constraint value). When expected is very small (e.g. 0.0 as used in the test), this produces inf due to division by near-zero after std::max with 1e-12. While this does not break correctness (the violation count tiebreaker dominates), it makes the score non-finite and the ordering among equally-counted pruned trials non-deterministic. Consider using std::max(std::abs(expected), 1e-12) or normalizing differently when the constraint target is near zero.

[LHT129]

I have completed a thorough review of this PR. The implementation is well-structured with clear logic for pruning memory-infeasible candidates, proper handling of both build-and-search and search-only paths, and comprehensive test coverage.

The existing inline comments from Copilot and LHT129 already cover the substantive issues I identified:

1. Search-only path drops shared_metrics (autotune_evaluation.cpp:367): In the search-only EvaluateCandidates, trial["metrics"] = metrics_json(metrics) overwrites the initial shared_metrics (containing index_memory_mb from GetMemoryUsage()). While search_metrics() also reads index_memory_mb from the eval result, the two sources can theoretically diverge. The build-and-search path correctly uses metrics.emplace() to merge. Consider applying the same merge approach in the search-only path for consistency.

2. std::max(expected, 1e-12) produces inf for near-zero constraints (autotune.cpp:755): When expected is very small (e.g. 0.0), the division produces inf, making score-based tiebreaking among equally-counted pruned trials non-deterministic. Consider using std::max(std::abs(expected), 1e-12).

3. Brittle test assertion (autotune_test.cpp:1085): The REQUIRE(trial["status"] == "success") assertion in the Pyramid existing-index test may break if Pyramid ever reports a positive GetMemoryUsage().

Otherwise, the PR looks good — the core logic is correct, the documentation updates are thorough, and the test coverage is solid.

[LHT129]

Reviewed the full diff across all 7 files. The implementation is solid: the pruning logic in both build-and-search and search-only paths is correct, the SelectResult changes properly prefer evaluated trials over pruned ones, and the test coverage is thorough (unit tests for SelectResult edge cases, integration tests for both evaluation paths, and a Pyramid-specific regression assertion).

The existing inline comments from Copilot and @LHT129 already cover the actionable items:

• search-only metrics overwrite: trial["metrics"] = metrics_json(metrics) drops shared_metrics in the search-only success path, unlike the build-and-search path which merges them. The search_metrics() helper does re-read index_memory_mb from the eval result, so this is not a correctness bug today, but the inconsistency is worth fixing for robustness.
• Pyramid test assertion: REQUIRE(trial["status"] == "success") is brittle against future index types that may report positive GetMemoryUsage().
• violation_rank near-zero denominator: std::max(expected, 1e-12) produces inf when expected is 0.0, making the score non-finite for that common constraint value.

No additional issues found. The documentation updates (EN + ZH) accurately describe the new pruned status and best_effort fallback semantics.

[LHT129]

Code review for PR #2689: perf(autotune): prune searches for memory-infeasible candidates

[LHT129]

test inline comment

[LHT129]

Thanks for this PR. The memory-based pruning optimization is well-designed and the implementation is clean. Here is a summary of my review:

What this PR does well:

• Clean separation of pruning logic into current_memory_metrics() and exceeds_memory_constraint() helpers
• Conservative pruning: only triggers when GetMemoryUsage() > 0 and strictly exceeds the constraint (> not >=), and gracefully handles exceptions from GetMemoryUsage()
• Correct SelectResult fallback chain: feasible → evaluated best-effort → pruned best-effort → failed
• Comprehensive test coverage including unit tests for SelectResult, integration tests for both build-and-search and search-only paths, and the Pyramid zero-memory edge case
• Well-documented API contract changes in both English and Chinese docs

Issues already flagged by other reviewers (I concur):

1. Search-only EvaluateCandidates overwrites shared_metrics on successful search, dropping index_memory_mb from GetMemoryUsage() — should merge like the build-and-search path does
2. violation_rank division by std::max(expected, 1e-12) produces extremely large scores when the constraint target is near zero — consider std::max(std::abs(expected), 1e-12)
3. The REQUIRE(trial["status"] == "success") assertion in the Pyramid test is brittle against future GetMemoryUsage() changes

No new blocking issues found. The PR is in good shape pending resolution of the above items.

[LHT129]

[suggestion] The best_pruned fallback condition !has_successful_trial is too restrictive and can incorrectly produce "failed" when pruned trials are available to explain infeasibility.

Consider the scenario where successful trials exist but none carry the objective metric (has_successful_trial == true, has_successful_objective == false). In that case best < 0, best_effort < 0, and best_pruned >= 0, yet the fallback is skipped because has_successful_trial is true. The result falls through to "failed" with objective_metric_unavailable, even though pruned trials could provide a meaningful best_effort.

The documented contract says no_feasible_candidate should be returned when pruned trials can explain infeasibility. The condition should guard against having a better evaluated best_effort, not against the mere existence of any successful trial:

} else if (best_pruned >= 0 && best_effort < 0) {

This ensures pruned trials are used as best_effort whenever no evaluated trial is a better candidate.

[LHT129]

I have reviewed this PR thoroughly. The existing inline reviews from Copilot and LHT129 have already covered the key issues:

1. shared_metrics overwrite in search-only path (autotune_evaluation.cpp:367): The trial["metrics"] = metrics_json(metrics) assignment drops the index_memory_mb value obtained from GetMemoryUsage(). The build-and-search path correctly uses emplace to merge. Consider merging shared_metrics into the measured metrics instead of replacing, for consistency between the two evaluation paths.

2. brittle test assertion (autotune_test.cpp:1085): The REQUIRE(trial["status"] == "success") assertion in the Pyramid test could break if Pyramid ever returns a positive GetMemoryUsage(). Consider relaxing to != "failed".

3. violation_rank division by near-zero (autotune.cpp:755): When expected is very small (e.g., 0.0), std::max(expected, 1e-12) produces a tiny denominator, leading to near-infinite scores. Consider using std::max(std::abs(expected), 1e-12).

4. best_pruned fallback condition (autotune.cpp:818): The !has_successful_trial guard may be too restrictive when successful trials exist but lack the objective metric. Consider using best_effort < 0 instead.

Beyond these already-identified issues, the PR is well-structured with comprehensive test coverage and clear documentation. The core logic for pruning memory-infeasible candidates is sound.

[LHT129]

Thank you for this PR. The memory-aware pruning optimization is a well-designed improvement that avoids wasted search work for infeasible candidates. The test coverage is thorough and the documentation updates are clear.

I have reviewed the full diff and the existing inline comments from Copilot and @LHT129 already cover the key issues I identified. To summarize the outstanding concerns that should be addressed before merging:

1. [critical] autotune_evaluation.cpp (search-only path): shared_metrics is dropped on successful search — In the EvaluateCandidates(SearchTuningRequest) path, trial["metrics"] = metrics_json(metrics) overwrites the shared_metrics (which contains index_memory_mb from GetMemoryUsage()). This differs from the build-and-search path which explicitly merges via metrics.emplace(name, value). The two sources (GetMemoryUsage() vs eval framework) can diverge, and dropping the authoritative value can cause incorrect constraint evaluation. (Already noted by Copilot and @LHT129)

2. [critical] autotune.cpp:818: best_pruned fallback condition is too restrictive — The condition best_pruned >= 0 && !has_successful_trial should be best_pruned >= 0 && best_effort < 0. When successful trials exist but none carry the objective metric (has_successful_trial == true, has_successful_objective == false), the result incorrectly falls through to "failed" even though pruned trials could provide a meaningful best_effort. (Already noted by @LHT129)

3. [suggestion] autotune.cpp:755: violation_rank score normalization — std::max(expected, 1e-12) produces inf when expected is very small (e.g. 0.0). Consider std::max(std::abs(expected), 1e-12) for more robust normalization. (Already noted by @LHT129)

4. [note] autotune_test.cpp:1085: brittle assertion on Pyramid trial status — The assertion REQUIRE(trial["status"] == "success") relies on Pyramid returning GetMemoryUsage() == 0. If Pyramid ever implements memory reporting, this test would break. Consider relaxing to REQUIRE(trial["status"] != "failed"). (Already noted by Copilot)

Otherwise the implementation is solid: the pruning logic is correctly placed before search execution in both paths, the SelectResult changes properly separate pruned-trial ranking from successful-trial ranking, and the E2E tests validate the full pipeline including artifact cleanup.

[LHT129]

I have reviewed this PR and the existing inline comments from Copilot and LHT129 already cover the key issues I identified:

1. Search-only path metrics overwrite (autotune_evaluation.cpp:362): In EvaluateCandidates for SearchTuningRequest, trial["metrics"] = metrics_json(metrics) overwrites shared_metrics (containing index_memory_mb from GetMemoryUsage()), unlike the build-and-search path which merges them via metrics.emplace(). While search_metrics() also reads index_memory_mb from the raw eval result, the two sources can diverge. This is the most impactful issue.

2. violation_rank division stability (autotune.cpp:755): std::max(expected, 1e-12) produces inf when expected is 0.0, making score-based tiebreaking non-deterministic for near-zero constraints. The violation-count tiebreaker still dominates, so correctness is preserved, but the ordering is not stable.

3. best_pruned fallback guard (autotune.cpp:818): The !has_successful_trial condition is arguably correct for the stated goal of "every candidate is pruned" → no_feasible_candidate, but the alternative best_effort < 0 would be more robust if the contract evolves.

4. Brittle test assertion (autotune_test.cpp:1085): REQUIRE(trial["status"] == "success") hardcodes an assumption that may break if Pyramid ever reports positive GetMemoryUsage().

The PR is well-structured with thorough test coverage (113 new test lines), clear documentation updates in both English and Chinese, and a clean implementation. No additional issues beyond those already flagged.

[LHT129]

Thank you for this PR. I have reviewed the changes thoroughly. The existing review comments from Copilot and LHT129 have already covered the key findings:

1. Search-only path drops shared_metrics on successful search (autotune_evaluation.cpp:367): The trial["metrics"] = metrics_json(metrics) overwrites the initial shared_metrics (which contains index_memory_mb from GetMemoryUsage()), unlike the build-and-search path which merges them. While search_metrics() also reads index_memory_mb from the eval framework, the two sources can differ.

2. violation_rank score becomes inf when constraint target is near zero (autotune.cpp:755): std::max(expected, 1e-12) with expected=0.0 produces division by 1e-12, yielding inf. While the violation count tiebreaker dominates, it makes the score non-finite and ordering non-deterministic among equally-counted pruned trials.

3. best_pruned fallback condition too restrictive (autotune.cpp:818): !has_successful_trial prevents pruned trials from serving as best_effort when successful trials exist but lack the objective metric. Using best_effort < 0 would be more correct per the documented contract.

4. Pyramid test assertion (autotune_test.cpp:1085): The REQUIRE(trial["status"] == "success") is brittle against future changes where Pyramid might report non-zero GetMemoryUsage().

The overall design is sound: pruning searches for memory-infeasible candidates, reporting them with status: "pruned", and using them for best_effort when no evaluated trial is feasible. The test coverage is comprehensive with both unit tests for SelectResult and integration tests for build-and-search and search-only paths.

[LHT129]

[suggestion] The best_pruned fallback condition !has_successful_trial is too restrictive, as noted in a prior review. To add to that analysis: there is no test covering the specific scenario where successful trials exist but lack the objective metric (has_successful_trial == true, has_successful_objective == false) while pruned trials are also present. The existing test "AutoTune prefers evaluated best effort over a closer pruned trial" covers the case where the evaluated trial HAS the objective metric. A test for the missing-objective + pruned-available scenario would catch this regression.

Consider adding a test case and changing the condition to best_effort < 0 as previously suggested.

[LHT129]

[suggestion] The search-only EvaluateCandidates path overwrites trial["metrics"] with metrics_json(metrics) from search_metrics(), dropping shared_metrics (including index_memory_mb from GetMemoryUsage()). The build-and-search path correctly merges via metrics.emplace(name, value).

One reason this bug manifests: search_config() sets enable_memory = false, so search_metrics() never populates index_memory_mb from the eval raw result. The only source of index_memory_mb in the search-only path is shared_metrics from GetMemoryUsage(), which gets dropped on overwrite.

When prune_searches is false (memory constraint is satisfied), the trial will incorrectly show a missing-metric violation for index_memory_mb during constraint_evaluation(), even though the constraint is actually satisfied. Consider merging shared_metrics into the measured metrics (as done in the build-and-search path) instead of replacing.

[LHT129]

[suggestion] The !has_successful_trial guard on the pruned fallback is too narrow. Consider the scenario where successful trials exist but all lack the objective metric (has_successful_trial == true, has_successful_objective == false). In that case best_effort remains -1, so the code falls through to failed with objective_metric_unavailable, losing the explanatory value of the pruned trial.

The guard should protect against having a better evaluated best_effort, not against the mere existence of any successful trial. Changing the condition to best_effort < 0 would correctly fall back to pruned trials only when no evaluated trial can serve as best_effort.

} else if (best_pruned >= 0 && best_effort < 0) {

A test covering the scenario where successful trials exist but lack the objective metric while pruned trials are also present would catch this regression.

[LHT129]

[critical] In the search-only EvaluateCandidates path, trial["metrics"] is overwritten with metrics_json(metrics) from search_metrics(), dropping shared_metrics (including index_memory_mb from GetMemoryUsage()).

The root cause: search_config() sets enable_memory = false, so search_metrics() never populates index_memory_mb from the eval raw result. The only source of index_memory_mb in the search-only path is shared_metrics from GetMemoryUsage(), which gets dropped on overwrite.

When prune_searches is false (memory constraint is satisfied), the trial will incorrectly show a missing-metric violation for index_memory_mb during constraint_evaluation(), even though the constraint is actually satisfied. This can cause SelectResult to incorrectly mark the trial as infeasible or distort best_effort selection.

The build-and-search path correctly merges via metrics.emplace(name, value). Apply the same merge approach here:

const auto metrics = search_metrics(raw, elapsed(measured_start));
for (const auto& [name, value] : shared_metrics) {
    metrics.emplace(name, value);
}
trial["metrics"] = metrics_json(metrics);

[LHT129]

[suggestion] The violation_rank lambda uses std::max(violation["expected"].get<double>(), 1e-12) as the denominator for normalization. When the constraint target is very small (e.g. index_memory_mb <= 0.0 as used in the test), std::max(0.0, 1e-12) yields 1e-12, making the normalized score actual / 1e-12 which can overflow to inf for any non-trivial memory value.

While this does not break correctness (the violation count tiebreaker dominates), it makes the score non-finite and the ordering among equally-counted pruned trials non-deterministic when expected is near zero.

Consider using std::max(std::abs(expected), 1e-12) to handle negative or zero constraint targets, or use a different normalization strategy for near-zero targets.

[LHT129]

[note] The assertion REQUIRE(trial["status"] == "success") added to the existing Pyramid search-only test is brittle against future behavior changes. Under the new contract, if a positive GetMemoryUsage() becomes available for Pyramid and exceeds the index_memory_mb constraint, these trials would become pruned instead of success, causing the test to fail for a reason unrelated to the feature under test.

Consider relaxing the assertion to accept both success and pruned as valid statuses, or assert != "failed" instead. If the intent is to verify that this specific test scenario does not trigger pruning, add a comment explaining why.

[LHT129]

I have completed a thorough review of this PR. The existing review comments from Copilot and LHT129 have already covered the key issues:

1. [critical] Search-only path drops shared_metrics on successful search (flagged by Copilot and LHT129 at autotune_evaluation.cpp:367): In the search-only EvaluateCandidates, trial["metrics"] is overwritten with metrics_json(metrics) from search_metrics(), which drops index_memory_mb from GetMemoryUsage(). Since search_config() sets enable_memory = false, search_metrics() never populates index_memory_mb from the eval raw result. The build-and-search path correctly merges via metrics.emplace(name, value). The search-only path needs the same merge.

2. [suggestion] violation_rank denominator can produce inf for near-zero constraint targets (flagged by LHT129 at autotune.cpp:755): When expected is very small (e.g. 0.0), std::max(expected, 1e-12) yields 1e-12, making the normalized score actual / 1e-12 which can overflow to inf. Consider std::max(std::abs(expected), 1e-12).

3. [note] Brittle REQUIRE(trial["status"] == "success") in Pyramid test (flagged by Copilot and LHT129 at autotune_test.cpp:1085): If Pyramid ever gains a positive GetMemoryUsage(), these trials would become pruned and the test would fail for reasons unrelated to the feature under test.

The core logic of the pruning feature (memory check, trial status propagation, SelectResult fallback chain) is sound and well-tested. The test coverage is comprehensive with unit tests for SelectResult edge cases and integration tests for both build-and-search and search-only paths.

No additional issues found beyond what has already been flagged.

[LHT129]

Thanks for this PR. The implementation is well-structured and the test coverage is thorough. I have reviewed the changes and found that the existing inline comments from other reviewers already cover all the substantive issues I identified:

1. search-only path shared_metrics overwrite — The trial["metrics"] overwrite drops index_memory_mb from GetMemoryUsage(), which can cause incorrect constraint evaluation. The build-and-search path correctly merges; the search-only path should do the same.

2. violation_rank denominator — std::max(expected, 1e-12) produces inf when expected is near zero, making ordering among equally-counted pruned trials non-deterministic. Consider std::max(std::abs(expected), 1e-12).

3. best_pruned fallback guard — The !has_successful_trial condition is too narrow; it should be best_effort < 0 to correctly fall back to pruned trials when successful trials exist but lack the objective metric.

4. Pyramid test assertion — REQUIRE(trial["status"] == "success") is brittle against future GetMemoryUsage() changes for Pyramid. Consider relaxing to != "failed".

The documentation updates (both English and Chinese) are accurate and complete. The new test cases cover the key scenarios well. No additional issues from my side.

[LHT129]

I have reviewed this PR and the existing inline comments from Copilot and LHT129 already cover the key findings:

1. [critical] Metrics overwrite in search-only EvaluateCandidates (autotune_evaluation.cpp:362): trial["metrics"] = metrics_json(metrics) drops shared_metrics (including index_memory_mb from GetMemoryUsage()). Since search_config() sets enable_memory = false, search_metrics() never populates index_memory_mb from the eval result. This means a trial that actually satisfies the memory constraint will incorrectly show a missing-metric violation during constraint_evaluation(). The build-and-search path correctly merges via metrics.emplace(name, value) at line 292 — the same merge should be applied here.

2. [suggestion] violation_rank denominator (autotune.cpp:755): std::max(expected, 1e-12) can produce inf when expected is near zero (e.g., index_memory_mb <= 0.0). Consider std::max(std::abs(expected), 1e-12).

3. [note] Test assertion brittleness (autotune_test.cpp:1085): The REQUIRE(trial["status"] == "success") assertion in the Pyramid test may break if Pyramid ever reports a positive GetMemoryUsage(). Consider relaxing to != "failed".

The overall design is sound — the pruning logic in both evaluation paths is clean, the SelectResult fallback to pruned trials is correctly gated by !has_successful_trial, and the test coverage (308 assertions, 27 test cases) is thorough. The documentation updates across both English and Chinese docs are consistent with the implementation.

[LHT129]

I have reviewed this PR thoroughly. The existing inline comments from Copilot and LHT129 already cover the key issues:

1. [critical] Search-only metrics merge bug (autotune_evaluation.cpp:367): In the search-only EvaluateCandidates, when prune_searches is false, trial["metrics"] is overwritten by metrics_json(metrics) from search_metrics(), dropping shared_metrics (including index_memory_mb from GetMemoryUsage()). Since search_config() sets enable_memory = false, the eval framework does not report index_memory(B), so search_metrics() never populates index_memory_mb. This causes a false missing-metric violation during constraint_evaluation(). The build-and-search path correctly merges via metrics.emplace(name, value) — the same approach should be applied here.

2. [suggestion] violation_rank denominator (autotune.cpp:755): std::max(violation["expected"].get<double>(), 1e-12) produces inf when the constraint target is near zero (e.g., index_memory_mb <= 0.0). While the violation count tiebreaker dominates, the non-finite score makes ordering non-deterministic among equally-counted pruned trials. Consider std::max(std::abs(expected), 1e-12).

3. [note] Brittle test assertion (autotune_test.cpp:1085): The REQUIRE(trial["status"] == "success") in the Pyramid search-only test is brittle — if Pyramid ever reports a positive GetMemoryUsage(), these trials would become pruned. Consider asserting != "failed" or accepting both statuses.

Beyond these, the implementation is well-structured: the pruning logic is cleanly separated, the SelectResult changes correctly prefer evaluated trials over pruned ones for best_effort, the test coverage is comprehensive (unit tests for selection logic + integration tests for both build-and-search and search-only paths), and the documentation updates accurately reflect the new behavior.

[LHT129]

I have completed a thorough review of this PR. The existing review comments from Copilot and LHT129 have already identified the key issues:

1. [critical] Search-only path drops shared_metrics (autotune_evaluation.cpp:367): In the search-only EvaluateCandidates, trial["metrics"] is overwritten with metrics_json(metrics) from search_metrics(), which drops index_memory_mb from GetMemoryUsage(). Since search_config() sets enable_memory = false, search_metrics() never populates index_memory_mb. This needs the same merge approach used in the build-and-search path.

2. [suggestion] violation_rank denominator (autotune.cpp:755): When the constraint target is near zero (e.g., index_memory_mb <= 0.0), std::max(expected, 1e-12) produces a very small denominator, causing the normalized score to overflow to inf. While the violation count tiebreaker dominates, this makes ordering among equally-counted pruned trials non-deterministic.

3. [note] Brittle test assertion (autotune_test.cpp:1085): The REQUIRE(trial["status"] == "success") assertion in the Pyramid search-only test is brittle against future behavior changes where Pyramid might start returning a positive GetMemoryUsage().

Beyond these already-flagged items, the PR is well-structured:

• The core logic in SelectResult correctly prefers fully-evaluated successful trials over pruned ones, and falls back to pruned trials for best_effort only when no successful trials exist
• The build-and-search path correctly merges shared_metrics into search metrics
• Test coverage is comprehensive with both unit tests for SelectResult and integration tests via RunAutoTune and TuneSearch
• Documentation updates in both English and Chinese are accurate and consistent
• The prune_searches decision is correctly scoped per build group in build-and-search mode and once upfront in search-only mode

[LHT129]

I have reviewed this PR thoroughly. The implementation is well-structured with good test coverage and documentation updates.

The existing review comments (from Copilot and LHT129) have already identified the key issues:

1. [critical] Shared metrics overwrite in search-only path (autotune_evaluation.cpp:367): In the search-only EvaluateCandidates, trial["metrics"] is overwritten with metrics_json(metrics) from search_metrics(), which drops shared_metrics (including index_memory_mb from GetMemoryUsage()). Since search_config() sets enable_memory = false, search_metrics() never populates index_memory_mb. The build-and-search path correctly merges via metrics.emplace(name, value). The same merge approach should be applied here.

2. [suggestion] Violation score normalization with near-zero expected values (autotune.cpp:755): std::max(violation["expected"].get<double>(), 1e-12) can produce non-finite scores when the constraint target is near zero (e.g., index_memory_mb <= 0.0). Consider std::max(std::abs(expected), 1e-12) for robustness.

3. [note] Brittle test assertion (autotune_test.cpp:1085): The REQUIRE(trial["status"] == "success") in the Pyramid search-only test could break if Pyramid ever returns a positive GetMemoryUsage() that exceeds the constraint. Consider relaxing to != "failed" or adding a comment explaining the assumption.

The overall design — pruning search trials when the index already exceeds the memory constraint, reporting them as pruned, and selecting best_effort from pruned trials only when no successful trials exist — is sound and well-documented.