Clean up error handling #394
Conversation
madsbk
added
improvement
| // 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()) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
The decision to drop the config file was made here: conda-forge/staged-recipes#21908 (comment)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@wence- my guess is that the threadpool is enabled by default in cufile.json and disabled in the default value embedded in libcufile.
There was a problem hiding this comment.
I can confirm that max_io_threads and parallel_io are 0 when no config is found.
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
@wence- I suggest we use this for now. Let's think of a more general solution to the
cufile.jsonissue 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 :).
|
/merge |
No description provided.