-
Notifications
You must be signed in to change notification settings - Fork 25
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
Expose env var handling to python #856
base: master
Are you sure you want to change the base?
Changes from all commits
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 | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -36,6 +36,19 @@ | |||||||||||||||||||||||||||||
return cpp_etc_lib().phare_build_config() | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
def env_vars(): | ||||||||||||||||||||||||||||||
return cpp_etc_lib().phare_env_vars() | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
def print_env_vars_info(): | ||||||||||||||||||||||||||||||
for k, v in cpp_etc_lib().phare_env_vars().items(): | ||||||||||||||||||||||||||||||
print(f"{k}: {v.desc}") | ||||||||||||||||||||||||||||||
print("Options:") | ||||||||||||||||||||||||||||||
for k, v in v.options: | ||||||||||||||||||||||||||||||
print(f" {k}: {v}") | ||||||||||||||||||||||||||||||
print("") | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
Comment on lines
+43
to
+50
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. Resolve variable shadowing in nested loops. The nested loop uses the same variable names - for k, v in v.options:
+ for option_key, option_value in v.options:
+ print(f" {option_key}: {option_value}") Committable suggestion
Suggested change
ToolsRuff
|
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
def build_config_as_json(): | ||||||||||||||||||||||||||||||
return json.dumps(build_config()) | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,118 @@ | ||
#ifndef PHARE_CORE_ENV_HPP | ||
#define PHARE_CORE_ENV_HPP | ||
|
||
// Single source for handling env vars | ||
|
||
#include <array> | ||
#include <cstdint> | ||
#include <optional> | ||
#include <string_view> | ||
#include <unordered_map> | ||
#include "core/utilities/types.hpp" | ||
#include "core/utilities/mpi_utils.hpp" | ||
|
||
namespace PHARE::env | ||
{ | ||
|
||
struct Var | ||
{ | ||
using value_type = std::string; | ||
using results_type = std::unordered_map<std::string, std::string>; | ||
auto constexpr static noop_results = [](Var const&) { return results_type{}; }; | ||
|
||
std::string_view const id; | ||
std::string_view const desc; | ||
std::vector<std::pair<std::string_view, std::string_view>> const options; | ||
|
||
std::optional<value_type> const _default = std::nullopt; | ||
std::function<results_type(Var const&)> const _results = noop_results; | ||
std::optional<value_type> const v = get(); | ||
results_type const results = _results(*this); | ||
|
||
std::optional<value_type> get() const | ||
{ | ||
std::string _id{id}; | ||
if (_default) | ||
return core::get_env(_id, *_default); | ||
return core::get_env(_id); | ||
} | ||
auto const& operator()() const { return v; } | ||
auto const& operator()(std::string const& s) const { return results.at(s); } | ||
auto const& operator()(std::string const& s, std::string const& default_) const | ||
{ | ||
if (results.count(s)) | ||
return results.at(s); | ||
return default_; | ||
} | ||
bool exists() const { return v != std::nullopt; } | ||
operator bool() const { return exists(); } | ||
}; | ||
|
||
} // namespace PHARE::env | ||
|
||
namespace PHARE | ||
{ | ||
class Env | ||
{ | ||
public: | ||
template<typename T> | ||
using map_t = std::unordered_map<std::string, T const* const>; | ||
|
||
static Env& INSTANCE() | ||
{ | ||
if (!self) | ||
self = std::make_unique<Env>(); | ||
return *self; | ||
} | ||
static auto& reinit() { return *(self = std::make_unique<Env>()); } | ||
|
||
env::Var const PHARE_LOG{ | ||
"PHARE_LOG", | ||
"Write logs to $CWD/.log", | ||
{{{"RANK_FILES", "Write logs $CWD/.log, a file per rank"}, | ||
{"DATETIME_FILES", "Write logs $CWD/.log, filename per rank and datetime"}, | ||
{"NONE", "print normally to std::cout"}}}, | ||
std::nullopt, | ||
[](auto const& self) { | ||
std::string static const file_key = "PHARE_LOG_FILE"; | ||
typename env::Var::results_type map; | ||
if (auto const& opt = self()) | ||
{ | ||
auto const& val = *opt; | ||
if (val == "RANK_FILES") | ||
map[file_key] = ".log/" + std::to_string(core::mpi::rank()) + ".out"; | ||
else if (val == "DATETIME_FILES") | ||
{ | ||
auto date_time = core::mpi::date_time(); | ||
auto rank = std::to_string(core::mpi::rank()); | ||
auto size = std::to_string(core::mpi::size()); | ||
map[file_key] = ".log/" + date_time + "_" + rank + "_of_" + size + ".out"; | ||
} | ||
else if (val != "NONE") | ||
throw std::runtime_error("PHARE_LOG invalid type, valid keys are " | ||
"RANK_FILES/DATETIME_FILES/NONE"); | ||
} | ||
return map; | ||
} // | ||
}; | ||
env::Var const PHARE_SCOPE_TIMING{ | ||
"PHARE_SCOPE_TIMING", "Enable function scope timing", {{{"1", "ON"}, {"0", "OFF"}}}, "0"}; | ||
|
||
map_t<env::Var> const vars = {{ | ||
{"PHARE_LOG", &PHARE_LOG}, | ||
{"PHARE_SCOPE_TIMING", &PHARE_SCOPE_TIMING}, | ||
}}; | ||
|
||
auto& operator()(std::string const& s) const | ||
{ | ||
assert(vars.count(s)); | ||
return *vars.at(s); | ||
} | ||
|
||
private: | ||
static inline std::unique_ptr<Env> self = nullptr; | ||
}; | ||
|
||
} // namespace PHARE | ||
|
||
#endif /* PHARE_CORE_ERRORS_H */ |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -69,10 +69,7 @@ class HighFiveFile | |
|
||
~HighFiveFile() {} | ||
|
||
NO_DISCARD HiFile& file() | ||
{ | ||
return h5file_; | ||
} | ||
NO_DISCARD HiFile& file() { return h5file_; } | ||
|
||
|
||
template<typename T, std::size_t dim = 1> | ||
|
@@ -146,15 +143,11 @@ class HighFiveFile | |
void write_attribute(std::string const& keyPath, std::string const& key, Data const& data) | ||
{ | ||
// assumes all keyPaths and values are identical, and no null patches | ||
// clang-format off | ||
PHARE_DEBUG_DO( | ||
PHARE_DEBUG_DO( // | ||
auto const paths = core::mpi::collect(keyPath, core::mpi::size()); | ||
for (auto const& path : paths) | ||
PHARE_LOG_LINE_STR(std::to_string(core::mpi::size()) << " " << path) | ||
if (!core::all(paths, [&](auto const& path) { return path == paths[0]; })) | ||
throw std::runtime_error("Function does not support different paths per mpi core"); | ||
if (!core::all(paths, [&](auto const& path) { return path == paths[0]; })) throw std:: | ||
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.
:\ |
||
runtime_error("Function does not support different paths per mpi core"); // | ||
) | ||
// clang-format on | ||
|
||
constexpr bool data_is_vector = core::is_std_vector_v<Data>; | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -4,6 +4,8 @@ | |||||||
|
||||||||
#include "core/def/phlop.hpp" // scope timing | ||||||||
|
||||||||
#include "core/env.hpp" // scope timing | ||||||||
|
||||||||
#include "simulator/simulator.hpp" | ||||||||
#include "core/utilities/algorithm.hpp" | ||||||||
#include "core/utilities/mpi_utils.hpp" | ||||||||
|
@@ -41,7 +43,7 @@ class SamraiLifeCycle | |||||||
= std::make_shared<StreamAppender>(StreamAppender{&std::cout}); | ||||||||
SAMRAI::tbox::Logger::getInstance()->setWarningAppender(appender); | ||||||||
PHARE_WITH_PHLOP( // | ||||||||
if (auto e = core::get_env("PHARE_SCOPE_TIMING", "false"); e == "1" || e == "true") | ||||||||
if (auto e = Env::INSTANCE().PHARE_SCOPE_TIMING(); e == "1" || e == "true") | ||||||||
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. Approved with a suggestion for readability improvement. The logic for checking the environment variable and initializing the scope timer is correct. However, consider simplifying the conditional check for better readability. - if (auto e = Env::INSTANCE().PHARE_SCOPE_TIMING(); e == "1" || e == "true")
+ auto envSetting = Env::INSTANCE().PHARE_SCOPE_TIMING();
+ if (envSetting == "1" || envSetting == "true") Committable suggestion
Suggested change
|
||||||||
phlop::ScopeTimerMan::INSTANCE() | ||||||||
.file_name(".phare_times." + std::to_string(core::mpi::rank()) + ".txt") | ||||||||
.init(); // | ||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
cmake_minimum_required (VERSION 3.20.1) | ||
|
||
project(test-phare-env) | ||
|
||
set(SOURCES test_env.cpp) | ||
|
||
add_executable(${PROJECT_NAME} ${SOURCES}) | ||
|
||
target_include_directories(${PROJECT_NAME} PRIVATE | ||
${GTEST_INCLUDE_DIRS} | ||
) | ||
|
||
target_link_directories(${PROJECT_NAME} PUBLIC ${MPI_LIBRARY_PATH}) | ||
|
||
target_link_libraries(${PROJECT_NAME} PRIVATE | ||
phare_core | ||
MPI::MPI_C | ||
${GTEST_LIBS} | ||
) | ||
|
||
add_phare_test(${PROJECT_NAME} ${CMAKE_CURRENT_BINARY_DIR}) |
Check notice
Code scanning / CodeQL
Nested loops with same variable Note
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it works, because of scoping, but it's not clear