Lazy implementation of costly parsing operations in the GlobalStatus#500
Open
dodu94 wants to merge 3 commits intodevelopingfrom
Open
Lazy implementation of costly parsing operations in the GlobalStatus#500dodu94 wants to merge 3 commits intodevelopingfrom
dodu94 wants to merge 3 commits intodevelopingfrom
Conversation
Contributor
WalkthroughThe pull request refactors status initialization in the JADE application from eager to lazy-loading with caching. Additionally, simulation success checkers are refactored from standalone functions to an object-oriented design with a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/jade/helper/aux_functions.py (2)
112-118: Serpent checker raises NotImplementedError at runtime.This is acceptable as a placeholder, but callers iterating over
CODE_CHECKERS(e.g., instatus.py) could encounter this exception if Serpent simulations exist in the folder structure.Consider whether call sites need defensive handling, or document that Serpent support is not yet available.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/jade/helper/aux_functions.py` around lines 112 - 118, The SerpentChecker currently raises NotImplementedError from SerpentChecker.check_success which can bubble up when code iterates CODE_CHECKERS (e.g., in status.py); change SerpentChecker.check_success to not raise—instead log a clear warning that Serpent support is not implemented and return False (or an appropriate failure value) so callers continue safely, and additionally harden the call sites that iterate CODE_CHECKERS (for example the check loop in status.py) to catch exceptions around checker.check_success and treat them as a failed check while logging the exception and checker name.
121-126: Minor inefficiency: D1SChecker instantiates MCNPChecker on every call.
D1SChecker.check_successcreates a newMCNPChecker()instance on each invocation. Since both classes are stateless, this could be simplified.♻️ Suggested simplification
class D1SChecker(SimulationChecker): - """Checker for D1S simulations.""" + """Checker for D1S simulations (uses same logic as MCNP).""" def check_success(self, files: list[str]) -> bool: """Check if D1S run was successful (uses same logic as MCNP).""" - return MCNPChecker().check_success(files) + return MCNPSimOutput.is_successfully_simulated(files)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/jade/helper/aux_functions.py` around lines 121 - 126, D1SChecker.check_success currently constructs a new MCNPChecker() on every call; make it use a single shared instance to avoid repeated instantiation by creating a module-level MCNPChecker instance (e.g., _mcnp_checker = MCNPChecker()) and have D1SChecker.check_success call _mcnp_checker.check_success(files) instead of instantiating a new MCNPChecker each time; reference: D1SChecker.check_success and MCNPChecker.src/jade/post/sim_output.py (1)
348-358: Minor inconsistency withretrieve_filemethod.The
is_successfully_simulatedmethod checks forfile.startswith("statepoint") and file.endswith(".h5"), butretrieve_file(line 332) only checksfile.startswith("statepoint")without the.h5extension requirement. While statepoint files are typically.h5, this inconsistency could cause a simulation to be considered successful byretrieve_filebut fail theis_successfully_simulatedcheck if a non-.h5 statepoint file exists.Consider aligning the checks for consistency:
Option 1: Relax is_successfully_simulated to match retrieve_file
`@staticmethod` def is_successfully_simulated(files: list[str]) -> bool: """Check if the simulation was successful by verifying output files exist.""" statepoint_found = False output_found = False for file in files: - if file.startswith("statepoint") and file.endswith(".h5"): + if file.startswith("statepoint"): statepoint_found = True elif file.endswith(".out"): output_found = True return statepoint_found and output_foundOption 2: Tighten retrieve_file to match is_successfully_simulated
for file_name in os.listdir(results_path): if file_name.endswith(".out"): file1 = file_name - elif file_name.startswith("statepoint"): + elif file_name.startswith("statepoint") and file_name.endswith(".h5"): file2 = file_name elif file_name == "volumes.json": file3 = file_name🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/jade/post/sim_output.py` around lines 348 - 358, The checks for statepoint files are inconsistent: is_successfully_simulated requires files to start with "statepoint" and end with ".h5" while retrieve_file only checks startswith("statepoint"); update retrieve_file to the stricter check (require file.startswith("statepoint") and file.endswith(".h5")) so both retrieve_file and is_successfully_simulated use the same criteria, and ensure any code paths relying on retrieve_file still behave correctly after adding the .h5 extension check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/jade/helper/aux_functions.py`:
- Around line 112-118: The SerpentChecker currently raises NotImplementedError
from SerpentChecker.check_success which can bubble up when code iterates
CODE_CHECKERS (e.g., in status.py); change SerpentChecker.check_success to not
raise—instead log a clear warning that Serpent support is not implemented and
return False (or an appropriate failure value) so callers continue safely, and
additionally harden the call sites that iterate CODE_CHECKERS (for example the
check loop in status.py) to catch exceptions around checker.check_success and
treat them as a failed check while logging the exception and checker name.
- Around line 121-126: D1SChecker.check_success currently constructs a new
MCNPChecker() on every call; make it use a single shared instance to avoid
repeated instantiation by creating a module-level MCNPChecker instance (e.g.,
_mcnp_checker = MCNPChecker()) and have D1SChecker.check_success call
_mcnp_checker.check_success(files) instead of instantiating a new MCNPChecker
each time; reference: D1SChecker.check_success and MCNPChecker.
In `@src/jade/post/sim_output.py`:
- Around line 348-358: The checks for statepoint files are inconsistent:
is_successfully_simulated requires files to start with "statepoint" and end with
".h5" while retrieve_file only checks startswith("statepoint"); update
retrieve_file to the stricter check (require file.startswith("statepoint") and
file.endswith(".h5")) so both retrieve_file and is_successfully_simulated use
the same criteria, and ensure any code paths relying on retrieve_file still
behave correctly after adding the .h5 extension check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f864a29f-20e8-4ec3-b2fa-beaf9277b972
📒 Files selected for processing (6)
src/jade/app/app.pysrc/jade/config/status.pysrc/jade/helper/aux_functions.pysrc/jade/post/sim_output.pysrc/jade/run/benchmark.pytests/app/test_app.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR tries to address #488.
As expected, the main bottleneck is the parsing of the simulation folders and raw data.
After some experimentation it turns out the parsing itself cannot be sped up that much. At least, lazy implementations have been added for the parsing costly operations. This grants that the slow parsing will only be performed when absolutely necessary. (For instance, no parsing of simulation when post-processing).
Summary by CodeRabbit
Refactor
Tests