Update JIT+LTO guide to reflect new automatic embedding system#2045
Conversation
KyleFromNVIDIA
added
doc
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughStep 5 of the JIT LTO guide was rewritten: Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/source/jit_lto_guide.md`:
- Line 480: Update the guide to remove the outdated manual embedding steps and
align later sections with the auto-embedding flow described for
generate_jit_lto_kernels(); specifically, eliminate instructions that require
EMBEDDED_INPUT_FILE and creating .cpp.in templates (references in Step 8 and the
Summary) and instead document that FRAGMENT_TAG_FORMAT and
FRAGMENT_TAG_HEADER_FILES are used to auto-generate the .cpp registration file
at build time; adjust wording, examples, and any parameter/type mentions to
consistently reference generate_jit_lto_kernels(), FRAGMENT_TAG_FORMAT, and
FRAGMENT_TAG_HEADER_FILES across the remaining sections.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 85de1015-8508-4b10-9b77-e7dd0035574e
📒 Files selected for processing (1)
docs/source/jit_lto_guide.md
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/source/jit_lto_guide.md (1)
722-722: ⚡ Quick winClarify supported header file formats.
The note states headers "should be enclosed in
</>" but the implementation ingenerate_jit_lto_kernels.cmake(lines 55-60) shows that headers can be:
- Enclosed in
</>for system headers- Enclosed in quotes for project headers
- Plain paths (auto-wrapped in quotes by CMake)
📝 Proposed clarification
-- `FRAGMENT_TAG_HEADER_FILES`: List of header files that provide the fragment tag types (should be enclosed in `<`/`>`) +- `FRAGMENT_TAG_HEADER_FILES`: List of header files that provide the fragment tag types (can be enclosed in `<`/`>` for system headers, quotes for project headers, or plain paths that CMake will automatically wrap in quotes)Or more concisely:
-- `FRAGMENT_TAG_HEADER_FILES`: List of header files that provide the fragment tag types (should be enclosed in `<`/`>`) +- `FRAGMENT_TAG_HEADER_FILES`: List of header files that provide the fragment tag types (e.g., `<system/header.hpp>`, `"project/header.hpp"`, or `path/to/header.hpp`)Based on learnings: Code snippet from generate_jit_lto_kernels.cmake shows automatic quote-wrapping for plain paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/source/jit_lto_guide.md` at line 722, Update the documentation entry for FRAGMENT_TAG_HEADER_FILES to list the three supported header formats (system headers enclosed in <...>, project headers enclosed in "..." and plain file paths which CMake will auto-wrap in quotes), and mention that generate_jit_lto_kernels.cmake handles automatic quoting of plain paths (see handling in generate_jit_lto_kernels.cmake around the FRAGMENT_TAG_HEADER_FILES usage).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/source/jit_lto_guide.md`:
- Around line 480-482: Add a short concrete example showing how to define the
lightweight fragment tag types (empty template structs) referenced in the guide:
create a suggested registration_tags.hpp containing definitions for
example::detail::fragment_tag_search,
example::detail::fragment_tag_compute_distance, and
example::detail::fragment_tag_filter (each as empty template struct templates
with the same template parameters used later: DataTag, OutTag, IdxTag,
Optimized, Veclen, etc.), and add a brief note linking that these are the types
constructed by the FRAGMENT_TAG_FORMAT CMake string (e.g.,
"example::detail::fragment_tag_search<tag_@type_abbrev@, ...>@") so readers can
see how the tag types map to the CMake FRAGMENT_TAG_FORMAT and are used with
add_static_fragment in AlgorithmPlanner.
---
Nitpick comments:
In `@docs/source/jit_lto_guide.md`:
- Line 722: Update the documentation entry for FRAGMENT_TAG_HEADER_FILES to list
the three supported header formats (system headers enclosed in <...>, project
headers enclosed in "..." and plain file paths which CMake will auto-wrap in
quotes), and mention that generate_jit_lto_kernels.cmake handles automatic
quoting of plain paths (see handling in generate_jit_lto_kernels.cmake around
the FRAGMENT_TAG_HEADER_FILES usage).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 27f75f35-e73c-4cbf-8f3b-372f28928386
📒 Files selected for processing (1)
docs/source/jit_lto_guide.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/source/jit_lto_guide.md`:
- Line 722: The doc line for FRAGMENT_TAG_HEADER_FILES is too strict about
requiring angle brackets; update the sentence to state that
FRAGMENT_TAG_HEADER_FILES should list header paths (angle brackets are optional)
and that the CMake helper accepts plain header paths and will auto-quote or wrap
them as needed, ensuring the wording reflects that both <header> and header.h
forms are valid and the helper will handle quoting/angle-bracketing
automatically; reference FRAGMENT_TAG_HEADER_FILES and the CMake helper in the
sentence so readers know which tool performs the auto-quoting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7c68bb4b-bf33-4a1c-80c2-9466b5db0722
📒 Files selected for processing (1)
docs/source/jit_lto_guide.md
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/source/jit_lto_guide.md (3)
623-627:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix SearchPlanner method name mismatch (Step 6 vs Step 7).
In Step 7 (Line 624-626), the guide calls
planner.add_compute_distance_device_function(...)andplanner.add_filter_device_function(...), but Step 6 definesadd_compute_distance_function(...)andadd_filter_function(...). As written, copy/paste will fail to compile.🛠️ Proposed fix
- planner.add_compute_distance_device_function<metric_tag, data_tag>(); - planner.add_filter_device_function<filter_tag, idx_tag>(); + planner.add_compute_distance_function<metric_tag, data_tag>(); + planner.add_filter_function<filter_tag, idx_tag>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/source/jit_lto_guide.md` around lines 623 - 627, The doc has a method-name mismatch: Step 6 defines add_compute_distance_function and add_filter_function but Step 7 calls planner.add_compute_distance_device_function and planner.add_filter_device_function which will not compile; update the Step 7 calls to use planner.add_compute_distance_function<metric_tag, data_tag>() and planner.add_filter_function<filter_tag, idx_tag>() (or alternatively rename the Step 6 definitions to include the "_device_" suffix) so the method names match across the guide.
517-517: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUpdate outdated “.cpp.in registration files” wording.
Line 517 says fragment keys must match “the corresponding
.cpp.inregistration files”. With the new automatic embedding system described in Step 5, registration code should be auto-generated (i.e., the.cppemitted bygenerate_jit_lto_kernels()), not sourced from user-provided.cpp.intemplates.🛠️ Proposed wording tweak
- **CRITICAL**: The fragment keys constructed in the planner methods must match **EXACTLY** with the keys used in the corresponding `.cpp.in` registration files. + **CRITICAL**: The fragment keys constructed in the planner methods must match **EXACTLY** with the keys used in the auto-generated registration code emitted by `generate_jit_lto_kernels()`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/source/jit_lto_guide.md` at line 517, Update the wording to replace references to user-provided “.cpp.in registration files” with the auto-generated registration source emitted by generate_jit_lto_kernels(); specifically state that fragment keys must match exactly the keys used in the auto-generated registration .cpp produced by generate_jit_lto_kernels() (as described in Step 5) so readers know the registration code is generated automatically rather than coming from .cpp.in templates.
591-600:⚠️ Potential issue | 🔴 CriticalFix non-existent enum value and incomplete case handling in the example code.
The example code has critical issues:
get_metric_tag()referencesDistanceType::Euclideanwhich does not exist. The actual enum containsInnerProduct(line 6 ofcpp/include/cuvs/distance/distance.hpp). The code should likely handleInnerProductinstead, or clarify which distance metric the example intends to demonstrate.
get_filter_tag()only handlesFilterType::None. The enum also includesBitmapandBitset, which must be handled. Add branches for both missing cases.Add all missing branches as compile-time errors (using
static_assertor#error) for any unhandled enum values to prevent silent failures. Verify the tag struct names match those defined inregistration_tags.hpp.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/source/jit_lto_guide.md` around lines 591 - 600, get_metric_tag() and get_filter_tag() must be corrected to cover real enum values and fail at compile time for unhandled cases: update get_metric_tag() to check for DistanceType::InnerProduct (or add branches for any other real DistanceType values present) and return the exact tag structs defined in registration_tags.hpp (e.g., tag_metric_inner_product if that’s the name); update get_filter_tag() to handle FilterType::Bitmap and FilterType::Bitset (returning the correct tag_filter_bitmap / tag_filter_bitset names from registration_tags.hpp) and add an unconditional constexpr static_assert(false, "...") (or a dependent false trick) as the final branch in each template to produce a compile-time error for any unhandled enum value so missing cases cannot silently compile.
♻️ Duplicate comments (1)
docs/source/jit_lto_guide.md (1)
746-748:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFRAGMENT_TAG_HEADER_FILES angle-bracket requirement is too strict (duplicate).
Line 747 still says
FRAGMENT_TAG_HEADER_FILES“should be enclosed in</>”. This was already flagged previously as overly strict; please reword to indicate that both quoted paths and<...>-style includes are acceptable and handled by the CMake helper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/source/jit_lto_guide.md` around lines 746 - 748, Update the wording for FRAGMENT_TAG_HEADER_FILES to remove the strict angle-bracket requirement and state that both quoted paths ("path/to/header.h") and angle-bracket includes (<path/to/header.h>) are accepted and handled by the CMake helper; replace the phrase “should be enclosed in `<`/`>`” with a short sentence indicating both forms are supported and adjust any nearby examples to show one quoted and one angle-bracket form so readers see both accepted formats.
🧹 Nitpick comments (1)
docs/source/jit_lto_guide.md (1)
572-589: ⚡ Quick winAdd explicit “unsupported type” handling in tag helpers.
get_data_type_tag,get_idx_type_tag, andget_out_type_taguseif constexprwithout anelse/fallback (Line 572-589). If a new matrix entry introduces a type not covered by the helper, this will produce a confusing “no return statement” style compile error.Consider adding a compile-time fallback (e.g.,
static_assert(false, ...)) or a clear default branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/source/jit_lto_guide.md` around lines 572 - 589, Add an explicit compile-time fallback for unsupported types in the tag helpers: in get_data_type_tag, get_idx_type_tag, and get_out_type_tag add an else branch that triggers a clear static_assert (using a dependent false helper like template<typename> inline constexpr bool always_false = false;) with a descriptive message (e.g., "unsupported data type in get_data_type_tag") so the compiler emits a readable error instead of a "no return" message; update each helper to use the always_false<T> pattern in the final branch to fail compilation for unknown types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/source/jit_lto_guide.md`:
- Around line 484-507: The registration_tags.hpp file is missing tags used by
the example device fragments; add definitions for tag_metric_inner_product and
tag_filter_bitset alongside the existing tag_metric_euclidean and
tag_filter_none, so the template mappings (e.g., fragment_tag_compute_distance
and fragment_tag_filter) can resolve the device fragments such as
compute_distance_inner_product.cuh and filter_bitset.cuh; ensure the new symbols
are declared as simple empty structs (e.g., struct tag_metric_inner_product{};
struct tag_filter_bitset{};) so Step 7’s tag mapping supports inner_product +
filter_bitset cases.
---
Outside diff comments:
In `@docs/source/jit_lto_guide.md`:
- Around line 623-627: The doc has a method-name mismatch: Step 6 defines
add_compute_distance_function and add_filter_function but Step 7 calls
planner.add_compute_distance_device_function and
planner.add_filter_device_function which will not compile; update the Step 7
calls to use planner.add_compute_distance_function<metric_tag, data_tag>() and
planner.add_filter_function<filter_tag, idx_tag>() (or alternatively rename the
Step 6 definitions to include the "_device_" suffix) so the method names match
across the guide.
- Line 517: Update the wording to replace references to user-provided “.cpp.in
registration files” with the auto-generated registration source emitted by
generate_jit_lto_kernels(); specifically state that fragment keys must match
exactly the keys used in the auto-generated registration .cpp produced by
generate_jit_lto_kernels() (as described in Step 5) so readers know the
registration code is generated automatically rather than coming from .cpp.in
templates.
- Around line 591-600: get_metric_tag() and get_filter_tag() must be corrected
to cover real enum values and fail at compile time for unhandled cases: update
get_metric_tag() to check for DistanceType::InnerProduct (or add branches for
any other real DistanceType values present) and return the exact tag structs
defined in registration_tags.hpp (e.g., tag_metric_inner_product if that’s the
name); update get_filter_tag() to handle FilterType::Bitmap and
FilterType::Bitset (returning the correct tag_filter_bitmap / tag_filter_bitset
names from registration_tags.hpp) and add an unconditional constexpr
static_assert(false, "...") (or a dependent false trick) as the final branch in
each template to produce a compile-time error for any unhandled enum value so
missing cases cannot silently compile.
---
Duplicate comments:
In `@docs/source/jit_lto_guide.md`:
- Around line 746-748: Update the wording for FRAGMENT_TAG_HEADER_FILES to
remove the strict angle-bracket requirement and state that both quoted paths
("path/to/header.h") and angle-bracket includes (<path/to/header.h>) are
accepted and handled by the CMake helper; replace the phrase “should be enclosed
in `<`/`>`” with a short sentence indicating both forms are supported and adjust
any nearby examples to show one quoted and one angle-bracket form so readers see
both accepted formats.
---
Nitpick comments:
In `@docs/source/jit_lto_guide.md`:
- Around line 572-589: Add an explicit compile-time fallback for unsupported
types in the tag helpers: in get_data_type_tag, get_idx_type_tag, and
get_out_type_tag add an else branch that triggers a clear static_assert (using a
dependent false helper like template<typename> inline constexpr bool
always_false = false;) with a descriptive message (e.g., "unsupported data type
in get_data_type_tag") so the compiler emits a readable error instead of a "no
return" message; update each helper to use the always_false<T> pattern in the
final branch to fail compilation for unknown types.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 89772680-1e08-4885-b881-820844cdcbfd
📒 Files selected for processing (1)
docs/source/jit_lto_guide.md
|
/merge |
3 participants
No description provided.