Replace bare except clauses with specific exceptions

[AshleyAHuang]

Part of #11 (finding 2), following the issue's suggestion to split the hygiene work into separate small PRs — this one only removes the package's three bare except: clauses and makes each replacement operational.

• ThermoModule.ParseDatabase — catches only ValueError around float-array conversion, preserving the established list fallback for non-numeric properties (for example CAS identifiers) while allowing malformed structured values (TypeError) and float-range failures (OverflowError) to retain their diagnostics.
• StatsModule.bootstrap_params — catches only numpy.linalg.LinAlgError, the documented failure from the package's Levenberg-Marquardt solve/inv operations. A singular bootstrap sample records a NaN row and emits a RuntimeWarning containing the sample index and original diagnostic; programming errors, KeyboardInterrupt, and SystemExit propagate.
• Crystallizers._BaseCryst — the requested jac_type='AD' mode cannot be used because its Jacobian callbacks are not implemented. Construction now warns at the user's call site and normalizes that request to the supported finite_diff mode; the dead JAX import/rebinding path and unreachable missing-callback branch are removed. The regression continues through set_ode_problem and asserts the numerical Jacobian and sensitivity handoff.

The focused regression module now has NumPy-style documentation and units/bases for its scientific fixtures. It reuses tests/assimulo_helpers.py; its remaining monkeypatch is limited to the optional Assimulo import/problem boundary so the real BatchCryst.set_ode_problem routing remains exercised in the core lane.

Mutation checks independently restore each defect class: a bare except: swallows both overflow and malformed-value errors, broad except Exception swallows the programming error, and leaving jac_type='AD' makes the fallback regression fail. flake8 --select=E722 PharmaPy tests/test_exception_handling.py reports zero E722 violations.

Local verification on Python 3.11:

• Core environment: pytest tests/ -m "not assimulo" — 83 passed, 5 skipped, 6 deselected.
• Assimulo 3.4.3 environment: pytest tests/ -m assimulo — 15 passed, 83 deselected.
• Focused module in both environments: pytest tests/test_exception_handling.py -q — 11 passed.
• Collection in the core environment: pytest --collect-only -q — 89 tests collected.
• Black check passes for the new test module; Ruff passes it plus StatsModule.py and ThermoModule.py. The remaining 12 Ruff findings in Crystallizers.py are unchanged findings already present on master and remain outside this exception-handling PR.

[bernalde]

Thank you for your contribution to our package!
I left a small comment in the testing function that should be addressed

[bernalde]

We would prefer to avoid monkeypatch tests

[bernalde]

Addressed in 6c54354. The local fake Assimulo module tree was removed in favor of tests/assimulo_helpers.py. The only remaining monkeypatch is the optional Explicit_Problem boundary needed by the core lane; its rationale is documented, and the real set_ode_problem routing is exercised.

[bernalde]

Reviewed current head b6ab056 against the current master tip as a clean merge result. Narrowing ParseDatabase to conversion errors is appropriate, and the new KeyboardInterrupt/JAX-warning regressions are genuinely red on the PR's merge base. Two blocking source issues remain.

The existing test-design thread also remains relevant: after updating onto current master, avoid duplicating the optional-Assimulo import stub. tests/assimulo_helpers.py is now the shared boundary helper; reuse it if the unit test still needs import isolation, or move this check to the Assimulo-enabled lane.

Blocking issues: 2.
Nonblocking issues: 1.
Questions: none.

Validation:

• Static safety pass over this external contribution: no workflow, dependency-install, subprocess, network, secret, or execution-hook changes.
• Local and GitHub changed-file sets match: four files.
• Clean merge into current master completed without conflicts.
• Solver-free Python 3.11: targeted tests 5 passed; core lane 66 passed, 5 skipped, 6 deselected. The five skips are tests that explicitly require optional Assimulo.
• Solver-enabled Python 3.11: core lane 66 passed, 15 deselected.
• Base regression probe: the KeyboardInterrupt and missing-JAX warning tests both fail against the merge base as intended.
• CRLF-aware diff check passed; the apparent default whitespace warnings are existing CRLF endings.
• Current PR-head GitHub checks: Core tests and Assimulo integration tests both passed.

Merge readiness: not ready. I would not merge this until the blocking issues above are addressed.

[bernalde]

Blocking: this handler says it is falling back to NumPy, but it leaves jac_type='AD'. set_ode_problem(eval_sens=True, ...) then selects self.jac_states_ad and self.jac_params_ad, neither of which exists, so the supposedly recovered object fails with AttributeError. Either reject unavailable AD immediately with a clear exception, or switch to a real supported mode such as finite_diff; then extend the regression through set_ode_problem so it proves the fallback is operational rather than only that construction succeeds.

[bernalde]

Addressed in 6c54354. Because the AD callbacks are not implemented regardless of JAX availability, construction now normalizes AD to finite_diff, and the unreachable missing-callback branch was removed. The regression calls set_ode_problem and asserts the numerical Jacobian and sensitivity handoff.

[bernalde]

Strengthened in 9ab73e9. The regression now explicitly asserts jac_params_fn == jac_params_numerical in addition to the state-Jacobian and rhs_sens handoffs. I replaced the production assignment with None in a disposable copy; the test then failed at this exact assertion, which confirms it now guards the complete finite-difference sensitivity setup.

[bernalde]

Blocking: Exception is still broad enough to swallow ordinary programming failures such as AttributeError and replace them with a NaN row plus a generic print, which preserves the debuggability problem from #11. Catch only the optimizer's documented failure exceptions (or introduce a dedicated optimization-failure exception), retain the cause in a warning/log message, and add a regression showing that an unrelated programming error propagates.

[bernalde]

Addressed in 6c54354. bootstrap_params now catches only numpy.linalg.LinAlgError, emits a warning containing the sample index and original diagnostic, and records that row as NaN. A separate regression proves AttributeError propagates; the interrupt regression remains.

[Mazhar331]

Second maintainer pass on b6ab056. I deliberately skipped everything already covered by @bernalde's reviews — the jac_type='AD' fallback leaving an unusable object, the breadth of except Exception in bootstrap_params, and the duplicated optional-Assimulo import stub all still stand and I have nothing to add to them. Everything below is new.

Good starting point: the two behavioral regressions really are red on the merge base, and an AST scan confirms zero bare except: handlers remain anywhere in the repository, not just in PharmaPy/.

Blocking: the ThermoModule narrowing has no test at all

Anchored inline at PharmaPy/ThermoModule.py:82. Both new ParseDatabase tests pass unchanged against the merge base (c7a38a4), because a bare except: also catches ValueError. I put the bug back — reverted line 82 to except: at this head and re-ran the file — and all 5 tests still passed, which means nothing in this PR guards the third of its three changes. That is a missing test for changed behavior, the same gap @bernalde asked you to close for StatsModule.

You do not need a monkeypatch for this one. A JSON field whose value overflows float64 is a real input that exercises the difference:

db = {"water": {"mw": 10 ** 400}, "ethanol": {"mw": 46}}

• merge base: returns dd_arrays["mw"] as a plain list, silently
• this head: raises OverflowError: int too large to convert to float

Assert that with pytest.raises(OverflowError) and the narrowing is genuinely pinned. While you are there, TypeError is in the tuple but no test reaches it; the only input I found that produces it is a nested dict value, which ParseDatabase already unwraps one level up — either cover it or drop it from the tuple.

Nonblocking: the new warning points at PharmaPy internals, not the caller

Anchored inline at PharmaPy/Crystallizers.py:126. warnings.warn defaults to stacklevel=1, so the emitted warning reads Crystallizers.py:126: RuntimeWarning: ... — it names the library line rather than the user's BatchCryst(...) call, which is the location the user can act on. Add stacklevel=2.

Nonblocking: the new test asserts on unrestored process-global state

Anchored inline at tests/test_exception_handling.py:124. _BaseCryst.np is a class attribute that jac_type='AD' rebinds for the whole process and nothing ever restores. With a jax module importable, I observed:

before:                                        _BaseCryst.np is numpy -> True
after one BatchCryst(jac_type='AD'):           _BaseCryst.np is numpy -> False
a later BatchCryst() with jac_type=None:       type(b).np is numpy    -> False
then re-running this test's scenario:          type(c).np is numpy    -> False

So the assertion holds today only because jax is absent from CI; in a jax-enabled environment it fails depending on what ran earlier in the session, for reasons unrelated to the handler under test. Pin it with monkeypatch.setattr(_BaseCryst, "np", np) before constructing.

The last line of that trace is also worth noting for the fallback question @bernalde raised: the message says "Falling back to numpy", but the handler performs no fallback — it just declines to rebind. Whatever a previous AD instance left on the class is what the object keeps using.

Nonblocking: test file does not follow the repo's documentation convention

Recent test modules here (tests/test_drying_latent_heat_factor.py, for example) carry a module docstring plus a one-line docstring per test stating the regression contract. tests/test_exception_handling.py has neither, and its name groups three unrelated modules by mechanism rather than by the module under test. A module docstring and one line per test naming which bare-except each case guards would bring it in line.

Question: is this better ordered behind #135?

Open PR #135 ("Make optional Assimulo imports lazy") states it keeps the solver-backed model modules importable without Assimulo and removes the temporary fake sys.modules Assimulo package tree. If that lands first, _stub_assimulo_modules/_import_batch_cryst here can be deleted outright rather than reworked onto tests/assimulo_helpers.py. That may be a cheaper resolution to the stub thread than either option currently on the table — worth a decision before you spend effort refactoring the helper.

---

Blocking issues: 1 (plus the 2 already open from @bernalde).
Nonblocking issues: 3.
Questions: 1.

Validation (Python 3.11, conda env with Assimulo installed and jax absent):

• Changed-file sets from git diff --name-only $(git merge-base origin/master HEAD) HEAD and gh pr diff --name-only match: four files.
• PR head: full suite 59 passed; tests/test_exception_handling.py 5 passed.
• Merge result: merged current master (8c7d14d) into the head with no conflicts; full suite 90 passed.
• Base regression probe at c7a38a4: 2 failed, 3 passed — the KeyboardInterrupt and JAX-warning tests are red as intended, and both ParseDatabase tests are green, which is the blocking finding above.
• Mutation probe: reverting ThermoModule.py:82 to a bare except: at this head leaves all 5 tests passing.
• ParseDatabase run over all 13 JSON databases shipped in the repo: identical outcomes at base and head. The four files that raise (minimum_modeling_objects.json, pfr_test_constructor_kwargs.json, compounds_lomustine.json, physical_property_descriptors.json) fail identically on both sides, from code outside the try block — pre-existing, not this PR.
• AST scan for bare except handlers across every .py in the repo: none remain. (flake8 is not installed in this environment, so this stands in for the --select=E722 claim.)
• Static safety pass: no workflow, packaging, subprocess, network, or secret-touching changes.
• GitHub checks on this head: Core tests SUCCESS, Assimulo integration tests SUCCESS.

Merge readiness: not ready. I would not merge this until the blocking issues above are addressed.

[Mazhar331]

Blocking: nothing in this PR guards this change. Both new ParseDatabase tests pass unchanged against the merge base c7a38a4, because a bare except: also catches ValueError. I put the bug back — reverted this line to except: at this head and re-ran tests/test_exception_handling.py — and all 5 tests still passed. That is the same missing-regression gap already flagged for StatsModule.

A monkeypatch is not needed here. A JSON value that overflows float64 is a real input that separates the two behaviors:

db = {"water": {"mw": 10 ** 400}, "ethanol": {"mw": 46}}

• merge base: silently returns dd_arrays["mw"] as a plain list
• this head: raises OverflowError: int too large to convert to float

Assert that with pytest.raises(OverflowError) and the narrowing is pinned.

Separately, TypeError is in the tuple but no test reaches it. The only input I found that produces it is a nested dict value, which ParseDatabase already unwraps a few lines above — either cover it or drop it from the tuple.

[bernalde]

Addressed in 6c54354. The handler now catches only ValueError; TypeError was dropped. Real JSON fixtures cover both float64 overflow and a malformed nested value, and restoring a bare except: makes both regressions fail as intended.

[Mazhar331]

Nonblocking: warnings.warn defaults to stacklevel=1, so this emits Crystallizers.py:126: RuntimeWarning: ... — it names the library line rather than the user's BatchCryst(...) call, which is the location they can actually act on. Add stacklevel=2.

[bernalde]

Addressed in 6c54354. This uses stacklevel=3 because the warning is emitted in _BaseCryst.__init__, one frame below the public subclass constructor; 3 is what points to the user's BatchCryst(...) call. The regression asserts the warning filename is the caller test file.

[Mazhar331]

Nonblocking: this asserts on process-global state that nothing restores. _BaseCryst.np is a class attribute rebound by jac_type='AD' for the lifetime of the process. With a jax module importable I observed:

before:                                 _BaseCryst.np is numpy -> True
after one BatchCryst(jac_type='AD'):    _BaseCryst.np is numpy -> False
a later BatchCryst() with jac_type=None: type(b).np is numpy   -> False
then re-running this test's scenario:   type(c).np is numpy    -> False

So this assertion holds today only because jax is absent from CI; in a jax-enabled environment it fails depending on what ran earlier in the session, for reasons unrelated to the handler under test. Pin it with monkeypatch.setattr(_BaseCryst, "np", np) before constructing.

The last line is also relevant to the fallback discussion above: the warning says "Falling back to numpy", but the handler performs no fallback — it only declines to rebind, so whatever a previous AD instance left on the class is what the object keeps using.

[bernalde]

Addressed in 6c54354 by removing the JAX class-attribute rebinding entirely. _BaseCryst.np now remains NumPy regardless of prior instances, and AD requests take the explicit finite-difference path before set_ode_problem is built.

[Mazhar331]

Please provide numpy documentation for all function modifications/additions, including tests

[bernalde]

Addressed in 6c54354. The test module, helper classes, helper methods, nested optimizer doubles, and tests now have NumPy-style docstrings. The modified production functions/methods also document parameters, returns, warnings/errors, shapes, physical units, and bases.

[Mazhar331]

Provide a brief description of the test file. Kindly avoid monkeypatches unless necessary, and provide appropriate reasons (existence of related issues, long execution time, etc.) in the PR body/comments when using

[bernalde]

Addressed in 6c54354. The module docstring summarizes all three regression areas and explains the optional-boundary test design. The duplicate monkeypatch helper is removed; the shared helper and narrow Explicit_Problem substitution are retained only to keep this handoff regression in the Assimulo-free core lane, with solver execution left to the Assimulo integration lane.

[bernalde]

The original review batch was addressed in 6c54354; current head is now 9ab73e9, after the warning-location follow-up in 0c8bb31 and this final regression strengthening.

@AshleyAHuang — I pushed the one-line follow-up directly to your branch because maintainer edits are enabled. I can revert it if you would prefer to take the change yourself.

Commit pushed:

• 9ab73e9 test: pin AD parameter Jacobian fallback

The AD fallback regression now checks every callback configured by the real set_ode_problem finite-difference route:

• problem.jac uses jac_states_numerical;
• crystallizer.jac_params_fn uses jac_params_numerical;
• problem.rhs_sens uses rhs_sensitivity.

This closes the remaining coverage gap in the original operational-fallback thread. In a disposable copy I replaced the production parameter-Jacobian assignment with None; the strengthened test failed on the new assertion, while the exact current head passes. That intended red result confirms the test now guards the parameter-sensitivity handoff rather than merely constructing the problem.

Verification at 9ab73e9:

• focused file in core and Assimulo environments: 11 passed in each;
• core lane: 83 passed, 5 skipped, 6 deselected;
• Assimulo lane: 15 passed, 83 deselected;
• Black and Ruff pass for tests/test_exception_handling.py;
• current-head CI: core, Assimulo, and locked pixi installation on Ubuntu and Windows all pass.

No review request was declined. The original handler narrowing, shared Assimulo helper, documentation, and warning-location fixes remain intact. GitHub reports mergeStateStatus: CLEAN; the formal decision remains CHANGES_REQUESTED until @Mazhar331 re-reviews or dismisses the standing request. Review threads remain unresolved for reviewer action.

[Mazhar331]

Went through 6c54354. I checked the fixes against the code rather than the summary — pulled the head tree into a scratch copy, ran the suite there, and put each original defect back to confirm the new tests actually catch it. The earlier round is fully resolved; one new thing came out of the stacklevel change that I would like tightened before this goes in.

Previous items

ThermoModule narrowing had no test. Closed. The handler is except ValueError only, TypeError is dropped, and the propagation is pinned instead of the tuple. Restoring the bare except: reddens test_parse_database_propagates_float_overflow and ..._malformed_nested_values, so the change is now genuinely guarded.

AD fallback left an unusable object. Closed, and the chosen fix is better than what I suggested — normalizing to finite_diff at construction and deleting the unreachable elif self.jac_type == 'AD' branch removes the failure mode instead of documenting around it. Deleting the jac_type = 'finite_diff' line reddens the regression, which now runs through set_ode_problem and asserts problem.jac / problem.rhs_sens.

except Exception was too broad. Closed. Widening it back to Exception reddens the new AttributeError test, and restoring the original bare except: reddens three tests including the KeyboardInterrupt one — so strengthening the assertions did not quietly cost the original guarantee, which was my main worry with a rewrite this size. The rationale also checks out against the code: LevMarq.py imports solve and inv from numpy.linalg and uses them at lines 101 and 156, so LinAlgError is the right thing to treat as recoverable.

_BaseCryst.np global. Closed, again better than my suggestion — removing the JAX rebinding entirely means there is no global left to restore.

Docstrings and test description. Closed. The module docstring covers all three regression areas, the helpers and test doubles are documented, and the production docstrings carry units and bases. I spot-checked the ones that are easy to get wrong: vol_ht really is threaded through the constructors without ever being stored, and the energy balances recompute it locally at 1570, 1894 and 2216 — so "retained for constructor compatibility, subclasses derive it internally" is accurate rather than a guess.

Monkeypatch reduction and #135. Closed. The local fake Assimulo tree is gone, tests/assimulo_helpers.py is confirmed present on master, and the remaining Explicit_Problem substitution is the thing that lets the test assert the real set_ode_problem routing, so it is earning its keep. Agreed that #135 is no longer a dependency.

New: stacklevel=3 is right for two of the three public crystallizers

stacklevel=3 is correct reasoning for BatchCryst and MSMPR, which call _BaseCryst.__init__ directly. But SemibatchCryst subclasses MSMPR, so its chain is one frame deeper and the warning lands on our own code instead of the caller:

BatchCryst       -> caller.py:12
MSMPR            -> caller.py:12
SemibatchCryst   -> Crystallizers.py:2097

Crystallizers.py:2097 is the super().__init__ call inside SemibatchCryst.__init__, which is exactly the kind of location the original comment was about. The new filename assertion only exercises BatchCryst, so nothing catches this.

A fixed stacklevel cannot be correct for a hierarchy of varying depth, so I would rather not just bump the number. The cheapest honest fix is to parameterize the existing warning test over BatchCryst, MSMPR and SemibatchCryst — that makes the gap visible — and then either emit the warning from the public constructors, or resolve the caller frame instead of hard-coding a count. Whatever you pick, the test should cover all three.

Two smaller notes

_BaseCryst.np = np is now vestigial. With the rebinding gone, nothing in Crystallizers.py reads self.np or .np; the only remaining consumer is the test's own assert type(crystallizer).np is np, which pins a value no code path uses. Not worth expanding this PR for — it is a natural pickup for the dead-code task in #11 — but the assertion is currently guarding nothing.

The LinAlgError rationale is scoped to the LM path. bootstrap_params forwards self.inst.opt_method, and optimize_fn also accepts 'IPOPT'. A cyipopt-side numerical failure is not a LinAlgError, so under that method one bad sample now aborts the whole bootstrap run rather than recording a NaN row. That may be the intended strictness, but the Notes currently reason only about Levenberg–Marquardt, so a sentence covering the other method would keep the docstring honest.

What I ran

Scratch copy of the head tree, verified byte-identical to git show 6c543549:<path>. Focused file 8 passed; full suite with Assimulo 3.4.3 installed 95 passed, which matches the 80 + 15 split in your summary. Six mutations, each reverting one fix, all red on the intended test only and green again after restoring: bare except: in ThermoModule (2 failed), except Exception in StatsModule (1), bare except: in StatsModule (3), dropping the AD → finite_diff normalization (1), and stacklevel 3→2 and 3→1 (1 each — so the 3 is pinned, not assumed). Line endings are clean: the three CRLF-tracked production files stayed pure CRLF and the new LF-only test file matches every other file under tests/. Core tests, Assimulo integration, and the locked pixi install on Ubuntu and Windows are all green at this head.

Where this leaves it

Everything from the previous round holds up, and the mutation evidence is genuinely reassuring — a failing test there is the outcome I was looking for. I am holding off on merging only for the SemibatchCryst stacklevel gap: the fix is incomplete for one of the three public entry points, and the test that was added to pin it does not cover that path, so it would go in looking verified when it is not. Once the warning test covers all three classes and points at the caller in each, this is good to go from my side. The two smaller notes are yours to take or leave.

[bernalde]

Addressed the review at 6c54354. Head is now 0c8bb31.

@AshleyAHuang — heads up that I pushed this commit directly to your branch rather than leaving it for you, since maintainer edits are enabled on the PR. Happy to revert it if you would rather take the fix yourself; the reasoning is all below either way. Thanks for the work on this one — the mutation evidence in the last round was what made it easy to review.

Commit pushed:

• 0c8bb31 fix: resolve the AD-fallback warning stacklevel from the call stack

stacklevel=3 is right for only two of the three crystallizers — fixed

Confirmed before changing anything. Constructing each public class with jac_type='AD' and reading back the recorded warning location reproduces your table exactly:

BatchCryst       -> <string>:7      (caller)
MSMPR            -> <string>:7      (caller)
SemibatchCryst   -> Crystallizers.py:2097

I took the second of your two suggestions — resolve the caller frame rather than emit from each public constructor — because it keeps the warning in one place and cannot drift as the hierarchy changes. _caller_stacklevel() walks outward to the first frame not executing in this module. It matches on the module globals rather than on __file__, so relative versus absolute import paths cannot change the answer. After the fix all three report <string>:7.

The warning test is now parameterized over BatchCryst, MSMPR and SemibatchCryst as you asked. I also made it assert the exact construction line rather than only the file, because a file-only check is weaker than it looks here: a stacklevel that is too deep can land on some other line of the test module and still pass.

That precision is what makes the mutation evidence conclusive. Pinning any fixed value now reddens the suite, and no single value works:

stacklevel	Result
3 (the value under review)	1 failed — SemibatchCryst only
4	3 failed — BatchCryst and MSMPR
2	4 failed — all cases
_caller_stacklevel()	11 passed

The stacklevel=3 row is the gap you found, now caught by the test rather than by review.

The two smaller notes — both taken

_BaseCryst.np assertion guards nothing.
I removed assert type(crystallizer).np is np from the test. You were right that it pins a value no code path reads once the JAX rebinding is gone, and a vacuous assertion is worse than none. I left _BaseCryst.np = np in production as you suggested — removing it is dead-code work that belongs with #11 rather than here, and it is now genuinely unreferenced rather than half-pinned by a test.

LinAlgError rationale is scoped to LM.
Verified against the code rather than taking it on trust: optimize_fn declares method : {'LM', 'IPOPT'}, the 'IPOPT' branch dispatches to minimize_ipopt through cyipopt, and bootstrap_params forwards self.inst.opt_method unchanged. So a cyipopt-side numerical failure is indeed not a LinAlgError and aborts the run. The Notes section now says the recovery is scoped to opt_method='LM' and states what happens under IPOPT. No behavior change — this documents the existing strictness rather than widening the handler, which would need its own regression and a decision about whether aborting is actually wrong.

Verification

All at 0c8bb31, with the Assimulo 3.4.3 environment for the solver lane:

Command	Result
pytest tests/test_exception_handling.py -q (both environments)	11 passed (was 8)
pytest tests/ -q with Assimulo	98 passed (was 95)
pytest tests/ -m "not assimulo" -q	83 passed, 5 skipped, 6 deselected
pytest tests/ -m assimulo -q	15 passed, 83 deselected
new tests in the Assimulo-free lane	3 passed, 8 deselected

The three new cases run in the core lane too, so the gap is covered where CI actually exercises it, not only in a solver environment.

Line endings held: Crystallizers.py and StatsModule.py are CRLF-tracked, so I applied those edits at byte level and confirmed both files stay pure CRLF with no bare-LF lines; the LF-only test file is unchanged in that respect. git diff --check with core.whitespace=cr-at-eol is clean. ruff reports the same 12 pre-existing findings as the merge base — all legacy unused imports, unused locals, and type() comparisons in Crystallizers.py, none on changed lines — and black --check passes on the test file. Current-head CI: core tests, Assimulo integration, and locked pixi install on Ubuntu and Windows all pass.

I updated the four test counts in the PR body, which the added cases made stale.

Nothing is resolved from my side, and the decision stays CHANGES_REQUESTED until @Mazhar331 re-reviews.