-
Notifications
You must be signed in to change notification settings - Fork 91
Add an overload getenv_or that supports env var alias. Add new env var KVIKIO_NUM_THREADS. Fix UB. #735
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
Add an overload getenv_or that supports env var alias. Add new env var KVIKIO_NUM_THREADS. Fix UB. #735
Changes from 7 commits
77941d3
fe8b4d8
119453c
9497d42
37516bd
66f16fc
64ba7cf
a612fca
5a78ace
bf3e750
312f869
39e0199
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,11 +18,13 @@ | |
|
|
||
| #include <cstddef> | ||
| #include <cstdlib> | ||
| #include <initializer_list> | ||
| #include <sstream> | ||
| #include <stdexcept> | ||
| #include <string> | ||
|
|
||
| #include <kvikio/compat_mode.hpp> | ||
| #include <kvikio/error.hpp> | ||
| #include <kvikio/http_status_codes.hpp> | ||
| #include <kvikio/shim/cufile.hpp> | ||
| #include <kvikio/threadpool_wrapper.hpp> | ||
|
|
@@ -56,6 +58,52 @@ CompatMode getenv_or(std::string_view env_var_name, CompatMode default_val); | |
| template <> | ||
| std::vector<int> getenv_or(std::string_view env_var_name, std::vector<int> default_val); | ||
|
|
||
| /** | ||
| * @brief Get the environment variable value from a candidate list | ||
| * | ||
| * @tparam T Type of the environment variable value | ||
| * @param env_var_names Candidate list containing the names of environment variable | ||
| * @param default_val Default value of the environment variable, if none of the candidates has been | ||
| * found | ||
| * @return A tuple of (`env_var_name`, `result`, `has_found`), where: | ||
| * - If the environment variable is not set by any of the candidates, `has_found` will be false, | ||
| * `result` be `default_val`, and `env_var_name` be empty. | ||
| * - If the environment variable is set by `env_var_name`, then `has_found` will be true, and | ||
| * `result` be the set value. | ||
| * | ||
| * @throws std::invalid_argument if: | ||
| * - `env_var_names` is empty | ||
| * - Multiple candidates have been set at the same time | ||
| * - An invalid value is given, e.g. value that cannot be converted to type T | ||
| */ | ||
| template <typename T> | ||
| std::tuple<std::string_view, T, bool> getenv_or( | ||
| std::initializer_list<std::string_view> env_var_names, T default_val) | ||
| { | ||
| KVIKIO_EXPECT(env_var_names.size() > 0, | ||
| "`env_var_names` must contain at least one environment variable name.", | ||
| std::invalid_argument); | ||
| std::string_view env_name_target; | ||
| std::string_view env_val_target; | ||
| bool has_already_been_set{false}; | ||
| for (auto const& env_var_name : env_var_names) { | ||
| auto const* env_val = std::getenv(env_var_name.data()); | ||
| if (env_val == nullptr) { continue; } | ||
| KVIKIO_EXPECT( | ||
| !has_already_been_set, | ||
| "Environment variable " + std::string{env_var_name} + " has already been set by its alias.", | ||
| std::invalid_argument); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how messy would it be to allow multiple aliases to be set, as long as they have the same value?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implemented, in a not so messy way. |
||
| has_already_been_set = true; | ||
| env_name_target = env_var_name; | ||
| env_val_target = env_val; | ||
| } | ||
|
|
||
| if (env_name_target.empty()) { return {env_name_target, default_val, false}; } | ||
|
|
||
| auto res = getenv_or<T>(env_name_target, default_val); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Simplified the logic so that each type's own version of
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. adds an extra call to |
||
| return {env_name_target, res, true}; | ||
| } | ||
|
|
||
| /** | ||
| * @brief Singleton class of default values used throughout KvikIO. | ||
| * | ||
|
|
@@ -183,6 +231,20 @@ class defaults { | |
| */ | ||
| static void set_thread_pool_nthreads(unsigned int nthreads); | ||
|
|
||
| /** | ||
| * @brief Alias of `thread_pool_nthreads` | ||
| * | ||
| * @return The number of threads | ||
| */ | ||
| [[nodiscard]] static unsigned int num_threads(); | ||
|
|
||
| /** | ||
| * @brief Alias of `set_thread_pool_nthreads` | ||
| * | ||
| * @param nthreads The number of threads to use | ||
| */ | ||
| static void set_num_threads(unsigned int nthreads); | ||
|
|
||
| /** | ||
| * @brief Get the default task size used for parallel IO operations. | ||
| * | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,10 +15,18 @@ | |
| */ | ||
|
|
||
| #include <stdexcept> | ||
| #include <vector> | ||
|
|
||
| #include <gmock/gmock.h> | ||
| #include <gtest/gtest.h> | ||
| #include <kvikio/defaults.hpp> | ||
|
|
||
| #include "kvikio/compat_mode.hpp" | ||
| #include "utils/env.hpp" | ||
|
|
||
| using ::testing::HasSubstr; | ||
| using ::testing::ThrowsMessage; | ||
|
|
||
| TEST(DefaultsTest, parse_compat_mode_str) | ||
| { | ||
| { | ||
|
|
@@ -72,3 +80,120 @@ TEST(DefaultsTest, parse_http_status_codes) | |
| } | ||
| } | ||
| } | ||
|
|
||
| TEST(DefaultsTest, alias_for_getenv_or) | ||
| { | ||
| // Passed initializer list is empty | ||
| { | ||
| EXPECT_THAT([=] { kvikio::getenv_or({}, 123); }, | ||
| ThrowsMessage<std::invalid_argument>(HasSubstr( | ||
| "`env_var_names` must contain at least one environment variable name"))); | ||
| } | ||
|
|
||
| // Env var has an empty value | ||
| { | ||
| kvikio::test::EnvVarContext env_var_ctx{{{"KVIKIO_TEST_ALIAS", ""}}}; | ||
| EXPECT_THAT( | ||
| [=] { kvikio::getenv_or({"KVIKIO_TEST_ALIAS"}, 123); }, | ||
| ThrowsMessage<std::invalid_argument>(HasSubstr("unknown config value KVIKIO_TEST_ALIAS="))); | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about a test where the empty variable is read as a string, so the value is valid?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
|
|
||
| // Env var has already been set by its alias | ||
| { | ||
| kvikio::test::EnvVarContext env_var_ctx{ | ||
| {{"KVIKIO_TEST_ALIAS_1", "10"}, {"KVIKIO_TEST_ALIAS_2", "20"}}}; | ||
| EXPECT_THAT([=] { kvikio::getenv_or({"KVIKIO_TEST_ALIAS_1", "KVIKIO_TEST_ALIAS_2"}, 123); }, | ||
| ThrowsMessage<std::invalid_argument>(HasSubstr( | ||
| "Environment variable KVIKIO_TEST_ALIAS_2 has already been set by its alias"))); | ||
| } | ||
|
|
||
| // Env var has invalid value | ||
| { | ||
| kvikio::test::EnvVarContext env_var_ctx{{{"KVIKIO_TEST_ALIAS", "abc"}}}; | ||
| EXPECT_THAT([=] { kvikio::getenv_or({"KVIKIO_TEST_ALIAS"}, 123); }, | ||
| ThrowsMessage<std::invalid_argument>( | ||
| HasSubstr("unknown config value KVIKIO_TEST_ALIAS=abc"))); | ||
| } | ||
|
|
||
| // 1st alias has a set value | ||
| { | ||
| kvikio::test::EnvVarContext env_var_ctx{{{"KVIKIO_TEST_ALIAS_1", "654.321"}}}; | ||
| auto const [env_var_name, result, has_found] = | ||
| kvikio::getenv_or({"KVIKIO_TEST_ALIAS_1", "KVIKIO_TEST_ALIAS_2"}, 123.456); | ||
| EXPECT_EQ(env_var_name, std::string_view{"KVIKIO_TEST_ALIAS_1"}); | ||
| EXPECT_EQ(result, 654.321); | ||
| EXPECT_TRUE(has_found); | ||
| } | ||
|
|
||
| // 2nd alias has a set value | ||
| { | ||
| kvikio::test::EnvVarContext env_var_ctx{{{"KVIKIO_TEST_ALIAS_2", "654.321"}}}; | ||
| auto const [env_var_name, result, has_found] = | ||
| kvikio::getenv_or({"KVIKIO_TEST_ALIAS_1", "KVIKIO_TEST_ALIAS_2"}, 123.456); | ||
| EXPECT_EQ(env_var_name, std::string_view{"KVIKIO_TEST_ALIAS_2"}); | ||
| EXPECT_EQ(result, 654.321); | ||
| EXPECT_TRUE(has_found); | ||
| } | ||
|
|
||
| // Neither alias has a set value | ||
| { | ||
| auto const [env_var_name, result, has_found] = | ||
| kvikio::getenv_or({"KVIKIO_TEST_ALIAS_1", "KVIKIO_TEST_ALIAS_2"}, 123.456); | ||
| EXPECT_TRUE(env_var_name.empty()); | ||
| EXPECT_EQ(result, 123.456); | ||
| EXPECT_FALSE(has_found); | ||
| } | ||
|
|
||
| // Special type: bool | ||
| { | ||
| kvikio::test::EnvVarContext env_var_ctx{{{"KVIKIO_TEST_ALIAS", "yes"}}}; | ||
| auto const [env_var_name, result, has_found] = kvikio::getenv_or({"KVIKIO_TEST_ALIAS"}, false); | ||
| EXPECT_EQ(env_var_name, std::string_view{"KVIKIO_TEST_ALIAS"}); | ||
| EXPECT_TRUE(result); | ||
| EXPECT_TRUE(has_found); | ||
| } | ||
| { | ||
| kvikio::test::EnvVarContext env_var_ctx{{{"KVIKIO_TEST_ALIAS", "OFF"}}}; | ||
| auto const [env_var_name, result, has_found] = kvikio::getenv_or({"KVIKIO_TEST_ALIAS"}, false); | ||
| EXPECT_EQ(env_var_name, std::string_view{"KVIKIO_TEST_ALIAS"}); | ||
| EXPECT_FALSE(result); | ||
| EXPECT_TRUE(has_found); | ||
| } | ||
|
|
||
| // Special type: CompatMode | ||
| { | ||
| kvikio::test::EnvVarContext env_var_ctx{{{"KVIKIO_TEST_ALIAS", "yes"}}}; | ||
| auto const [env_var_name, result, has_found] = | ||
| kvikio::getenv_or({"KVIKIO_TEST_ALIAS"}, kvikio::CompatMode::AUTO); | ||
| EXPECT_EQ(env_var_name, std::string_view{"KVIKIO_TEST_ALIAS"}); | ||
| EXPECT_EQ(result, kvikio::CompatMode::ON); | ||
| EXPECT_TRUE(has_found); | ||
| } | ||
| { | ||
| kvikio::test::EnvVarContext env_var_ctx{{{"KVIKIO_TEST_ALIAS", "FALSE"}}}; | ||
| auto const [env_var_name, result, has_found] = | ||
| kvikio::getenv_or({"KVIKIO_TEST_ALIAS"}, kvikio::CompatMode::AUTO); | ||
| EXPECT_EQ(env_var_name, std::string_view{"KVIKIO_TEST_ALIAS"}); | ||
| EXPECT_EQ(result, kvikio::CompatMode::OFF); | ||
| EXPECT_TRUE(has_found); | ||
| } | ||
| { | ||
| kvikio::test::EnvVarContext env_var_ctx{{{"KVIKIO_TEST_ALIAS", "aUtO"}}}; | ||
| auto const [env_var_name, result, has_found] = | ||
| kvikio::getenv_or({"KVIKIO_TEST_ALIAS"}, kvikio::CompatMode::ON); | ||
| EXPECT_EQ(env_var_name, std::string_view{"KVIKIO_TEST_ALIAS"}); | ||
| EXPECT_EQ(result, kvikio::CompatMode::AUTO); | ||
| EXPECT_TRUE(has_found); | ||
| } | ||
|
|
||
| // Special type: std::vector<int> | ||
| { | ||
| kvikio::test::EnvVarContext env_var_ctx{{{"KVIKIO_TEST_ALIAS", "109, 108, 107"}}}; | ||
| auto const [env_var_name, result, has_found] = | ||
| kvikio::getenv_or({"KVIKIO_TEST_ALIAS"}, std::vector<int>{111, 112, 113}); | ||
| EXPECT_EQ(env_var_name, std::string_view{"KVIKIO_TEST_ALIAS"}); | ||
| std::vector<int> expected{109, 108, 107}; | ||
| EXPECT_EQ(result, expected); | ||
| EXPECT_TRUE(has_found); | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.