Better C++ API for constructors (and methods that return new `ucxx` objects)

[pentschev]

Summary

I'm getting a bit concerned and annoyed with passing parameters to constructors. Take for example ucxx::createRequestTag:

[[nodiscard]] std::shared_ptr<RequestTag> createRequestTag(
  std::shared_ptr<Component> endpointOrWorker,
  const std::variant<data::TagSend, data::TagReceive> requestData,
  const bool enablePythonFuture,
  RequestCallbackUserFunction callbackFunction,
  RequestCallbackUserData callbackData);

The above is generally not called directly by the user, and the user normally would call ucxx::Endpoint::tag{Send,Recv}(...) as a proxy, so we would have to implement something similar for those calls just like to the direct constructors. For simplicity, I'll focus solely on the constructor createRequestTag here.

First, the constructor should have defined defaults for enablePythonFuture/callbackFunction/callbackData like in the private constructor and tagSend/tagRecv, which have been missed and is a minor issue, but still an inconsistency.

I'll soon need to introduce another parameter to the requests allowing user to query for UCP debug information for each request, and thus another constructor argument is necessary. What I would prefer to have is some API where users don't have to remember order of arguments and is less prone to API/ABI breakage and to avoid introducing yet another argument. For that, I came up with 3 suggestions below.

Suggestion 1: Using a struct

This is the simplest, and would look something as follows:

struct RequestTagOptions {
  std::shared_ptr<Component> endpointOrWorker;
  std::variant<data::TagSend, data::TagReceive> requestData;
  bool enablePythonFuture{false};
  RequestCallbackUserFunction callbackFunction{nullptr};
  RequestCallbackUserData callbackData{nullptr};
};

class RequestTag : public Request {
...
  friend std::shared_ptr<RequestTag> createRequestTag(
    RequestTagOptions options);
...
};

The caller could simply construct a new request as follows:

// Only required arguments
auto req = createRequestTag(
  RequestTagOptions{
    .endpointOrWorker = endpoint,
    .requestData = data::TagSend(buffer, length, tag)});

// Required arguments + optional callback
auto req = createRequestTag(
  RequestTagOptions{
    .endpointOrWorker = endpoint,
    .requestData = data::TagSend(buffer, length, tag),
    .callbackFunction = callback});

This is very straightforward but it has at least one annoying shortcoming: the user is not required to set endpointOrWorker/requestData and that is valid at compile-time, we could introduce runtime checks but that's IMO suboptimal. Alternatively, we could provide a constructor, and it would then look like:

struct RequestTagOptions {
...
  RequestTagOptions(
    std::shared_ptr<Component> endpointOrWorker,
    std::variant<data::TagSend, data::TagReceive> requestData);
};

But that makes it also annoying for the caller, since then specifying non-default values become cumbersome because it now requires multiple statements when a non-default option needs to be specified:

// Only required arguments
auto opts = RequestTagOptions(endpoint, data::TagSend(buffer, length, tag));
opts.callbackFunction = callback;
auto req = createRequestTag(opts);

And if we now choose to add other options to constructors we would be back with the initial problem that we now need multiple and/or lengthy constructors.

Suggestion 2: Nested structs

This is similar to Suggestion 1, but it would allow for some more flexibility to manage the construction of non-default values:

struct RequestTagOptionsRequired {
  std::shared_ptr<Component> endpointOrWorker;
  std::variant<data::TagSend, data::TagReceive> requestData;

  RequestTagOptionsRequired(
    std::shared_ptr<Component> endpointOrWorker,
    std::variant<data::TagSend, data::TagReceive> requestData);
};

struct RequestTagOptions {
  RequestTagOptionsRequired required;
  bool enablePythonFuture{false};
  RequestCallbackUserFunction callbackFunction{nullptr};
  RequestCallbackUserData callbackData{nullptr};
};

class RequestTag : public Request {
...
  friend std::shared_ptr<RequestTag> createRequestTag(
    RequestTagOptions options);
...
};

And the user would be able to construct in a single-statement:

auto req = createRequestTag(
  RequestTagOptions{
    .required = RequestTagOptionsRequired(
      endpoint,
      data::TagSend(buffer, length, tag)),
    .callback = callback);

The advantage is we can still catch errors at compile-time, but major downside of this approach is it gets more verbose, both in the UCXX implementation and user-side, which is not great.

Suggestion 3: class with multiple setters

class RequestTagOptions {
  std::shared_ptr<Component> endpointOrWorker;
  std::variant<data::TagSend, data::TagReceive> requestData;
  bool enablePythonFuture{false};
  RequestCallbackUserFunction callbackFunction{nullptr};
  RequestCallbackUserData callbackData{nullptr};

public:
  RequestTagOptions(
    std::shared_ptr<Component> endpointOrWorker,
    std::variant<data::TagSend, data::TagReceive> requestData);

  RequestTagOptions& enablePythonFuture();
  RequestTagOptions& callbackFunction(RequestCallbackUserFunction callback);
  RequestTagOptions& callbackData(RequestCallbackUserData callbackData);
};

class RequestTag : public Request {
...
  friend std::shared_ptr<RequestTag> createRequestTag(
    RequestTagOptions options);
...
};

The user could then construct a new request calling:

auto req = createRequestTag(
  RequestTagOptions(
    endpoint,
    data::TagSend(buffer, length, tag))
    .callback(callback));

The downside I see, and that's probably just a matter of preference, is that this looks the ugliest and possibly less intuitive for users.

Suggestion 4: Separate Required/Optional structs

Similar to Suggestion 2, but separate Required and Optional entities as separate arguments:

// In request_tag.h
struct RequestTagParams {
  struct Required {
    std::shared_ptr<Component> endpointOrWorker;
    std::variant<data::TagSend, data::TagReceive> requestData;
    
    Required(
      std::shared_ptr<Component> ep,
      std::variant<data::TagSend, data::TagReceive> data)
      : endpointOrWorker(std::move(ep))
      , requestData(std::move(data)) {}
  };
  
  struct Optional {
    bool enablePythonFuture{false};
    RequestCallbackUserFunction callbackFunction{nullptr};
    RequestCallbackUserData callbackData{nullptr};
    bool enableDebugInfo{false}; // New parameter
    
    Optional() = default;
  };
};

std::shared_ptr<RequestTag> createRequestTag(
  RequestTagParams::Required required,
  RequestTagParams::Optional optional = {});

Allows clean argument specification:

// Required arguments only
auto req1 = createRequestTag({endpoint, data::TagSend(buffer, length, tag)});

// Required and optional arguments
auto req2 = createRequestTag(
  {endpoint, data::TagSend(buffer, length, tag)},
  {.enablePythonFuture = true, .enableDebugInfo = true}
);

The only con I can think of for this suggestion is the fact that two arguments need to be passed when specifying optional arguments, but that is looking cleaner than any of the other alternatives to me now.

Conclusion

All suggestions have pros and cons, and although suggestion 3 is IMO the ugliest and less intuitive I'm at this moment leaning more towards that (scratched in favor of suggestion 4, see UPDATE below). I'm interested in hearing opinions and possibly alternative ideas.

@madsbk @wence- WDYT?

UPDATE: I added Suggestion 4, which now seems to be a better and cleaner option than 3.

[pentschev]

Exploring a bit more, I added a new Suggestion 4 to the description now.

[madsbk]

I common approach is to use a builder pattern where you can chain parameters like:

MyObject obj = MyObjectBuilder()
    .setArg1(42)
    .setArg2("example")
    .setArg3(3.14)
    .build();

And .build() construct the class. Alternatively, you can skip .build() and pass the class instance to a function like createRequestTag.

[madsbk]

As an alternative to Suggestion 4, instead of two separate arguments, you can require wrapping non-optional arguments in a Required class like:

// Required arguments + optional callback
auto req = createRequestTag(
  RequestTagOptions{
    .endpointOrWorker = Required(endpoint),
    .requestData = data::TagSend(buffer, length, tag)),
    .callbackFunction = callback
  }
);

[pentschev]

I common approach is to use a builder pattern where you can chain parameters like:

Yes, that is suggestion 3, should have named it "builder pattern" there for clarity. But do you think this is a better/clearer approach than suggestion 2/4? I'm a bit over the edge about it, personally I find it to look weird, in particular when it comes to methods like tagSend/tagRecv, which will need to follow a similar approach to whatever we choose for constructors.

As an alternative to Suggestion 4, instead of two separate arguments, you can require wrapping non-optional arguments in a Required class like:

That would be the same as suggestion 2, I think, no?

[madsbk]

Ah yes, I guess they are covered in your suggestions :)

[madsbk]

Overall, I think all the approaches look a bit ugly :)

I have a slight preference for suggestion 2.

[pentschev]

Overall, I think all the approaches look a bit ugly :)

I have to agree with that. Do you have any ideas that would look less ugly?

I have a slight preference for suggestion 2.

Yeah, I think that might in the end be the less unintuitive.

[pentschev]

I guess another alternative would be using templates for SFINAE. I put up a PR #436 containing a sample of that, it doesn't compile currently due to circular imports that do not properly resolve constructors. I think this is a nice alternative from the user's perspective because it would allow arbitrary arguments and ordering, like:

  // Send using named parameters in arbitrary order
  requests.push_back(_ep->tagSend(ucxx::request_tag_params::RequestDataParam{ucxx::data::TagSend{
                                    _sendPtr[0], _messageSize, ucxx::Tag{0}}},
                                  ucxx::request_tag_params::EndpointParam{endpoint},
                                  ucxx::request_tag_params::EnablePythonFutureParam{false}));

  // Receive using named parameters in different order
  requests.push_back(_ep->tagRecv(ucxx::request_tag_params::EnablePythonFutureParam{false},
                                  ucxx::request_tag_params::EndpointParam{endpoint},
                                  ucxx::request_tag_params::RequestDataParam{ucxx::data::TagReceive{
                                    _recvPtr[0], _messageSize, ucxx::Tag{0}, ucxx::TagMaskFull}}));

From a maintenance cost perspective I'm not quite sure about it, it requires a lot more boilerplate code than suggestions discussed previously.

WDYT @madsbk ?

[madsbk]

From a maintenance cost perspective I'm not quite sure about it, it requires a lot more boilerplate code than suggestions discussed previously.

Agreed, this can be hard to read. If we move to C++20, it might become a bit easier to maintain.
Still, I think I prefer Suggestion 2 or 3.

The nice thing about Suggestion 3 (the builder pattern) is that it’s a well-known approach,
and you can link to documentation that explains it.

[pentschev]

Thanks for all the feedback, Mads! I'll think a bit more about this during the week and try to put up a PoC PR.