-
Notifications
You must be signed in to change notification settings - Fork 974
Allow multiple calls to cudf::initialize
and cudf::deinitialize
#20111
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
base: branch-25.12
Are you sure you want to change the base?
Conversation
Removed comments related to environment variable flags.
Removed comments for clarity in context tests.
get_context_ptr_ref() != nullptr, "context has already been deinitialized", std::runtime_error); | ||
get_context_ptr_ref().reset(); | ||
} | ||
void deinitialize() { get_context_ptr_ref().reset(); } |
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.
If this is called before initialization, deref of nullptr will happen! So keeping the check CUDF_EXPECTS(get_context_ptr_ref() != nullptr
seems better to inform the user about this error.
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.
We're not dereferencing the unique_ptr
here. When the context is not initialized, get_context_ptr_ref
returns a reference to an empty unique_ptr
and we call reset
on this object, which is well-defined. Anything that includes get_context_ptr_ref->
would be ill-formed.
cudf::deinitialize(); | ||
|
||
EXPECT_NO_THROW(cudf::deinitialize()); | ||
} |
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.
What would the expected behavior be if we deinitialize before initialization?
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.
Current behavior is to return without any real impact, since the function will call reset
on an already empty unique_ptr
. I added a test for this.
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.
apparently I clicked a wrong button and my comments are a part of a review 🤦
get_context_ptr_ref() != nullptr, "context has already been deinitialized", std::runtime_error); | ||
get_context_ptr_ref().reset(); | ||
} | ||
void deinitialize() { get_context_ptr_ref().reset(); } |
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.
We're not dereferencing the unique_ptr
here. When the context is not initialized, get_context_ptr_ref
returns a reference to an empty unique_ptr
and we call reset
on this object, which is well-defined. Anything that includes get_context_ptr_ref->
would be ill-formed.
cudf::deinitialize(); | ||
|
||
EXPECT_NO_THROW(cudf::deinitialize()); | ||
} |
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.
Current behavior is to return without any real impact, since the function will call reset
on an already empty unique_ptr
. I added a test for this.
/ok to test b876505 |
/ok to test 73b86f7 |
Description
Closes #20022
Allow
cudf::initialize
to incrementally perform initialization steps if called multiple times. Also allow multiple calls tocudf::deinitialize
. In both cases, redundant calls have no effect.Also adds tests for this utility, since the logic is now non-trivial.
Checklist