Fix diskann compilation without default-features and add CI tests.#722
Open
hildebrandmw wants to merge 2 commits intomainfrom
Open
Fix diskann compilation without default-features and add CI tests.#722hildebrandmw wants to merge 2 commits intomainfrom
diskann compilation without default-features and add CI tests.#722hildebrandmw wants to merge 2 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes compilation of the diskann crate when the tracing feature is disabled and adds comprehensive CI testing to prevent similar issues. The fix addresses issue #720 by providing conditional implementations of tracing macros that compile to no-ops when the feature is disabled.
Changes:
- Added feature-gated implementations of
tracked_*macros with aused!helper that validates format strings without runtime overhead - Replaced direct tracing macro calls (
trace!,debug!) withtracked_*variants indiskann/src/graph/index.rs - Added CI job to test individual crates with
--no-default-featuresto catch feature unification issues - Improved CI reproducibility by adding
--lockedflag to all cargo commands
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| diskann/src/tracing.rs | Added conditional implementations of tracking macros for both with/without tracing feature, introduced used! helper macro for no-op validation |
| diskann/src/graph/index.rs | Removed direct tracing imports and replaced trace! and debug! calls with tracked_trace! and tracked_debug! |
| .github/workflows/ci.yml | Added clippy-no-default-features job for testing crates individually, added --locked flag throughout for reproducibility |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #722 +/- ##
==========================================
- Coverage 89.01% 89.01% -0.01%
==========================================
Files 428 428
Lines 78151 78151
==========================================
- Hits 69566 69563 -3
- Misses 8585 8588 +3
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Compilation of
diskannwithout thetracingfeature is currently broken (see #720). This fixes the issue by:tracked_*macros indiskann.tracked_*macros to a fancy no-op when thetracingfeature is disabled.This fancy no-op uses
format_args!to still validate the format of the string and to suppress "unused variable" warnings when the feature is disable. Compiler explorer suggests that with optimizations enabled, the construction offmt::Argumentsis removed, meaning this should have no runtime cost in optimized builds.Additionally,
clippytests are added for individual crates with features. Feature unification across the workspace results in issues like this not being caught when running CI with the--workspaceflag. I tried to be selective with these tests to minimize CI overhead.