Mpi nightly build trials #917
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications primarily to enhance error handling in the Changes
Assessment against linked issues
Possibly related PRs
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
pyphare/pyphare/pharein/simulation.py (1)
501-501: The fix looks good but could be more robust.The added condition helps prevent the KeyError when "options" is commented out, which addresses the issue #896. However, we can make the code more robust by combining the conditions.
Consider this more concise and robust approach:
- if "options" in diag_options and "mode" in diag_options["options"]: + if diag_options.get("options", {}).get("mode"):This change:
- Uses
dict.get()with a default empty dict to safely handle missing "options"- Chains another
get()to safely access "mode"- Reduces nesting and improves readability
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- pyphare/pyphare/pharein/simulation.py (1 hunks)
- tests/functional/harris/CMakeLists.txt (1 hunks)
- tests/functional/harris/harris_2d.py (0 hunks)
- tests/simulator/refinement/CMakeLists.txt (1 hunks)
💤 Files with no reviewable changes (1)
- tests/functional/harris/harris_2d.py
🔇 Additional comments (4)
tests/functional/harris/CMakeLists.txt (2)
11-13: LGTM: Improved readability with proper spacing.The added empty lines and clear comment about HighFive dependency improve the readability of the CMake configuration.
Also applies to: 17-17
15-15: Verify the impact of reducing MPI processes.The reduction from 10 to 4 MPI processes could affect test coverage and execution characteristics. Please ensure this change:
- Maintains adequate parallel execution testing
- Doesn't compromise the test's ability to detect race conditions
- Aligns with the minimum required processes for the harris_2d test
✅ Verification successful
Let me gather more information about the harris_2d test to understand its MPI requirements better.
Let me check the MPI execution configuration pattern across the codebase to understand if there's a standard requirement.
Reducing MPI processes from 10 to 4 is acceptable for this test
Based on the analysis of the codebase:
- The harris_2d test is a 2D simulation that doesn't have any strict MPI process count requirements in its implementation
- Other similar 2D tests in the codebase run with 3-4 MPI processes (e.g., test_diagnostics.py runs with both 3 and 4 processes)
- The test's grid configuration (200x400 cells) can be effectively parallelized with 4 processes
- The reduction aligns with the general pattern seen in other tests where most MPI tests use between 3-4 processes
The change appears to be part of a broader effort to standardize MPI process counts across tests, making the test suite more resource-efficient while maintaining adequate parallel execution coverage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any configuration or documentation that might specify process requirements rg -g '!*.pyc' -g '!*.so' "harris_2d" -A 5 -B 5Length of output: 1312
Script:
#!/bin/bash # Look for the actual test file content and any configuration cat tests/functional/harris/harris_2d.py # Search for any MPI-related patterns in the test rg -g '!*.pyc' -g '!*.so' "mpi" -i tests/functional/harris/Length of output: 4950
Script:
#!/bin/bash # Look for other phare_mpi_python3_exec usages to understand process count patterns rg "phare_mpi_python3_exec" -A 1 -B 1 # Check the CMake function definition fd -e cmake phare_mpi_python3_execLength of output: 4022
tests/simulator/refinement/CMakeLists.txt (2)
19-19: Verify test parameters and dependenciesThe new test execution looks good, but we should ensure it's an adequate replacement for the disabled MPI test.
Let's verify the test script exists and check its contents:
#!/bin/bash # Description: Verify test script existence and contents test_file="${PWD}/tests/simulator/refinement/test_2d_2_core.py" if [ -f "$test_file" ]; then echo "Test file exists" # Look for specific test configurations rg -A 5 "def test|class.*Test" "$test_file" else echo "Test file not found" fi
13-16: LGTM! Well-documented test disablingThe comment properly references the GitHub issue (#853) explaining why the MPI test was disabled, which helps maintain code clarity and traceability.
Let's verify if the referenced issue is still open and relevant:
✅ Verification successful
Let me try to gather more information about this issue and the test files to provide a better verification.
Let me try one more verification to check the content of both test files to understand the relationship between them:
Disabled test is properly documented and replaced with a simpler version
Based on the verification results, I can confirm that:
- The disabled test (
test_2d_10_core.py) and its replacement (test_2d_2_core.py) are similar tests that check Yee Lattice centering discrepancies- The main difference is that the disabled test was more complex and required 10 cores, while the new test is simpler and requires only 2 cores
- The issue reference (test_refine_field shape mismatch #853) is properly documented in the CMakeLists.txt file
Since we couldn't verify the GitHub issue status but found that the test has been replaced with a simpler version that tests the same functionality, the change appears to be a valid optimization of the test suite.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if issue #853 is still open gh issue view 853 --json state,titleLength of output: 292
Script:
#!/bin/bash # Check if the issue reference exists in git history or comments rg -i "issue.*853|#853" --type txt # Look for related test files fd "test_2d.*\.py" tests/simulator/refinement/ # Check the content of the commented test file rg "test_2d_10_core.py" tests/simulator/refinement/ -A 5Length of output: 1204
Script:
#!/bin/bash # Compare the content and structure of both test files echo "=== test_2d_2_core.py ===" head -n 20 tests/simulator/refinement/test_2d_2_core.py echo -e "\n=== test_2d_10_core.py ===" head -n 20 tests/simulator/refinement/test_2d_10_core.py # Check if these are similar test cases rg "class.*Test" tests/simulator/refinement/test_2d_2_core.py tests/simulator/refinement/test_2d_10_core.pyLength of output: 1284
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tests/functional/harris/harris_2d_lb.py (1)
18-18: Consider documenting the SCOPE_TIMING change impact.The default value change from
TruetoFalsefor SCOPE_TIMING means timing data won't be collected by default. While this might improve performance, it could affect debugging and performance analysis capabilities.Consider adding a comment explaining:
- Why timing is now opt-in rather than default
- How to enable timing when needed for debugging
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- tests/functional/harris/CMakeLists.txt (1 hunks)
- tests/functional/harris/harris_2d_lb.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/functional/harris/CMakeLists.txt
🔇 Additional comments (1)
tests/functional/harris/harris_2d_lb.py (1)
Line range hint
91-94: Verify numerical stability of temperature calculation.The temperature calculation involves division by density and subtraction operations which could potentially lead to numerical instability:
def T(x, y): K = 0.7 temp = 1.0 / density(x, y) * (K - b2(x, y) * 0.5) assert np.all(temp > 0) return tempLet's verify the numerical stability:
✅ Verification successful
Let me analyze the density implementation and related tests to verify the numerical stability concerns.
Temperature calculation is numerically stable
The density function implementation shows that it's numerically stable and cannot be zero:
- Base density of 0.4 (or 0.2 in some variants)
- Plus two positive hyperbolic secant terms that are always positive
- The hyperbolic secant terms use well-defined parameters (L, fixed positions at 0.3L and 0.7L)
The temperature calculation is also protected:
- Has an assertion
assert np.all(temp > 0)to catch any violations- Uses a constant K (0.7) and b2(x, y) term which is squared (thus non-negative)
- The density denominator is guaranteed to be positive and bounded away from zero
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any existing test cases that verify temperature bounds rg -l "def test.*temp" tests/ # Search for any existing assertions on density to ensure it's never zero rg "assert.*density" tests/Length of output: 61
Script:
#!/bin/bash # Search for density function implementation to understand its bounds ast-grep --pattern 'def density(x, y): $$$' # Search for any test files related to Harris model fd -e py harris tests/ # Look for any assertions or checks in the Harris model files rg "assert|check" -A 2 -B 2 tests/functional/harris/Length of output: 4649
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tests/simulator/CMakeLists.txt (1)
31-31: Remove extra whitespace for consistent indentation.There's an extra blank line with whitespace that should be removed to maintain consistent indentation throughout the file.
else() phare_python3_exec(11 test_diagnostic_timestamps test_diagnostic_timestamps.py ${CMAKE_CURRENT_BINARY_DIR}) - endif(testMPI)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
tests/functional/harris/CMakeLists.txt(1 hunks)tests/simulator/CMakeLists.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/functional/harris/CMakeLists.txt
🔇 Additional comments (1)
tests/simulator/CMakeLists.txt (1)
29-32: Verify the impact of moving test_diagnostic_timestamps execution.
The restructuring to execute test_diagnostic_timestamps.py only when testMPI is false looks logical. However, please verify that this change in test execution timing doesn't impact the test coverage or CI pipeline expectations.
✅ Verification successful
Moving test_diagnostic_timestamps.py to non-MPI execution is safe
After examining the test file and its history, this change is appropriate because:
- The test uses
startMPI()only for setup and doesn't rely on multiple MPI ranks - The test focuses on diagnostic timestamps and file I/O operations, not MPI-specific functionality
- Git history shows no MPI-specific requirements or past issues related to MPI execution
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if test_diagnostic_timestamps.py has any MPI-specific code or dependencies
# that might be affected by this change
# Search for MPI-related imports or usage
echo "Checking for MPI usage in test_diagnostic_timestamps.py:"
rg -i "mpi|comm|rank|size" test_diagnostic_timestamps.py
# Check if the test was previously being executed in CI logs
echo "Checking previous CI runs for test execution patterns:"
gh run list --limit 10 --json conclusion,name | jq '.[] | select(.name | contains("test-diagnostic-timestamps"))'
Length of output: 486
Script:
#!/bin/bash
# Let's first locate the test file and check its contents
echo "Locating test_diagnostic_timestamps.py:"
fd "test_diagnostic_timestamps.py"
# If found, let's examine its contents and dependencies
if fd "test_diagnostic_timestamps.py" -x cat {};
# Let's also check the CMake configuration for any related test setup
echo -e "\nChecking CMake test configurations:"
rg "test_diagnostic_timestamps" -A 3 -B 3
Length of output: 255
Script:
#!/bin/bash
# Now that we found the file, let's examine its contents
echo "Contents of test_diagnostic_timestamps.py:"
cat tests/simulator/test_diagnostic_timestamps.py
# Let's also check if this test was previously in a different location
echo -e "\nChecking git history for any moves:"
git log --full-history --follow tests/simulator/test_diagnostic_timestamps.py
# Check the broader context of the CMake configuration
echo -e "\nChecking broader CMake context:"
cat tests/simulator/CMakeLists.txt
Length of output: 9753
Closes #896
Wip
Summary by CodeRabbit
Bug Fixes
KeyErrorexceptions.New Features