Open
Conversation
Contributor
WalkthroughAdds a new Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as "utilities.py"
participant App as "JadeApp"
participant Status as "SimulationStatus"
User->>CLI: run with --unfinished
activate CLI
CLI->>App: call print_unfinished_runs()
activate App
App->>Status: iterate self.status.simulations
activate Status
loop for each simulation with failed_simulations
Status-->>App: simulation metadata + failed_simulations
App->>App: log code, lib, bench, failed entries
end
deactivate Status
deactivate App
deactivate CLI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 unit tests (beta)
📝 Coding Plan
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 Tip You can generate walkthrough in a markdown collapsible section to save space.Enable the |
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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jade/helper/aux_functions.py (1)
109-129:⚠️ Potential issue | 🔴 CriticalAvoid truncating blank inputs and duplicating existing
RMODEcards.After Line 119,
linescan be empty; Line 127 then raisesIndexErrorafter the file has already been reopened in write mode, leaving that.ifile truncated. Also,pattern.match(line)only recognizesRMODE 0with no indentation and exactly one space, so inputs containing variants like leading spaces orRMODE 0are treated as missing and get a second card appended.🐛 Proposed fix
-def add_rmode0(path: PathLike) -> None: +def add_rmode0(path: PathLike) -> None: """Given a folder, iteratively search for MCNP input files and add the RMODE 0 card if it is not present.""" - pattern = re.compile(r"rmode 0", re.IGNORECASE) + pattern = re.compile(r"^\s*rmode\s+0(?:\s|$)", re.IGNORECASE) for pathroot, folder, filelist in os.walk(path): # if the folder name is mcnp if os.path.basename(pathroot) == "mcnp": for file in filelist: if file.endswith(".i"): - with open(os.path.join(pathroot, file)) as f: + filepath = os.path.join(pathroot, file) + with open(filepath) as f: lines = f.readlines() # Remove trailing empty lines at the end of file while lines and lines[-1].strip() == "": lines.pop() - with open(os.path.join(pathroot, file), "w") as f: - found = False - for line in lines: - if pattern.match(line): - found = True - f.write(line) - if not found: - if not lines[-1].endswith("\n"): - f.write("\n") - f.write("RMODE 0\n") + + found = any(pattern.match(line) for line in lines) + if not found: + if lines and not lines[-1].endswith("\n"): + lines[-1] = f"{lines[-1]}\n" + lines.append("RMODE 0\n") + + with open(filepath, "w") as f: + f.writelines(lines or ["RMODE 0\n"])🤖 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 109 - 129, The code can truncate files when `lines` is empty and duplicates RMODE entries because `pattern.match(line)` only matches at column 0; change the regex `pattern` to r"^\s*rmode\s+0\b" with re.IGNORECASE and use pattern.search or pattern.match (since the ^ handles leading whitespace) to detect existing RMODE lines anywhere in the line; also avoid reopening the file for writing until you've determined whether you need to change it—check if `lines` is empty before referencing `lines[-1]` and only open for write if you will append (or else write the original content back unchanged), and when appending ensure you add a newline if the last line does not end with one before writing "RMODE 0\n".
🧹 Nitpick comments (1)
src/jade/app/app.py (1)
358-365: Consider providing feedback when no unfinished simulations are found.The method works correctly, but if all simulations completed successfully, it exits silently with no output. Users might appreciate confirmation that the check was performed.
💡 Optional enhancement for user feedback
def print_unfinished_runs(self): """Print the simulations that were not completed.""" + found_unfinished = False for key, status in self.status.simulations.items(): if len(status.failed_simulations) > 0: + found_unfinished = True code, lib, bench = key logging.info(f"Unfinished simulations for {code} {lib} {bench}:") for sim in status.failed_simulations: logging.info(f"\t{sim}") + if not found_unfinished: + logging.info("All simulations completed successfully.")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/jade/app/app.py` around lines 358 - 365, The print_unfinished_runs method currently logs only when status.failed_simulations contains entries, leaving silent success cases; modify print_unfinished_runs (which iterates self.status.simulations and checks status.failed_simulations) to emit a clear confirmation when no unfinished simulations are found—either by tracking a "found" flag while iterating and logging a single INFO like "No unfinished simulations found" after the loop if none were discovered, or by detecting an empty self.status.simulations or all-empty failed_simulations and logging the confirmation accordingly; keep existing per-key logging behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/jade/utilities.py`:
- Around line 43-44: The call to app.print_unfinished_runs() can raise
AttributeError because JadeApp.__init__ may return early and never set
self.status; update the caller to guard before invoking the method (e.g., if
hasattr(app, "status") and app.status is not None: app.print_unfinished_runs())
or alternatively update JadeApp.print_unfinished_runs() to handle a missing or
None self.status (check for getattr(self, "status", None) and return/print a
friendly message when absent); reference JadeApp.__init__,
app.print_unfinished_runs, and self.status.simulations when making the change.
---
Outside diff comments:
In `@src/jade/helper/aux_functions.py`:
- Around line 109-129: The code can truncate files when `lines` is empty and
duplicates RMODE entries because `pattern.match(line)` only matches at column 0;
change the regex `pattern` to r"^\s*rmode\s+0\b" with re.IGNORECASE and use
pattern.search or pattern.match (since the ^ handles leading whitespace) to
detect existing RMODE lines anywhere in the line; also avoid reopening the file
for writing until you've determined whether you need to change it—check if
`lines` is empty before referencing `lines[-1]` and only open for write if you
will append (or else write the original content back unchanged), and when
appending ensure you add a newline if the last line does not end with one before
writing "RMODE 0\n".
---
Nitpick comments:
In `@src/jade/app/app.py`:
- Around line 358-365: The print_unfinished_runs method currently logs only when
status.failed_simulations contains entries, leaving silent success cases; modify
print_unfinished_runs (which iterates self.status.simulations and checks
status.failed_simulations) to emit a clear confirmation when no unfinished
simulations are found—either by tracking a "found" flag while iterating and
logging a single INFO like "No unfinished simulations found" after the loop if
none were discovered, or by detecting an empty self.status.simulations or
all-empty failed_simulations and logging the confirmation accordingly; keep
existing per-key logging behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c93cd112-fb8e-4c3e-b965-fa497697c45c
📒 Files selected for processing (14)
.github/workflows/pytest.ymldocs/source/usage/utilities.rstpyproject.tomlsrc/jade/app/app.pysrc/jade/helper/aux_functions.pysrc/jade/run/input.pysrc/jade/utilities.pytests/app/test_app.pytests/helper/res/__init__.pytests/helper/res/rmode/__init__.pytests/helper/res/rmode/sphere_more_spaces.itests/helper/res/rmode/sphere_no_spaces.itests/helper/res/rmode/sphere_rmode.itests/helper/test_auxiliary_func.py
Comment on lines
+43
to
+44
| if args.unfinished: | ||
| app.print_unfinished_runs() |
Contributor
There was a problem hiding this comment.
Potential AttributeError when using --unfinished on fresh installation.
If this utility is run on a directory that hasn't been initialized yet, JadeApp.__init__() takes an early-return path (see src/jade/app/app.py lines 43-47) where self.status is never assigned. Calling app.print_unfinished_runs() will then raise an AttributeError when accessing self.status.simulations.
Consider adding a guard to check if the app was fully initialized before calling this method, or handle the case gracefully in print_unfinished_runs().
🛡️ Proposed fix
if args.unfinished:
+ if not hasattr(app, 'status'):
+ logging.warning("JADE is not initialized. No simulations to report.")
+ else:
- app.print_unfinished_runs()
+ app.print_unfinished_runs()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/jade/utilities.py` around lines 43 - 44, The call to
app.print_unfinished_runs() can raise AttributeError because JadeApp.__init__
may return early and never set self.status; update the caller to guard before
invoking the method (e.g., if hasattr(app, "status") and app.status is not None:
app.print_unfinished_runs()) or alternatively update
JadeApp.print_unfinished_runs() to handle a missing or None self.status (check
for getattr(self, "status", None) and return/print a friendly message when
absent); reference JadeApp.__init__, app.print_unfinished_runs, and
self.status.simulations when making the change.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/jade/helper/aux_functions.py`:
- Around line 123-125: The function add_rmode0 can crash for empty or blank-only
files because after the trailing-blank trim loop the list lines may be empty but
the code later accesses lines[-1]; update add_rmode0 to check for emptiness
after trimming (e.g., if not lines: return or handle as a special-case) before
any access to lines[-1], and apply the same guard where lines[-1] is used around
the referenced lines 133–134 so empty/blank-only inputs are handled safely.
Ensure you reference the local variable lines and the function add_rmode0 when
adding the guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a417927-9c27-4d0b-bef2-5fa8d0a1846a
📒 Files selected for processing (1)
src/jade/helper/aux_functions.py
Comment on lines
+123
to
+125
| # Remove trailing empty lines at the end of file | ||
| while lines and lines[-1].strip() == "": | ||
| lines.pop() |
Contributor
There was a problem hiding this comment.
Handle empty/blank-only files before accessing lines[-1].
After trimming trailing blanks (Line 124–125), lines can become empty. Line 133 then raises IndexError, so add_rmode0 can crash on valid edge-case inputs.
Proposed fix
# Remove trailing empty lines at the end of file
while lines and lines[-1].strip() == "":
lines.pop()
with open(os.path.join(pathroot, file), "w") as f:
found = False
for line in lines:
if pattern.match(line):
found = True
f.write(line)
if not found:
- if not lines[-1].endswith("\n"):
+ if lines and not lines[-1].endswith("\n"):
f.write("\n")
f.write("RMODE 0\n")Also applies to: 133-134
🤖 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 123 - 125, The function
add_rmode0 can crash for empty or blank-only files because after the
trailing-blank trim loop the list lines may be empty but the code later accesses
lines[-1]; update add_rmode0 to check for emptiness after trimming (e.g., if not
lines: return or handle as a special-case) before any access to lines[-1], and
apply the same guard where lines[-1] is used around the referenced lines 133–134
so empty/blank-only inputs are handled safely. Ensure you reference the local
variable lines and the function add_rmode0 when adding the guard.
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.
to be reviewed after #485
It is not uncommon to have failures in the simulations when massive JADE runs are performed on many benchmarks at the same time. There is already a utility to continue these runs without having to regenerate the entire benchmark, but it can also be useful to print to video what are currently in tree the simulation that failed or have not been run yet. This PR adds a new utility to do just that.
Summary by CodeRabbit
New Features
--unfinishedflag to show unfinished simulation runs.Bug Fixes
Documentation
Tests
Chores