Repository navigation
Refactor grid - #776
Refactor grid#776MarkWolters wants to merge 5 commits into
Conversation
|
Before you submit for review:
If you did not complete any of these, then please explain below. |
tlwillke
left a comment
There was a problem hiding this comment.
Thanks for this refactor. This will be much easier to work on and having BenchYAML and AutoBenchYAML share one pipeline is great.
I have a few comments, roughly in order of importance.
1. Wrong build time when the index comes from the cache (Grid.runOneGraph, lines 248–249, 260, 301)
When every feature set for a build config loads from the index cache, built.buildTimeS is null, so indexBuildTimes isn't updated. The values read at lines 260 and 301 then return the previous build config's time for that dataset. This was already true on main, but AutoBench's "Index Build Time" now feeds the regression comparison, so a cached run can report another config's number. Passing built.buildTimeS straight into SearchContext and ConstructionMetrics would fix it and avoid the static map.
2. Indexes not closed when a search fails (Grid.runOneGraph, lines 310–312 and 318)
The indexes are closed only after the search loop finishes normally. If testConfiguration throws, the finally block at line 318 deletes the graph files but leaves the indexes open. AutoBench moves on to the next dataset after a failure, so open files and memory maps pile up, and on Windows the files can't be deleted. Moving the close loop into the finally would fix this.
3. AutoBench result order changed (GridSpec.fromConfig, lines 55–60; Grid.testConfiguration, lines 395 and 397)
AutoBench used to loop M → efConstruction → neighborOverflow → addHierarchy → refineFinalGraph, with topK outside usePruning. It now uses BenchYAML's order: addHierarchy is outermost, and usePruning is outside topK. This is fine if everything that reads AutoBench's JSON matches rows by their parameters. Could you confirm that and mention the change in the release notes alongside the feature-set ordering change?
4. Compressor cache never hits (Grid, lines 361 and 597)
cachedCompressors is an IdentityHashMap, but its keys come from idStringFor(), which builds a new string with String.format on every call. As a result computeIfAbsent never finds an existing entry. Results are still correct because the compressor is reloaded from pq_cache, but every lookup does that reload and the map fills with duplicates. This was already broken on main, but switching to a HashMap is a one-word fix.
5. Possible remaining flakiness (TestBufferedGraphCompatibility.assertSameAnswer, lines 49–60)
Allowing a small tolerance on scores is a good improvement. Node order (line 55) and the visited and reranked counts (lines 58–59) are still compared exactly, though. Those can shift from the same rounding drift when two candidates nearly tie, so a rarer form of the flake may remain. This isn't a blocker; it's only worth revisiting if it shows up in CI again.
Smaller suggestions
6. Sweep setup is duplicated (Bench.main, lines 98–112; Grid.runAllAndCollectResults, lines 513–527)
Both build the same six-dimension GridSpec by hand, in the same order as GridSpec.fromConfig. Adding a construction parameter, which is the main thing this refactor is meant to make easy, means editing three lists and keeping their order in sync. A GridSpec factory that takes the list grids, called from both places, would leave one place to change.
7. Fused-PQ indexes are benchmarked once per search compressor (Grid.runOneGraph, lines 263 and 273)
Indexes with fused PQ ignore the search compressor, but the outer loop over search compressors still runs the full query sweep for each one. With three search compressors, each fused index is benchmarked three times with identical settings, producing rows that differ only in a search-compressor label that wasn't used. Running fused indexes once per build config would save the time and remove the duplicate rows. This was already the case on main.
8. Features without a provider are skipped silently (FeatureProviders.configureWriter, line 82)
If a configured feature has no registered FeatureProvider, the writer prints a message and builds the index without it. The results are still labeled with the requested feature set. Now that adding a provider is the documented way to benchmark a new feature, failing fast when GridSpec is built would catch a missed registration before a long run produces mislabeled results.




Refactors
Grid, the engine behindBenchYAMLandAutoBenchYAML, so concurrent benchmarking work lands in separate files instead of conflicting inGrid.java(now 692 lines, down from 1,395). AutoBenchYAML now shares BenchYAML's pipeline instead of a drifted copy. Build parameters travel asBuildConfig/SearchConfigobjects instead of positional arguments, and the hand-nested sweep loops become aGridSpecofSweepDimensions mapped from YAML inGridSpec.fromConfig. Graph construction moves intoIndexBuildStrategyimplementations backed by a singleGraphBuilderFactory, theFeatureIdswitch becomes oneFeatureProviderper on-disk feature, and console tables, experiments.csv rows and AutoBench results becomeGridListeners that all read build parameters fromBuildConfig.reportedParameters(). Command lines, YAML configs,run-config.ymland the experiments.csv schema are unchanged. Intended behavior changes: AutoBench builds one graph per build config instead of rebuilding for every feature set and search compressor, closes its indexes, and adds construction metrics to its JSON; feature sets run in configured order instead ofHashMaporder; and fused-PQ rows no longer report stale search timings. Also fixesTestBufferedGraphCompatibility, which was flaky on machines without the native vector library because Panama float rounding changes once C2 compiles the code. Harness usage and porting notes are indocs/release notes/4.0.3/776.testing.md.