Case checking moved to Python! #1066
Conversation
|
CodeAnt AI is reviewing your PR. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRemoved many Fortran input-validation subroutines across common, simulation, pre-process, and post-process checkers; added a new Python stage-aware CaseValidator and integrated it into input generation; updated scheduler thread exception propagation, changed input-generation call site, and adjusted CI to install Python 3.14. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI / Orchestrator
participant MFC as MFCInputFile
participant PyVal as CaseValidator
participant Case as Case (generator)
CLI->>MFC: generate(target)
MFC->>PyVal: validate_case_constraints(params, stage=target)
alt validation OK
PyVal-->>MFC: success
MFC->>Case: case.generate(target)
Case-->>MFC: generated inputs/FPP
MFC-->>CLI: return success
else validation fails
PyVal-->>MFC: raises CaseConstraintError(errors)
MFC-->>CLI: raises MFCException(with aggregated errors)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-11-24T21:50:46.879ZApplied to files:
🧬 Code graph analysis (1)toolchain/mfc/case_validator.py (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
🔇 Additional comments (1)
Comment |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
codeant-ai
bot
added
the
size:XXL
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![qodo-merge-pro[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
toolchain/mfc/case_validator.py (1)
1318-1319: Address static analysis hint: Rename ambiguous variablel.The variable name
l(lowercase L) can be confused with1(one) orI(uppercase i). Using a more descriptive name improves readability.- self.prohibit(any(l is not None for l in length), + self.prohibit(any(length_val is not None for length_val in length), f"Circle Patch {i} can't have lengths defined")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/common/m_checker_common.fpp(0 hunks)src/post_process/m_checker.fpp(0 hunks)src/pre_process/m_checker.fpp(0 hunks)src/simulation/m_checker.fpp(2 hunks)toolchain/mfc/case_validator.py(1 hunks)toolchain/mfc/run/input.py(2 hunks)
💤 Files with no reviewable changes (3)
- src/post_process/m_checker.fpp
- src/common/m_checker_common.fpp
- src/pre_process/m_checker.fpp
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{fpp,f90}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{fpp,f90}: Use 2-space indentation; continuation lines align beneath &
Use lower-case keywords and intrinsics (do, end subroutine, etc.)
Name modules with m_ pattern (e.g., m_transport)
Name public subroutines with s_ pattern (e.g., s_compute_flux)
Name public functions with f_ pattern
Keep subroutine size ≤ 500 lines, helper subroutines ≤ 150 lines, functions ≤ 100 lines, files ≤ 1000 lines
Limit routine arguments to ≤ 6; use derived-type params struct if more are needed
Forbid goto statements (except in legacy code), COMMON blocks, and save globals
Every argument must have explicit intent; use dimension/allocatable/pointer as appropriate
Call s_mpi_abort() for errors, never use stop or error stop
**/*.{fpp,f90}: Indent 2 spaces; continuation lines align under&
Use lower-case keywords and intrinsics (do,end subroutine, etc.)
Name modules withm_<feature>prefix (e.g.,m_transport)
Name public subroutines ass_<verb>_<noun>(e.g.,s_compute_flux) and functions asf_<verb>_<noun>
Keep private helpers in the module; avoid nested procedures
Enforce size limits: subroutine ≤ 500 lines, helper ≤ 150, function ≤ 100, module/file ≤ 1000
Limit subroutines to ≤ 6 arguments; otherwise pass a derived-type 'params' struct
Avoidgotostatements (except unavoidable legacy); avoid global state (COMMON,save)
Every variable must haveintent(in|out|inout)specification and appropriatedimension/allocatable/pointer
Uses_mpi_abort(<msg>)for error termination instead ofstop
Use!>style documentation for header comments; follow Doxygen Fortran format with!! @paramand!! @returntags
Useimplicit nonestatement in all modules
Useprivatedeclaration followed by explicitpublicexports in modules
Use derived types with pointers for encapsulation (e.g.,pointer, dimension(:,:,:) => null())
Usepureandelementalattributes for side-effect-free functions; combine them for array ...
Files:
src/simulation/m_checker.fpp
src/simulation/**/*.{fpp,f90}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/simulation/**/*.{fpp,f90}: Wrap tight GPU loops with !$acc parallel loop gang vector default(present) reduction(...); add collapse(n) when safe; declare loop-local variables with private(...)
Allocate large GPU arrays with managed memory or move them into persistent !$acc enter data regions at start-up
Avoid stop/error stop inside GPU device code
Ensure GPU code compiles with Cray ftn, NVIDIA nvfortran, GNU gfortran, and Intel ifx/ifort compilers
src/simulation/**/*.{fpp,f90}: Mark GPU-callable helpers with$:GPU_ROUTINE(function_name='...', parallelism='[seq]')immediately after declaration
Do not use OpenACC or OpenMP directives directly; use Fypp macros fromsrc/common/include/parallel_macros.fppinstead
Wrap tight loops with$:GPU_PARALLEL_FOR(private='[...]', copy='[...]')macro; addcollapse=nfor safe nested loop merging
Declare loop-local variables withprivate='[...]'in GPU parallel loop macros
Allocate large arrays withmanagedor move them into a persistent$:GPU_ENTER_DATA(...)region at start-up
Do not placestoporerror stopinside device code
Files:
src/simulation/m_checker.fpp
src/**/*.fpp
📄 CodeRabbit inference engine (.cursor/rules/mfc-agent-rules.mdc)
src/**/*.fpp: Use.fppfile extension for Fypp preprocessed files; CMake transpiles them to.f90
Start module files with Fypp include for macros:#:include 'macros.fpp'
Use the fyppASSERTmacro for validating conditions:@:ASSERT(predicate, message)
Use fypp macro@:ALLOCATE(var1, var2)for device-aware allocation instead of standard Fortranallocate
Use fypp macro@:DEALLOCATE(var1, var2)for device-aware deallocation instead of standard Fortrandeallocate
Files:
src/simulation/m_checker.fpp
🧠 Learnings (4)
📚 Learning: 2025-11-24T21:50:16.684Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.684Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Ensure GPU code compiles with Cray ftn, NVIDIA nvfortran, GNU gfortran, and Intel ifx/ifort compilers
Applied to files:
src/simulation/m_checker.fpp
📚 Learning: 2025-11-24T21:50:16.684Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T21:50:16.684Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Avoid stop/error stop inside GPU device code
Applied to files:
src/simulation/m_checker.fpp
📚 Learning: 2025-11-24T21:50:46.879Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.879Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Do not use OpenACC or OpenMP directives directly; use Fypp macros from `src/common/include/parallel_macros.fpp` instead
Applied to files:
src/simulation/m_checker.fpp
📚 Learning: 2025-11-24T21:50:46.879Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.879Z
Learning: Applies to src/simulation/**/*.{fpp,f90} : Do not place `stop` or `error stop` inside device code
Applied to files:
src/simulation/m_checker.fpp
🧬 Code graph analysis (2)
toolchain/mfc/run/input.py (2)
toolchain/mfc/case_validator.py (2)
validate_case_constraints(1709-1720)CaseConstraintError(21-22)toolchain/mfc/common.py (1)
MFCException(31-32)
toolchain/mfc/case_validator.py (1)
toolchain/mfc/common.py (1)
MFCException(31-32)
🪛 Ruff (0.14.6)
toolchain/mfc/run/input.py
106-106: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
106-106: Avoid specifying long messages outside the exception class
(TRY003)
toolchain/mfc/case_validator.py
1318-1318: Ambiguous variable name: l
(E741)
1702-1702: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Code Cleanliness Check
- GitHub Check: Coverage Test on CodeCov
- GitHub Check: Github (macos, mpi, no-debug, false)
- GitHub Check: Github (ubuntu, no-mpi, single, no-debug, false)
- GitHub Check: Github (macos, mpi, debug, false)
- GitHub Check: Github (ubuntu, mpi, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, no-debug, true)
- GitHub Check: Github (ubuntu, mpi, debug, false)
- GitHub Check: Github (ubuntu, mpi, debug, true)
- GitHub Check: Georgia Tech | Phoenix (NVHPC) (cpu)
- GitHub Check: Georgia Tech | Phoenix (NVHPC) (gpu)
- GitHub Check: Agent
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Build & Publish
🔇 Additional comments (6)
toolchain/mfc/case_validator.py (3)
21-22: LGTM on the exception class design.Inheriting from
MFCExceptionkeeps the exception hierarchy consistent with the existing codebase. The docstring is clear and concise.
1684-1706: LGTM on the main validation entry point.The stage-based dispatch is clean. Aggregating errors before raising a single exception provides a better user experience. The error formatting with bullet points is user-friendly.
1-19: Well-structured module with clear organization.The module docstring clearly explains the purpose and lists the source Fortran files this replaces. The pylint disable is appropriately justified. The imports are minimal and appropriate.
toolchain/mfc/run/input.py (1)
109-114: LGTM on the validation integration point.Calling
validate_constraintsbeforegenerate_inpandgenerate_fppensures errors are caught early, before any files are written. This is the correct fail-fast approach.src/simulation/m_checker.fpp (2)
25-44: LGTM on the streamlined validation flow.The remaining checks are appropriately kept in Fortran as they depend on:
- Compiler-specific preprocessor directives (
MFC_OpenACC,__PGI,_CRAYFTN)- Runtime-computed values (
num_stcls_min)- Precision-specific preprocessor checks (
MFC_SINGLE_PRECISION)- GPU-specific flags (
__NVCOMPILER_GPU_UNIFIED_MEM)These cannot be validated in Python before compilation.
54-64: LGTM on the WENO/MUSCL validation subroutines.These subroutines correctly validate against
num_stcls_min*weno_orderandnum_stcls_min*muscl_order, which are runtime-computed values. The error messages include the computed threshold value for clarity.Also applies to: 66-76
There was a problem hiding this comment.
Pull request overview
This PR migrates case parameter validation from Fortran to Python by introducing a comprehensive case_validator.py module that validates MFC case parameters before execution. The validation is now invoked early in the Python toolchain workflow (in input.py), providing better error messages and catching configuration errors before the Fortran codes run.
Key Changes
- Added
toolchain/mfc/case_validator.py(~1720 lines) withCaseValidatorclass implementing all parameter constraint checks - Modified
toolchain/mfc/run/input.pyto call validation before generating input files - Removed duplicate validation logic from Fortran files (
src/simulation/m_checker.fpp,src/pre_process/m_checker.fpp,src/post_process/m_checker.fpp,src/common/m_checker_common.fpp)
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| toolchain/mfc/case_validator.py | New comprehensive Python validator module with stage-specific validation methods (simulation, pre_process, post_process) |
| toolchain/mfc/run/input.py | Integrates validator by calling validate_constraints() before generating input files |
| src/simulation/m_checker.fpp | Removed most validation subroutines, keeping only compiler/WENO/MUSCL/geometry checks |
| src/pre_process/m_checker.fpp | Removed most validation subroutines, keeping only parallel I/O check |
| src/post_process/m_checker.fpp | Removed most validation subroutines, keeping only output format precision check and FFT-specific checks |
| src/common/m_checker_common.fpp | Removed all common validation subroutines that are now in Python |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 6 files
Prompt for AI agents (all 1 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="toolchain/mfc/case_validator.py">
<violation number="1" location="toolchain/mfc/case_validator.py:975">
Typo in error message: the parameter is `adap_dt` but the message says "adapt_dt". This could confuse users.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
|
/improve |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
toolchain/mfc/case_validator.py (1)
1352-1353: Rename loop variablelin thelengthcomprehension for clarityUsing
linany(l is not None for l in length)is flagged as ambiguous and hurts readability.Consider renaming it (e.g.,
length_val) to avoid confusion and satisfy linters:- self.prohibit(any(l is not None for l in length), + self.prohibit(any(length_val is not None for length_val in length), f"Circle Patch {i} can't have lengths defined")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
toolchain/mfc/case_validator.py(1 hunks)toolchain/mfc/run/input.py(2 hunks)toolchain/mfc/run/run.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
toolchain/mfc/case_validator.py (1)
toolchain/mfc/common.py (1)
MFCException(31-32)
toolchain/mfc/run/run.py (1)
toolchain/mfc/run/input.py (1)
generate(109-114)
toolchain/mfc/run/input.py (3)
toolchain/mfc/case_validator.py (2)
validate_case_constraints(1740-1751)CaseConstraintError(21-22)toolchain/mfc/common.py (1)
MFCException(31-32)toolchain/mfc/case.py (2)
validate_params(71-81)Case(19-292)
🪛 Ruff (0.14.6)
toolchain/mfc/case_validator.py
1352-1352: Ambiguous variable name: l
(E741)
toolchain/mfc/run/input.py
106-106: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Github (ubuntu, mpi, no-debug, false)
- GitHub Check: Github (ubuntu, no-mpi, single, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, no-debug, true)
- GitHub Check: Github (ubuntu, mpi, debug, true)
- GitHub Check: Github (macos, mpi, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, debug, false)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Code Cleanliness Check
- GitHub Check: Coverage Test on CodeCov
- GitHub Check: Build & Publish
🔇 Additional comments (3)
toolchain/mfc/run/run.py (1)
116-121: Good:runnow goes through the unified generate path (with validation)Switching to
case.generate(target)ensures the PythonCaseValidatorruns before both.inpandcase.fppgeneration, fixing the previous gap wheremfc runbypassed validation by callinggenerate_inpdirectly.toolchain/mfc/run/input.py (1)
98-115: I'll help you verify the review comment about validator wiring and potentialgenerate_inpcall sites. Let me start by exploring the codebase to verify the concerns raised.
<function_calls>
#!/bin/bashFirst, verify the import statement and current state of input.py
echo "=== Checking input.py imports and key methods ==="
rg -n "from.*case_validator|import.*case_validator" --type pyecho ""
echo "=== Searching for all generate_inp call sites ==="
rg -n "generate_inp" -A 2 -B 2 --type py
</function_calls>Looking at your verification request, I need to search for all call sites of
generate_inpto ensure they're not bypassing the new validation layer. Let me execute a comprehensive search:
<function_calls>
#!/bin/bashSearch for all generate_inp method calls and definitions
echo "=== All generate_inp references ==="
rg -n ".generate_inp(" --type py -C 3echo ""
echo "=== Check if MFCInputFile is instantiated and generate_inp called directly ==="
rg -n "MFCInputFile(" --type py -A 5
</function_calls>Let me also check how
generateis being called from the main run flow:
<function_calls>
#!/bin/bashCheck run.py for how generate is called
echo "=== Checking run.py for input file generation calls ==="
fd -e py -path "/run/run.py" -o -path "/run.py" | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && cat -n {} | head -100'echo ""
echo "=== Direct generate calls on MFCInputFile ==="
rg -n ".generate(" --type py -C 2 | grep -A 2 -B 2 "MFCInputFile|input_file"
</function_calls>Let me refine my search to find the exact file structure and understand the execution flow:
<function_calls>
#!/bin/bashFind run.py files in the toolchain
echo "=== Locating run files ==="
fd -e py "run.py" toolchain/ 2>/dev/nullecho ""
echo "=== Find where MFCInputFile is used ==="
rg -n "MFCInputFile" --type py -B 1 -A 1echo ""
echo "=== Check for generate_fpp direct calls ==="
rg -n ".generate_fpp(" --type py -B 2 -A 2
</function_calls>toolchain/mfc/case_validator.py (1)
1059-1077: Based on the web search results, I've gathered important evidence about the GRCBC flag structure. Let me document my findings:Key Finding: According to MFC documentation, GRCBC flags in the case dictionary are defined as:
bc_xgrcbc_in,bc_xgrcbc_out,bc_xgrcbc_vel_out(and similarly for y, z)- These are direction-level flags (one per direction), not per-boundary
- Case files define them as Python booleans (
True/False), not Fortran stringsThis directly validates the first concern in the review: the code reads direction-level flags (
bc_{dir}%grcbc_in) but then validates against direction+boundary pairs (bc_{dir}%begandbc_{dir}%end). The logic inconsistency is real.However, the sandbox environment prevents me from:
- Verifying the
get()method's actual return type (whether it preserves Python booleans or converts to strings)- Confirming how other boolean flags in the file are normalized
- Determining the intended per-boundary vs. direction-level behavior
Rewritten Review Comment:
GRCBC flag validation logic has structural inconsistency
The code reads direction-level flags (
bc_{dir}%grcbc_in/out/vel_out) but validates against direction+boundary pairs. Per MFC documentation, GRCBC flags are direction-level parameters (one per direction), not per-boundary. The current logic requiring bothbc_beg != -7 AND bc_end != -7to be true is inconsistent with this structure—it should check either one boundary or be restructured if per-boundary enforcement is intended.Additionally, verify whether
self.get()returns Python booleans or string values ("T"/"F"). If strings, the direct truthiness checks (if grcbc_in_beg:) will treat"F"as truthy, causing the validation to always trigger regardless of the flag's logical value. Other boolean flags in this file should be checked for comparison to ensure consistent normalization (e.g.,self.get(..., 'F') == 'T').
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 (3)
toolchain/mfc/sched.py (3)
73-83: Critical control-flow bug: exception is raised even whencompleted_successfullyis True.The
passstatement at line 77 does nothing, so execution falls through to line 83 which unconditionally raisesthreadHolder.thread.exc. This contradicts the comment stating "Don't re-raise - this is expected behavior."Additionally, the pipeline flags trailing whitespace and unnecessary
elifafter the raise.Apply this diff to fix the logic, address trailing whitespace, and reduce branch complexity:
# Check for and propagate any exceptions that occurred in the worker thread # But only if the worker function didn't complete successfully # (This allows test failures to be handled gracefully by handle_case) - if threadHolder.thread.exc is not None: - if threadHolder.thread.completed_successfully: - # Test framework handled the exception gracefully (e.g., test failure) - # Don't re-raise - this is expected behavior - pass - elif hasattr(threadHolder.thread, 'exc_info') and threadHolder.thread.exc_info: - # Unhandled exception - this indicates a real problem - error_msg = f"Worker thread {threadID} failed with unhandled exception:\n{threadHolder.thread.exc_info}" - raise RuntimeError(error_msg) from threadHolder.thread.exc - - raise threadHolder.thread.exc + if threadHolder.thread.exc is not None and not threadHolder.thread.completed_successfully: + # Unhandled exception - propagate with full traceback if available + if hasattr(threadHolder.thread, 'exc_info') and threadHolder.thread.exc_info: + error_msg = f"Worker thread {threadID} failed with unhandled exception:\n{threadHolder.thread.exc_info}" + raise RuntimeError(error_msg) from threadHolder.thread.exc + raise threadHolder.thread.excThis fix:
- Correctly skips re-raising when
completed_successfullyis True- Removes trailing whitespace
- Reduces branch count by combining conditions
- Eliminates the unnecessary
elifafter implicit return path
26-31: Type annotation fordevicesshould beOptionalto match usage.At line 131,
use_devices(which can beNone) is passed toWorkerThreadHolder. The field type should reflect this nullable behavior.@dataclasses.dataclass class WorkerThreadHolder: # pylint: disable=too-many-instance-attributes thread: threading.Thread ppn: int load: float - devices: typing.Set[int] + devices: typing.Optional[typing.Set[int]]
108-113: Fix trailing whitespace flagged by pipeline.The pipeline reports trailing whitespace on lines 110 and 113. Please remove any trailing spaces on these lines to pass the lint check.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
toolchain/mfc/sched.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
toolchain/mfc/sched.py (1)
toolchain/mfc/test/test.py (1)
test(93-180)
🪛 GitHub Actions: Lint Toolchain
toolchain/mfc/sched.py
[error] 73-73: pylint: C0303 trailing-whitespace detected.
[error] 77-77: pylint: C0303 trailing-whitespace detected.
[error] 110-110: pylint: C0303 trailing-whitespace detected.
[error] 113-113: pylint: C0303 trailing-whitespace detected.
[error] 78-78: pylint: R1705 Unnecessary "elif" after "return" (no-else-return).
[error] 87-87: pylint: R1705 Unnecessary "else" after "return" (no-else-return).
[error] 94-94: pylint: R0912 Too many branches (13/12) (too-many-branches).
🪛 Ruff (0.14.6)
toolchain/mfc/sched.py
41-41: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Code Cleanliness Check
- GitHub Check: Github (ubuntu, no-mpi, single, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, no-debug, true)
- GitHub Check: Github (ubuntu, mpi, debug, true)
- GitHub Check: Github (ubuntu, mpi, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, debug, false)
- GitHub Check: Coverage Test on CodeCov
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Build & Publish
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
toolchain/mfc/sched.py (1)
73-78: Remove redundanthasattrcheck.Since
exc_infois initialized inWorkerThread.__init__(line 10), it will always be present as an attribute. Thehasattrcheck is redundant; only the truthiness check is needed.Apply this diff:
- if hasattr(threadHolder.thread, 'exc_info') and threadHolder.thread.exc_info: + if threadHolder.thread.exc_info:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
toolchain/mfc/sched.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (2)
toolchain/mfc/sched.py (2)
26-31: LGTM! Optional type annotations now explicit.The changes correctly address the previous PEP 484 violation by making the
Optionaltype explicit for thedevicesparameter and field. This resolves the static analysis warning.Also applies to: 41-41
56-68: Well-designed timeout and exception handling.The 30-second timeout prevents infinite hangs, and the exception propagation logic correctly distinguishes between completed tasks and unhandled exceptions. The full traceback preservation enhances debuggability for worker thread failures.
Also applies to: 73-78
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
toolchain/mfc/sched.py (1)
45-45: Avoid shadowing the built-inidfunction.The variable name
idin the dictionary comprehension shadows Python's built-inid()function. While functionally correct, this reduces code clarity.Apply this diff to use a more descriptive name:
- sched.LOAD = { id: 0.0 for id in devices or [] } + sched.LOAD = { device_id: 0.0 for device_id in devices or [] }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
toolchain/mfc/sched.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Core MFC Line Difference
- GitHub Check: Github (ubuntu, mpi, debug, false)
- GitHub Check: Github (ubuntu, mpi, no-debug, true)
- GitHub Check: Code Cleanliness Check
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Detect File Changes
🔇 Additional comments (3)
toolchain/mfc/sched.py (3)
9-11: LGTM! Enhanced exception tracking improves error reporting.The addition of
exc_infofor full traceback capture andcompleted_successfullyfor explicit state tracking are good improvements that enable better debugging and error propagation in worker threads.Also applies to: 19-19, 22-23
31-31: Good fix: explicitOptionaltype hints address PEP 484 compliance.The explicit
typing.Optionalannotations ondevicesparameters correctly address the static analysis warning and prior review feedback.Also applies to: 41-41
56-68: Robust join timeout and error handling.The 30-second timeout with explicit hang detection (lines 60-63) and join exception handling (lines 65-68) provide good protection against thread lifecycle issues. Error messages are descriptive and helpful for debugging.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
toolchain/mfc/case_validator.py (1)
387-389: Message inconsistency: check allows zero but message says "positive".The check
cv < 0allows zero values, but the error message states "must be positive" which typically excludes zero. This appears to be the same issue flagged in a previous review.
🧹 Nitpick comments (1)
toolchain/mfc/case_validator.py (1)
25-27: Consider extracting feature-specific validators for future maintainability.The class is large (~1720 lines) but well-organized by validation stage. As a future improvement, consider extracting domain-specific validators (e.g.,
BubbleValidator,MHDValidator) as the number of constraints grows. This is not urgent given the current clean organization.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
toolchain/mfc/case_validator.py(1 hunks)toolchain/mfc/sched.py(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-24T21:50:46.879Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.879Z
Learning: Applies to **/*.{fpp,f90} : Use `wp` (working precision) parameter from `m_precision_select` instead of hardcoded precision like `real*8`
Applied to files:
toolchain/mfc/case_validator.py
🧬 Code graph analysis (1)
toolchain/mfc/case_validator.py (1)
toolchain/mfc/common.py (1)
MFCException(31-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Github (macos, mpi, debug, false)
- GitHub Check: Github (macos, mpi, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, debug, true)
- GitHub Check: Github (ubuntu, mpi, no-debug, true)
- GitHub Check: Github (ubuntu, no-mpi, single, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, debug, false)
- GitHub Check: Coverage Test on CodeCov
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Code Cleanliness Check
- GitHub Check: Build & Publish
🔇 Additional comments (9)
toolchain/mfc/sched.py (3)
27-31: LGTM! Type annotations properly updated.The
Optional[typing.Set[int]]type hint correctly indicates thatdevicescan beNone, addressing PEP 484 compliance.
41-41: LGTM! Function signature correctly updated.The
Optional[typing.Set[int]]type hint for thedevicesparameter properly reflects that it can beNone, satisfying PEP 484 requirements.
71-76: LGTM! Exception propagation logic improved.The reworked exception handling now properly propagates exceptions with full traceback information when available via
exc_info, providing better debugging context. The logic correctly handles both cases: whenexc_infois available (wraps inRuntimeErrorwith traceback) and when it isn't (raises the original exception).toolchain/mfc/case_validator.py (6)
1-23: Well-structured module with clear documentation.The module docstring clearly explains the purpose and references the original Fortran sources. The
CaseConstraintErrorproperly extendsMFCExceptionfor consistent error handling across the toolchain.
25-43: Clean validator design with declarative constraint checking.The
prohibit()helper provides a clean, declarative API for expressing constraints. The error collection pattern allows reporting all violations at once rather than failing on the first error.
1117-1160: Moving boundary validation logic is correct.The implementation properly validates that slip-wall (-15) boundaries cannot have non-normal velocities, while no-slip walls (-16) can. The separation of
begandendboundary checks addresses previously flagged issues.
1720-1746: Well-designed validation entry point with graceful handling of unknown stages.The
validate()method cleanly handles different stages and silently skips validation for auxiliary targets like 'syscheck'. The error aggregation produces user-friendly output with bullet points.
1748-1759: Clean public API for case validation.The convenience function provides a simple interface for external callers without needing to manage the
CaseValidatorinstance directly.
600-603: Bug: MissingNoneguards foravg_stateandwave_speedschecks.Both conditions will incorrectly trigger when the values are not set (None), since
None != 2andNone != 1evaluate toTrue.Apply this diff:
- self.prohibit(avg_state is not None and avg_state != 2, + self.prohibit(avg_state is not None and avg_state != 2, "6-equation model (model_eqns = 3) requires avg_state = 2") - self.prohibit(wave_speeds is not None and wave_speeds != 1, + self.prohibit(wave_speeds is not None and wave_speeds != 1, "6-equation model (model_eqns = 3) requires wave_speeds = 1")Wait, looking again at the code - these checks already have the
is not Noneguards. Let me verify:
- Line 600:
avg_state is not None and avg_state != 2✓- Line 602:
wave_speeds is not None and wave_speeds != 1✓
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
toolchain/mfc/case_validator.py (1)
387-389: Clarify: Message-condition mismatch forcvvalidation.The error message says "must be positive" but the condition
cv < 0allows zero. For consistency, either:
- Change message to "must be non-negative" (if zero is acceptable), or
- Change condition to
cv <= 0(if zero should be rejected)Apply this diff if zero should be rejected:
if cv is not None: - self.prohibit(cv < 0, - f"fluid_pp({i})%cv must be positive") + self.prohibit(cv <= 0, + f"fluid_pp({i})%cv must be positive")Or apply this diff if zero is acceptable:
if cv is not None: self.prohibit(cv < 0, - f"fluid_pp({i})%cv must be positive") + f"fluid_pp({i})%cv must be non-negative")Note: A past review comment flagged this issue and was marked as addressed in commit 4c98690, but the inconsistency appears to remain in the current code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
toolchain/mfc/case_validator.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-24T21:50:46.879Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.879Z
Learning: Applies to **/*.{fpp,f90} : Use `wp` (working precision) parameter from `m_precision_select` instead of hardcoded precision like `real*8`
Applied to files:
toolchain/mfc/case_validator.py
🧬 Code graph analysis (1)
toolchain/mfc/case_validator.py (2)
toolchain/mfc/common.py (1)
MFCException(31-32)toolchain/mfc/case.py (2)
Case(19-292)validate_params(71-81)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Coverage Test on CodeCov
- GitHub Check: Code Cleanliness Check
- GitHub Check: Github (macos, mpi, no-debug, false)
- GitHub Check: Github (macos, mpi, debug, false)
- GitHub Check: Github (ubuntu, mpi, no-debug, true)
- GitHub Check: Github (ubuntu, mpi, no-debug, false)
- GitHub Check: Github (ubuntu, no-mpi, single, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, debug, true)
- GitHub Check: Github (ubuntu, mpi, debug, false)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Build & Publish
🔇 Additional comments (8)
toolchain/mfc/case_validator.py (8)
25-44: LGTM: Clean validator design.The class structure follows a clear pattern with error accumulation via
prohibit(). The helper methods (get,is_set,prohibit) provide a solid foundation for the validation checks.
976-999: LGTM: Adaptive time stepping validation is comprehensive.The checks properly validate
time_stepper,model_eqns, and compatibility constraints withqbmm,polytropic,bubbles_lagrange, andadv_n. All None guards are in place.
1001-1023: LGTM: Alternative sound speed validation is complete.All required checks are present including
model_eqns,riemann_solver,num_fluids,bubbles_euler, andavg_stateconstraints. None guards are properly applied.
1117-1160: LGTM: Moving boundary condition validation is correct.The logic properly:
- Loads velocities separately for beg and end boundaries
- Correctly identifies tangential (non-normal) components for each direction
- Enforces that slip wall (-15) cannot have tangential velocities
- Requires wall boundaries (-15 or -16) when any velocity is set
1166-1380: LGTM: Pre-process validation is thorough and well-structured.The pre-process checks cover:
- Restart parameters with proper dependency validation
- QBMM pre-process constraints
- Parallel I/O and down-sampling requirements
- Grid stretching with coordinate bounds validation
- Density perturbation constraints
- BC patch geometry validation with geometry-specific rules
The chemistry check appropriately delegates to runtime validation since
num_speciesis set by Cantera dynamically.
1386-1397: Consider: Add build precision validation for output format.A past review comment suggested prohibiting
precision = 2(double-precision output) when the code is built with single precision. The current implementation doesn't include this check.This may be intentional if:
- Build configuration isn't available at Python validation time, or
- The check is deferred to runtime in Fortran
If the build precision can be determined (e.g., via an environment variable or config file), consider adding:
wp_is_single = self.get('wp_is_single', False) # or from env/config if precision is not None: self.prohibit(precision == 2 and wp_is_single, "precision = 2 (double) not supported with single-precision build")Can the Python validator determine the working precision at validation time? If so, this check should be added to match the Fortran validation.
1608-1639: LGTM: Flow variable coverage check is comprehensive.This validation ensures at least one output variable is selected for post-processing, checking all scalar flags and array-based variables across fluids and dimensions. The approach of defaulting
num_fluidsto 1 when unset is appropriate for the coverage check.
1720-1759: LGTM: Well-designed validation orchestration.The validation entry points provide:
- Clear stage-based routing (simulation, pre_process, post_process)
- Appropriate handling of non-validation stages (silent skip for auxiliary targets)
- Good error aggregation and formatting with bullet points
- Convenient top-level API via
validate_case_constraints()The design properly separates common checks from stage-specific checks and avoids code duplication.
|
CodeAnt AI is running Incremental review |
codeant-ai
bot
added
size:XXL
|
CodeAnt AI Incremental review completed. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
toolchain/mfc/case_validator.py (1)
1362-1374: Consider adding build-precision validation if feasible.The Fortran code prohibits
precision = 2(double-precision output) when MFC is built with single precision. This validation is missing here, which means users won't discover the incompatibility until runtime.If build-time precision information can be made available to the Python validator (e.g., via an environment variable or build configuration file), add:
# Check if this info is available in params or environment wp_is_single = self.get('wp_is_single', False) # or from env/config if precision == 2 and wp_is_single: self.prohibit(True, "precision = 2 (double-precision output) requires MFC to be built with double precision")If this information is not accessible at input-generation time, document this as a known limitation of Python-side validation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
toolchain/mfc/case_validator.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-24T21:50:46.879Z
Learnt from: CR
Repo: MFlowCode/MFC PR: 0
File: .cursor/rules/mfc-agent-rules.mdc:0-0
Timestamp: 2025-11-24T21:50:46.879Z
Learning: Applies to **/*.{fpp,f90} : Use `wp` (working precision) parameter from `m_precision_select` instead of hardcoded precision like `real*8`
Applied to files:
toolchain/mfc/case_validator.py
🧬 Code graph analysis (1)
toolchain/mfc/case_validator.py (1)
toolchain/mfc/common.py (1)
MFCException(31-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Coverage Test on CodeCov
- GitHub Check: Oak Ridge | Frontier (CCE) (gpu)
- GitHub Check: Georgia Tech | Phoenix (NVHPC) (gpu)
- GitHub Check: Georgia Tech | Phoenix (NVHPC) (gpu)
- GitHub Check: Code Cleanliness Check
- GitHub Check: Github (macos, mpi, debug, false)
- GitHub Check: Github (ubuntu, mpi, no-debug, true)
- GitHub Check: Github (ubuntu, no-mpi, single, no-debug, false)
- GitHub Check: Github (macos, mpi, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, no-debug, false)
- GitHub Check: Github (ubuntu, mpi, debug, true)
- GitHub Check: Self Hosted (cpu, none, frontier)
- GitHub Check: Github (ubuntu, mpi, debug, false)
- GitHub Check: Self Hosted (cpu, none, gt)
- GitHub Check: Self Hosted (gpu, omp, gt)
- GitHub Check: Self Hosted (gpu, acc, frontier)
- GitHub Check: Self Hosted (gpu, omp, frontier)
- GitHub Check: Self Hosted (gpu, acc, gt)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Build & Publish
🔇 Additional comments (2)
toolchain/mfc/case_validator.py (2)
1-44: LGTM: Clean class structure with error aggregation.The validator class structure is well-organized with a simple error collection pattern. The
prohibitmethod allows validation logic to accumulate all errors before raising, providing better UX than failing on the first error.
1621-1734: LGTM: Well-organized stage-aware validation framework.The main validation entry points correctly organize checks by stage (common, simulation, pre-process, post-process) and provide clear error reporting. The stage-based approach ensures that only relevant constraints are checked for each MFC target, improving error messages and reducing false positives.
The error aggregation pattern (collecting all violations before raising) provides better user experience than failing on the first error.
- Keep typing.Optional[typing.Set[int]] for devices parameter (from our branch) - Integrate dimension-aware long-running test notifications (from upstream PR MFlowCode#1067) - Combine both WorkerThreadHolder fields and sched function signature improvements
|
/improve |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1066 +/- ##
==========================================
- Coverage 44.36% 44.08% -0.28%
==========================================
Files 71 71
Lines 20590 20284 -306
Branches 1994 1974 -20
==========================================
- Hits 9134 8943 -191
+ Misses 10310 10209 -101
+ Partials 1146 1132 -14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Case checking moved to Python! (MFlowCode#1066)
1 participant
User description
User description
Case checking moved to Python!
PR Type
Enhancement, Tests
Description
Move case validation from Fortran to Python for early error detection
Remove redundant Fortran constraint checks from multiple modules
Implement comprehensive CaseValidator class with 60+ constraint methods
Integrate validation into input file generation pipeline
Diagram Walkthrough
File Walkthrough
case_validator.py
Comprehensive Python case parameter validator implementationtoolchain/mfc/case_validator.py
parameter constraints
pre-process, and post-process stages
reconstruction schemes, boundary conditions, bubble models, MHD,
acoustic sources, and output parameters
input.py
Integrate case validation into input generationtoolchain/mfc/run/input.py
file generation
m_checker.fpp
Remove Fortran validation checks moved to Pythonsrc/simulation/m_checker.fpp
model_eqns, acoustic_src, hypoelasticity, bubbles, adapt_dt,
alt_soundspeed, grcbc, mhd, continuum_damage, stiffened_eos,
body_forces, misc)
m_checker_common.fpp
Remove redundant common validation checkssrc/common/m_checker_common.fpp
m_checker.fpp
Remove redundant pre-process validation checkssrc/pre_process/m_checker.fpp
m_checker.fpp
Remove redundant post-process validation checkssrc/post_process/m_checker.fpp
CodeAnt-AI Description
Validate case parameters in Python before generating inputs
What Changed
Impact
✅ Clearer case validation errors✅ Earlier failure for invalid inputs✅ Propagate worker-thread exceptions💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
Refactor
New Features
Behavioral Change
Chores
✏️ Tip: You can customize this high-level summary in your review settings.