feat(core): engineering-hardening pass for 1.14.0#109
Conversation
Addresses a repo-wide review with nine items covering fail-closed validation, fidelity transparency, solver auditability, parser diagnostics, typing, coverage and the public API boundary. Changed - Pre-solve ERROR findings now fail closed. Tower.run / RotatingBlade.run raise pybmodes.checks.ModelValidationError on any ERROR-severity finding instead of warning and feeding the eigensolver non-physical input. New on_error="raise"|"warn" keyword (default raise); "warn" restores the prior behaviour and check_model=False skips the pass. WARN findings still warn. Shared routing in checks.apply_findings. Added - ModalResult.diagnostics (SolverDiagnostics): path taken, sparse to dense fallback and reason, mode-count guarantee, per-mode backward residuals and a mass-matrix conditioning estimate. The general path warns when it recovers fewer valid modes than requested. Transient telemetry, excluded from equality and not serialised. - ModalResult.ignored_physics: names parsed-but-not-modelled physics (distr_m today) so a result is honest about its fidelity. Persisted when non-empty and surfaced in the bundled report. - Report completeness stamp. generate_report gains a status argument; run_windio sets complete / partial / screening and carries it on WindioResult.report_status. Fixed - WindIO discovery now parses each candidate yaml structurally instead of scanning the text for "components:" / "floating_platform:", so a yaml that merely mentions those words is no longer mis-selected. - The .bmi reader raises a structured BMIParseError (file, line, section context, offending text) instead of a bare ValueError / IndexError / EOFError. Still a ValueError subclass for legacy callers. Internal - Strict mypy ratchet gains checks, coords, io.errors, workflows._base. - Enforced coverage floor (fail_under = 85), replacing the informational-only report. - pybmodes.campbell stops re-exporting its private helpers at the package root; the supported surface is CampbellResult / campbell_sweep / plot_campbell and internals are imported from their sub-modules. 907 tests pass, coverage 87 percent, ruff and mypy clean.
The 1.14.0 fail_under=85 floor applies to every pytest run, including the integration step which collects only a subset (nothing on a runner without upstream data, ~17 percent coverage). That turned the tolerated exit 5 into a spurious exit 1. Pass --no-cov on the integration step so the comprehensive gate stays on the default step where it belongs.
![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.
Reviewed commit: 5d6576cf21
ℹ️ 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".
| except Exception: | ||
| return None |
There was a problem hiding this comment.
Do not swallow all exceptions here: when pybmodes[windio] is not installed and a caller passes a directory to discover_windio_inputs, _require_yaml() raises ModuleNotFoundError but this blanket except converts it to None, so discovery later fails with a misleading FileNotFoundError ("no WindIO ontology .yaml found"). This is a regression from the previous scan-based discovery path and hides the actionable install hint; re-raise dependency/import errors (or only catch YAML parse/shape errors) so users get the correct failure mode.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, fixed in 8c97f07. _require_yaml is now resolved outside the try so a missing [windio] extra propagates its ModuleNotFoundError with the install hint, and only YAML parse or file-read errors map to None. Added a regression test that the missing-PyYAML error propagates.
_load_windio_doc resolved PyYAML inside a blanket try/except that swallowed the ModuleNotFoundError _require_yaml raises when the [windio] extra is absent, turning the actionable install hint into a misleading 'no WindIO ontology .yaml found' later. Resolve PyYAML outside the try and catch only YAML parse / file-read errors, so a genuinely-unparseable candidate still maps to None while a missing dependency propagates with its hint.
A bare scratch yaml under a broad workspace parent (e.g. D:\repos) could climb four ancestor levels and land on an unrelated OpenFAST / MoorDyn clone, flipping a screening preview into a wrong-turbine deck-backed solve. Limit the ancestor climb from 4 to 3 levels so discovery stays scoped to the turbine layout itself; a bare yaml then correctly stays in labelled screening mode.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f586e4c057
ℹ️ 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".
| if self._pos >= len(self._lines): | ||
| raise self._fail( | ||
| "reached the end of the file while a string value was " | ||
| "expected", line_index=len(self._lines) - 1, |
There was a problem hiding this comment.
When parsing an empty .bmi, read_str() calls _fail(..., line_index=len(self._lines)-1), which passes -1; _fail then reports line=0. That violates the new parser contract of 1-based line diagnostics and produces misleading error metadata for the most basic malformed-input case (empty file), making automated triage and user debugging harder. Use a non-negative fallback index (or clamp to line 1) when no lines exist.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a235500. _fail now clamps the reported line to at least 1, so an empty .bmi reports line 1 rather than 0. Added a regression test for the empty-file case.
read_str / _skip_blanks pass line_index = len(lines) - 1 on EOF, which is -1 for an empty file, and _fail then reported line 0, breaking the 1-based diagnostic contract for the most basic malformed input. Clamp the reported line to at least 1 in _fail.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2355006c5
ℹ️ 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".
| except (OSError, yaml.YAMLError): | ||
| return None |
There was a problem hiding this comment.
discover_windio_inputs() now calls _load_windio_doc() for every *.yaml under a directory, but _load_windio_doc() only catches OSError and yaml.YAMLError. A non-UTF-8 YAML file (or any file that triggers UnicodeDecodeError while reading with encoding='utf-8') will raise out of the filter and abort discovery, so one bad sidecar file can prevent selecting an otherwise valid ontology in the same tree.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9ec20e2. _load_windio_doc now also catches UnicodeDecodeError, so a non-UTF-8 sidecar yaml is skipped instead of aborting the directory scan; a valid ontology in the same tree is still selected. Added tests for both the single-file skip and the directory-scan case.
_load_windio_doc caught only OSError and yaml.YAMLError, so a non-UTF-8 sidecar yaml raised UnicodeDecodeError (a ValueError, not an OSError) out of the directory filter and aborted discovery before a valid ontology in the same tree could be selected. Add UnicodeDecodeError to the caught set so a bad candidate is skipped. Tests cover the single-file skip and the directory-scan-finds-valid case.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ec20e2d88
ℹ️ 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".
| # farther reaches broad workspace parents such as ``D:\repos`` | ||
| # that may contain unrelated OpenFAST clones, turning a scratch | ||
| # yaml into a wrong-turbine deck-backed solve. | ||
| for anc in list(yaml_path.parents)[:3]: |
There was a problem hiding this comment.
Limitating the ancestor scan to yaml_path.parents[:3] regresses valid layouts where the ontology is nested more than two directories below the turbine root (for example <root>/.../.../.../turbine.yaml with <root>/OpenFAST). In that case discover_windio_inputs no longer finds OpenFAST, returns no companion decks, and run_windio silently downgrades to screening/partial fidelity even though industry-grade decks are present.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ca84883. Agreed the depth cap was the wrong mechanism. Replaced it with a project-boundary walk: climb until an OpenFAST tree is found or a .git project root is reached (generous numeric backstop). A deeply-nested ontology now resolves its decks at any depth within its turbine repo, while the walk still stops at the repo root instead of climbing into a broad multi-project workspace. Added tests for the deeply-nested case and the project-boundary stop.
…p (Codex P1) The 4->3 ancestor-depth reduction regressed deeply-nested ontologies: a yaml more than two directories below its turbine root no longer found the root's OpenFAST tree, so run_windio silently downgraded an industry-grade run to screening. A fixed depth cap can't satisfy both 'do not climb into the workspace' and 'do not miss deeply-nested decks'. Replace it with a project-boundary walk: climb until an OpenFAST tree is found OR a .git project root is reached, with a generous numeric backstop. A deeply-nested ontology now resolves its decks at any depth within its repo, while the walk still stops at the repo root rather than ascending into a broad multi-project workspace. Tests cover both.
|
@codex review |
…ocs pages The doc-page links pointed at docs/*.rst source files, which render as raw reStructuredText in the browser. Point them at the live Read the Docs pages instead (verified live), so the table and the inline references open the rendered docs. CHANGELOG.md / CONTRIBUTING.md stay as canonical GitHub markdown.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca84883909
ℹ️ 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".
| if n_modes is not None and n_returned < n_modes: | ||
| warnings.warn( | ||
| f"solve_modes recovered only {n_returned} of the requested " | ||
| f"{n_modes} modes via the {path} path. The eigenproblem is " | ||
| f"likely near-degenerate or defective (a non-symmetric " |
There was a problem hiding this comment.
This branch warns whenever n_returned < n_modes, but that also happens on valid symmetric solves when n_modes exceeds available DOFs because _solve_dense_symmetric intentionally truncates to min(n_modes, ngd). In that normal case, the new warning incorrectly claims the problem is defective and that modes were filtered out as complex/non-positive, which can mislead users and can fail runs that treat warnings as errors. The warning should be gated to the actual failure mode (the dense general path filtering step), not every shortfall.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8f8d5a5. The shortfall warning is now gated to the dense_general path, where eigenvalue filtering actually drops modes. The symmetric path's benign truncation to min(n_modes, ngd) no longer warns (it would have misled and failed warnings-as-errors callers); the diagnostics still record n_returned for every path. Split the test into the benign symmetric no-warn case and a genuine general-path shortfall.
The shortfall RuntimeWarning fired for any n_returned < n_modes, including the benign case where the symmetric path truncates to min(n_modes, ngd) because the request exceeds available DOFs. That wrongly described a normal solve as defective and could fail warnings-as-errors callers. Gate the warning to the dense_general path, where eigenvalue filtering actually drops modes; the diagnostics still record n_returned for every path. Tests split into the benign symmetric-truncation no-warn case and a genuine general-path shortfall.
1 participant
Summary
An engineering-hardening pass addressing a repo-wide review with nine items. The library now fails closed on non-physical input, is honest about what it could not model, reports the numerical health of every solve, and tightens its parser diagnostics, typing, coverage and public API boundary. One behaviour change (item 1), everything else additive. 907 tests pass, coverage 87 %, ruff and mypy clean.
Changed (one behaviour change)
Tower.run/RotatingBlade.runused to route everycheck_modelfinding, including ERROR-severity ones (NaN section properties, non-positive mass, a malformed support matrix), throughUserWarningand then feed the eigensolver the non-physical input. They now raisepybmodes.checks.ModelValidationError(aValueErrorsubclass) on any ERROR finding. Newon_error="raise"|"warn"keyword, defaultraise. Passon_error="warn"for the prior behaviour orcheck_model=Falseto skip. WARN findings still warn. This only affects models that were already producing meaningless output, so it is a safety hardening rather than a feature removal.Added
ModalResult.ignored_physicsnames parsed-but-not-modelled physics (distributed added massdistr_mtoday) so a result is honest about its fidelity. Persisted when non-empty and shown in the bundled report.ModalResult.diagnostics(SolverDiagnostics) records the path taken, whether the sparse path silently fell back to dense and why, the mode-count guarantee, per-mode backward-error residuals||K x - λ M x|| / ||K x||, and a mass-matrix conditioning estimate. The general path now warns when it recovers fewer valid modes than requested. Transient telemetry, excluded from equality and not serialised.generate_reportgains astatusargument;run_windiosetscomplete/partial/screeningand carries it onWindioResult.report_status.Fixed
discover_windio_inputsno longer searches the raw file text for"components:"/"floating_platform:"(which mis-selected any yaml that merely mentioned the words and missed valid ontologies whose key sat past the scanned window). It parses each candidate as YAML and checks the structure..bmireader raised bareValueError/IndexError/EOFErrorwith no context on a truncated array, an empty value line or an early EOF. It now raisespybmodes.io.errors.BMIParseErrorcarrying the file, the 1-based line, the offending line text and the section. Still aValueErrorsubclass, soexcept ValueErrorcallers are unaffected.Internal
pybmodes.checks,pybmodes.coords,pybmodes.io.errors,pybmodes.workflows._base.fail_under = 85) replaces the informational-only report.pybmodes.campbellstops re-exporting its private helpers at the package root. The supported surface isCampbellResult/campbell_sweep/plot_campbell; internals are imported from their sub-modules.(Item 7 from the review, the stale version fallback, shipped already in 1.13.1 with a guard test.)
Verification
test_checks), solver diagnostics on hand-built matrices and the run path (test_solver_diagnostics), the BMI ParseError context (test_parser_negative_paths), structured WindIO discovery + report status + ignored-physics (test_review_hardening), and theignored_physics/ transient-diagnosticsserialisation contract (test_serialize).Released as 1.14.0.