Fix Homebrew install crash from namelist parser#1154
Fix Homebrew install crash from namelist parser#1154sbryngelson merged 1 commit intoMFlowCode:masterfrom
Conversation
…g Fortran sources The namelist parser (added post-v5.2.0) reads src/*/m_start_up.fpp at runtime to determine valid parameters per target. Homebrew installs don't include these Fortran source files, causing a FileNotFoundError crash. When sources are unavailable, fall back to a built-in copy of the parsed parameter sets (generated from the same Fortran namelists). This maintains correct per-target filtering while working without the source tree. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
📝 WalkthroughWalkthroughThe pull request introduces a built-in fallback parameter system in the namelist parser to gracefully handle missing Fortran source files for pre_process, simulation, and post_process targets. When required source files are unavailable, Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
codeant-ai
bot
added
the
size:M
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Nitpicks 🔍
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| # Use built-in fallback parameters. | ||
| return dict(_FALLBACK_PARAMS) |
There was a problem hiding this comment.
Suggestion: Only use the built-in fallback parameters when all Fortran source files are missing and otherwise raise an explicit error to avoid silently masking partial misconfigurations. [custom_rule]
Severity Level: Minor
| # Use built-in fallback parameters. | |
| return dict(_FALLBACK_PARAMS) | |
| # Use built-in fallback parameters when all Fortran sources are missing. | |
| if all(not p.exists() for p in targets.values()): | |
| return dict(_FALLBACK_PARAMS) | |
| raise FileNotFoundError(f"Fortran source not found: {filepath}") |
Why it matters? ⭐
The current code returns the built-in fallback as soon as any single target file is missing, which can silently mask partial repository misconfigurations (e.g., one target's sources missing while others are present). The proposed change tightens the behavior so the fallback is only used when all Fortran source files are absent, and otherwise raises FileNotFoundError for the specific missing file. This directly improves correctness and aligns with the repository intent documented in the module (fallback only when Fortran sources are unavailable), avoiding partial-missing-source surprises that could lead to inconsistent target parameter sets.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** toolchain/mfc/params/namelist_parser.py
**Line:** 173:174
**Comment:**
*Custom Rule: Only use the built-in fallback parameters when all Fortran source files are missing and otherwise raise an explicit error to avoid silently masking partial misconfigurations.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
toolchain/mfc/params/namelist_parser.py (2)
170-175: Early return on first missing file applies fallback to all targets.If any single source file is missing, the function returns the entire fallback dict without attempting to parse the remaining files. This is fine for the Homebrew case (all sources absent), but if only one file is missing due to corruption or partial install, the user gets no warning and silently loses live-parsed params for the targets whose sources are present.
Consider logging a warning so the fallback isn't completely silent, e.g.:
Suggested improvement
+import logging + +log = logging.getLogger(__name__) + ... for target_name, filepath in targets.items(): if not filepath.exists(): - # Source files not available (e.g. Homebrew install). - # Use built-in fallback parameters. - return dict(_FALLBACK_PARAMS) + # Source files not available (e.g. Homebrew install). + # Use built-in fallback parameters. + log.debug("Fortran source %s not found; using built-in fallback parameters", filepath) + return dict(_FALLBACK_PARAMS) result[target_name] = parse_namelist_from_file(filepath)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/mfc/params/namelist_parser.py` around lines 170 - 175, The loop in parse_namelist_from_file caller currently returns the entire _FALLBACK_PARAMS on the first missing filepath, losing parsed results for other targets; change it to log a warning (e.g., using logging.warning or warnings.warn) mentioning the missing filepath and target_name, assign a per-target fallback into result (use _FALLBACK_PARAMS.get(target_name, dict(_FALLBACK_PARAMS)) or similar) and continue the loop so remaining targets are parsed with parse_namelist_from_file; remove the early return and ensure result is populated per-target and returned after the loop.
174-174: Shallow copy shares mutable set references with_FALLBACK_PARAMS.
dict(_FALLBACK_PARAMS)copies the dict but theSet[str]values are shared objects. If any future code path adds/removes from the returned sets (or from_TARGET_PARAMS_CACHE), it silently mutates the module-level constant. Current usage is read-only so this is safe today, but a deep copy would be more defensive:- return dict(_FALLBACK_PARAMS) + return {k: set(v) for k, v in _FALLBACK_PARAMS.items()}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@toolchain/mfc/params/namelist_parser.py` at line 174, The current return dict(_FALLBACK_PARAMS) returns a shallow copy that still shares the Set[str] values; change it to return a deep copy of the sets so callers cannot mutate the module-level constants. Replace the shallow copy with either return {k: set(v) for k, v in _FALLBACK_PARAMS.items()} or use copy.deepcopy(_FALLBACK_PARAMS) (importing copy), ensuring you update the return near the dict(_FALLBACK_PARAMS) site and keep references to _FALLBACK_PARAMS and _TARGET_PARAMS_CACHE consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@toolchain/mfc/params/namelist_parser.py`:
- Around line 17-19: The comment "To regenerate: python3
toolchain/mfc/params/namelist_parser.py" is misleading because the __main__
block only prints a summary and does not emit the _FALLBACK_PARAMS dict; either
change the comment to accurately describe the actual manual regeneration steps
or modify the script so its __main__ prints a copy-pasteable _FALLBACK_PARAMS
literal (or writes it to stdout/file). Also add a CI check (a small test/step)
that runs the namelist_parser to parse the live Fortran namelists and compares
the resulting parameter set to the _FALLBACK_PARAMS dict, failing the build if
any keys/values diverge so Homebrew and other consumers notice stale fallbacks.
Ensure references to _FALLBACK_PARAMS and the __main__ summary routine are used
when implementing the print-or-compare behavior.
---
Nitpick comments:
In `@toolchain/mfc/params/namelist_parser.py`:
- Around line 170-175: The loop in parse_namelist_from_file caller currently
returns the entire _FALLBACK_PARAMS on the first missing filepath, losing parsed
results for other targets; change it to log a warning (e.g., using
logging.warning or warnings.warn) mentioning the missing filepath and
target_name, assign a per-target fallback into result (use
_FALLBACK_PARAMS.get(target_name, dict(_FALLBACK_PARAMS)) or similar) and
continue the loop so remaining targets are parsed with parse_namelist_from_file;
remove the early return and ensure result is populated per-target and returned
after the loop.
- Line 174: The current return dict(_FALLBACK_PARAMS) returns a shallow copy
that still shares the Set[str] values; change it to return a deep copy of the
sets so callers cannot mutate the module-level constants. Replace the shallow
copy with either return {k: set(v) for k, v in _FALLBACK_PARAMS.items()} or use
copy.deepcopy(_FALLBACK_PARAMS) (importing copy), ensuring you update the return
near the dict(_FALLBACK_PARAMS) site and keep references to _FALLBACK_PARAMS and
_TARGET_PARAMS_CACHE consistent.
| # Fallback parameters for when Fortran source files are not available. | ||
| # Generated from the namelist definitions in src/*/m_start_up.fpp. | ||
| # To regenerate: python3 toolchain/mfc/params/namelist_parser.py |
There was a problem hiding this comment.
Regeneration comment is misleading; consider adding an automated staleness check.
Line 19 says # To regenerate: python3 toolchain/mfc/params/namelist_parser.py, but the __main__ block (lines 229–261) only prints a summary — it doesn't emit the _FALLBACK_PARAMS dict literal. Either update the script to actually print a copy-pasteable dict, or change this comment to reflect the real process.
Additionally, when a new parameter is added to the Fortran namelists, this fallback dict must also be updated or Homebrew users will silently lose access to that parameter. Consider adding a CI check that parses the live Fortran sources and compares against _FALLBACK_PARAMS, failing if they diverge.
Based on learnings: "When adding a new parameter to toolchain/mfc/params/definitions.py, update the namelist /user_inputs/ in the corresponding src/*/m_start_up.fpp" — _FALLBACK_PARAMS is now a third location that must stay in sync.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@toolchain/mfc/params/namelist_parser.py` around lines 17 - 19, The comment
"To regenerate: python3 toolchain/mfc/params/namelist_parser.py" is misleading
because the __main__ block only prints a summary and does not emit the
_FALLBACK_PARAMS dict; either change the comment to accurately describe the
actual manual regeneration steps or modify the script so its __main__ prints a
copy-pasteable _FALLBACK_PARAMS literal (or writes it to stdout/file). Also add
a CI check (a small test/step) that runs the namelist_parser to parse the live
Fortran namelists and compares the resulting parameter set to the
_FALLBACK_PARAMS dict, failing the build if any keys/values diverge so Homebrew
and other consumers notice stale fallbacks. Ensure references to
_FALLBACK_PARAMS and the __main__ summary routine are used when implementing the
print-or-compare behavior.
There was a problem hiding this comment.
Pull request overview
This PR fixes a crash in Homebrew installations where Fortran source files are not included in the package. The namelist parser (introduced post-v5.2.0) attempts to read these source files at runtime to validate parameters per target, but when sources are unavailable, it throws a FileNotFoundError.
Changes:
- Added a built-in fallback parameter set for all three targets (pre_process, simulation, post_process) extracted from the current Fortran namelist definitions
- Modified
parse_all_namelists()to return the fallback when source files are not available instead of raising an error - Removed an extraneous blank line in a docstring in
case_dicts.py
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| toolchain/mfc/params/namelist_parser.py | Added _FALLBACK_PARAMS dict with hardcoded parameter sets and modified parse_all_namelists() to use fallback when source files are missing |
| toolchain/mfc/run/case_dicts.py | Removed blank line in docstring (formatting) |
| for target_name, filepath in targets.items(): | ||
| if not filepath.exists(): | ||
| raise FileNotFoundError(f"Fortran source not found: {filepath}") | ||
| # Source files not available (e.g. Homebrew install). | ||
| # Use built-in fallback parameters. | ||
| return dict(_FALLBACK_PARAMS) |
There was a problem hiding this comment.
The early return on the first missing file assumes that if any source file is missing, all are missing (Homebrew install scenario). However, this could mask a corrupted or partial installation where only some files are missing. Consider checking all three files first and either using fallback for all (if all are missing) or raising an error (if only some are missing). This would make debugging installation issues easier.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1154 +/- ##
=======================================
Coverage 44.07% 44.07%
=======================================
Files 70 70
Lines 20431 20431
Branches 1974 1974
=======================================
Hits 9004 9004
Misses 10291 10291
Partials 1136 1136 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
User description
Summary
src/*/m_start_up.fppat runtime to determine valid parameters per target. Homebrew installs don't include these Fortran source files, causing aFileNotFoundErrorcrash when running any case.Test plan
bogus_paramrejected)run_time_infocorrectly excluded frompre_processin fallback🤖 Generated with Claude Code
CodeAnt-AI Description
Avoid crash when Fortran sources are missing by using built-in namelist parameters
What Changed
Impact
✅ No runtime crash on Homebrew installs✅ Correct per-target parameter validation without source files✅ Homebrew CI succeeds for macOS targets💡 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
Bug Fixes
Documentation