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

Apply the cuFile error work around to data_sink as well #15335

Merged
merged 7 commits into from
Apr 1, 2024
Merged

Apply the cuFile error work around to data_sink as well #15335

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0763508
move ctx init to a global to cover all cases
vuule Mar 19, 2024
098e3be
Merge branch 'branch-24.04' of https://github.com/rapidsai/cudf into …
vuule Mar 19, 2024
69f3af7
too early; go back to in line calls
vuule Mar 19, 2024
773a7b1
Merge branch 'branch-24.06' into bug-cuda-ctx-always-global-init
vuule Mar 21, 2024
3ef4d32
periods.
vuule Mar 25, 2024
bcc53b1
Merge branch 'branch-24.06' into bug-cuda-ctx-always-global-init
vuule Mar 25, 2024
34c8b26
Merge branch 'branch-24.06' into bug-cuda-ctx-always-global-init
vuule Apr 1, 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
1 change: 1 addition & 0 deletions cpp/src/io/utilities/data_sink.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ class file_sink : public data_sink {
public:
explicit file_sink(std::string const& filepath)
{
detail::force_init_cuda_context();
_output_stream.open(filepath, std::ios::out | std::ios::binary | std::ios::trunc);
if (!_output_stream.is_open()) { detail::throw_on_file_open_failure(filepath, true); }

Expand Down
6 changes: 1 addition & 5 deletions cpp/src/io/utilities/datasource.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,8 @@ class file_source : public datasource {
public:
explicit file_source(char const* filepath) : _file(filepath, O_RDONLY)
{
detail::force_init_cuda_context();
if (detail::cufile_integration::is_kvikio_enabled()) {
// Workaround for https://github.com/rapidsai/cudf/issues/14140, where cuFileDriverOpen errors
// out if no CUDA calls have been made before it. This is a no-op if the CUDA context is
// already initialized
cudaFree(0);

_kvikio_file = kvikio::FileHandle(filepath);
CUDF_LOG_INFO("Reading a file using kvikIO, with compatibility mode {}.",
_kvikio_file.is_compat_mode_on() ? "on" : "off");
Expand Down
8 changes: 8 additions & 0 deletions cpp/src/io/utilities/file_io_utilities.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,14 @@ namespace cudf {
namespace io {
namespace detail {

void force_init_cuda_context()
{
// Workaround for https://github.com/rapidsai/cudf/issues/14140, where cuFileDriverOpen errors
// out if no CUDA calls have been made before it. This is a no-op if the CUDA context is already
// initialized.
cudaFree(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we hard code 0?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we hard code 0?

This initializes the context without actually doing anything.
https://docs.nvidia.com/cuda/cuda-runtime-api/group__CUDART__MEMORY.html#group__CUDART__MEMORY_1ga042655cbbf3408f01061652a075e094

Copy link
Contributor

Choose a reason for hiding this comment

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

This is confusing. Maybe the CUDA doc is not accurate? If the input parameter is a pointer, we should pass in nullptr instead of 0.

Copy link
Contributor Author

@vuule vuule Mar 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

As far as I can tell, cudaFree(0) is the idiomatic way to do this. nullptr would be a better option in general, but it looks like cudaFree(0) is recognized as the idiom. I haven't seen a single reference where cudaFree(nullptr) is used.
Edit: google search for "cudaFree(nullptr)": 121 hits vs cudaFree(0) 1220 hits 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the idiom that I am familiar with as well.

}

[[noreturn]] void throw_on_file_open_failure(std::string const& filepath, bool is_create)
{
// save errno because it may be overwritten by subsequent calls
Expand Down
3 changes: 3 additions & 0 deletions cpp/src/io/utilities/file_io_utilities.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ namespace detail {

[[noreturn]] void throw_on_file_open_failure(std::string const& filepath, bool is_create);

// Call before any cuFile API calls to ensure the CUDA context is initialized.
void force_init_cuda_context();

/**
* @brief Class that provides RAII for file handling.
*/
Expand Down
Loading