run multiple simulations in // from python #64
Conversation
PhilipDeegan
force-pushed
the
simulators
branch
2 times, most recently
from
2f5d825 to
1dc320f
Compare
PhilipDeegan
force-pushed
the
simulators
branch
2 times, most recently
from
017173f to
50d8b6f
Compare
PhilipDeegan
force-pushed
the
simulators
branch
from
50d8b6f to
3e1349f
Compare
WalkthroughWalkthroughThe PyPHARE codebase has undergone several enhancements, including improvements to simulation management, comparison features, and build processes. Notably, the Changes
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 X ? TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 25
Configuration used: CodeRabbit UI
Files selected for processing (24)
- pyphare/pyphare/cpp/init.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy.py (2 hunks)
- pyphare/pyphare/pharesee/particles.py (1 hunks)
- pyphare/pyphare/pharesee/run.py (1 hunks)
- pyphare/pyphare/simulator/simulator.py (1 hunks)
- pyphare/pyphare/simulator/simulators.py (1 hunks)
- tests/functional/alfven_wave/alfven_wave1d.py (4 hunks)
- tests/simulator/init.py (1 hunks)
- tests/simulator/advance/test_fields_advance_1d.py (2 hunks)
- tests/simulator/advance/test_fields_advance_2d.py (2 hunks)
- tests/simulator/initialize/test_fields_init_1d.py (1 hunks)
- tests/simulator/initialize/test_fields_init_2d.py (3 hunks)
- tests/simulator/initialize/test_particles_init_1d.py (3 hunks)
- tests/simulator/initialize/test_particles_init_2d.py (2 hunks)
- tests/simulator/refinement/test_2d_10_core.py (1 hunks)
- tests/simulator/test_advance.py (7 hunks)
- tests/simulator/test_initialization.py (10 hunks)
- tests/simulator/test_restarts.py (6 hunks)
- tools/build_compare_2_branches.py (1 hunks)
- tools/build_compare_2_branches.sh (1 hunks)
- tools/build_compare_top_2_commits.py (1 hunks)
- tools/build_compare_top_2_commits.sh (1 hunks)
- tools/python3/cmake.py (1 hunks)
- tools/python3/git.py (2 hunks)
Files skipped from review due to trivial changes (7)
- pyphare/pyphare/cpp/init.py
- pyphare/pyphare/simulator/simulator.py
- tests/simulator/initialize/test_fields_init_1d.py
- tests/simulator/initialize/test_particles_init_2d.py
- tools/build_compare_2_branches.sh
- tools/build_compare_top_2_commits.py
- tools/build_compare_top_2_commits.sh
Additional comments: 44
pyphare/pyphare/pharesee/particles.py (1)
- 194-201: The changes made to the
all_assert_sortedfunction seem to be reasonable. The addition of thertolparameter to thenp.testing.assert_allclosecalls forpart1.deltasandpart2.deltaswith a value of 0 ensures that the relative tolerance between the two arrays is 0, meaning they must be exactly equal within the absolute tolerance. The change of theatolparameter for thenp.testing.assert_allclosecalls forpart1.vandpart2.vto 1e-12 makes the absolute tolerance stricter, which could be necessary depending on the precision required in your application. However, it's important to ensure that these changes don't cause any unintended side effects in your tests.tests/simulator/__init__.py (3)
240-241: The
runmethod has been modified to set the_outcomeattribute and then call the parent class'srunmethod. This change seems to be fine, but ensure that the_outcomeattribute is used correctly elsewhere in the code.243-247: The
unique_diag_dir_for_test_casemethod has been added. This method generates a unique diagnostic directory path. This is a good practice for avoiding conflicts when running multiple tests in parallel.240-251: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [249-262]
The
clean_up_diags_dirsmethod has been added. This method deletes the diagnostic directories after the tests have run. This is a good practice for cleaning up resources after tests. However, it's important to ensure that the directories are not needed for any post-test analysis or debugging.tests/simulator/initialize/test_particles_init_1d.py (3)
26-32: The class name change from
InitializationTesttoInitialization1DTestis appropriate as it better reflects the purpose of the class. Ensure that all references to this class in the codebase have been updated.45-54: The
ndimparameter has been removed from thegetHierarchyfunction call. This change is acceptable if thegetHierarchyfunction no longer requires this parameter. Ensure that the function definition has been updated accordingly and that all calls to this function throughout the codebase have been updated to match the new signature.85-98: The
ndimparameter has been removed from thegetHierarchyfunction call. This change is acceptable if thegetHierarchyfunction no longer requires this parameter. Ensure that the function definition has been updated accordingly and that all calls to this function throughout the codebase have been updated to match the new signature.tests/functional/alfven_wave/alfven_wave1d.py (1)
- 37-44: The
Simulationinstantiation has been updated to use the global variablesTIME_STEP_NBRandTIMESTEP. Ensure that these changes do not affect the behavior of the simulation. Also, consider adding comments to explain the purpose of these variables.128:
Theconfigfunction now returnsgv.sim. Ensure that all calls to this function throughout the codebase have been updated to handle the returned value.pyphare/pyphare/pharesee/run.py (1)
- 339-343: The loop is trying to call each function with the
timeargument. If any of these function calls fail, it simply passes and continues with the next function. This could potentially hide errors and lead to unexpected behavior. If these functions are expected to raise specific exceptions under normal circumstances, those exceptions should be caught and handled explicitly. If they're not expected to raise exceptions, then allowing the exception to propagate might be the correct behavior.tests/simulator/initialize/test_fields_init_2d.py (3)
5-11: The import of
numpyandmatplotlibis fine. However, ensure thatmatplotlibis used in the code, otherwise, it's an unnecessary import.19-25: The renaming of the class
InitializationTesttoInitialization2DTestis fine. Ensure that all references to this class in the codebase have been updated.37-43: The modification of the method
test_density_decreases_as_1overSqrtNto accept an additional argumentcellsis fine. Ensure that all calls to this method throughout the codebase have been updated to match the new signature.tools/python3/cmake.py (3)
13-26: The function
make_config_strhas been modified to use the global variable_SAMRAI_DIR. Ensure that this change does not affect other parts of the code that call this function.28-32: The function
confighas been modified to use the global variable_SAMRAI_DIR. Ensure that this change does not affect other parts of the code that call this function.35-36: The function
buildhas been modified to use the global variable_USE_NINJA. Ensure that this change does not affect other parts of the code that call this function.tools/python3/git.py (3)
2-2: The
atexitmodule is imported to register exit handlers. This is a good practice for cleaning up resources or resetting states when the script exits.32-37: The
run()function calls now include thecheck=Trueargument. This means that if the subprocess call returns a non-zero exit code, aCalledProcessErrorwill be raised. This is a good practice for error handling as it allows the script to fail fast when a subprocess call fails.39-44: The
git_branch_reset_at_exit()function is a new addition. It registers an exit handler that checks out the current Git branch when the script exits. This is a good practice for scripts that change the Git branch during their execution, as it ensures that the Git branch is reset to its original state even if the script fails or is interrupted.tests/simulator/test_restarts.py (5)
193-209: The variable
dimhas been renamed tondimfor clarity. Ensure that all references to this variable have been updated throughout the codebase.224-230: The function
unique_diag_dir_for_test_caseis now called with different arguments. Ensure that the function can handle these new arguments correctly.267-284: The variable
dimhas been renamed tondimfor clarity. Ensure that all references to this variable have been updated throughout the codebase.299-307: The function
unique_diag_dir_for_test_caseis now called with different arguments. Ensure that the function can handle these new arguments correctly.347-360: The function
unique_diag_dir_for_test_caseis now called with different arguments. Ensure that the function can handle these new arguments correctly.tests/simulator/advance/test_fields_advance_2d.py (2)
34-46: The
ndimparameter is now passed to thegetHierarchyfunction. Ensure that the function definition has been updated to accept this new parameter and that it is used correctly within the function.59-75: The
ndimparameter is also passed to thegetHierarchyfunction here. The same considerations apply as in the previous comment.pyphare/pyphare/pharesee/hierarchy.py (4)
171-185: The
comparemethod in theFieldDataclass is well implemented. It checks if the other object is an instance ofFieldData, asserts that their names are the same, and then compares their datasets. It handles NaN values correctly by creating a mask that excludes them. It usesnp.testing.assert_allclosein debug mode to compare the datasets, andnp.allcloseotherwise. If the comparison fails, it raises an exception with a helpful error message.188-198: The
meshgridmethod is also well implemented. It creates a meshgrid based on the dimensions of theFieldDatainstance and returns a selected subset of the meshgrid if a selection is provided.201-202: The
__eq__method is overridden to use thecomparemethod for equality comparison. This is a good practice as it allows for more complex comparison logic.1715-1750: The
hierarchy_comparefunction is well implemented. It checks if the two objects are instances ofPatchHierarchy, compares their dimensions and domain boxes, compares their time hierarchies, and then compares their patch levels and patch datas. If any of these comparisons fail, it returns False. If all comparisons pass, it returns True.tests/simulator/test_advance.py (4)
38-41: The
getHierarchyfunction signature has been updated to include a new parameterndim. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.55-58: The
diag_outputsparameter is now being set to the return value of theunique_diag_dir_for_test_casefunction. This change seems to be intended to ensure unique diagnostic directory paths for each test case. Ensure that this change does not affect any other parts of the codebase that rely on the previous value ofdiag_outputs.769-773: The
getHierarchyfunction is being called with the updated signature. This is consistent with the changes made to the function signature.56-59: The
global_vars.simis being set toNoneafter thediag_outputsis set. This seems to be a way to reset the simulation state before each test. Ensure that this does not have any unintended side effects on the tests.tests/simulator/advance/test_fields_advance_1d.py (2)
33-45: The
ndimparameter is now passed to thegetHierarchyfunction. Ensure that the function definition has been updated to accept this new argument and that all calls to this function throughout the codebase have been updated to match the new signature.57-73: The
ndimparameter is now passed to thegetHierarchyfunction. Ensure that the function definition has been updated to accept this new argument and that all calls to this function throughout the codebase have been updated to match the new signature.tests/simulator/test_initialization.py (9)
36-40: The
getHierarchyfunction now accepts an additional argumentndim. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.52-54: The
diag_outputsargument in thegetHierarchyfunction now has a default value of an empty string. This change is acceptable as long as the function can handle an empty string as a valid value fordiag_outputs.266-269: The
_test_B_is_as_provided_by_userfunction now accepts an additional argumentdim. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.331-336: The
_test_bulkvel_is_as_provided_by_userfunction now accepts an additional argumentdim. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.459-464: The
_test_density_decreases_as_1overSqrtNfunction now accepts additional argumentsdim,nbr_particles, andcells. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.529-554: The
_test_density_decreases_as_1overSqrtNfunction has been significantly modified. It now includes a loop that iterates overnbr_particlesand callsgetHierarchywith different parameters. This change seems to be a performance optimization, but it's important to verify that the new logic is correct and that it doesn't introduce any new issues.612-619: The
_test_nbr_particles_per_cell_is_as_providedfunction now accepts an additional argumentdim. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.652-656: The
_test_domainparticles_have_correct_split_from_coarser_particlefunction now accepts an additional argumentndim. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.702-708: The
_test_patch_ghost_on_refined_level_casefunction now accepts an additional argumentdim. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.
| import numpy as np | ||
| mpl.use('Agg') | ||
|
|
||
| # Override if you do want it seeded | ||
| MODEL_INIT={} | ||
| TIME_STEP_NBR=100000 | ||
| TIMESTEP=.01 |
There was a problem hiding this comment.
Global variables are generally discouraged in Python due to their mutable nature and the potential for unexpected behavior. Consider encapsulating these variables within a class or function, or passing them as arguments where necessary. However, if these variables are constants and their values do not change throughout the program, their usage can be justified.
| def GetAllAvailableQties(self, time, pops): | ||
| assert self.single_hier_for_all_quantities == True # can't work otherwise | ||
|
|
||
| self.GetParticles(time, pops) | ||
| self.GetB(time) | ||
| self.GetE(time) | ||
| self.GetNi(time) | ||
| self.GetVi(time) | ||
|
|
||
| for pop in pops: | ||
| self.GetFlux(time, pop) | ||
| self.GetN(time, pop) | ||
| for fn in [self.GetB, self.GetE, self.GetNi, self.GetVi]: | ||
| try: | ||
| fn(time) | ||
| except: | ||
| pass | ||
|
|
||
| try: | ||
| self.GetParticles(time, pops) | ||
| for pop in pops: | ||
| self.GetFlux(time, pop) | ||
| self.GetN(time, pop) | ||
| except: | ||
| pass | ||
|
|
||
| return self.hier |
There was a problem hiding this comment.
The use of a bare except: clause is generally discouraged as it can catch unexpected errors and make debugging more difficult. It's better to catch specific exceptions that you expect might be raised in the try block. If you're not sure what exceptions might be raised, you can catch Exception, which catches all non-exit exceptions. Also, it would be helpful to log or print the exception for debugging purposes.
- except:
+ except Exception as e:
+ print(f"An error occurred: {e}")Commitable suggestion (Beta)
| def GetAllAvailableQties(self, time, pops): | |
| assert self.single_hier_for_all_quantities == True # can't work otherwise | |
| self.GetParticles(time, pops) | |
| self.GetB(time) | |
| self.GetE(time) | |
| self.GetNi(time) | |
| self.GetVi(time) | |
| for pop in pops: | |
| self.GetFlux(time, pop) | |
| self.GetN(time, pop) | |
| for fn in [self.GetB, self.GetE, self.GetNi, self.GetVi]: | |
| try: | |
| fn(time) | |
| except: | |
| pass | |
| try: | |
| self.GetParticles(time, pops) | |
| for pop in pops: | |
| self.GetFlux(time, pop) | |
| self.GetN(time, pop) | |
| except: | |
| pass | |
| return self.hier | |
| def GetAllAvailableQties(self, time, pops): | |
| assert self.single_hier_for_all_quantities == True # can't work otherwise | |
| for fn in [self.GetB, self.GetE, self.GetNi, self.GetVi]: | |
| try: | |
| fn(time) | |
| except Exception as e: | |
| print(f"An error occurred: {e}") | |
| try: | |
| self.GetParticles(time, pops) | |
| for pop in pops: | |
| self.GetFlux(time, pop) | |
| self.GetN(time, pop) | |
| except Exception as e: | |
| print(f"An error occurred: {e}") | |
| return self.hier |
| try: | ||
| self.GetParticles(time, pops) | ||
| for pop in pops: | ||
| self.GetFlux(time, pop) | ||
| self.GetN(time, pop) | ||
| except: | ||
| pass |
There was a problem hiding this comment.
The same comments apply to this try/except block. It's trying to call GetParticles, GetFlux, and GetN with the appropriate arguments, and if any of these calls fail, it simply passes and continues. This could potentially hide errors and lead to unexpected behavior.
| _USE_NINJA=True | ||
|
|
There was a problem hiding this comment.
Global variables can make code harder to understand and maintain. Consider encapsulating these variables in a class or passing them as arguments to functions where necessary.
| _globals = dict(servers=dict(), busy=0) | ||
| phare_runs_dir = Path(os.getcwd()) / "phare_runs" # can't contain period "." |
There was a problem hiding this comment.
Global variables are generally discouraged in Python due to their mutable nature and the potential for unexpected side effects. Consider encapsulating these in a class or passing them as function arguments where necessary.
| def advance(self, compare=False): | ||
| if not self.states["init"]: | ||
| self.init() | ||
|
|
||
| atomic_set(0, self.shr.name, 0, int(compare)) | ||
| self._state_machine_set_per_simulation(SharedSimulationStateEnum.CAN_ADVANCE, 1) | ||
|
|
||
| if compare: | ||
| self._state_machine_set_per_simulation(SharedSimulationStateEnum.IS_BUSY, 1) | ||
| # we have to wait for all simulations to finish the current timestep | ||
| self.wait_for_simulations() | ||
| self.compare() | ||
|
|
||
| def compare(self): |
There was a problem hiding this comment.
The advance method is doing a lot of work and could benefit from being broken down into smaller, more manageable methods. This would improve readability and maintainability.
| for i, simulation in enumerate(self.simulations): | ||
| assert ( | ||
| _globals["servers"][self.starting_sim_id + i].exitcode is None | ||
| ), "or it crashed early" | ||
|
|
||
| sim_times = self._state_machine_list_for_value(SharedSimulationStateEnum.CURRENT_TIME) | ||
|
|
||
| times = { | ||
| i: (0.0 + sim_times[i]) / 1e9 # :( it's an int so after decimal is dropped | ||
| for i, simulation in enumerate(self.simulations) | ||
| } | ||
|
|
||
| if len(times) < 2: | ||
| return | ||
|
|
||
| for i, simulation in enumerate(self.simulations): | ||
| for j in range(i + 1, len(times)): | ||
| if times[i] == times[j]: | ||
| run0 = Run( | ||
| build_diag_dir(self.starting_sim_id + i), | ||
| single_hier_for_all_quantities=True, | ||
| ) | ||
| run1 = Run( | ||
| build_diag_dir(self.starting_sim_id + j), | ||
| single_hier_for_all_quantities=True, | ||
| ) | ||
|
|
||
| print( | ||
| f"comparing {self.starting_sim_id + i} and {self.starting_sim_id + j} at time {times[i]}" | ||
| ) | ||
| assert hierarchy_compare( | ||
| run0.GetAllAvailableQties(times[i], []), | ||
| run1.GetAllAvailableQties(times[i], []), | ||
| ) | ||
| print("OK!") | ||
|
|
There was a problem hiding this comment.
The compare method is doing a lot of work and could benefit from being broken down into smaller, more manageable methods. This would improve readability and maintainability.
| def __del__(self): | ||
| self.kill() | ||
|
|
||
| def kill(self): | ||
| time.sleep(2) | ||
|
|
||
| if self.shr: | ||
| self.shr.close() | ||
| self.shr.unlink() | ||
| self.shr = None | ||
|
|
||
| stop_servers() |
There was a problem hiding this comment.
The kill method is missing error handling for potential exceptions that could occur during the shared memory and server termination.
| import sys | ||
| import shutil | ||
| from pathlib import Path | ||
|
|
||
| from pyphare.simulator.simulators import Simulators | ||
| from tools.python3 import pushd, cmake, git | ||
| from tests.functional.alfven_wave import alfven_wave1d | ||
|
|
||
| # we want it seeded | ||
| alfven_wave1d.MODEL_INIT={"seed": 1337} | ||
| alfven_wave1d.TIME_STEP_NBR = 10 | ||
|
|
||
| if len(sys.argv) != 3: | ||
| print("Incorrect input arguments, expects two branch names", sys.argv) | ||
| exit(0) | ||
|
|
||
| # register exit handler | ||
| git.git_branch_reset_at_exit() | ||
|
|
||
| branches = sys.argv[1:] | ||
|
|
||
| # check branches exist | ||
| for branch in branches: | ||
| git.checkout(branch) | ||
|
|
||
| build_dir = Path("build") | ||
|
|
||
| for branch in branches: | ||
| b = (build_dir / branch) | ||
| b.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| git.checkout(branch) | ||
| with pushd(b): | ||
| cmake.config("../..") | ||
|
|
||
| for branch in branches: | ||
| git.checkout(branch) | ||
| with pushd(build_dir / branch): | ||
| cmake.build() | ||
|
|
||
| simulators = Simulators() | ||
| for branch in branches: | ||
| # alfven_wave1d.config() will be identical here even if different on branches | ||
| # as it is already parsed before we change branch | ||
| simulators.register(alfven_wave1d.config(), build_dir=str(build_dir / branch)) | ||
| simulators.run(compare=True) |
There was a problem hiding this comment.
The script seems to be well-structured and follows good practices. However, there are a few points to consider:
-
Error handling: The script doesn't handle exceptions that might occur during the execution of the script. For example, if the git checkout, cmake configuration, or build fails, the script will continue to execute, which might lead to unexpected behavior or results. Consider adding try/except blocks around these operations to handle potential errors and exit the script gracefully.
-
User feedback: The script could benefit from more user feedback. For example, printing a message when each major step begins and ends (e.g., checking out branches, building, running simulations) would make it easier for users to understand what the script is doing.
-
Exit code: In line 15, the script exits with a status code of 0 even when the input arguments are incorrect. By convention, a non-zero exit status indicates that the program encountered an error. Consider changing this to a non-zero value.
Here's a diff with the suggested changes:
if len(sys.argv) != 3:
print("Incorrect input arguments, expects two branch names", sys.argv)
- exit(0)
+ exit(1)
# register exit handler
git.git_branch_reset_at_exit()
branches = sys.argv[1:]
# check branches exist
for branch in branches:
+ print(f"Checking out branch {branch}...")
try:
git.checkout(branch)
except Exception as e:
print(f"Failed to checkout branch {branch}: {e}")
exit(1)
+ print(f"Successfully checked out branch {branch}")
build_dir = Path("build")
for branch in branches:
b = (build_dir / branch)
b.mkdir(parents=True, exist_ok=True)
git.checkout(branch)
with pushd(b):
+ print(f"Configuring CMake for branch {branch}...")
try:
cmake.config("../..")
except Exception as e:
print(f"Failed to configure CMake for branch {branch}: {e}")
exit(1)
+ print(f"Successfully configured CMake for branch {branch}")
for branch in branches:
git.checkout(branch)
with pushd(build_dir / branch):
+ print(f"Building project for branch {branch}...")
try:
cmake.build()
except Exception as e:
print(f"Failed to build project for branch {branch}: {e}")
exit(1)
+ print(f"Successfully built project for branch {branch}")
simulators = Simulators()
for branch in branches:
# alfven_wave1d.config() will be identical here even if different on branches
# as it is already parsed before we change branch
+ print(f"Registering simulation for branch {branch}...")
simulators.register(alfven_wave1d.config(), build_dir=str(build_dir / branch))
+ print(f"Successfully registered simulation for branch {branch}")
+print("Running simulations...")
simulators.run(compare=True)
+print("Simulations completed")Commitable suggestion (Beta)
| import sys | |
| import shutil | |
| from pathlib import Path | |
| from pyphare.simulator.simulators import Simulators | |
| from tools.python3 import pushd, cmake, git | |
| from tests.functional.alfven_wave import alfven_wave1d | |
| # we want it seeded | |
| alfven_wave1d.MODEL_INIT={"seed": 1337} | |
| alfven_wave1d.TIME_STEP_NBR = 10 | |
| if len(sys.argv) != 3: | |
| print("Incorrect input arguments, expects two branch names", sys.argv) | |
| exit(0) | |
| # register exit handler | |
| git.git_branch_reset_at_exit() | |
| branches = sys.argv[1:] | |
| # check branches exist | |
| for branch in branches: | |
| git.checkout(branch) | |
| build_dir = Path("build") | |
| for branch in branches: | |
| b = (build_dir / branch) | |
| b.mkdir(parents=True, exist_ok=True) | |
| git.checkout(branch) | |
| with pushd(b): | |
| cmake.config("../..") | |
| for branch in branches: | |
| git.checkout(branch) | |
| with pushd(build_dir / branch): | |
| cmake.build() | |
| simulators = Simulators() | |
| for branch in branches: | |
| # alfven_wave1d.config() will be identical here even if different on branches | |
| # as it is already parsed before we change branch | |
| simulators.register(alfven_wave1d.config(), build_dir=str(build_dir / branch)) | |
| simulators.run(compare=True) | |
| import sys | |
| import shutil | |
| from pathlib import Path | |
| from pyphare.simulator.simulators import Simulators | |
| from tools.python3 import pushd, cmake, git | |
| from tests.functional.alfven_wave import alfven_wave1d | |
| # we want it seeded | |
| alfven_wave1d.MODEL_INIT={"seed": 1337} | |
| alfven_wave1d.TIME_STEP_NBR = 10 | |
| if len(sys.argv) != 3: | |
| print("Incorrect input arguments, expects two branch names", sys.argv) | |
| exit(1) | |
| # register exit handler | |
| git.git_branch_reset_at_exit() | |
| branches = sys.argv[1:] | |
| # check branches exist | |
| for branch in branches: | |
| print(f"Checking out branch {branch}...") | |
| try: | |
| git.checkout(branch) | |
| except Exception as e: | |
| print(f"Failed to checkout branch {branch}: {e}") | |
| exit(1) | |
| print(f"Successfully checked out branch {branch}") | |
| build_dir = Path("build") | |
| for branch in branches: | |
| b = (build_dir / branch) | |
| b.mkdir(parents=True, exist_ok=True) | |
| git.checkout(branch) | |
| with pushd(b): | |
| print(f"Configuring CMake for branch {branch}...") | |
| try: | |
| cmake.config("../..") | |
| except Exception as e: | |
| print(f"Failed to configure CMake for branch {branch}: {e}") | |
| exit(1) | |
| print(f"Successfully configured CMake for branch {branch}") | |
| for branch in branches: | |
| git.checkout(branch) | |
| with pushd(build_dir / branch): | |
| print(f"Building project for branch {branch}...") | |
| try: | |
| cmake.build() | |
| except Exception as e: | |
| print(f"Failed to build project for branch {branch}: {e}") | |
| exit(1) | |
| print(f"Successfully built project for branch {branch}") | |
| simulators = Simulators() | |
| for branch in branches: | |
| print(f"Registering simulation for branch {branch}...") | |
| simulators.register(alfven_wave1d.config(), build_dir=str(build_dir / branch)) | |
| print(f"Successfully registered simulation for branch {branch}") | |
| print("Running simulations...") | |
| simulators.run(compare=True) | |
| print("Simulations completed") |
| L0L1_datahier = check_hier(get_hier(L0L1_diags)) | ||
| extra_collections = [] | ||
| errors = test.base_test_overlaped_fields_are_equal(L0L1_datahier, new_time) | ||
| # errors = test.base_test_field_level_ghosts_via_subcycles_and_coarser_interpolation(L0_datahier, L0L1_datahier) | ||
| errors = ( | ||
| test.base_test_field_level_ghosts_via_subcycles_and_coarser_interpolation( | ||
| L0_datahier, L0L1_datahier | ||
| ) | ||
| ) | ||
| print(f"errors {type(errors)}") |
There was a problem hiding this comment.
The variable errors is reassigned on line 186, which makes the assignment on line 184 redundant. If the function base_test_overlaped_fields_are_equal has side effects that are needed, consider calling it without assigning the result to errors. If not, line 184 can be removed.
- errors = test.base_test_overlaped_fields_are_equal(L0L1_datahier, new_time)
errors = (
test.base_test_field_level_ghosts_via_subcycles_and_coarser_interpolation(
L0_datahier, L0L1_datahier
)
)Commitable suggestion (Beta)
| L0L1_datahier = check_hier(get_hier(L0L1_diags)) | |
| extra_collections = [] | |
| errors = test.base_test_overlaped_fields_are_equal(L0L1_datahier, new_time) | |
| # errors = test.base_test_field_level_ghosts_via_subcycles_and_coarser_interpolation(L0_datahier, L0L1_datahier) | |
| errors = ( | |
| test.base_test_field_level_ghosts_via_subcycles_and_coarser_interpolation( | |
| L0_datahier, L0L1_datahier | |
| ) | |
| ) | |
| print(f"errors {type(errors)}") | |
| L0L1_datahier = check_hier(get_hier(L0L1_diags)) | |
| extra_collections = [] | |
| errors = ( | |
| test.base_test_field_level_ghosts_via_subcycles_and_coarser_interpolation( | |
| L0_datahier, L0L1_datahier | |
| ) | |
| ) | |
| print(f"errors {type(errors)}") |
PhilipDeegan
force-pushed
the
simulators
branch
from
3e1349f to
b2ed3a1
Compare
PhilipDeegan
force-pushed
the
simulators
branch
from
b2ed3a1 to
4181929
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: .coderabbit.yaml
Files selected for processing (13)
- pyphare/pyphare/cpp/init.py (1 hunks)
- pyphare/pyphare/pharesee/hierarchy.py (3 hunks)
- pyphare/pyphare/pharesee/particles.py (1 hunks)
- pyphare/pyphare/pharesee/run.py (1 hunks)
- pyphare/pyphare/simulator/simulator.py (1 hunks)
- pyphare/pyphare/simulator/simulators.py (1 hunks)
- tests/functional/alfven_wave/alfven_wave1d.py (4 hunks)
- tools/build_compare_2_branches.py (1 hunks)
- tools/build_compare_2_branches.sh (1 hunks)
- tools/build_compare_top_2_commits.py (1 hunks)
- tools/build_compare_top_2_commits.sh (1 hunks)
- tools/python3/cmake.py (1 hunks)
- tools/python3/git.py (2 hunks)
Files skipped from review due to trivial changes (1)
- tools/build_compare_top_2_commits.sh
Files skipped from review as they are similar to previous changes (9)
- pyphare/pyphare/pharesee/hierarchy.py
- pyphare/pyphare/pharesee/particles.py
- pyphare/pyphare/pharesee/run.py
- pyphare/pyphare/simulator/simulator.py
- tests/functional/alfven_wave/alfven_wave1d.py
- tools/build_compare_2_branches.py
- tools/build_compare_2_branches.sh
- tools/build_compare_top_2_commits.py
- tools/python3/git.py
Additional comments: 12
pyphare/pyphare/cpp/__init__.py (1)
- 4-26: The introduction of a caching mechanism for the C++ module import using the
_globalsdictionary is a good performance optimization. It reduces the overhead of repeatedly importing the same module. However, ensure that this change does not introduce any side effects in a multi-threaded environment where the import might be expected to be fresh or different across threads.pyphare/pyphare/simulator/simulators.py (8)
28-29: The previous comment about global variables is still valid. Consider encapsulating these in a class or passing them as function arguments where necessary.
43-47: The previous comment about wrapping the creation of shared memory blocks in a try/except block is still valid to handle potential exceptions, such as insufficient memory.
53-57: The previous comment about thread safety is still valid. Consider using a context manager (
with lock:) to ensure the lock is properly released even if an error occurs.67-97: The previous comment about breaking down the
state_machinefunction into smaller, more manageable functions is still valid to improve readability and maintainability.113-119: The previous comment about error handling for the case where the
Simulatorinitialization fails is still valid. Consider adding a try/except block to handle potential exceptions.122-125: The previous comment about breaking down the
advancemethod into smaller, more manageable methods is still valid to improve readability and maintainability.346-354: The previous comment about missing error handling for potential exceptions that could occur during the shared memory and server termination in the
killmethod is still valid.146-146: It's good to see an assertion here to ensure that a server for a given
sim_iddoes not already exist before starting a new process. This helps prevent accidental overwrites or conflicts.tools/python3/cmake.py (3)
8-11:
The introduction of_USE_NINJAand_SAMRAI_DIRas global variables centralizes the build configuration settings, which can simplify the build process and ensure consistency across the build functions.13-25:
Themake_config_strfunction now uses the_USE_NINJAand_SAMRAI_DIRglobal variables. Verify that all calls to this function have been updated to reflect the removal of theuse_ninjaparameter and the new behavior of thesamrai_dirparameter.28-32:
Theconfigfunction now uses the_USE_NINJAand_SAMRAI_DIRglobal variables. Verify that all calls to this function have been updated to reflect the removal of theuse_ninjaparameter.
| for i in range(1, try_count + 1): | ||
| time.sleep(0.5) | ||
| try: | ||
| assert servers[sim_id].exitcode is None # or it crashed/exited early | ||
| return | ||
| except Exception as e: | ||
| if i == try_count: | ||
| raise e |
There was a problem hiding this comment.
The retry logic here is a good addition for robustness, but it should include a backoff strategy to avoid hammering the system with rapid retries. Also, consider logging the exception details for better debugging.
+ for i in range(1, try_count + 1):
+ time.sleep(0.5 * i) # backoff strategy
+ try:
+ assert servers[sim_id].exitcode is None # or it crashed/exited early
+ return
+ except Exception as e:
+ if i == try_count:
+ logging.exception(f"Server process for sim_id {sim_id} failed to start.")
+ raise eCommittable 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.
| for i in range(1, try_count + 1): | |
| time.sleep(0.5) | |
| try: | |
| assert servers[sim_id].exitcode is None # or it crashed/exited early | |
| return | |
| except Exception as e: | |
| if i == try_count: | |
| raise e | |
| for i in range(1, try_count + 1): | |
| time.sleep(0.5 * i) # backoff strategy | |
| try: | |
| assert servers[sim_id].exitcode is None # or it crashed/exited early | |
| return | |
| except Exception as e: | |
| if i == try_count: | |
| logging.exception(f"Server process for sim_id {sim_id} failed to start.") | |
| raise e |
|
|
||
| lock.release() | ||
| existing_shm.close() | ||
|
|
||
| def wait_for_simulations(self): | ||
| # we have to wait for all simulations to finish the current timestep | ||
| while True: | ||
| if ( | ||
| np.sum( | ||
| self._state_machine_list_for_value( | ||
| SharedSimulationStateEnum.IS_BUSY | ||
| ) | ||
| ) | ||
| == 0 | ||
| ): | ||
| return | ||
| time.sleep(1) | ||
|
|
||
| def advance(self, compare=False): | ||
| if not self.states["init"]: | ||
| self.init() | ||
|
|
||
| atomic_set(0, self.shr.name, 0, int(compare)) | ||
| self._state_machine_set_per_simulation(SharedSimulationStateEnum.CAN_ADVANCE, 1) | ||
|
|
||
| if compare: | ||
| self._state_machine_set_per_simulation(SharedSimulationStateEnum.IS_BUSY, 1) | ||
| # we have to wait for all simulations to finish the current timestep | ||
| self.wait_for_simulations() | ||
| self.compare() | ||
|
|
||
| def compare(self): | ||
| for i, simulation in enumerate(self.simulations): | ||
| assert ( | ||
| _globals["servers"][self.starting_sim_id + i].exitcode is None | ||
| ), "or it crashed early" | ||
|
|
||
| sim_times = self._state_machine_list_for_value( | ||
| SharedSimulationStateEnum.CURRENT_TIME | ||
| ) | ||
|
|
||
| times = { | ||
| i: (0.0 + sim_times[i]) / 1e9 # :( it's an int so after decimal is dropped | ||
| for i, simulation in enumerate(self.simulations) | ||
| } | ||
|
|
||
| if len(times) < 2: | ||
| return | ||
|
|
||
| for i, simulation in enumerate(self.simulations): | ||
| for j in range(i + 1, len(times)): | ||
| if times[i] == times[j]: | ||
| run0 = Run( | ||
| build_diag_dir(self.starting_sim_id + i), | ||
| single_hier_for_all_quantities=True, | ||
| ) | ||
| run1 = Run( | ||
| build_diag_dir(self.starting_sim_id + j), | ||
| single_hier_for_all_quantities=True, | ||
| ) | ||
|
|
||
| print( | ||
| f"comparing {self.starting_sim_id + i} and {self.starting_sim_id + j} at time {times[i]}" | ||
| ) | ||
| assert hierarchy_compare( | ||
| run0.GetAllAvailableQties(times[i], []), | ||
| run1.GetAllAvailableQties(times[i], []), | ||
| ) | ||
| print("OK!") | ||
|
|
||
| def run(self, compare=False): | ||
| if not self.states["init"]: | ||
| self.init() | ||
|
|
||
| while self.simulations: | ||
| self.advance(compare=compare) | ||
|
|
||
| is_busy = self._state_machine_list_for_value( | ||
| SharedSimulationStateEnum.IS_BUSY | ||
| ) | ||
| is_finished = self._state_machine_list_for_value( | ||
| SharedSimulationStateEnum.IS_FINISHED | ||
| ) | ||
|
|
||
| # trim finished simulations | ||
| self.simulations = [ | ||
| sim | ||
| for i, sim in enumerate(self.simulations) | ||
| if is_busy[i] or not is_finished[i] | ||
| ] | ||
|
|
||
| print("running simulations", len(self.simulations)) | ||
| time.sleep(1) | ||
|
|
||
| self.kill() | ||
|
|
||
| def __del__(self): | ||
| self.kill() | ||
|
|
||
| def kill(self): |
There was a problem hiding this comment.
The Simulators class encapsulates the management of multiple simulations, which is a good design choice for maintainability and modularity. However, the class is quite large and does a lot of work. Consider breaking down some of its responsibilities into smaller, more focused classes or helper functions.
| time.sleep(2) | ||
|
|
||
| if self.shr: | ||
| self.shr.close() | ||
| self.shr.unlink() | ||
| self.shr = None |
There was a problem hiding this comment.
The kill method should handle exceptions that may occur when closing or unlinking the shared memory. This is important to ensure that resources are cleaned up properly even if an error occurs.
+ def kill(self):
+ time.sleep(2)
+ try:
+ if self.shr:
+ self.shr.close()
+ self.shr.unlink()
+ self.shr = None
+ except Exception as e:
+ logging.exception("An error occurred while cleaning up shared memory.")
+ raise eCommittable 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.
| time.sleep(2) | |
| if self.shr: | |
| self.shr.close() | |
| self.shr.unlink() | |
| self.shr = None | |
| def kill(self): | |
| time.sleep(2) | |
| try: | |
| if self.shr: | |
| self.shr.close() | |
| self.shr.unlink() | |
| self.shr = None | |
| except Exception as e: | |
| logging.exception("An error occurred while cleaning up shared memory.") | |
| raise e |
| def build(threads=1): | ||
| run("ninja" if _USE_NINJA else f"make -j{threads}", capture_output=False) |
There was a problem hiding this comment.
💡 NOTE
Codebase verification is a beta feature.
The search results show that the build function is called in several scripts. The calls in tools/build_compare_top_2_commits.py and tools/build_compare_2_branches.py have been updated correctly and do not pass any parameters. However, the call in tools/bench/functional/run_gen_plots.py still uses the use_ninja parameter, which is no longer valid after the update to the build function. This needs to be addressed.
- The call to
cmake.build()intools/bench/functional/run_gen_plots.pystill uses theuse_ninjaparameter, which has been removed from thebuildfunction definition. This call should be updated to remove theuse_ninjaparameter.
Analysis chain
The build function now uses the _USE_NINJA global variable directly. Verify that all calls to this function have been updated to reflect the removal of the use_ninja parameter.
The ast-grep command used to search for calls to the build function did not produce any output. This could indicate that there are no calls to this function, or the pattern used might not have been able to match the function calls correctly. To ensure that we have the correct information, I will generate a new shell script with a revised pattern to search for the build function calls again.
The rg command failed due to the use of an invalid --python flag. I will correct the command to search for calls to the build function without specifying the language, as ripgrep searches through text files by default and will include Python files in its search.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for calls to `build` and ensure they have been updated.
ast-grep --lang python --pattern $'build($_)'
Length of output: 46
Script:
#!/bin/bash
# Search for calls to `build` function in the codebase with a more inclusive pattern.
rg --python 'build\('
Length of output: 434
Script:
#!/bin/bash
# Search for calls to `build` function in the codebase.
rg 'build\('
Length of output: 426
|
|
||
| if _globals["module"] is None: | ||
| _globals["module"] = get_module() | ||
| print("Loaded C++ module", _globals["module"].__file__) |
There was a problem hiding this comment.
Consider if the print statement indicating the loading of the C++ module is appropriate for production code. It may be beneficial for debugging but could clutter the output in a production environment. If it's intended for debugging purposes only, it might be better to use a logging framework with appropriate log levels.
| return importlib.import_module("pybindlibs.cpp_dbg") | ||
| except ImportError as err: | ||
| return importlib.import_module("pybindlibs.cpp") |
There was a problem hiding this comment.
The get_module function swallows the exception message from ImportError. It might be useful to log the error message for debugging purposes, especially if the fallback import fails as well. Consider adding logging for the exception or re-raising it with additional context.
try:
return importlib.import_module("pybindlibs.cpp_dbg")
+ except ImportError as err:
+ logging.error(f"Failed to import pybindlibs.cpp_dbg: {err}")
return importlib.import_module("pybindlibs.cpp")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.
| return importlib.import_module("pybindlibs.cpp_dbg") | |
| except ImportError as err: | |
| return importlib.import_module("pybindlibs.cpp") | |
| try: | |
| return importlib.import_module("pybindlibs.cpp_dbg") | |
| except ImportError as err: | |
| logging.error(f"Failed to import pybindlibs.cpp_dbg: {err}") | |
| return importlib.import_module("pybindlibs.cpp") |
Summary by CodeRabbit
New Features:
Simulatorsclass to manage multiple simulations, supporting parallel execution and shared memory communication.finished()method in thesimulatorclass to check if a simulation has completed.comparemethod in theFieldDataclass for data comparison and ameshgridmethod for generating a meshgrid.GetAllAvailableQtiesmethod to handle exceptions and ensure continued execution.Refactor:
cpp_libfunction to cache the imported module, improving performance.all_assert_sortedfunction inparticles.pyfor better precision control.getHierarchyfunction across multiple test files to include an additional parameterndim.Tests:
unique_diag_dir_for_test_casemethod for generating unique diagnostic directory paths.Chores: