Add an overload getenv_or that supports env var alias. Add new env var KVIKIO_NUM_THREADS. Fix UB.#735
Conversation
kingcrimsontianyu
added
bug
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
kingcrimsontianyu
added
non-breaking
|
|
||
| if (env_name_target.empty()) { return {env_name_target, default_val, false}; } | ||
|
|
||
| auto res = getenv_or<T>(env_name_target, default_val); |
There was a problem hiding this comment.
Simplified the logic so that each type's own version of getenv_or<T> is in charge of value extraction.
There was a problem hiding this comment.
adds an extra call to getenv, but AFAIK this is fine - repeated calls are VERY fast.
vuule
left a comment
vuule
left a comment
There was a problem hiding this comment.
looks good, just a few ideas
cpp/include/kvikio/defaults.hpp
Outdated
| KVIKIO_EXPECT( | ||
| !has_already_been_set, | ||
| "Environment variable " + std::string{env_var_name} + " has already been set by its alias.", | ||
| std::invalid_argument); |
There was a problem hiding this comment.
how messy would it be to allow multiple aliases to be set, as long as they have the same value?
There was a problem hiding this comment.
Implemented, in a not so messy way.
|
|
||
| if (env_name_target.empty()) { return {env_name_target, default_val, false}; } | ||
|
|
||
| auto res = getenv_or<T>(env_name_target, default_val); |
There was a problem hiding this comment.
adds an extra call to getenv, but AFAIK this is fine - repeated calls are VERY fast.
cpp/tests/test_defaults.cpp
Outdated
| // 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="))); | ||
| } |
There was a problem hiding this comment.
How about a test where the empty variable is read as a string, so the value is valid?
There was a problem hiding this comment.
Done.
When a string stream initialized with an empty string is extracted to a string, its fail bit will still be set. So implementation has been changed (with if constexpr) to permit empty value for string type.
Co-authored-by: Vukasin Milovanovic <vmilovanovic@nvidia.com>
|
/merge |
KvikIO unit test has a utility class `EnvVarContext` class, introduced in #700, and slightly improved in #735. It has been identified that this class was incorrectly initialized, resulting in UB: it causes the unit test failure in C++20, which by fluke was not observed in C++17. This PR fixes this error. Specifically, the constructor of `EnvVarContext` is: ``` EnvVarContext(std::initializer_list<std::pair<std::string_view, std::string_view>> env_var_entries); ``` There are several ways of instantiation: ``` // Direct initialization EnvVarContext env_var_ctx({{"env_1", "v1"}, {"env_2", "v2"}}); // Direct list initialization EnvVarContext env_var_ctx{{"env_1", "v1"}, {"env_2", "v2"}}; // Copy list initialization EnvVarContext env_var_ctx = {{"env_1", "v1"}, {"env_2", "v2"}}; ``` The erroneous instantiation performed is: ``` // Extra pair of braces // {}: brace-enclosed initializer list // {{"env_1", "v1"}, {"env_2", "v2"}}: one element of type pair<std::string_view, std::string_view> // {"env_1", "v1"}: first // {"env_2", "v2"}: second EnvVarContext env_var_ctx{{{"env_1", "v1"}, {"env_2", "v2"}}}; ``` As a result, the initializer list only has 1 pair, with the key being `{"env_1", "v1"}` and value being `{"env_2", "v2"}`. For the key, for instance, the 5-th overload (https://en.cppreference.com/w/cpp/string/basic_string_view/basic_string_view.html) of the constructor was used, where `first` points to "env_1" and `last` points to "v1". Since the two iterators do not form a valid range, UB ensues. Authors: - Tianyu Liu (https://github.com/kingcrimsontianyu) Approvers: - Mads R. B. Kristensen (https://github.com/madsbk) URL: #751
3 participants
This PR performs the following items:
kvikio::getenv_orthat supports env var alias, i.e. multiple different env vars referring to the same property.KVIKIO_NUM_THREADSas the alias ofKVIKIO_NTHREADS, and add new C++ APInum_threads/set_thread_pool_nthreadsas the alias of existingthread_pool_nthreads/set_thread_pool_nthreads. This makes C++ and Python API (i.e.kvikio.defaults.get("num_threads")) consistent.std::initializer_list.