Unify cuDF operators with a common base class architecture#16934
Unify cuDF operators with a common base class architecture#16934coreylammie wants to merge 3 commits intofacebookincubator:mainfrom
Conversation
meta-cla
bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
coreylammie
requested review from
bdice,
devavret,
karthikeyann and
mhaseeb123
as code owners
devavret
left a comment
devavret
left a comment
There was a problem hiding this comment.
Thanks for the contribution. Mostly the thing that sticks out to me is the possibility of seeing the same operator name in nvtx ranges.
| inline nvtx3::event_attributes create_nvtx_attributes(const std::string& category, const std::string& tag) { | ||
| std::hash<std::string> hasher; | ||
| nvtx3::named_category_in<facebook::velox::cudf_velox::VeloxDomain> category_{ | ||
| static_cast<uint32_t>(hasher(category)), | ||
| category.c_str() | ||
| }; | ||
| return nvtx3::event_attributes{tag, category_}; | ||
| } |
There was a problem hiding this comment.
What is this being used for? I couldn't find any reference to it in this PR or in nvtx headers
| if (CudfConfig::getInstance().debugEnabled) { | ||
| VLOG(2) << "Calling " << className_ << "::addInput"; | ||
| } | ||
| VELOX_NVTX_OPERATOR_FUNC_RANGE(); |
There was a problem hiding this comment.
You can add a new one called VELOX_NVTX_OPERATOR_FUNC_RANGE_IF() modeled after NVTX3_V1_FUNC_RANGE_IF(). With it, you can put the condition inside the macro and have a single doAddInput call
There was a problem hiding this comment.
I think the macro won't able to serve us now. I think it'll capture the base class' name now? We want to see CudfHashAggregation::addInput and not CudfOperatorBase::addInput. please correct me if i'm wrong.
| operatorId, | ||
| fmt::format("[{}]", topNNode->id())), | ||
| "CudfTopN", | ||
| nvtx3::rgb{255, 140, 0}, // Dark Orange |
There was a problem hiding this comment.
nit: try to retain the previous colors. We've sort of gotten used to them :)
| className_(operatorName), | ||
| nvtxMethods_(nvtxMethods) {} | ||
|
|
||
| void addInput(RowVectorPtr input) override { |
There was a problem hiding this comment.
I think you'd also want to mark them final instead of override
| operatorName, | ||
| spillConfig), | ||
| NvtxHelper( | ||
| color.value_or(nvtx3::rgb{160, 82, 45}), |
There was a problem hiding this comment.
how about we let NvtxHelper manage the default.
2 participants
Corresponding PR for #16885.