Unify cuDF operators with a common base class architecture

[coreylammie]

Description

I would like to make a PR to unify the cuDF operators with a common base class.

Motivation

Currently, cuDF operators (CudfTopN, CudfLimit, CudfOrderBy, etc.) don't extend from a common base class. Each operator directly extends exec::Operator and NvtxHelper, leading to:

• Code duplication (across operators)
• Inconsistent debug logging patterns (some operators log, others don't)
• No common type for passing operators - meaning, e.g., no direct access to NVTX/cuDF features without casting
• Difficult to enforce consistent behavior across operators

Proposed Solution

Introduce a unified base class architecture with two complementary classes:

1. CudfOperator - Lightweight base for user-defined GPU operators:

• Extends NvtxHelper only, based on feat(cudf): Support user defined gpu operator #15375.

2. CudfOperatorBase - Comprehensive base for built-in operators:

• Extends from both CudfOperator and exec::Operator.
• Implements template method pattern: overrides addInput(), getOutput(), noMoreInput(), and close() to call corresponding do* methods (doAddInput(), doGetOutput(), doNoMoreInput(), doClose())
• Each wrapper method adds VELOX_NVTX_OPERATOR_FUNC_RANGE() profiling and VLOG(2) debug logging (guarded by CudfConfig::getInstance().debugEnabled)
• Derived operators override only the do* methods they need (all have default implementations)
• Enforces consistent NVTX profiling and debug logging across all built-in cuDF operators

[zoltan]

@karthikeyann @devavret

[devavret]

Enforces consistent NVTX profiling and debug logging across all built-in cuDF operators

I like this. My only (unfounded) concern is that this may pollute the nsys profiles with ranges for functions that aren't doing anything. Most operators don't do anything in the doNoMoreInput(), doClose().

CudfOperator - Lightweight base for user-defined GPU operators
CudfOperatorBase - Comprehensive base for built-in operators

Can these be merged? I don't see the need for the distinction between user defined CudfOperator and "in-built" CudfOperatorBase. All cuDF operators can extend CudfOperator and both things can be true:

• The driver adapter can simply skip over the ones that extend CudfOperator
• CudfOperator defines pure virtual do*() calls.

[coreylammie]

I like this. My only (unfounded) concern is that this may pollute the nsys profiles with ranges for functions that aren't doing anything. Most operators don't do anything in the doNoMoreInput(), doClose().

Good point! For this, would you have any objection to adding something along the lines of NvtxMethodFlags enabledMethods = NvtxMethodFlags::kAll as an additional argument to the constructor of the (new) base class?

The new enum could be defined as follows:

enum NvtxMethodFlag : uint8_t {
  kNone = 0,
  kAddInput = 1 << 0,
  kGetOutput = 1 << 1,
  kNoMoreInput = 1 << 2,
  kClose = 1 << 3,
  kAll = kAddInput | kGetOutput | kNoMoreInput | kClose,
};

Can these be merged? I don't see the need for the distinction between user defined CudfOperator and "in-built" CudfOperatorBase. All cuDF operators can extend CudfOperator and both things can be true:

• The driver adapter can simply skip over the ones that extend CudfOperator
• CudfOperator defines pure virtual do*() calls.

Sure, my only concern here would be that https://github.com/facebookincubator/velox/blob/main/velox/experimental/cudf/exec/CudfOperator.h#L27-L37 doesn't extend from exec::Operator. Having two separate base classes retains backwards compatibility with CudfOperator.

Adding virtual do*() calls makes perfect sense - I think aside from doAddInput() and doGetOutput(), with defaults would be better, though, e.g.,

virtual void doNoMoreInput() {
    Operator::noMoreInput();
}

[devavret]

Having two separate base classes retains backwards compatibility with CudfOperator.

We're a pretty young project and hopyfully @jinchengchenghh will not mind changing over. It would require some tandem PRs in gluten to not break their usage of this but I don't know of any other project that makes use if this custom CudfOperator

[jinchengchenghh]

CudfOperator: Operator may cause some issue, so I agree we need two separate class for compatibility.
Now in Gluten usage, and SourceOperator is a subclass of Operator, it may meets ambiguous issue when CudfValueStream access the Operator member.
class CudfValueStream : public facebook::velox::exec::SourceOperator, public facebook::velox::cudf_velox::CudfOperator

[coreylammie]

CudfOperator: Operator may cause some issue, so I agree we need two separate class for compatibility. Now in Gluten usage, and SourceOperator is a subclass of Operator, it may meets ambiguous issue when CudfValueStream access the Operator member. class CudfValueStream : public facebook::velox::exec::SourceOperator, public facebook::velox::cudf_velox::CudfOperator

OK great. @devavret @jinchengchenghh before I make a PR, more concretely, this is what I currently have: https://github.com/coreylammie/velox/blob/cudf_operator_nvtx_tagging/velox/experimental/cudf/exec/CudfOperator.h. Happy to address further specific concerns in the PR, but first, do you both agree on the base class architecture?

Thanks!

[devavret]

@jinchengchenghh There is a new OperatorAdapter class added since adding CudfOperator. With this, you can define operators which should be retained and whether they take gpu input and produce gpu output. Instead of CudfValueStream inheriting from CudfOperator, you can register your own CudfValueStreamAdapter to signal to cudf driver adapter that CudfValueStream produces gpu output and should not be replaced.

With this, @coreylammie can implement CudfOperatorBase that doesn't inherit CudfOperator. Does this suit everyone?

[jinchengchenghh]

Yes, I read OperatorAdapter, I agree CudfOperatorBase that doesn't inherit CudfOperator

[coreylammie]

Created the PR here: #16934.

[karthikeyann]

I have a few different viewpoints on the problems described:

Each operator directly extends exec::Operator and NvtxHelper

This can be addressed by introducing a single CudfOperatorBase that combines the constructor arguments of both exec::Operator and NvtxHelper.

Additionally, fmt::format("[{}]") can removed in cudf operators if they inherited from CudfOperatorBase.

---

Inconsistent debug logging patterns (some operators log, others don't)

During initial development, debug logging was useful, but it is no longer required. We have already removed most of it.

If needed, it is reasonable to keep logging in individual operators until they mature. However, a better long-term approach would be to have Velox provide a consistent debug logging mechanism for all operators, not just cuDF operators.

---

No common type for passing operators (no direct access to NVTX/cuDF features without casting)

The proposed PR does not fully solve both of the problems. It adds NVTX profiling for these methods, does still no access to NVTX & Cudf features inside those methods.

Introducing virtual methods in CudfOperatorBase adds another layer of indirection and increases complexity. Developers now need to understand both the CPU operator interface and the cuDF operator interface.

While I understand the intent (profiling), Renaming all Velox methods to do*() just to enable profiling feels like a workaround. The actual solution should likely be simpler and implemented at a different layer. This area needs more discussion.
I have strong reservations about the do* renaming approach. I’d prefer we explore other alternatives before committing to this direction. Happy to collaborate on finding a simpler solution.

---

Suggestions

• CudfOperator.h

  • Add exec::Operator as a base class
  • Provide a protected constructor that forwards all parameters

• Core cuDF operators (e.g., CudfLimit, CudfTopN, etc.)

  • Replace inheritance from exec::Operator and NvtxHelper with CudfOperator
  • Simplify constructor initialization lists

• ToCudf.cpp::getOperatorProperties

  • Change unconditional CudfOperator override to else if (adapter should take priority)

• consider dropping the do* renaming idea for now and continue discussion on NVTX and debug logging

[coreylammie]

I have a few different viewpoints on the problems described:

Each operator directly extends exec::Operator and NvtxHelper

This can be addressed by introducing a single CudfOperatorBase that combines the constructor arguments of both exec::Operator and NvtxHelper.

Additionally, fmt::format("[{}]") can removed in cudf operators if they inherited from CudfOperatorBase.

OK great, we are all in agreement adding a baseCudfOperatorBase class in some form is a good idea.

Inconsistent debug logging patterns (some operators log, others don't)

During initial development, debug logging was useful, but it is no longer required. We have already removed most of it.

If needed, it is reasonable to keep logging in individual operators until they mature. However, a better long-term approach would be to have Velox provide a consistent debug logging mechanism for all operators, not just cuDF operators.

I don't mind this, but I would still prefer it to be configurable as if it's inconsistent, trying to automatically parse the log files etc. becomes problematic.

Introducing virtual methods in CudfOperatorBase adds another layer of indirection and increases complexity. Developers now need to understand both the CPU operator interface and the cuDF operator interface.

While I understand the intent (profiling), Renaming all Velox methods to do*() just to enable profiling feels like a workaround. The actual solution should likely be simpler and implemented at a different layer. This area needs more discussion. I have strong reservations about the do* renaming approach. I’d prefer we explore other alternatives before committing to this direction. Happy to collaborate on finding a simpler solution.

What about the following:

• Move profiling instrumentation to the Driver layer. The Driver's CALL_OPERATOR macro already wraps operator calls with timing and exception context. NVTX profiling could be added at the same layer, rather than the callee wrapping itself via do*() indirection.

• Make CudfOperatorBase a thin utility base (no method renaming).
  Instead of the template method pattern, CudfOperatorBase can provide cuDF context to subclasses via protected accessors:

class CudfOperatorBase : public exec::Operator {
 protected:
  // cuDF utilities available to all cuDF operators
  rmm::cuda_stream_view cudfStream() const;
  rmm::device_async_resource_ref memoryResource() const;
  int64_t preferredBatchSize() const;

  // NVTX helper for sub-ranges *inside* methods
  NvtxHelper& nvtx();

 private:
  NvtxHelper nvtxHelper_;
};

Operators would then inherit normally and override addInput() / getOutput() directly:

class CudfHashJoin : public CudfOperatorBase {
  void addInput(RowVectorPtr input) override {
    auto stream = cudfStream();                        // cuDF context access
    auto range = nvtx().scopedRange("hash_build");     // sub-range profiling
    // ... build phase ...
  }
};

[devavret]

Move profiling instrumentation to the Driver layer. The Driver's CALL_OPERATOR macro already wraps operator calls with timing and exception context. NVTX profiling could be added at the same layer, rather than the callee wrapping itself via do*() indirection.

This does fit the velox style and I'm thinking if it's possible.
The withDeltaCpuWallTimer function where you're thinking of adding this needs access to the things we need for constructing the range name which looks like this: "CudfHashAggregation::addInput [9]". It has access to two of them (operatorid, plannodeid) but not the third (operator method). @coreylammie I'd also like to bring this issue to your attention because this looks related: #16722

But I'm apprehensive about this for a different reason: Core velox changes (outside of experimental/cudf) generally receives much more pushback than changes touching cudf specific code. So must be certain this is absolutely the best way to implement this.

Make CudfOperatorBase a thin utility base (no method renaming).
Instead of the template method pattern, CudfOperatorBase can provide cuDF context to subclasses via protected accessors:

I'm not sure this is much better or different than the status quo? Besides, cudf operators mostly don't store a stream and operate on the CudfVector's stored stream.

Renaming all Velox methods to do*() just to enable profiling feels like a workaround.

This also has other benefits: We can put common operations inside CudfOperatorBase like addInput can check if its input is a CudfVector. More can come up later as we build out.