Expose env var handling to python #856
Conversation
WalkthroughWalkthroughThe updates introduce a centralized handling system for environment variables in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Env
participant Var
User->>Env: Access INSTANCE()
Env-->>User: Return singleton instance
User->>Env: Call PHARE_LOG("PHARE_LOG_FILE")
Env->>Var: Fetch PHARE_LOG variable
Var-->>Env: Return PHARE_LOG value
Env-->>User: Return PHARE_LOG value
User->>Env: Call PHARE_SCOPE_TIMING()
Env->>Var: Fetch PHARE_SCOPE_TIMING variable
Var-->>Env: Return PHARE_SCOPE_TIMING value
Env-->>User: Return PHARE_SCOPE_TIMING value
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Outside diff range and nitpick comments (1)
src/simulator/simulator.hpp (1)
Line range hint
114-165: Refactor logging setup to ensure resource safety and exception safety.The current implementation of the logging setup in the
Simulatorclass might lead to resource leaks or undefined behavior in case of exceptions. Consider using RAII techniques to manage resources more safely.+ struct LogGuard { + std::ofstream& log_out; + std::streambuf* original_buf; + LogGuard(std::ofstream& log) : log_out(log), original_buf(std::cout.rdbuf(log.rdbuf())) {} + ~LogGuard() { std::cout.rdbuf(original_buf); } + }; - if (auto log = Env::INSTANCE().PHARE_LOG(); log and log != "NONE") { + if (auto log = Env::INSTANCE().PHARE_LOG(); log and log != "NONE") { + LogGuard log_guard(log_out); + }
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (9)
- res/cmake/test.cmake (1 hunks)
- src/core/env.hpp (1 hunks)
- src/core/utilities/mpi_utils.cpp (1 hunks)
- src/core/utilities/mpi_utils.hpp (1 hunks)
- src/core/utilities/types.hpp (2 hunks)
- src/python3/cpp_etc.cpp (2 hunks)
- src/simulator/simulator.hpp (3 hunks)
- tests/core/utilities/env/CMakeLists.txt (1 hunks)
- tests/core/utilities/env/test_env.cpp (1 hunks)
Files skipped from review due to trivial changes (1)
- res/cmake/test.cmake
Additional context used
Path-based instructions (4)
src/core/env.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/core/utilities/mpi_utils.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/core/utilities/types.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/simulator/simulator.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
Additional comments not posted (9)
tests/core/utilities/env/CMakeLists.txt (3)
3-3: Proper project naming.The project name
test-phare-envis clear and follows conventional naming practices for test projects. Good job on this clarity.
1-1: Ensure CMake version compatibility.The specified CMake minimum version is 3.20.1. Verify that all CMake features used in this file are compatible with this version to prevent potential issues on systems with older CMake versions.
Verification successful
To verify the CMake version compatibility, we need to list all CMake commands used in the
CMakeLists.txtfile and check if any of them require a version higher than 3.20.1.
The commands listed in the
CMakeLists.txtfile are all compatible with CMake version 3.20.1. Here are the details:
cmake_minimum_required: Introduced before 3.20.1.project: Introduced before 3.20.1.set: Introduced before 3.20.1.add_executable: Introduced before 3.20.1.add_phare_test: Custom command, not a standard CMake command.target_include_directories: Introduced before 3.20.1.target_link_directories: Introduced before 3.20.1.target_link_libraries: Introduced before 3.20.1.All the commands used are compatible with CMake version 3.20.1 or earlier. Therefore, the specified CMake version is appropriate.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of CMake features introduced after version 3.20.1. ast-grep --lang cmake --pattern $'$_ > 3.20.1' ./tests/core/utilities/env/CMakeLists.txtLength of output: 200
Script:
#!/bin/bash # Description: List all unique CMake commands used in the CMakeLists.txt file. grep -oP '^\s*[^#]*\b(\w+)\b' ./tests/core/utilities/env/CMakeLists.txt | sort -uLength of output: 496
7-7: Check consistency in target configurations.Ensure that the executable name, link directories, and test setup are consistently using the
${PROJECT_NAME}variable. This maintains consistency and reduces hardcoding errors.Also applies to: 13-13, 21-21
Verification successful
The corrected script successfully identified the use of
${PROJECT_NAME}in target configurations within theCMakeLists.txtfile. The following instances were found:
target_include_directories(${PROJECT_NAME} PRIVATEtarget_link_directories(${PROJECT_NAME} PUBLIC ${MPI_LIBRARY_PATH})target_link_libraries(${PROJECT_NAME} PRIVATEThis indicates that
${PROJECT_NAME}is being used consistently in the target configurations.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Ensure consistent use of `${PROJECT_NAME}` in target configurations. rg --type cmake --multiline $'^target_.*${PROJECT_NAME}.*$' ./tests/core/utilities/env/CMakeLists.txtLength of output: 225
Script:
#!/bin/bash # Description: Ensure consistent use of `${PROJECT_NAME}` in target configurations. rg --type cmake --multiline '^target_.*\$\{PROJECT_NAME\}.*$' ./tests/core/utilities/env/CMakeLists.txtLength of output: 270
tests/core/utilities/env/test_env.cpp (3)
3-5: Correct inclusion of necessary headers.The inclusion of
core/env.hppandgtest/gtest.his essential for the tests, and it's done correctly. Including<cstdlib>for environment manipulation functions (setenv, etc.) is also appropriate.
41-46: Proper initialization and execution of tests.The main function correctly initializes Google Test and the MPI lifecycle, ensuring proper setup and teardown for MPI-dependent tests. This is crucial for tests that depend on MPI configurations.
10-22: Review the implementation of test cases.The test cases are well-structured and seem to correctly test the functionality of the environment variable handling. However, ensure that the expectations set in the tests (like
EXPECT_EQ(var->result(), ".log/" + std::to_string(core::mpi::rank()) + ".out")in lines 35-36) are valid under all test environments.Also applies to: 24-37
Verification successful
The search results indicate that
var->result()andcore::mpi::rank()are implemented in the codebase, and their usage in the test case seems consistent with their definitions. Specifically,var->result()returns a string that includes the rank fromcore::mpi::rank(), which matches the expectation in the test case.
src/core/env.hppcontains the implementation ofresult()which constructs a string usingcore::mpi::rank().src/core/env.hppand other files show the use ofcore::mpi::rank()to append the rank to strings, consistent with the test expectations.Therefore, the expectations set in the tests appear to be valid under all test environments.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the validity of expectations in test cases under different environments. # This script checks if the expected results are valid in all configurations. rg --type cpp 'EXPECT_EQ' ./tests/core/utilities/env/test_env.cppLength of output: 415
Script:
#!/bin/bash # Description: Gather details about var->result() and core::mpi::rank() # This script checks the implementation of var->result() and core::mpi::rank() # Find the definition of var->result() ast-grep --lang cpp --pattern $'function result' > result_impl.txt # Find the definition of core::mpi::rank() ast-grep --lang cpp --pattern $'namespace core { namespace mpi { function rank' > rank_impl.txt # Output the results cat result_impl.txt rank_impl.txtLength of output: 511
Script:
#!/bin/bash # Description: Gather details about var->result() and core::mpi::rank() # This script checks the implementation of var->result() and core::mpi::rank() # Find the definition of var->result() ast-grep --lang cpp --pattern 'function result()' > result_impl.txt # Find the definition of core::mpi::rank() ast-grep --lang cpp --pattern 'namespace core { namespace mpi { function rank()' > rank_impl.txt # Output the results cat result_impl.txt rank_impl.txtLength of output: 515
Script:
#!/bin/bash # Description: Gather details about var->result() and core::mpi::rank() # This script checks the implementation of var->result() and core::mpi::rank() # Search for the definition of var->result() rg --type cpp 'result()' > result_impl.txt # Search for the definition of core::mpi::rank() rg --type cpp 'core::mpi::rank()' > rank_impl.txt # Output the results cat result_impl.txt rank_impl.txtLength of output: 3954
src/core/utilities/mpi_utils.cpp (1)
30-30: Review the implementation of theanyfunction.The
anyfunction uses MPI_Allreduce to compute a global sum, effectively determining if any processes have the boolean valuetrue. This is a correct and efficient use of MPI for this purpose. Ensure that the function is used appropriately throughout the codebase.src/python3/cpp_etc.cpp (2)
5-5: Correct inclusion of environment handling header.The inclusion of
core/env.hppis necessary for the new environment variable handling functions. This is correctly done and ensures the Python module can interact with the C++ environment variables.
60-69: Review the new Python bindings for environment variable handling.The new functions (
phare_env_exists,phare_env_val,phare_env_result) are correctly implemented to interact with the C++ environment variable handling system. Ensure that these functions are used correctly in the Python code and handle potential exceptions, especially with the assertions used.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
src/hdf5/detail/h5/h5_file.hpp (2)
72-72: Consider adding a more descriptive function comment.The
file()function returns a reference toh5file_, which is used frequently within the class. Adding a comment explaining the purpose and usage of this function could enhance readability and maintainability.
Line range hint
72-149: General suggestion: Improve error handling and documentation.While the class effectively handles various data operations in an MPI environment, adding more detailed error messages and comprehensive documentation for each method could greatly improve maintainability and ease of use. This is particularly important in a complex environment like MPI where debugging can be challenging.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (10)
- res/cmake/test.cmake (1 hunks)
- src/core/env.hpp (1 hunks)
- src/core/utilities/mpi_utils.cpp (1 hunks)
- src/core/utilities/mpi_utils.hpp (1 hunks)
- src/core/utilities/types.hpp (2 hunks)
- src/hdf5/detail/h5/h5_file.hpp (2 hunks)
- src/python3/cpp_etc.cpp (2 hunks)
- src/simulator/simulator.hpp (3 hunks)
- tests/core/utilities/env/CMakeLists.txt (1 hunks)
- tests/core/utilities/env/test_env.cpp (1 hunks)
Files skipped from review as they are similar to previous changes (9)
- res/cmake/test.cmake
- src/core/env.hpp
- src/core/utilities/mpi_utils.cpp
- src/core/utilities/mpi_utils.hpp
- src/core/utilities/types.hpp
- src/python3/cpp_etc.cpp
- src/simulator/simulator.hpp
- tests/core/utilities/env/CMakeLists.txt
- tests/core/utilities/env/test_env.cpp
Additional context used
Path-based instructions (1)
src/hdf5/detail/h5/h5_file.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (12)
- pyphare/pyphare/cpp/init.py (1 hunks)
- res/cmake/test.cmake (1 hunks)
- src/core/env.hpp (1 hunks)
- src/core/utilities/mpi_utils.cpp (1 hunks)
- src/core/utilities/mpi_utils.hpp (1 hunks)
- src/core/utilities/types.hpp (2 hunks)
- src/hdf5/detail/h5/h5_file.hpp (2 hunks)
- src/phare/phare.hpp (2 hunks)
- src/python3/cpp_etc.cpp (2 hunks)
- src/simulator/simulator.hpp (3 hunks)
- tests/core/utilities/env/CMakeLists.txt (1 hunks)
- tests/core/utilities/env/test_env.cpp (1 hunks)
Files skipped from review as they are similar to previous changes (10)
- res/cmake/test.cmake
- src/core/env.hpp
- src/core/utilities/mpi_utils.cpp
- src/core/utilities/mpi_utils.hpp
- src/core/utilities/types.hpp
- src/hdf5/detail/h5/h5_file.hpp
- src/python3/cpp_etc.cpp
- src/simulator/simulator.hpp
- tests/core/utilities/env/CMakeLists.txt
- tests/core/utilities/env/test_env.cpp
Additional context used
Path-based instructions (1)
src/phare/phare.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
Ruff
pyphare/pyphare/cpp/__init__.py
25-25: Local variable
erris assigned to but never used (F841)Remove assignment to unused variable
err
47-47: Loop control variable
voverrides iterable it iterates (B020)
GitHub Check: CodeQL
pyphare/pyphare/cpp/__init__.py
[notice] 47-47: Nested loops with same variable
Nested for statement uses loop variable 'k' of enclosing for statement.
Nested for statement uses loop variable 'v' of enclosing for statement.
Additional comments not posted (1)
pyphare/pyphare/cpp/__init__.py (1)
39-41: Approved: Functionenv_varslooks good.This function correctly fetches and returns the environment variables.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
pyphare/pyphare/cpp/__init__.py (1)
Line range hint
25-25: Remove unused variable assignment.The variable
erris assigned but never used, which is unnecessary and can be removed to clean up the code.- except ImportError as err: + except ImportError:Tools
Ruff
47-47: Loop control variable
voverrides iterable it iterates (B020)
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (12)
- pyphare/pyphare/cpp/init.py (1 hunks)
- res/cmake/test.cmake (1 hunks)
- src/core/env.hpp (1 hunks)
- src/core/utilities/mpi_utils.cpp (1 hunks)
- src/core/utilities/mpi_utils.hpp (1 hunks)
- src/core/utilities/types.hpp (2 hunks)
- src/hdf5/detail/h5/h5_file.hpp (2 hunks)
- src/phare/phare.hpp (2 hunks)
- src/python3/cpp_etc.cpp (2 hunks)
- src/simulator/simulator.hpp (3 hunks)
- tests/core/utilities/env/CMakeLists.txt (1 hunks)
- tests/core/utilities/env/test_env.cpp (1 hunks)
Files skipped from review due to trivial changes (1)
- src/simulator/simulator.hpp
Files skipped from review as they are similar to previous changes (10)
- res/cmake/test.cmake
- src/core/env.hpp
- src/core/utilities/mpi_utils.cpp
- src/core/utilities/mpi_utils.hpp
- src/core/utilities/types.hpp
- src/hdf5/detail/h5/h5_file.hpp
- src/phare/phare.hpp
- src/python3/cpp_etc.cpp
- tests/core/utilities/env/CMakeLists.txt
- tests/core/utilities/env/test_env.cpp
Additional context used
Ruff
pyphare/pyphare/cpp/__init__.py
25-25: Local variable
erris assigned to but never used (F841)Remove assignment to unused variable
err
47-47: Loop control variable
voverrides iterable it iterates (B020)
Additional comments not posted (1)
pyphare/pyphare/cpp/__init__.py (1)
39-40: New function to expose environment variables to Python.The function
env_vars()has been added to fetch environment variables from C++ through thecpp_etc_lib(). This is in line with the PR's objective to expose environment variable handling to Python.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
pyphare/pyphare/cpp/__init__.py (1)
Line range hint
25-25: Remove unused variableerr.The variable
erris assigned but never used, which can lead to cleaner code by removing it.- except ImportError as err: + except ImportError:Tools
Ruff
47-47: Loop control variable
voverrides iterable it iterates (B020)GitHub Check: CodeQL
[notice] 47-47: Nested loops with same variable
Nested for statement uses loop variable 'k' of enclosing for statement.
Nested for statement uses loop variable 'v' of enclosing for statement.src/simulator/simulator.hpp (1)
10-10: Ensure appropriate encapsulation of includes.Consider using forward declarations to reduce compile-time dependencies and improve compilation speed, especially for large projects like this one.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (12)
- pyphare/pyphare/cpp/init.py (1 hunks)
- res/cmake/test.cmake (1 hunks)
- src/core/env.hpp (1 hunks)
- src/core/utilities/mpi_utils.cpp (1 hunks)
- src/core/utilities/mpi_utils.hpp (1 hunks)
- src/core/utilities/types.hpp (2 hunks)
- src/hdf5/detail/h5/h5_file.hpp (2 hunks)
- src/phare/phare.hpp (2 hunks)
- src/python3/cpp_etc.cpp (2 hunks)
- src/simulator/simulator.hpp (3 hunks)
- tests/core/utilities/env/CMakeLists.txt (1 hunks)
- tests/core/utilities/env/test_env.cpp (1 hunks)
Files skipped from review due to trivial changes (1)
- src/hdf5/detail/h5/h5_file.hpp
Files skipped from review as they are similar to previous changes (9)
- res/cmake/test.cmake
- src/core/env.hpp
- src/core/utilities/mpi_utils.cpp
- src/core/utilities/mpi_utils.hpp
- src/core/utilities/types.hpp
- src/phare/phare.hpp
- src/python3/cpp_etc.cpp
- tests/core/utilities/env/CMakeLists.txt
- tests/core/utilities/env/test_env.cpp
Additional context used
Path-based instructions (1)
src/simulator/simulator.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
Ruff
pyphare/pyphare/cpp/__init__.py
25-25: Local variable
erris assigned to but never used (F841)Remove assignment to unused variable
err
47-47: Loop control variable
voverrides iterable it iterates (B020)
GitHub Check: CodeQL
pyphare/pyphare/cpp/__init__.py
[notice] 47-47: Nested loops with same variable
Nested for statement uses loop variable 'k' of enclosing for statement.
Nested for statement uses loop variable 'v' of enclosing for statement.
Additional comments not posted (2)
src/simulator/simulator.hpp (2)
114-114: Review logging initialization.The use of
Env::INSTANCE().PHARE_LOG("PHARE_LOG_FILE")for initializinglog_outis a good practice as it centralizes environment variable management. Ensure that thePHARE_LOG_FILEis appropriately managed and documented in the environment configuration.
165-165: Check conditional logging setup.The conditional setup for logging based on the
PHARE_LOGenvironment variable is a robust design choice. It allows for flexible logging configuration, which can be especially useful in a simulation environment. Ensure that theNONEoption is clearly documented and handled in all relevant parts of the application.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
pyphare/pyphare/cpp/__init__.py (1)
Line range hint
25-25: Remove assignment to unused variable.The variable
erris assigned in the except block but never used, which is unnecessary. You can omit it to clean up the code.- except ImportError as err: + except ImportError:Tools
Ruff
47-47: Loop control variable
voverrides iterable it iterates (B020)
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (12)
- pyphare/pyphare/cpp/init.py (1 hunks)
- res/cmake/test.cmake (1 hunks)
- src/core/env.hpp (1 hunks)
- src/core/utilities/mpi_utils.cpp (1 hunks)
- src/core/utilities/mpi_utils.hpp (1 hunks)
- src/core/utilities/types.hpp (2 hunks)
- src/hdf5/detail/h5/h5_file.hpp (2 hunks)
- src/phare/phare.hpp (2 hunks)
- src/python3/cpp_etc.cpp (2 hunks)
- src/simulator/simulator.hpp (3 hunks)
- tests/core/utilities/env/CMakeLists.txt (1 hunks)
- tests/core/utilities/env/test_env.cpp (1 hunks)
Files skipped from review due to trivial changes (1)
- src/simulator/simulator.hpp
Files skipped from review as they are similar to previous changes (10)
- res/cmake/test.cmake
- src/core/env.hpp
- src/core/utilities/mpi_utils.cpp
- src/core/utilities/mpi_utils.hpp
- src/core/utilities/types.hpp
- src/hdf5/detail/h5/h5_file.hpp
- src/phare/phare.hpp
- src/python3/cpp_etc.cpp
- tests/core/utilities/env/CMakeLists.txt
- tests/core/utilities/env/test_env.cpp
Additional context used
Ruff
pyphare/pyphare/cpp/__init__.py
25-25: Local variable
erris assigned to but never used (F841)Remove assignment to unused variable
err
47-47: Loop control variable
voverrides iterable it iterates (B020)
Additional comments not posted (1)
pyphare/pyphare/cpp/__init__.py (1)
39-40: Expose environment variables to Python.This function provides a Python interface to access environment variables managed by the C++ layer, aligning with the PR's objectives.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
pyphare/pyphare/cpp/__init__.py (1)
Line range hint
25-25: Enhance error handling by using the error information.The variable
erris caught but not used. It's good practice to use the error information to provide more context in the error handling.- except ImportError as err: + except ImportError as err: + print(f"Failed to import pybindlibs.cpp_dbg: {err}") + raiseTools
Ruff
47-47: Loop control variable
voverrides iterable it iterates (B020)
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (12)
- pyphare/pyphare/cpp/init.py (1 hunks)
- res/cmake/test.cmake (1 hunks)
- src/core/env.hpp (1 hunks)
- src/core/utilities/mpi_utils.cpp (1 hunks)
- src/core/utilities/mpi_utils.hpp (1 hunks)
- src/core/utilities/types.hpp (2 hunks)
- src/hdf5/detail/h5/h5_file.hpp (2 hunks)
- src/phare/phare.hpp (2 hunks)
- src/python3/cpp_etc.cpp (2 hunks)
- src/simulator/simulator.hpp (3 hunks)
- tests/core/utilities/env/CMakeLists.txt (1 hunks)
- tests/core/utilities/env/test_env.cpp (1 hunks)
Files skipped from review due to trivial changes (1)
- src/simulator/simulator.hpp
Files skipped from review as they are similar to previous changes (10)
- res/cmake/test.cmake
- src/core/env.hpp
- src/core/utilities/mpi_utils.cpp
- src/core/utilities/mpi_utils.hpp
- src/core/utilities/types.hpp
- src/hdf5/detail/h5/h5_file.hpp
- src/phare/phare.hpp
- src/python3/cpp_etc.cpp
- tests/core/utilities/env/CMakeLists.txt
- tests/core/utilities/env/test_env.cpp
Additional context used
Ruff
pyphare/pyphare/cpp/__init__.py
25-25: Local variable
erris assigned to but never used (F841)Remove assignment to unused variable
err
47-47: Loop control variable
voverrides iterable it iterates (B020)
Additional comments not posted (1)
pyphare/pyphare/cpp/__init__.py (1)
39-40: Functionenv_varsis correctly implemented.This function properly delegates to the
cpp_etc_libto fetch environment variables, aligning with the PR's objectives to expose environment variable handling to Python.
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range, codebase verification and nitpick comments (5)
tests/core/utilities/env/test_env.cpp (2)
10-23: Consider adding more assertions to cover edge cases.The test function verifies the basic functionality of setting and retrieving environment variables. Consider adding assertions to cover edge cases, such as:
- Unsetting an environment variable and ensuring it returns the default value.
- Setting an environment variable to an empty string and verifying the behavior.
26-32: Consider adding more assertions to cover additional scenarios.The test function verifies the logging functionality when the
PHARE_LOGenvironment variable is set toRANK_FILES. Consider adding assertions to cover additional scenarios, such as:
- Verifying the behavior when
PHARE_LOGis set to other valid values (e.g.,DATETIME_FILES,NONE).- Ensuring that the log file is created and contains the expected content.
src/core/utilities/mpi_utils.cpp (1)
Line range hint
30-35:
Consider adding comments to explain the purpose and usage of the function.The function performs an MPI reduction to check if any process has a true value. Adding comments to explain its purpose and usage would improve readability and maintainability.
pyphare/pyphare/cpp/__init__.py (1)
43-51: Consider adding comments to explain the purpose and usage of the function.The function prints information about environment variables and their options. Adding comments to explain its purpose and usage would improve readability and maintainability.
src/phare/phare.hpp (1)
Line range hint
15-46:
Consider adding comments to explain the purpose and usage of the constructor.The constructor initializes the SAMRAI MPI and Manager, sets up logging, and initializes the scope timer if the
PHARE_SCOPE_TIMINGenvironment variable is set. Adding comments to explain its purpose and usage would improve readability and maintainability.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (12)
- pyphare/pyphare/cpp/init.py (2 hunks)
- res/cmake/test.cmake (1 hunks)
- src/core/env.hpp (1 hunks)
- src/core/utilities/mpi_utils.cpp (1 hunks)
- src/core/utilities/mpi_utils.hpp (1 hunks)
- src/core/utilities/types.hpp (2 hunks)
- src/hdf5/detail/h5/h5_file.hpp (1 hunks)
- src/phare/phare.hpp (2 hunks)
- src/python3/cpp_etc.cpp (2 hunks)
- src/simulator/simulator.hpp (3 hunks)
- tests/core/utilities/env/CMakeLists.txt (1 hunks)
- tests/core/utilities/env/test_env.cpp (1 hunks)
Files skipped from review due to trivial changes (2)
- res/cmake/test.cmake
- tests/core/utilities/env/CMakeLists.txt
Additional context used
Path-based instructions (6)
src/phare/phare.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/core/env.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/core/utilities/mpi_utils.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/hdf5/detail/h5/h5_file.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/core/utilities/types.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/simulator/simulator.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
Additional comments not posted (15)
tests/core/utilities/env/test_env.cpp (1)
36-41: LGTM!The
mainfunction is correctly set up for running the tests.pyphare/pyphare/cpp/__init__.py (2)
39-41: LGTM!The function is correctly implemented.
52-54: LGTM!The function is correctly implemented.
src/phare/phare.hpp (2)
Line range hint
48-55:
LGTM!The destructor is correctly implemented.
Line range hint
57-65:
LGTM!The method is correctly implemented.
src/python3/cpp_etc.cpp (3)
2-7: Ensure necessary imports.The new imports for
core/env.hppandcore/def/phare_config.hppare necessary for the added functionality. Ensure that these headers are required and correctly placed.
14-15: Opaque type definition forstd::unordered_map.The opaque type definition for
std::unordered_map<std::string, PHARE::env::Var*>is correctly defined and necessary for the functionality.
61-75: New functions and bindings for environment variable handling.The new functions
phare_env_exists,phare_env_val, andphare_env_varsare correctly implemented and necessary for the functionality. The bindings forenv::Varare also correctly defined.src/core/env.hpp (3)
1-15: Ensure necessary includes and namespace declarations.The new includes and namespace declarations for environment variable handling are necessary and correctly placed.
17-49: StructVarfor handling environment variables.The struct
Varis correctly defined and necessary for handling environment variables. The use of member variables and functions is appropriate.
55-114: ClassEnvfor managing environment variables.The class
Envis correctly defined and necessary for managing environment variables. The use of member variables, functions, and singleton pattern is appropriate.src/core/utilities/types.hpp (2)
258-264: LGTM! Ensurefrom_stringhandles errors.The
get_env_asfunction is correctly implemented.Ensure that the
from_stringfunction handles conversion errors appropriately.
267-273: LGTM! Ensurefrom_stringhandles errors.The
get_env_asfunction with a default value is correctly implemented.Ensure that the
from_stringfunction handles conversion errors appropriately.src/simulator/simulator.hpp (2)
114-114: LGTM! Ensure robust environment variable handling.The logging initialization in the
Simulatorclass is correctly implemented.Ensure that the environment variable handling in
Env::INSTANCE().PHARE_LOGis robust and handles potential issues gracefully.
Line range hint
165-170:
LGTM! Ensure robust environment variable handling.The logging function in the anonymous namespace is correctly implemented.
Ensure that the environment variable handling in
Env::INSTANCE().PHARE_LOGis robust and handles potential issues gracefully.
There was a problem hiding this comment.
Actionable comments posted: 8
Outside diff range, codebase verification and nitpick comments (3)
tests/core/utilities/test_mpi_utils.hpp (1)
15-23: Improve error message for MPI initialization failure.Provide more context in the error message to help with debugging.
- std::cerr << "MPI Initialization failed with error code: " << err << std::endl; + std::cerr << "MPI Initialization failed with error code: " << err << ". Please ensure that MPI is correctly installed and configured." << std::endl;tests/core/utilities/env/test_env.cpp (1)
24-24: Add a comment to explain the default value check.Adding a comment will improve readability and help others understand the purpose of the check.
+ // Check the default value of PHARE_SCOPE_TIMING EXPECT_EQ(env.PHARE_SCOPE_TIMING(), "0"); // defaultpyphare/pyphare/cpp/__init__.py (1)
43-45: Add a comment to explain the source of environment variables.Adding a comment will improve readability and help others understand where the environment variables are coming from.
+ // Retrieve environment variables from cpp_etc_lib().phare_env_vars() for env_var_name, env_var in cpp_etc_lib().phare_env_vars().items():
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (13)
- pyphare/pyphare/cpp/init.py (2 hunks)
- res/cmake/test.cmake (1 hunks)
- src/core/env.hpp (1 hunks)
- src/core/utilities/mpi_utils.cpp (1 hunks)
- src/core/utilities/mpi_utils.hpp (1 hunks)
- src/core/utilities/types.hpp (2 hunks)
- src/hdf5/detail/h5/h5_file.hpp (2 hunks)
- src/phare/phare.hpp (2 hunks)
- src/python3/cpp_etc.cpp (2 hunks)
- src/simulator/simulator.hpp (3 hunks)
- tests/core/utilities/env/CMakeLists.txt (1 hunks)
- tests/core/utilities/env/test_env.cpp (1 hunks)
- tests/core/utilities/test_mpi_utils.hpp (1 hunks)
Files skipped from review due to trivial changes (4)
- src/core/utilities/mpi_utils.cpp
- src/core/utilities/mpi_utils.hpp
- src/hdf5/detail/h5/h5_file.hpp
- tests/core/utilities/env/CMakeLists.txt
Additional context used
Path-based instructions (5)
tests/core/utilities/test_mpi_utils.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/phare/phare.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/core/env.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/core/utilities/types.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.src/simulator/simulator.hpp (1)
Pattern
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.
Additional comments not posted (16)
tests/core/utilities/env/test_env.cpp (1)
28-34: LGTM!The test function correctly checks the logging functionality with rank files.
pyphare/pyphare/cpp/__init__.py (3)
25-25: LGTM!The change simplifies error handling without altering the control flow.
39-40: LGTM!The function correctly retrieves the environment variable configurations.
Line range hint
55-56:
LGTM!The function correctly retrieves the splitter type based on dimensions, interpolation, and number of particles.
src/phare/phare.hpp (2)
7-7: LGTM! Necessary include directive.The inclusion of
core/env.hppis necessary to support the new method of accessing the environment variable.
46-46: LGTM! Improved encapsulation and maintainability.The change to use
Env::INSTANCE().PHARE_SCOPE_TIMING()enhances encapsulation and maintainability by centralizing environment variable management within theEnvclass instance.res/cmake/test.cmake (1)
21-21: LGTM! Expanded testing framework.The addition of
add_subdirectory(tests/core/utilities/env)expands the testing framework by incorporating additional test cases or functionalities related to theenvutilities.src/python3/cpp_etc.cpp (7)
2-3: LGTM! Necessary include directives.The inclusion of
pybind11/stl.h,pybind11/stl_bind.h, andcore/env.hppis necessary to support the new functionalities introduced in the file.Also applies to: 7-7
14-14: LGTM! Namespace alignment.The update to use
env::Var*instead ofPHARE::env::Var*aligns with the new namespace usage for theenvmodule.
61-62: LGTM! Enhanced functionality.The addition of the method
phare_env_existsenhances the module's functionality by providing a way to check for the existence of environment variables.
63-63: LGTM! Enhanced functionality.The addition of the method
phare_env_valenhances the module's functionality by providing a way to retrieve the value of environment variables.
64-69: LGTM! Enhanced functionality.The addition of the class
phare_env_varenhances the module's functionality by providing access to the properties of environment variables directly from Python.
71-71: LGTM! Enhanced functionality.The addition of the macro
py::bind_mapenhances the module's functionality by enabling Python code to manipulate the map of environment variables.
73-75: LGTM! Enhanced performance and usability.The addition of the method
phare_env_varsenhances performance and usability by avoiding unnecessary copies.src/simulator/simulator.hpp (2)
Line range hint
171-175:
Ensure proper handling of logging conditions.The logging condition checks if the retrieved log variable is not only non-empty but also not equal to
"NONE". Ensure that this condition is correctly handled and does not lead to any runtime errors.Verification successful
Logging condition is properly handled.
The logging condition checks if the retrieved log variable is not empty and not equal to "NONE". The
PHARE_LOGfunction in theEnvclass ensures that the value is valid or throws an error. Additionally, thestd::cout.rdbuf()is correctly restored in the destructor, preventing runtime errors.
src/core/env.hpp: Definition ofPHARE_LOGin theEnvclass.src/simulator/simulator.hpp: Proper handling ofstd::cout.rdbuf()in the destructor.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the logging conditions in the Simulator class. # Test: Search for the logging conditions. Expect: Proper handling without runtime errors. rg --type cpp -A 5 $'logging'Length of output: 1329
Script:
#!/bin/bash # Description: Verify the definition and return type of Env::INSTANCE().PHARE_LOG() # Search for the definition of PHARE_LOG in the Env class ast-grep --lang cpp --pattern $'class Env { $$$ auto PHARE_LOG() const { $$$ } $$$ }'Length of output: 443
Script:
#!/bin/bash # Description: Verify the context and usage of log_out in the logging method # Search for the usage of log_out in the src/simulator/simulator.hpp file rg --type cpp log_out -A 5 -B 5 src/simulator/simulator.hppLength of output: 1289
Script:
#!/bin/bash # Description: Verify the handling of std::cout.rdbuf() to ensure no runtime errors # Search for the usage of std::cout.rdbuf() in the src/simulator/simulator.hpp file rg --type cpp 'std::cout.rdbuf(' -A 5 -B 5 src/simulator/simulator.hppLength of output: 155
Script:
#!/bin/bash # Description: Verify the definition and return type of Env::INSTANCE().PHARE_LOG() # Search for the definition of PHARE_LOG in the Env class rg --type cpp 'PHARE_LOG' -A 5 -B 5Length of output: 72270
Script:
#!/bin/bash # Description: Verify the handling of std::cout.rdbuf() to ensure no runtime errors # Correct the regex and search for the usage of std::cout.rdbuf() in the src/simulator/simulator.hpp file rg --type cpp 'std::cout\.rdbuf\(' -A 5 -B 5 src/simulator/simulator.hppLength of output: 809
120-120: Ensure proper initialization oflog_out.The
log_outmember variable is initialized usingEnv::INSTANCE().PHARE_LOG("PHARE_LOG_FILE", ""). Ensure that this initialization does not lead to any runtime errors or undefined behavior.Verification successful
Initialization of
log_outis properly handled.The
log_outmember variable is initialized usingEnv::INSTANCE().PHARE_LOG("PHARE_LOG_FILE", ""), which is correctly defined and provides valid logging configurations. There should be no runtime errors or undefined behavior related to this initialization.
src/core/env.hpp: Definition ofEnvclass andPHARE_LOGvariable.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the initialization of log_out in the Simulator class. # Test: Search for the usage of log_out. Expect: Proper initialization and usage without runtime errors. rg --type cpp -A 5 $'log_out'Length of output: 1555
Script:
#!/bin/bash # Description: Verify the definition and behavior of the PHARE_LOG method in the Env class. # Search for the definition of the PHARE_LOG method in the Env class. ast-grep --lang cpp --pattern $'class Env { $$$ auto PHARE_LOG($_, $_) { $$$ } $$$ }'Length of output: 443
Script:
#!/bin/bash # Description: Verify the definition and behavior of the PHARE_LOG method in the Env class. # Search for the definition of the Env class. ast-grep --lang cpp --pattern $'class Env { $$$ }' # Search for the PHARE_LOG method in the codebase. rg --type cpp 'PHARE_LOG'Length of output: 12234
Script:
#!/bin/bash # Description: Verify the definition and behavior of the PHARE_LOG method in the Env class. # Search for the definition of the Env class using a more general pattern. rg --type cpp 'class Env' # Search for the PHARE_LOG method or macro definition across the codebase. rg --type cpp 'PHARE_LOG'Length of output: 11890
Script:
#!/bin/bash # Description: Inspect the Env class and the definition of PHARE_LOG in src/core/env.hpp. # Display the Env class definition in src/core/env.hpp. rg --type cpp -A 20 'class Env' src/core/env.hpp # Display the definition of PHARE_LOG in src/core/env.hpp. rg --type cpp -A 5 'PHARE_LOG' src/core/env.hppLength of output: 1674
| ~Lifecycle() | ||
| { | ||
| if (!error_occurred) | ||
| { | ||
| int err = MPI_Finalize(); | ||
| if (err != MPI_SUCCESS) | ||
| { | ||
| std::cerr << "MPI Finalization failed with error code: " << err << std::endl; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Check MPI_Finalize return value even if error_occurred is true.
It's good practice to always check the return value of MPI_Finalize to ensure that resources are properly released.
- if (!error_occurred)
- {
- int err = MPI_Finalize();
- if (err != MPI_SUCCESS)
- {
- std::cerr << "MPI Finalization failed with error code: " << err << std::endl;
- }
- }
+ int err = MPI_Finalize();
+ if (err != MPI_SUCCESS)
+ {
+ std::cerr << "MPI Finalization failed with error code: " << err << std::endl;
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ~Lifecycle() | |
| { | |
| if (!error_occurred) | |
| { | |
| int err = MPI_Finalize(); | |
| if (err != MPI_SUCCESS) | |
| { | |
| std::cerr << "MPI Finalization failed with error code: " << err << std::endl; | |
| } | |
| } | |
| } | |
| ~Lifecycle() | |
| { | |
| int err = MPI_Finalize(); | |
| if (err != MPI_SUCCESS) | |
| { | |
| std::cerr << "MPI Finalization failed with error code: " << err << std::endl; | |
| } | |
| } |
| 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(); } | ||
| }; |
There was a problem hiding this comment.
Consider using explicit constructors for Var struct to enhance type safety.
The Var struct is initialized with several member variables that are likely to be required upon creation. Consider using an explicit constructor to ensure that all necessary fields are initialized correctly and to improve code readability.
struct Var
{
explicit Var(std::string_view id, std::string_view desc, std::vector<std::pair<std::string_view, std::string_view>> options, std::optional<value_type> def = std::nullopt, std::function<results_type(Var const&)> res = noop_results)
: id{id}, desc{desc}, options{std::move(options)}, _default{def}, _results{res}, v{get()}, 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); | ||
| } |
There was a problem hiding this comment.
Ensure proper error handling in get method.
The get method retrieves environment variables but does not handle potential errors from core::get_env. Consider adding error handling or logging to capture any issues that may arise.
std::optional<value_type> get() const
{
std::string _id{id};
try {
if (_default)
return core::get_env(_id, *_default);
return core::get_env(_id);
} catch (const std::exception& e) {
// Handle or log the error
return std::nullopt;
}
}| 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; | ||
| }; |
There was a problem hiding this comment.
Consider making Env class a singleton.
The Env class is designed to be instantiated only once. Consider making it a singleton to enforce this design pattern and simplify access to the instance.
class Env
{
public:
static Env& INSTANCE()
{
static Env instance;
return instance;
}
// Delete copy constructor and assignment operator
Env(const Env&) = delete;
Env& operator=(const Env&) = delete;
// Other methods and member variables...
};There was a problem hiding this comment.
it is a singleton, just in this case we can reset the internal memory easily
There was a problem hiding this comment.
@PhilipDeegan, thank you for the clarification. Your approach to implementing the singleton pattern while allowing for the internal memory to be reset provides the necessary flexibility for reinitialization scenarios. This design ensures that the Env instance can be reinitialized when needed, which can be beneficial in certain contexts.
| 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; | ||
| } // | ||
| }; |
There was a problem hiding this comment.
Add validation for PHARE_LOG options.
The PHARE_LOG variable has specific valid options. Consider adding validation to ensure that only valid options are accepted.
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;
} //
};| template<typename T> | ||
| NO_DISCARD inline std::optional<T> get_env_as(std::string const& key) | ||
| { | ||
| if (auto e = get_env(key)) | ||
| return from_string<T>(*e); | ||
| return std::nullopt; | ||
| } |
There was a problem hiding this comment.
Handle potential conversion errors in get_env_as.
The get_env_as function should handle potential conversion errors to avoid undefined behavior and provide meaningful error messages.
template<typename T>
NO_DISCARD inline std::optional<T> get_env_as(std::string const& key)
{
if (auto e = get_env(key))
{
try {
return from_string<T>(*e);
} catch (const std::exception& ex) {
std::cerr << "Error converting environment variable " << key << ": " << ex.what() << std::endl;
}
}
return std::nullopt;
}| template<typename T> | ||
| NO_DISCARD inline T get_env_as(std::string const& key, T const& t) | ||
| { | ||
| if (auto e = get_env(key)) | ||
| return from_string<T>(*e); | ||
| return t; | ||
| } |
There was a problem hiding this comment.
Handle potential conversion errors in get_env_as with default value.
The get_env_as function should handle potential conversion errors to avoid undefined behavior and provide meaningful error messages.
template<typename T>
NO_DISCARD inline T get_env_as(std::string const& key, T const& t)
{
if (auto e = get_env(key))
{
try {
return from_string<T>(*e);
} catch (const std::exception& ex) {
std::cerr << "Error converting environment variable " << key << ": " << ex.what() << std::endl;
}
}
return t;
}
For #831
To allow python to know about env vars from C++
suggestive as is
Summary by CodeRabbit
New Features
Bug Fixes
Tests