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

Clean up error handling #394

Merged
merged 3 commits into from
Jun 25, 2024
Merged

Clean up error handling #394

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6b46822
use CUFILE_CHECK_BYTES_DONE
madsbk Jun 19, 2024
7ca4cb3
config_path()
madsbk Jun 19, 2024
382fe13
define CUFILEOP_BASE_ERR
madsbk Jun 19, 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
8 changes: 4 additions & 4 deletions cpp/examples/basic_io.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2021-2023, NVIDIA CORPORATION.
* Copyright (c) 2021-2024, NVIDIA CORPORATION.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -254,16 +254,16 @@ int main()
// After synchronizing `stream`, we can read the number of bytes written
check(cudaStreamSynchronize(stream) == cudaSuccess);
// Note, `*bytes_done_p` might be negative, which indicate an IO error thus we
// use `CUFILE_CHECK_STREAM_IO` to check for errors.
CUFILE_CHECK_STREAM_IO(bytes_done_p);
// use `CUFILE_CHECK_BYTES_DONE` to check for errors.
CUFILE_CHECK_BYTES_DONE(*bytes_done_p);
check(*bytes_done_p == SIZE);
cout << "File async write: " << *bytes_done_p << endl;

// Let's async read the data back into device memory
*bytes_done_p = 0;
f_handle.read_async(c_dev, &io_size, &f_off, &d_off, bytes_done_p, stream);
check(cudaStreamSynchronize(stream) == cudaSuccess);
CUFILE_CHECK_STREAM_IO(bytes_done_p);
CUFILE_CHECK_BYTES_DONE(*bytes_done_p);
check(*bytes_done_p == SIZE);
cout << "File async read: " << *bytes_done_p << endl;
check(cudaFreeHost((void*)bytes_done_p) == cudaSuccess);
Expand Down
48 changes: 48 additions & 0 deletions cpp/include/kvikio/cufile_config.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
/*
* Copyright (c) 2024, NVIDIA CORPORATION.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
#pragma once

#include <cstdlib>
#include <filesystem>
#include <string>

namespace kvikio {
namespace detail {

[[nodiscard]] inline const std::string lookup_config_path()
{
const char* env = std::getenv("CUFILE_ENV_PATH_JSON");
if (env != nullptr && std::filesystem::exists(env)) { return env; }
if (std::filesystem::exists("/etc/cufile.json")) { return "/etc/cufile.json"; }
return std::string();
}

} // namespace detail

/**
* @brief Get the filepath to cuFile's config file (`cufile.json`) or the empty string
*
* This lookup is cached.
*
* @return The filepath to the cufile.json file or the empty string if it isn't found.
*/
[[nodiscard]] inline const std::string config_path()
{
static const std::string ret = detail::lookup_config_path();
return ret;
}

} // namespace kvikio
55 changes: 30 additions & 25 deletions cpp/include/kvikio/error.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/
#pragma once

#include <cstring>
#include <exception>
#include <system_error>

Expand Down Expand Up @@ -61,37 +62,41 @@ struct CUfileException : public std::runtime_error {
GET_CUFILE_TRY_MACRO(__VA_ARGS__, CUFILE_TRY_2, CUFILE_TRY_1) \
(__VA_ARGS__)
#define GET_CUFILE_TRY_MACRO(_1, _2, NAME, ...) NAME
#define CUFILE_TRY_2(_call, _exception_type) \
do { \
CUfileError_t const error = (_call); \
if (error.err != CU_FILE_SUCCESS) { \
if (error.err == CU_FILE_CUDA_DRIVER_ERROR) { \
CUresult const cuda_error = error.cu_err; \
CUDA_DRIVER_TRY(cuda_error); \
} \
throw(_exception_type){std::string{"cuFile error at: "} + __FILE__ + ":" + \
KVIKIO_STRINGIFY(__LINE__) + ": " + \
cufileop_status_error(error.err)}; \
} \
#define CUFILE_TRY_2(_call, _exception_type) \
do { \
CUfileError_t const error = (_call); \
if (error.err != CU_FILE_SUCCESS) { \
if (error.err == CU_FILE_CUDA_DRIVER_ERROR) { \
CUresult const cuda_error = error.cu_err; \
CUDA_DRIVER_TRY(cuda_error); \
} \
throw(_exception_type){std::string{"cuFile error at: "} + __FILE__ + ":" + \
KVIKIO_STRINGIFY(__LINE__) + ": " + \
cufileop_status_error((CUfileOpError)std::abs(error.err))}; \
} \
} while (0)
#define CUFILE_TRY_1(_call) CUFILE_TRY_2(_call, kvikio::CUfileException)
#endif

#ifndef CUFILE_CHECK_STREAM_IO
#define CUFILE_CHECK_STREAM_IO(...) \
GET_CUFILE_CHECK_STREAM_IO_MACRO( \
__VA_ARGS__, CUFILE_CHECK_STREAM_IO_2, CUFILE_CHECK_STREAM_IO_1) \
#ifndef CUFILE_CHECK_BYTES_DONE
#define CUFILE_CHECK_BYTES_DONE(...) \
GET_CUFILE_CHECK_BYTES_DONE_MACRO( \
__VA_ARGS__, CUFILE_CHECK_BYTES_DONE_2, CUFILE_CHECK_BYTES_DONE_1) \
(__VA_ARGS__)
#define GET_CUFILE_CHECK_STREAM_IO_MACRO(_1, _2, NAME, ...) NAME
#define CUFILE_CHECK_STREAM_IO_2(_nbytes_done, _exception_type) \
do { \
auto const _nbytes = *(_nbytes_done); \
if (_nbytes < 0) { \
throw(_exception_type){std::string{"cuFile error at: "} + __FILE__ + ":" + \
KVIKIO_STRINGIFY(__LINE__) + ": " + std::to_string(_nbytes)}; \
} \
#define GET_CUFILE_CHECK_BYTES_DONE_MACRO(_1, _2, NAME, ...) NAME
#define CUFILE_CHECK_BYTES_DONE_2(_nbytes_done, _exception_type) \
do { \
auto const _nbytes = (_nbytes_done); \
if (_nbytes < 0) { \
auto const err = std::abs(_nbytes); \
auto const msg = (err > CUFILEOP_BASE_ERR) \
? std::string(cufileop_status_error((CUfileOpError)err)) \
: std::string(std::strerror(err)); \
throw(_exception_type){std::string{"cuFile error at: "} + __FILE__ + ":" + \
KVIKIO_STRINGIFY(__LINE__) + ": " + msg}; \
} \
} while (0)
#define CUFILE_CHECK_STREAM_IO_1(_call) CUFILE_CHECK_STREAM_IO_2(_call, kvikio::CUfileException)
#define CUFILE_CHECK_BYTES_DONE_1(_call) CUFILE_CHECK_BYTES_DONE_2(_call, kvikio::CUfileException)
#endif

} // namespace kvikio
17 changes: 8 additions & 9 deletions cpp/include/kvikio/file_handle.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include <utility>

#include <kvikio/buffer.hpp>
#include <kvikio/cufile_config.hpp>
#include <kvikio/defaults.hpp>
#include <kvikio/error.hpp>
#include <kvikio/parallel_operation.hpp>
Expand Down Expand Up @@ -307,13 +308,7 @@ class FileHandle {
}
ssize_t ret = cuFileAPI::instance().Read(
_handle, devPtr_base, size, convert_size2off(file_offset), convert_size2off(devPtr_offset));
if (ret == -1) {
throw std::system_error(errno, std::generic_category(), "Unable to read file");
}
if (ret < -1) {
throw CUfileException(std::string{"cuFile error at: "} + __FILE__ + ":" +
KVIKIO_STRINGIFY(__LINE__) + ": " + CUFILE_ERRSTR(ret));
}
CUFILE_CHECK_BYTES_DONE(ret);
return ret;
}

Expand Down Expand Up @@ -526,7 +521,9 @@ class FileHandle {
ssize_t* bytes_read_p,
CUstream stream)
{
if (kvikio::is_batch_and_stream_available() && !_compat_mode) {
// When checking for availability, we also check if cuFile's config file exist. This is because
// even when the stream API is available, it doesn't work if no config file exist.
if (kvikio::is_batch_and_stream_available() && !_compat_mode && !config_path().empty()) {
Comment on lines +524 to +526
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, this is horrible. Is this a documented restriction, or an empirically observed one?

If the former: can we please link to the docs in this comment.

If the latter: can we please open a bug that requests that this "feature" is fixed?

In either case, can we provide a warning when constructing the config_path singleton for the first time if a config file was not found, since this is not a discoverable failure mode.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is the latter, by default the thread pool isn't enabled, which is required by the stream API.

AFAICT, the conda cufile package doesn't install a config file thus a warning would be triggered always. If we want a warning, we should make sure that the conda and pip package of cufile installs a config file.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, so if there is no config file, then no thread pool, and therefore no stream.

Alternatively, if there is a config file, then there is a thread pool? Or is it that there is a thread pool by default, and it can be turned off.

If the conda/pip version of cufile doesn't install a default config file (which I am guessing must live in /etc/cufile.json), then we need to fix something. But neither of those systems is allowed to write to /etc, so we would need to have a way to control the "root" location of the default configuration file.

But in any case, absence of a default config file should (IMO) result in all defaults being applied, so that there should be no runtime difference between /etc/cufile.json exists and is filled with default values and /etc/cufile.json doesn't exist.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a config file but we do not ship it in $CONDA_PREFIX/etc. Is this desirable? https://github.com/conda-forge/libcufile-feedstock/blob/3c84e7c8ade4501345445ea56691db2767085b94/recipe/build.sh#L7

Copy link
Contributor

Choose a reason for hiding this comment

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

The decision to drop the config file was made here: conda-forge/staged-recipes#21908 (comment)

Copy link
Member Author

@madsbk madsbk Jun 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe set CUFILE_ENV_PATH_JSON in the conda activation script if CUFILE_ENV_PATH_JSON isn't already defined and /etc/cufile.json doesn't exist?

Copy link
Member Author

@madsbk madsbk Jun 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@wence- my guess is that the threadpool is enabled by default in cufile.json and disabled in the default value embedded in libcufile.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can confirm that max_io_threads and parallel_io are 0 when no config is found.

Copy link
Member Author

@madsbk madsbk Jun 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@wence- I suggest we use this for now. Let's think of a more general solution to the cufile.json issue in a new PR.
Maybe KvikIO should parse the config file to provide more helpful errors and warnings?

Copy link
Contributor

Choose a reason for hiding this comment

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

@wence- I suggest we use this for now. Let's think of a more general solution to the cufile.json issue in a new PR.

Makes sense.

Maybe KvikIO should parse the config file to provide more helpful errors and warnings?

Preferably not, if we can avoid it :).

CUFILE_TRY(cuFileAPI::instance().ReadAsync(
_handle, devPtr_base, size_p, file_offset_p, devPtr_offset_p, bytes_read_p, stream));
return;
Expand Down Expand Up @@ -616,7 +613,9 @@ class FileHandle {
ssize_t* bytes_written_p,
CUstream stream)
{
if (kvikio::is_batch_and_stream_available() && !_compat_mode) {
// When checking for availability, we also check if cuFile's config file exist. This is because
// even when the stream API is available, it doesn't work if no config file exist.
if (kvikio::is_batch_and_stream_available() && !_compat_mode && !config_path().empty()) {
CUFILE_TRY(cuFileAPI::instance().WriteAsync(
_handle, devPtr_base, size_p, file_offset_p, devPtr_offset_p, bytes_written_p, stream));
return;
Expand Down
2 changes: 1 addition & 1 deletion cpp/include/kvikio/posix_io.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ ssize_t posix_host_io(int fd, const void* buf, size_t count, off_t offset, bool
const std::string name = IsReadOperation ? "pread" : "pwrite";
if (errno == EBADF) {
throw CUfileException{std::string{"POSIX error on " + name + " at: "} + __FILE__ + ":" +
KVIKIO_STRINGIFY(__LINE__) + ": unsupported file open flags"};
KVIKIO_STRINGIFY(__LINE__) + ": Operation not permitted"};
}
throw CUfileException{std::string{"POSIX error on " + name + " at: "} + __FILE__ + ":" +
KVIKIO_STRINGIFY(__LINE__) + ": " + strerror(errno)};
Expand Down
1 change: 1 addition & 0 deletions cpp/include/kvikio/shim/cufile_h_wrapper.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
using CUfileHandle_t = void*;
using CUfileOpError = int;
#define CUFILE_ERRSTR(x) ("KvikIO not compiled with cuFile.h")
#define CUFILEOP_BASE_ERR 5000
#define CU_FILE_SUCCESS 0
#define CU_FILE_CUDA_DRIVER_ERROR 1

Expand Down
8 changes: 4 additions & 4 deletions cpp/include/kvikio/stream.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2023, NVIDIA CORPORATION.
* Copyright (c) 2023-2024, NVIDIA CORPORATION.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -132,9 +132,9 @@ class StreamFuture {
CUDA_DRIVER_TRY(cudaAPI::instance().StreamSynchronize(_stream));
}

CUFILE_CHECK_STREAM_IO(&_val->bytes_done);
// At this point, we know `*_val->bytes_done` is a positive value otherwise
// CUFILE_CHECK_STREAM_IO() would have raised an exception.
CUFILE_CHECK_BYTES_DONE(_val->bytes_done);
// At this point, we know `_val->bytes_done` is a positive value otherwise
// CUFILE_CHECK_BYTES_DONE() would have raised an exception.
return static_cast<std::size_t>(_val->bytes_done);
}

Expand Down
2 changes: 1 addition & 1 deletion python/kvikio/tests/test_async_io.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ def test_read_write(tmp_path, size):
assert f.raw_write_async(a, stream.ptr).check_bytes_done() == a.nbytes

# Try to read file opened in write-only mode
with pytest.raises(RuntimeError, match="unsupported file open flags"):
with pytest.raises(RuntimeError, match="Operation not permitted"):
# The exception is raised when we call the raw_read_async API.
future_stream = f.raw_read_async(a, stream.ptr)
future_stream.check_bytes_done()
Expand Down
4 changes: 2 additions & 2 deletions python/kvikio/tests/test_basic_io.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Copyright (c) 2021-2023, NVIDIA CORPORATION. All rights reserved.
# Copyright (c) 2021-2024, NVIDIA CORPORATION. All rights reserved.
# See file LICENSE for terms.

import os
Expand Down Expand Up @@ -36,7 +36,7 @@ def test_read_write(tmp_path, xp, gds_threshold, size, nthreads, tasksize):
assert f.write(a) == a.nbytes

# Try to read file opened in write-only mode
with pytest.raises(RuntimeError, match="unsupported file open flags"):
with pytest.raises(RuntimeError, match="Operation not permitted"):
f.read(a)

# Close file
Expand Down