Skip to content
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

[BUG] stop throwing when configuring default host mr #15814

Closed
abellina opened this issue May 22, 2024 · 0 comments · Fixed by #15815
Closed

[BUG] stop throwing when configuring default host mr #15814

abellina opened this issue May 22, 2024 · 0 comments · Fixed by #15815
Labels
bug Something isn't working

Comments

@abellina
Copy link
Contributor

In this PR #15665 we added a function config_default_host_memory_resource that takes in parameters that allow us to disable the default pinned behavior in cuDF.

I tested this manually and neglected to run the unit tests in spark. The unit tests will, in the same process, re-initialize our plugin. Since all of the storage is static in this cuDF code around the pinned resource, we wind up calling config_default_host_memory_resource multiple times, causing the exception.

We'd like to propose a change and I can put up a bug fix for this today. Instead of a config_default_host_memory_resource, we'd like to pass the host_mr_options that were being passed here, directly to set_host_memory_resource. We'd also like set_host_memory_resource to not throw if called multiple times.

Here is a high level change that I can try:

rmm::host_async_resource_ref set_host_memory_resource(rmm::host_async_resource_ref mr, host_mr_options const& default_mr_opts)
{
  std::scoped_lock lock{host_mr_mutex()};
  auto last_mr = host_mr(opts);
  host_mr()    = mr;
  return last_mr;
}

CUDF_EXPORT rmm::host_async_resource_ref& host_mr(std::optional<host_mr_options> const& opts)
{
  static rmm::host_async_resource_ref mr_ref = make_host_mr(opts);
  return mr_ref;
}

I think we can also remove the exception from make_host_mr and simplify that a bit.

I think this will unblock us and the api is cleaner: call set_host_memory_resource and pass the default options (which in our case are to always disable the default pool).

@abellina abellina added the bug Something isn't working label May 22, 2024
@abellina abellina added this to libcudf May 22, 2024
rapids-bot bot pushed a commit that referenced this issue May 23, 2024
…15815)

Closes #15814

This adds a boolean return value from `cudf::io::config_host_memory_resource` to allow the caller to handle the case where the memory resource has already been configured in the past. Before this the function would throw, forcing callers to try/catch.

Authors:
  - Alessandro Bellina (https://github.com/abellina)
  - Vukasin Milovanovic (https://github.com/vuule)

Approvers:
  - Vukasin Milovanovic (https://github.com/vuule)
  - https://github.com/nvdbaranec
  - Nghia Truong (https://github.com/ttnghia)

URL: #15815
@GregoryKimball GregoryKimball removed this from libcudf Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant