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.

Sign up for GitHub

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

Jump to bottom

Enable prefetching in cudf.pandas.install() #16439

Merged
merged 2 commits into from
Jul 31, 2024
Merged

Enable prefetching in cudf.pandas.install() #16439

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
26aadf7
Enable prefetching in cudf.pandas.install()
bdice Jul 30, 2024
ed38fbf
Fix prefetch bug for uninitialized strings columns.
bdice Jul 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion cpp/src/column/column_view.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,10 @@ void prefetch_col_data(ColumnView& col, void const* data_ptr, std::string_view k
key, data_ptr, col.size() * size_of(col.type()), cudf::get_default_stream());
} else if (col.type().id() == type_id::STRING) {
strings_column_view scv{col};

if (data_ptr == nullptr) {
// Do not call chars_size if the data_ptr is nullptr.
return;
}
Comment on lines +48 to +51
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a bug. We can't call chars_size on string column views that are not fully initialized like this one:

cudf/cpp/src/column/column_factories.cu

Lines 60 to 62 in 8def2ec

// Since we are setting every row to the scalar, the fill() never needs to access
// any of the children in the strings column which would otherwise cause an exception.
column_view sc{value.type(), size, nullptr, nullptr, 0};

Errors from calling chars_size() looked like this:

terminate called after throwing an instance of 'cudf::logic_error'
  what():  CUDF failure at: /home/coder/cudf/cpp/src/strings/strings_column_view.cpp:34: strings column has no children
Aborted (core dumped)

cudf::experimental::prefetch::detail::prefetch_noexcept(
key,
data_ptr,
Expand Down
14 changes: 14 additions & 0 deletions cpp/src/utilities/prefetch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,20 @@ cudaError_t prefetch_noexcept(std::string_view key,
rmm::cuda_stream_view stream,
rmm::cuda_device_id device_id) noexcept
{
// Don't try to prefetch nullptrs or empty data. Sometimes libcudf has column
// views that use nullptrs with a nonzero size as an optimization.
if (ptr == nullptr) {
if (prefetch_config::instance().debug) {
std::cerr << "Skipping prefetch of nullptr" << std::endl;
}
return cudaSuccess;
}
if (size == 0) {
if (prefetch_config::instance().debug) {
std::cerr << "Skipping prefetch of size 0" << std::endl;
}
return cudaSuccess;
}
Comment on lines +54 to +67
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really sure if this is necessary or not, but it seems like it would improve our library safety to avoid calling the cudaMemPrefetchAsync API unless we know we have a non-null pointer and non-zero size.

if (prefetch_config::instance().get(key)) {
if (prefetch_config::instance().debug) {
std::cerr << "Prefetching " << size << " bytes for key " << key << " at location " << ptr
Expand Down
19 changes: 17 additions & 2 deletions python/cudf/cudf/pandas/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@

import rmm.mr

from cudf._lib import pylibcudf

from .fast_slow_proxy import is_proxy_object
from .magics import load_ipython_extension
from .profiler import Profiler
Expand All @@ -16,6 +18,19 @@

LOADED = False

_SUPPORTED_PREFETCHES = {
"column_view::get_data",
"mutable_column_view::get_data",
"gather",
"hash_join",
}


def _enable_managed_prefetching(rmm_mode):
if "managed" in rmm_mode:
for key in _SUPPORTED_PREFETCHES:
pylibcudf.experimental.enable_prefetching(key)


def install():
"""Enable Pandas Accelerator Mode."""
Expand All @@ -33,7 +48,7 @@ def install():
f"cudf.pandas detected an already configured memory resource, ignoring 'CUDF_PANDAS_RMM_MODE'={str(rmm_mode)}",
UserWarning,
)
return rmm_mode
return

free_memory, _ = rmm.mr.available_device_memory()
free_memory = int(round(float(free_memory) * 0.80 / 256) * 256)
Expand All @@ -57,7 +72,7 @@ def install():
elif rmm_mode != "cuda":
raise ValueError(f"Unsupported {rmm_mode=}")
rmm.mr.set_current_device_resource(new_mr)
return rmm_mode
_enable_managed_prefetching(rmm_mode)


def pytest_load_initial_conftests(early_config, parser, args):
Expand Down
12 changes: 1 addition & 11 deletions python/cudf/cudf/pandas/__main__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,17 +72,7 @@ def main():

args = parser.parse_args()

rmm_mode = install()
if "managed" in rmm_mode:
for key in {
"column_view::get_data",
"mutable_column_view::get_data",
"gather",
"hash_join",
}:
from cudf._lib import pylibcudf

pylibcudf.experimental.enable_prefetching(key)
install()
with profile(args.profile, args.line_profile, args.args[0]) as fn:
args.args[0] = fn
if args.module:
Expand Down
Loading