-
Notifications
You must be signed in to change notification settings - Fork 47
Context builder #535
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Context builder #535
Conversation
cpp/include/ucxx/context.h
Outdated
| /** | ||
| * @brief Create a `shared_ptr<ucxx::Context>` with default parameters. | ||
| * | ||
| * This function immediately constructs and returns a `Context` with default parameters. | ||
| * To customize parameters, use the builder pattern by chaining methods on the | ||
| * returned builder. | ||
| * | ||
| * @code{.cpp} | ||
| * // Default context | ||
| * auto context = ucxx::createContext(); // Returns shared_ptr<Context> | ||
| * | ||
| * // Customized context using builder pattern | ||
| * auto context = ucxx::createContext() | ||
| * .configMap({{"TLS", "tcp"}}) | ||
| * .featureFlags(UCP_FEATURE_TAG); // Returns shared_ptr<Context> | ||
| * @endcode | ||
| * | ||
| * @return A `ContextBuilder` object that implicitly converts to `shared_ptr<Context>`. | ||
| */ | ||
| inline ContextBuilder createContext() { return ContextBuilder(); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This docstring is very confused. This definitely doesn't return a ucxx::Context.
I see that you've got an implicit conversion operator to shared_ptr<Context> but I am not sure that is the best?
I think we want a distinction between creating a context and creating a builder.
What about, instead?
// Make a context with everything up front
auto ctx = ucxx::createContext(config_options, feature_flags);
// Get a context builder via static method on `Context`.
auto builder = ucxx::Context::builder();
// Set flags and build
auto ctx = builder.configMap(...).featureFlags(...).build();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a definition perspective, you're right. The point was to show that ucxx::createContext() is still a valid way to create a context as previously just by calling the ucxx::createContext(). I think I would still prefer the method we have here, that we can call ucxx::createContext().[...].build() and that will continue to work as previously, with the benefit of allowing us to pass the parameters, and this way it will be less of a hassle to migrate.
Do you see problems with the approach I've taken here? Would just improving docs suffice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I buy the argument for migration being less painful. I guess one might ask if it makes sense to have a default-configured context.
I think maybe improve the docs a little?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @wence- , after giving it some more thought, I agree, maybe it's best not to have a default-configured/default-constructable context, at least we should make the user specify the feature flags needed. I've made those changes in 7d0d4f7 along with some hopefully improved docs. Additionally, I think it's good to put all changes in a ucxx::experimental namespace to prevent new users from accidentally using the builder-pattern constructor while we're still potentially making big changes, this change was done in a360734, and I think once we've covered all classes (or at least the more important ones) and are confident this makes sense we can move them into the regular ucxx namespace.
Let me know if you think this is better or if you still see room for improvement.
|
Thanks @wence- ! |
|
/merge |
bdice
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CMake approval.
Continuation of #435 and follow-up to #535, adds a new `ucxx::WorkerBuilder` to be used for `ucxx::Worker` construction allowing to specify only the options desired. For now, this change is backwards-compatible but we may want to remove the constructors at some point and break the API once to do that (to be discussed). The Python implementation is not adapted to the new constructor because it uses `ucxx::python::Worker` specialization of `ucxx::Worker` that will be covered in a follow-up PR. Authors: - Peter Andreas Entschev (https://github.com/pentschev) Approvers: - Mads R. B. Kristensen (https://github.com/madsbk) - https://github.com/jakirkham - Bradley Dice (https://github.com/bdice) URL: #544
As discussed in #435 we need better constructors in libucxx to allow specifying only the options necessary to match the user's needs. The current constructors are bad because adding new parameters will be necessarily API breaking and forces the user to specify all the parameters even if the default values for some are desired. The new constructor proposed in this change (starting with a change only to
ucxx::Contextclass) adds a newucxx::ContextBuilderallowing to specify only the options desired. For example, the following should all work:For now, this change is backwards-compatible but we may want to remove the constructors at some point and break the API once to do that (to be discussed).
The Python implementation is also adapted to the new constructor to show that this is also adaptable.