feat: add AutoTune V1 orchestration - #2569
Conversation
|
/label status/waiting-for-review |
|
Automated pull request review completed. Review effort: Submitted 5 inline comments. |
jac0626
added
kind/feature
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
|
vsag-bot
left a comment
vsag-bot
left a comment
There was a problem hiding this comment.
Automated inline review completed.
Review effort: high (6342 changed lines across 41 files).
Submitted 1 inline comment.
Reviewed commit 998445f.
| uint64_t maximum = 1; | ||
| auto low = range->start; | ||
| while (low < range->stop) { | ||
| const auto high = low > range->stop / 2 ? range->stop : low * 2; |
There was a problem hiding this comment.
[suggestion] maximum_trial_count: same low * 2 overflow as EvaluateEfSearchRange
Same signed integer overflow risk at line 334: low * 2 when low > INT64_MAX / 2. The fix should mirror whatever approach is taken for the corresponding expression in autotune_evaluation.cpp:161.
| report_path = context.result_path.empty() | ||
| ? context.workspace_path + "/" + run_name + ".json" | ||
| : context.result_path; | ||
| stage = "report"; |
There was a problem hiding this comment.
[note] stage = "report" set before prepare_report_path may misclassify path errors
At line 893, stage is set to "report" before prepare_report_path(report_path) is called at line 894. If prepare_report_path throws (e.g., permission denied), the failure lambda will use stage == "report" and classify the error as "execution_failed" rather than "invalid_request". Since the report path is part of the user-provided request configuration, a path error is arguably a validation failure.
Consider moving stage = "report" to after the prepare_report_path call, or using a more specific stage name for the report-path validation.
vsag-bot
left a comment
vsag-bot
left a comment
There was a problem hiding this comment.
Automated inline review completed.
Review effort: high (6838 changed lines across 43 files).
Submitted 3 inline comments.
Reviewed commit 5f1c4cf.
|
|
||
| if (range["start"].is_number_integer() && range["stop"].is_number_integer() && | ||
| range["step"].is_number_integer()) { | ||
| const auto start = range["start"].get<int64_t>(); |
There was a problem hiding this comment.
[P2] [P2] Reject unsigned range endpoints before signed conversion
is_number_integer() also accepts nlohmann's unsigned-number type, so a JSON bound such as 9223372036854775808 reaches get<int64_t>() and is converted out of range (the reproducer emits INT64_MIN). A one-point range at that value is consequently reported and evaluated as a negative parameter instead of being rejected or preserved, silently changing the requested candidate space. Validate unsigned start/stop/step against INT64_MAX before conversion, or expand them using an unsigned representation.
| (step < 0.0 && current < stop && stop - current > tolerance)) { | ||
| return; | ||
| } | ||
| if (std::abs(current - stop) <= tolerance) { |
There was a problem hiding this comment.
[P3] [P3] Do not snap the beginning of a floating range to its end
The tolerance is scaled by the absolute endpoint magnitude, so it can exceed the entire range span. For example, {start: 1e15, stop: 1e15+1, step: 0.25} generates exactly one candidate—the stop—even though the start and intermediate values are representable. This violates the documented inclusive-range behavior and can skip the best candidate; endpoint snapping should only apply when arithmetic has reached the expected final step, not merely whenever a value is within several endpoint ULPs.
| for (const auto& item : search.items()) { | ||
| require(normalize(item.key()) == index.name, | ||
| path + ".search_parameter_space." + item.key() + " is unsupported"); | ||
| index.search_params[index.name] = item.value(); |
There was a problem hiding this comment.
[P3] [P3] Reject duplicate case-normalized search namespaces
Each top-level search key is normalized and then assigned to the same index.search_params[index.name] slot. Thus {"HGRAPH": {...}, "hgraph": {...}} passes validation but the later entry silently overwrites the earlier one, causing the planned Cartesian product to omit requested candidates. Reject multiple keys that normalize to the index name, or merge them with explicit duplicate-field validation.
5f1c4cf to
f9d1e64
Compare
| } | ||
|
|
||
| void | ||
| known_keys(const JsonType& object, |
There was a problem hiding this comment.
[suggestion] known_keys iterates by value, copying each string
The function signature is bool known_keys(const JsonType& json, std::unordered_set<std::string> known). The known parameter is passed by value, which is fine since the caller may want to keep the original set. However, inside the loop for (const auto& key : json.items()), each known.count(key.key()) call constructs a temporary std::string because std::unordered_set<std::string> does not support heterogeneous lookup by default.
Consider changing the parameter to const std::unordered_set<std::string>& known and having callers pass a copy explicitly when needed. This is a minor performance note for the validation path.
| } | ||
|
|
||
| auto dataset = std::make_shared<EvalDataset>(); | ||
| dataset->base_dataset_ = base; |
There was a problem hiding this comment.
[suggestion] EvalDataset::FromSearchDatasets leaves number_of_base_ as 0 but GetNumberOfBase() is called by SearchEvalCase
FromSearchDatasets sets train_shape_ = {0, dataset->dim_} and number_of_base_ is default-initialized to 0. This is semantically correct for a search-only dataset, but SearchEvalCase::RunInMemory() calls this->dataset_ptr_->GetNumberOfBase() indirectly through the recall monitor. If use_id_based_recall is false, RecallMonitor will try to access base vectors via GetOneTrainById(), which returns nullptr when train_ is null — this is handled gracefully. However, if someone adds a code path that assumes GetNumberOfBase() > 0 for a search-only dataset, it would fail silently.
Consider documenting that GetNumberOfBase() returns 0 for search-only datasets, or adding a HasBase() predicate to make the contract explicit.
| serialize_index(index, index_path); | ||
| shared_metrics = build_metrics(raw, index_path); | ||
| if (request.include_raw_eval) { | ||
| build["raw_eval_result"] = std::move(raw); |
There was a problem hiding this comment.
[suggestion] EvaluateCandidates for IndexTuningRequest does not validate search_param before passing to EvaluateSearch
In autotune_evaluation.cpp, when evaluating an IndexTuningRequest, the code constructs eval_config.search_param from trial["search_params"].dump(). If the candidate generation produces a trial with empty search_params (e.g., due to a bug in fill_* functions), EvaluateSearch will receive an empty string as the search parameter, which may cause confusing errors deep in the index implementation rather than a clear validation failure.
Consider adding a check that search_param is non-empty before calling EvaluateSearch, or validating it at candidate generation time.
| JsonType | ||
| EvaluateBuild(const IndexPtr& index, const EvalDatasetPtr& dataset, const EvalConfig& config) { | ||
| validate(index, dataset); | ||
| BuildEvalCase eval_case("", "", index, config, dataset); |
There was a problem hiding this comment.
[suggestion] EvaluateBuild in evaluator.cpp does not set num_threads_building from the config
In evaluator.cpp, EvaluateBuild() creates a BuildEvalCase and calls RunInMemory(). The EvalConfig passed in has num_threads_building defaulting to 1, but the caller in autotune_evaluation.cpp's build_config() function does not set this field. The do_build() method in BuildEvalCase does not call omp_set_num_threads() before building, unlike do_knn_search() which does. This means the build always uses the ambient OpenMP thread count rather than a controlled value.
If the intent is to use the ambient thread count (set by ScopedOpenMpThreads), consider documenting this. If the intent is to control build parallelism, consider plumbing num_threads_building through build_config().
| std::ifstream input(argv[1]); | ||
| if (!input.good()) { | ||
| throw std::runtime_error("failed to open request file: " + std::string(argv[1])); | ||
| } |
There was a problem hiding this comment.
[suggestion] main.cpp disables the VSAG logger globally but never restores it
In main.cpp, vsag::Options::Instance().logger()->SetLevel(vsag::Logger::kOFF) disables all VSAG logging for the duration of the process. This is fine for a CLI tool that exits immediately, but if the AutoTune library is ever embedded in a longer-running process (e.g., through the C++ API), this side effect persists after RunAutoTune() returns. The C++ API entry points (TuneIndex, TuneSearch) do not disable the logger, which is correct — but the inconsistency between CLI and API behavior could be surprising.
Consider using a RAII scoped logger-level guard that restores the original level on return, or documenting that the CLI disables all VSAG logging globally.
| } | ||
| if (emitted && current == previous) { | ||
| throw std::invalid_argument("$range step is too small to advance"); | ||
| } |
There was a problem hiding this comment.
[suggestion] autotune_candidate.cpp expand_range for floating-point uses step default of 1.0 which may produce too many candidates
In autotune_candidate.cpp, the floating-point expand_range function defaults step to 1.0 when not specified. For a range like {"$range": {"start": 1, "stop": 1000}} on a float parameter (e.g., alpha), this would generate 1000 candidates. The max_trials check catches this before evaluation, but the candidate generation itself allocates a large vector.
Consider using a smarter default step for floating-point ranges (e.g., based on the magnitude of stop - start), or validating the step against the range span at parse time.
LHT129
left a comment
LHT129
left a comment
There was a problem hiding this comment.
Review Summary
This is a well-structured PR that adds AutoTune V1 orchestration with both CLI and C++ API. The architecture cleanly separates concerns: request parsing, candidate generation, evaluation, and result selection. The test coverage is thorough, covering validation edge cases, error paths, and the full tuning pipeline.
What I reviewed
- All core implementation files:
autotune.cpp,autotune_candidate.cpp,autotune_evaluation.cpp,main.cpp,autotune_test.cpp - New in-memory evaluation entry points:
evaluator.cpp,eval_dataset.cpp,search_eval_case.cpp,build_eval_case.cpp - Build system changes:
CMakeLists.txt, CI workflow - Public API headers:
autotune.h,evaluator.h
New findings (6 inline comments)
known_keysparameter passing — minor perf note about string copies in validationFromSearchDatasetsbase count —GetNumberOfBase()returns 0 for search-only datasets, which is correct but implicitEvaluateCandidatessearch_param validation — empty search params could cause confusing errors downstreamEvaluateBuildthread count —num_threads_buildingnot plumbed throughbuild_config()main.cpplogger side effect — global logger disable persists afterRunAutoTune()returnsexpand_rangefloat step default — default step of 1.0 could generate excessive candidates for wide ranges
Existing findings (already addressed or under discussion)
The earlier review round by vsag-bot and LHT129 covered: TPS monitoring in BuildEvalCase (fixed), ScopedOpenMpThreads destructor (confirmed correct per OpenMP spec), SearchFailure deadlock (confirmed no issue), aliasing shared_ptr lifetime, same_path edge cases, integer overflow in expand_range (confirmed not present), global mutex serialization, ef_search binary search efficiency, and exception type consistency.
Overall assessment
The PR is in good shape. The new findings are all suggestions (not critical), focused on code clarity and defensive validation. The core logic is sound and the test coverage gives confidence in correctness.
vsag-bot
left a comment
vsag-bot
left a comment
There was a problem hiding this comment.
Automated inline review completed.
Review effort: high (6838 changed lines across 43 files).
No actionable inline findings were found.
Reviewed commit f9d1e64.
f9d1e64 to
4254cfc
Compare
LHT129
left a comment
LHT129
left a comment
There was a problem hiding this comment.
Automated review: additional findings
| Evaluation evaluation; | ||
| std::map<std::string, std::vector<uint64_t>> groups; | ||
| for (uint64_t i = 0; i < candidates.size(); ++i) { | ||
| const auto key = candidates[i].index_name + "\n" + candidates[i].create_params.dump(); |
There was a problem hiding this comment.
[suggestion] Build deduplication key depends on JSON key ordering
EvaluateCandidates(IndexTuningRequest) groups candidates by index_name + "\n" + create_params.dump(). nlohmann::json::dump() does not guarantee stable key ordering — two JsonType objects with the same logical content but keys inserted in different orders will produce different strings, causing unnecessary duplicate builds.
Consider using a canonical form for the key, e.g. sorting keys before serialization or computing a hash over the sorted key-value pairs.
| } | ||
|
|
||
| void | ||
| check_file(const std::string& path, const std::string& field) { |
There was a problem hiding this comment.
[suggestion] check_file has a TOCTOU race between validation and use
check_file verifies the file is a readable regular file, but the actual consumer (EvalDataset::Load or std::ifstream for index deserialization) opens it later. On shared filesystems or under concurrent writers, the file could be truncated or replaced between the check and the open, leading to a hard-to-diagnose failure.
Since this is a CLI tool and the race window is small, this is low-risk. Consider opening the file once and passing the stream, or documenting that the caller is responsible for filesystem stability.
| } | ||
|
|
||
| Evaluation | ||
| EvaluateCandidates(const SearchTuningRequest& tuning_request, |
There was a problem hiding this comment.
[suggestion] EvaluateCandidates(SearchTuningRequest) does not validate that the index is built
The function calls eval::EvaluateSearch(tuning_request.index, ...) without checking whether the index has been built. If a caller passes an unbuilt index, the failure surfaces as a generic search_evaluation_failed error deep in the eval layer rather than a clear early validation error.
Consider adding a pre-condition check (e.g. GetNumElements() > 0) and failing with a descriptive message before entering the trial loop.
vsag-bot
left a comment
vsag-bot
left a comment
There was a problem hiding this comment.
Automated inline review completed.
Review effort: high (6838 changed lines across 43 files).
Submitted 1 inline comment.
Reviewed commit 4254cfc.
| if (config.num_threads_searching <= 0) { | ||
| throw std::invalid_argument("evaluation search thread count must be positive"); | ||
| } | ||
| if (config.search_query_count == 0) { |
There was a problem hiding this comment.
[P2] Bound the query count to the signed search loop range
search_query_count is a uint64_t, but SearchEvalCase::do_knn_search() iterates over the resulting bound with an int64_t id. A direct EvaluateSearch caller can therefore pass a value above INT64_MAX; with latency disabled and recall or memory monitoring enabled, the counter eventually overflows, causing undefined behavior or nontermination. Reject counts above INT64_MAX here, or use a loop/index type capable of representing every accepted count.
Signed-off-by: jc543239 <jc543239@antgroup.com> Assisted-by: Codex:gpt-5
4254cfc to
dc59b20
Compare
vsag-bot
left a comment
vsag-bot
left a comment
There was a problem hiding this comment.
Automated inline review completed.
Review effort: high (6838 changed lines across 43 files).
Submitted 5 inline comments.
Reviewed commit dc59b20.
|
|
||
| if (range["start"].is_number_integer() && range["stop"].is_number_integer() && | ||
| range["step"].is_number_integer()) { | ||
| const auto start = range["start"].get<int64_t>(); |
There was a problem hiding this comment.
[P2] Reject unsigned range values before converting to int64_t
is_number_integer() also accepts nlohmann unsigned numbers, so a bound such as 9223372036854775808 reaches get<int64_t>() and becomes an out-of-range signed value. The resulting candidate space can silently contain a negative parameter instead of the requested value. Check unsigned start/stop/step against INT64_MAX before conversion or represent the range as unsigned.
| (step < 0.0 && current < stop && stop - current > tolerance)) { | ||
| return; | ||
| } | ||
| if (std::abs(current - stop) <= tolerance) { |
There was a problem hiding this comment.
[P3] Do not snap the first floating candidate to the endpoint
The tolerance scales with the absolute endpoint magnitude and can exceed the entire range span. For example, start=1e15, stop=1e15+1, and step=0.25 immediately replaces the representable start with the stop, omitting all earlier candidates. Endpoint snapping should only apply after arithmetic reaches the expected final step.
| for (const auto& item : search.items()) { | ||
| require(normalize(item.key()) == index.name, | ||
| path + ".search_parameter_space." + item.key() + " is unsupported"); | ||
| index.search_params[index.name] = item.value(); |
There was a problem hiding this comment.
[P3] Reject duplicate case-normalized search namespaces
A parameter object containing both HGRAPH and hgraph passes the normalization check, but each iteration assigns the same index.search_params[index.name] field, so one requested space silently overwrites the other. Reject a second normalized namespace instead of dropping candidates.
| if (config.num_threads_searching <= 0) { | ||
| throw std::invalid_argument("evaluation search thread count must be positive"); | ||
| } | ||
| if (config.search_query_count == 0) { |
There was a problem hiding this comment.
[P2] Bound query counts to the signed OpenMP loop range
search_query_count is unsigned, but SearchEvalCase::do_knn_search() iterates to that bound with an int64_t counter. With latency disabled and recall or memory monitoring enabled, a value above INT64_MAX is accepted and eventually causes signed overflow or nontermination. Reject such counts here or use an unsigned-capable iteration scheme.
| validate_search(dataset, config); | ||
| ScopedOpenMpThreads openmp_threads; | ||
| SearchEvalCase eval_case("", "", index, config, dataset); | ||
| return eval_case.RunInMemory(); |
There was a problem hiding this comment.
[P2] Execute a search when all monitors are disabled
If recall, latency/QPS, percentile, and memory monitoring are all disabled, SearchEvalCase has no monitors and also skips its fallback pass. EvaluateSearch then reports success without invoking KnnSearch, so invalid parameters such as ef_search=0 are silently accepted. Require a monitor or perform an unconditional validation/statistics search pass.
| } | ||
|
|
||
| const auto buckets = create_params["index_param"]["buckets_count"].get<uint64_t>(); | ||
| if (buckets <= 16) { |
There was a problem hiding this comment.
[P2] fill_ivf_search calls get<uint64_t>() on buckets_count which may be stored as int64_t by nlohmann::json, causing a type_error at runtime.
nlohmann::json stores small integers (like buckets_count) as int64_t by default. Calling get<uint64_t>() on an int64_t-stored value will throw nlohmann::json::type_error because the library does not perform implicit sign conversion between integer types.
// tools/autotune/autotune_candidate.cpp:245
const auto buckets = create_params["index_param"]["buckets_count"].get<uint64_t>();Fix: Use get<int64_t>() and cast to uint64_t if needed, or use .get<uint64_t>() only after confirming the value is stored as unsigned. Since buckets_count is a count value that should always be non-negative, using get<int64_t>() with an assertion or get<uint64_t>() with a try/catch fallback would be safer.
| params["scan_buckets_count"] = std::set<uint64_t>{1, 4, 16, 64}; | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
[P2] fill_ivf_search calls get<uint64_t>() on buckets_count which may be stored as int64_t by nlohmann::json
In autotune_candidate.cpp:243, create_params["index_param"]["buckets_count"].get<uint64_t>() will throw nlohmann::json::type_error if the value was parsed from a JSON number without a decimal point and fits in int64_t (nlohmann::json's default storage for small integers). The positive_int64() helper at line 57 correctly handles both uint64_t and int64_t JSON types. Consider using positive_int64() or a similar dual-type accessor here to avoid a runtime crash when buckets_count is a small integer like 16.
// Suggested fix:
const auto buckets = static_cast<uint64_t>(positive_int64(create_params["index_param"], "buckets_count"));
Change Type
Linked Issue
What Changed
eval_performancewhile preserving its existing file-based flow.V1 intentionally supports one dense float32 KNN workload. SINDI, Multi, sparse vectors, cross-request caching, model-based proposals, and a public
Factory::CreateIndexWithConstraintsAPI remain out of scope.Test Evidence
make fmtmake lintmake testmake cov, run tests, and collect coverageTest details:
Compatibility Impact
eval_performancecommands remain supported.Performance and Concurrency Impact
Documentation Impact
README.mdDEVELOPMENT.mdCONTRIBUTING.mddocs/docs/{en,zh}/src/resources/autotune*.mdandtools/autotune/README*.mdRisk and Rollback
Checklist
kind/bugandkind/feature; see "Linked Issue" above)[skip ci]prefix)