Conversation
divyegala
added
doc
dantegd
left a comment
dantegd
left a comment
There was a problem hiding this comment.
Nice PR overall, just had a few comments
| * Include ``<cuvs/neighbors/ivf_flat.hpp>`` and define a metric with ``CUVS_METRIC(MyName, { ... })``. | ||
| Set ``search_params.metric_udf`` to the string returned by ``MyName_udf()``. | ||
| * Prefer the helpers documented next to the macro (``squared_diff``, ``abs_diff``, ``dot_product``, | ||
| ``point`` element access, and so on) so the same definition works across ``float``, ``int8_t`` / |
There was a problem hiding this comment.
"and so on" kinda leaves the user guessing, almost like "left as an exercise to the reader". A user writing a custom metric needs to know exactly what's available, a brief table listing all the helpers (name, signature, description) or at minimum a cross-reference to the header line numbers where they're defined would be helpful here.
| CUVS_METRIC(my_chebyshev, { | ||
| auto d = abs_diff(x, y); | ||
| acc = (d > acc) ? d : acc; | ||
| }) |
There was a problem hiding this comment.
Small thing, but this differs from the example in ivf_flat.hpp where we don't use the helper, would be good to make them match
| } // namespace example::detail | ||
| ``` | ||
|
|
||
| ### Step 7b: Example — NVRTC UDFs for `compute_distance` and `apply_filter` |
There was a problem hiding this comment.
This is clever, however, the explanatory text leading into the code snippets is a really large wall of text that mixes:
- Architectural rationale (why NVRTC and not static fatbins)
- Implementation constraints (explicit instantiations, forwarding definitions)
- Usage instructions (which planner API to call)
Which makes it really hard to read and follow, particularly for someone that has never used NVRTC or familiar with the UDF infra at all.
Consider restructuring into something like:
- What you're building (1-2 sentences)
- Architecture diagram or flow (entry kernel → forward declarations → NVRTC TU → explicit instantiations → planner registration)
- Code with inline comments
- Pitfalls/constraints as a separate callout
| void add_metric_udf_fragment(std::unique_ptr<UDFFatbinFragment> fragment) | ||
| { | ||
| add_fragment(std::move(fragment)); | ||
| } | ||
|
|
||
| void add_filter_udf_fragment(std::unique_ptr<UDFFatbinFragment> fragment) | ||
| { | ||
| add_fragment(std::move(fragment)); | ||
| } |
There was a problem hiding this comment.
These do the same and is just semantic sugar, no? Would be good to add a comment to state so for readability and documentation.
| template <DistanceType Metric> | ||
| constexpr auto get_metric_tag() { | ||
| if constexpr (Metric == DistanceType::Euclidean) return tag_metric_euclidean{}; | ||
| else if constexpr (Metric == DistanceType::MetricUdf) return tag_metric_custom_udf{}; | ||
| } | ||
|
|
||
| template <FilterType Filter> | ||
| constexpr auto get_filter_tag() { | ||
| if constexpr (Filter == FilterType::None) return tag_filter_none{}; | ||
| else if constexpr (Filter == FilterType::FilterUdf) return tag_filter_custom_udf{}; | ||
| } |
There was a problem hiding this comment.
I think this is similar to a comment I had in the UDF infra PR, if a new enum value is added later but these functions aren't updated, the compiler will give a confusing "no return" error rather than a clear message. Adding an else { static_assert(...); } fallback would be a good idea. Since this is documentation/example code, arguably it's even more important here to model best practices.
| enum class DistanceType { Euclidean, MetricUdf }; | ||
| enum class FilterType { None, FilterUdf }; |
There was a problem hiding this comment.
| enum class DistanceType { Euclidean, MetricUdf }; | |
| enum class FilterType { None, FilterUdf }; | |
| // Extend the DistanceType / FilterType enums from Step 7: | |
| enum class DistanceType { Euclidean, MetricUdf }; | |
| enum class FilterType { None, FilterUdf }; |
Small comment that might help readability
divyegala
changed the title
2 participants
Closes #1873