Slim down ANNError - #1282
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors DiskANN’s error handling to slim down ANNError (removing the embedded “kind” tagging and legacy log_* constructors), standardize conversions via a new convert_error! macro, and update downstream crates to use the new constructors/messages. It also introduces a diskann-disk-local typed error payload (diskann-disk::error::{Error, ErrorKind}) to retain category-style assertions in that crate’s tests without re-expanding ANNError.
Changes:
- Replace
ANNError(kind, payload)+ANNErrorKindwith a smallerANNError(payload)andANNError::message(display); remove legacylog_*constructors andopaque(). - Add
convert_error!macro to reduce boilerplateFrom<T> for ANNErrorimpls and update many crates accordingly. - Add
diskann-diskcrate-localErrorKind+diskann_error!helper to preserve typed categorization in that crate.
Reviewed changes
Copilot reviewed 89 out of 89 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| diskann/src/utils/vector_repr.rs | Switch NativeTypeLengthError -> ANNError conversion to convert_error!. |
| diskann/src/utils/async_tools.rs | Use convert_error! for async utility error conversions. |
| diskann/src/test/cmp.rs | Update test helpers to use ANNError::new(...) instead of removed opaque(). |
| diskann/src/lib.rs | Remove top-level export of ANNErrorKind; keep only ANNError/ANNResult. |
| diskann/src/graph/test/provider.rs | Migrate test provider errors to convert_error! / ANNError::new. |
| diskann/src/graph/search/range_search.rs | Replace explicit From impl with convert_error!. |
| diskann/src/graph/search/paged.rs | Replace legacy paged-search error constructors with ANNError::message. |
| diskann/src/graph/search/knn_search.rs | Replace explicit From impl with convert_error!. |
| diskann/src/graph/search/inline_filter_search.rs | Replace explicit From impl with convert_error!. |
| diskann/src/graph/internal/prune.rs | Update transient escalation path to use new ANNError::new(self) signature. |
| diskann/src/graph/index.rs | Update async/join/mutex error mapping to ANNError::new / ANNError::message. |
| diskann/src/graph/config/mod.rs | Convert config error promotion to convert_error!. |
| diskann/src/flat/test/provider.rs | Convert test provider errors to convert_error!. |
| diskann/src/flat/strategy.rs | Convert local test error promotion to convert_error!. |
| diskann/src/error/ranked.rs | Update tests and conversions to new ANNError APIs; adopt convert_error!. |
| diskann/src/error/mod.rs | Remove re-exports of kind/tag types; adjust message! macro to new message() API. |
| diskann/src/error/ann_error.rs | Core change: shrink ANNError, remove ANNErrorKind/legacy constructors, add convert_error!. |
| diskann-tools/src/utils/search_index_utils.rs | Replace legacy index-error constructors with ANNError::message. |
| diskann-tools/src/utils/cmd_tool_error.rs | Update test to construct ANNError via new signature. |
| diskann-providers/src/utils/vector_data_iterator.rs | Convert SkipElementsError promotion to diskann::convert_error!; adjust tests accordingly. |
| diskann-providers/src/utils/sampling.rs | Replace legacy constructors with ANNError::message; adopt lazy_format! in hot error paths. |
| diskann-providers/src/utils/rayon_util.rs | Map Rayon thread-pool build errors via ANNError::new. |
| diskann-providers/src/utils/medoid.rs | Replace legacy constructors with ANNError::message. |
| diskann-providers/src/utils/file_util.rs | Replace legacy constructors with ANNError::message; use lazy_format! for deferred formatting. |
| diskann-providers/src/storage/pq_storage.rs | Replace PQ/index-config legacy constructors with ANNError::message. |
| diskann-providers/src/storage/index_storage.rs | Replace legacy constructors with ANNError::message + lazy_format!. |
| diskann-providers/src/storage/bin.rs | Replace legacy constructors with ANNError::message. |
| diskann-providers/src/model/pq/views.rs | Replace legacy PQ error conversions with convert_error! / ANNError::new. |
| diskann-providers/src/model/pq/strided.rs | Replace legacy PQ error conversions with ANNError::new. |
| diskann-providers/src/model/pq/pq_construction.rs | Replace legacy PQ constructors and mapping with ANNError::message / ANNError::new. |
| diskann-providers/src/model/pq/generate_pivot_arguments.rs | Replace legacy PQ error conversion with diskann::convert_error!; update tests. |
| diskann-providers/src/model/pq/fixed_chunk_pq_table.rs | Replace legacy PQ constructors; use lazy_format!; convert bridge error via diskann::convert_error!. |
| diskann-providers/src/model/pq/distance/dynamic.rs | Replace dimension mismatch legacy constructor with ANNError::message(lazy_format!(...)); adjust tests. |
| diskann-providers/src/model/graph/provider/determinant_diversity.rs | Replace custom kind-mapping conversion with diskann::convert_error!. |
| diskann-providers/src/model/graph/provider/async_/simple_neighbor_provider.rs | Replace legacy constructors with ANNError::message(lazy_format!(...)). |
| diskann-providers/src/model/graph/provider/async_/memory_vector_provider.rs | Replace legacy constructors with ANNError::message. |
| diskann-providers/src/model/graph/provider/async_/memory_quant_vector_provider.rs | Replace legacy constructors with ANNError::message. |
| diskann-providers/src/model/graph/provider/async_/inmem/spherical.rs | Replace SQ error-kind conversions with convert_error! and new error APIs. |
| diskann-providers/src/model/graph/provider/async_/inmem/scalar.rs | Replace SQ error conversion with diskann::convert_error!. |
| diskann-providers/src/model/graph/provider/async_/inmem/provider.rs | Replace legacy constructors with ANNError::message(lazy_format!(...)). |
| diskann-providers/src/model/graph/provider/async_/fast_memory_vector_provider.rs | Replace legacy constructors with ANNError::message. |
| diskann-providers/src/model/graph/provider/async_/fast_memory_quant_vector_provider.rs | Replace kind assertions in tests with message-based assertions. |
| diskann-providers/src/model/graph/provider/async_/experimental/multi_pq_async.rs | Replace legacy constructors with ANNError::message / lazy_format!. |
| diskann-providers/src/model/graph/provider/async_/common.rs | Replace legacy constructors with ANNError::message. |
| diskann-providers/src/index/wrapped_async.rs | Replace internal invariant conversion with diskann::convert_error!; update message() call shape. |
| diskann-providers/src/common/minmax_repr.rs | Switch conversion to ANNError::message(lazy_format!(...)). |
| diskann-label-filter/src/inline_beta_search/encoded_document_accessor.rs | Update message() usage to new signature (no kind). |
| diskann-label-filter/src/encoded_attribute_provider/roaring_attribute_store.rs | Remove kind-based messages; replace with ANNError::message(...) on lock errors and invariants. |
| diskann-label-filter/src/encoded_attribute_provider/document_provider.rs | Replace ANNErrorKind uses and ANNError::new(kind, e) with ANNError::new(e) / lazy_format!. |
| diskann-label-filter/src/encoded_attribute_provider/ast_label_id_mapper.rs | Remove ANNErrorKind usage; update new(...) and message(...) calls; adjust tests. |
| diskann-garnet/src/provider.rs | Replace provider error promotion with diskann::convert_error!. |
| diskann-disk/src/utils/partition.rs | Switch to diskann-disk typed error payload (diskann_error!(ErrorKind::...)). |
| diskann-disk/src/utils/math_util.rs | Switch to diskann-disk typed error payload and map sgemm errors accordingly. |
| diskann-disk/src/utils/kmeans.rs | Switch to diskann-disk typed error payload; update tests to assert via error_kind(). |
| diskann-disk/src/storage/quant/pq/pq_generation.rs | Switch PQ generation errors to diskann_error!(ErrorKind::PQError, ...). |
| diskann-disk/src/storage/quant/pq/pq_dataset.rs | Switch PQ dataset errors to diskann_error!(...). |
| diskann-disk/src/storage/quant/generator.rs | Switch quant generator validation errors to diskann_error!(...). |
| diskann-disk/src/storage/disk_index_writer.rs | Replace legacy constructors with diskann_error!(...) and simplify IO error propagation. |
| diskann-disk/src/storage/disk_index_reader.rs | Remove kind assertions; rely on message content. |
| diskann-disk/src/storage/cached_reader.rs | Replace legacy constructors with diskann_error!(...). |
| diskann-disk/src/search/provider/disk_vertex_provider.rs | Replace legacy constructors with diskann_error!(...) for bounds/serde errors. |
| diskann-disk/src/search/provider/disk_vertex_provider_factory.rs | Replace legacy constructors with diskann_error!(...) when allocating/alignment fails. |
| diskann-disk/src/search/provider/disk_sector_graph.rs | Replace legacy constructors with diskann_error!(...) for allocation and bounds checks. |
| diskann-disk/src/search/provider/disk_provider.rs | Replace kind-based errors with diskann_error!(...); map Knn construction error into disk error kind. |
| diskann-disk/src/search/provider/aligned_file_reader/reader/windows.rs | Simplify Windows IO error handling to use ?/From conversions. |
| diskann-disk/src/search/provider/aligned_file_reader/reader/linux.rs | Replace legacy push/io errors with diskann_error!(...) / Into<ANNError>. |
| diskann-disk/src/search/provider/aligned_file_reader/aligned_read.rs | Switch alignment errors to diskann_error!(ErrorKind::DiskIOAlignmentError, ...). |
| diskann-disk/src/search/pq/pq_scratch.rs | Switch PQ scratch allocation/dim errors to diskann_error!(...); update tests to assert via error_kind(). |
| diskann-disk/src/lib.rs | Export new diskann-disk::error module publicly. |
| diskann-disk/src/error.rs | New: crate-local ErrorKind + payload Error + diskann_error! helper + test-only error_kind() extractor. |
| diskann-disk/src/data_model/graph_metadata.rs | Replace legacy parse-slice error construction with diskann_error!(ErrorKind::SerdeError, ...). |
| diskann-disk/src/data_model/graph_layout_version.rs | Replace legacy parse-slice error construction with diskann_error!(ErrorKind::SerdeError, ...). |
| diskann-disk/src/data_model/graph_header.rs | Replace legacy conversions with diskann_error!(...); update tests to assert via error_kind(). |
| diskann-disk/src/data_model/cache.rs | Replace legacy constructors with diskann_error!(...) for capacity/dimension errors. |
| diskann-disk/src/build/configuration/disk_index_build_parameter.rs | Replace index-config legacy constructors with diskann_error!(ErrorKind::IndexConfigError(...), ...); update tests. |
| diskann-disk/src/build/builder/tokio.rs | Replace legacy tokio-runtime init error construction with diskann_error!(...). |
| diskann-disk/src/build/builder/quantizer.rs | Replace legacy index-config error construction with diskann_error!(...). |
| diskann-disk/src/build/builder/build.rs | Replace legacy constructors with diskann_error!(...) for task/thread/mutex failures. |
| diskann-bftree/src/vectors.rs | Replace legacy constructors with ANNError::message / lazy_format!; update ANNError::new signature usage. |
| diskann-bftree/src/quant.rs | Replace legacy constructors and SQ error mapping with ANNError::new / ANNError::message(lazy_format!(...)); relax tests. |
| diskann-bftree/src/provider.rs | Replace legacy constructors with ANNError::message / lazy_format!. |
| diskann-bftree/src/neighbors.rs | Replace legacy constructors with ANNError::message. |
| diskann-bftree/src/lib.rs | Update error promotions and transient escalation to new ANNError APIs. |
| diskann-benchmark/src/inputs/save_and_load.rs | Replace legacy index-config error creation with ANNError::message. |
| diskann-benchmark/src/index/streaming/full_precision.rs | Update message construction to new ANNError::message signature. |
| diskann-benchmark/src/index/build.rs | Replace kind-tagging of start-point compute errors with ANNError::new. |
| diskann-benchmark-core/src/search/graph/strategy.rs | Replace opaque() conversion with diskann::convert_error!; adjust imports for tests. |
| diskann-benchmark-core/src/build/ids.rs | Replace opaque() and kind-based messages with ANNError::new and lazy_format!. |
| diskann-benchmark-core/src/build/graph/multi.rs | Replace kind-based message(...) call with new single-arg message(...). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if idx >= self.npts { | ||
| return Err(ANNError::log_index_error(format!( | ||
| let npts = self.npts - 1; | ||
| return Err(ANNError::message(lazy_format!( | ||
| move, | ||
| "Vector index {} is out of bounds (max: {})", |
| // Top level exports. | ||
| pub use error::ann_error::{ANNError, ANNErrorKind, ANNResult}; | ||
| pub use error::ann_error::{ANNError, ANNResult}; |
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (72.68%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #1282 +/- ##
==========================================
+ Coverage 90.62% 90.70% +0.08%
==========================================
Files 512 513 +1
Lines 98807 98434 -373
==========================================
- Hits 89541 89284 -257
+ Misses 9266 9150 -116
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 89 out of 89 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
diskann/src/error/ann_error.rs:144
ANNError::newwraps errors asLocated<E>, butis::<E>()only checks forEand will therefore returnfalsefor most errors created by this type. This makes the method misleading/inaccurate.
Mirror the downcast_ref behavior by also checking for Located<E>.
| if num_nodes_to_cache == 0 { | ||
| ANNError::log_index_error( | ||
| diskann_error!( | ||
| ErrorKind::IndexError, | ||
| "num_nodes_to_cache should be greater than 0 for StaticCacheWithBfsNodes caching strategy", | ||
| ); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 89 out of 89 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
diskann-disk/src/search/provider/disk_vertex_provider_factory.rs:160
diskann_error!(...)is constructed but not returned whennum_nodes_to_cache == 0, so the function continues with an invalid configuration. This also leaves a likely-unused expression (warning under-D warnings).
if num_nodes_to_cache == 0 {
diskann_error!(
ErrorKind::IndexError,
"num_nodes_to_cache should be greater than 0 for StaticCacheWithBfsNodes caching strategy",
);
diskann-providers/src/utils/sampling.rs:185
- When
self.npts == 0, the out-of-bounds branch computesself.npts - 1, which underflows (panic in debug; wraps in release) and produces a misleading max value. Handle the empty-dataset case explicitly before subtracting.
diskann/src/error/ann_error.rs:139 - Doc comment grammar/typos: "Return
trueis the the type" / "type ... of in the" reads incorrectly and makes rustdoc harder to follow.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 90 out of 90 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
diskann/src/error/ann_error.rs:139
- Doc comment grammar/typos: "is the the" / "of in" reads incorrectly and is confusing.
diskann-label-filter/src/encoded_attribute_provider/roaring_attribute_store.rs:137 - Error message typo: "atleast" should be "at least".
diskann-disk/src/utils/partition.rs:261 - Error message typo: "dont" should be "don't".
|
how about impl_ann_error_from!? |
|
Could we document the intended usage pattern for |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 96 out of 96 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
diskann-providers/src/utils/sampling.rs:189
self.npts - 1can underflow when the dataset header containsnpts == 0, causing a panic/wrap and producing a confusing max index in the error message. Handle the empty-dataset case explicitly or use checked arithmetic before formatting the max index.
diskann/src/error/ann_error.rs:161- Doc comment has grammatical errors ("is the the" / "of in") that make the
isAPI description harder to understand.
diskann-disk/src/error.rs:18 - The module docs refer to
crate::diskann_error!and contain a typo ("marco"). The macro is defined/re-exported in this module, so the correct path iscrate::error::diskann_error!(or re-export it at crate root if that was intended).
//! The [`crate::diskann_error!`] marco should be used to get most of the benefits from
Remove
ANNErrorKindfromANNError, removing all the oldlog_constructors and slimming the constructors tonew,message, and `context.Since
diskann-diskleans pretty heavily to using the kinds in its tests, a newdiskann_disk::error::Errortype is introduces along with adiskann_error!macro to largely preserve test structure and error kind tagging. See the review notes for more information.Why
The error kind enum is largely unused at this point, is not applied consistently, and complicates the API for
ANNError. It's time to remove it and bring the size ofANNErrordown to 8-bytes.Review Order
diskannerror/ann_error.rs: Removal of theANNErrorKindenum and thelog_*style constructors. As a bonus, I added aconvert_error!macro (happy to bike-shed) to stamp out simple conversion cases while preservingtrack_caller.The rest of the changes in
diskannare removing theANNErrorKindand switching over to theconvert_error!macro.diskann-tools/diskann-providers/diskann-garnet/diskann-label-filter/diskann-bftree/diskann-benchmark-core/diskann-benchmarkThese mainly consist of updating the constructors and conversions. Note that where possible (and easily achievable), I use the moving form of
lazy_format!to avoid eager string formatting.**
diskann-diskThis is the perhaps a more controversial change.
diskann-disk's tests lean heavily into the error kind andlog*style constructors. I tried to preserve as much of the original behavior as possible by introducing an error typediskann_disk::error::Errorconsisting of a string-message an adiskann_disk::error::ErrorKindenum. A macrodiskann_disk::error::diskann_error!helps embed this newErrorinside of anANNError. While this goes against the ethos of "don't have crate level errors", here is my rationale:log_*constructors,diskann_disk's error types are already fundamentally tagged string messages.Errortype means we can still check theKindenum by downcasting the resultingANNErrordown to the originalError.diskann_diskcode is already doing eager string formatting/allocating at its error construction callsites, this isn't introduction a regression. Indeed, I've added some tricks to avoid allocating when constructingdiskann_disk::error::Errorfrom string literals, which is fairly common.diskann_diskallows the disk index to add error types without disruptingANNError.Alternatives