Expose env var handling to python #856
Conversation
WalkthroughWalkthroughThe changes introduce extensive enhancements to environment variable management and testing within the PHARE namespace. This includes new structures and functions for handling environment variables, MPI lifecycle management, and type conversion utilities. Additionally, various files integrate the newly added environment handling capabilities, and new tests verify these functionalities. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Simulator
participant Env
participant Var
User->>Simulator: Start simulation
Simulator->>Env: Get PHARE_SCOPE_TIMING
Env->>Var: Retrieve value
Var-->>Env: Return timing setting
Env-->>Simulator: Return scope timing value
Simulator-->>User: Run simulation with scope timing
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 Configration 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}
| for k, v in cpp_etc_lib().phare_env_vars().items(): | ||
| print(f"{k}: {v.desc}") | ||
| print("Options:") | ||
| for k, v in v.options: |
Check notice
Code scanning / CodeQL
Nested loops with same variable Note
There was a problem hiding this comment.
I think it works, because of scoping, but it's not clear
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.
| @@ -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.
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
‼️ 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.
| if (auto e = Env::INSTANCE().PHARE_SCOPE_TIMING(); e == "1" || e == "true") | |
| auto envSetting = Env::INSTANCE().PHARE_SCOPE_TIMING(); | |
| if (envSetting == "1" || envSetting == "true") |
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.
| 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("") | ||
|
|
There was a problem hiding this comment.
Resolve variable shadowing in nested loops.
The nested loop uses the same variable names k and v for both the outer and inner loops, which can lead to confusion and errors due to variable shadowing. Consider using different variable names for the inner loop.
- for k, v in v.options:
+ for option_key, option_value in v.options:
+ print(f" {option_key}: {option_value}")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.
| 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("") | |
| def print_env_vars_info(): | |
| for k, v in cpp_etc_lib().phare_env_vars().items(): | |
| print(f"{k}: {v.desc}") | |
| print("Options:") | |
| for option_key, option_value in v.options: | |
| print(f" {option_key}: {option_value}") | |
| print("") |
Tools
Ruff
47-47: Loop control variable
voverrides iterable it iterates (B020)
| 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:: |
For #831
To allow python to know about env vars from C++
suggestive as is
Summary by CodeRabbit
New Features
Enhancements
Testing
Envclass to verify logging behavior under different configurations.Documentation