Refactor logging and timing infrastructure for consistent CLI output #234
Conversation
WalkthroughAdds a public Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-11-03T13:34:35.907ZApplied to files:
📚 Learning: 2025-11-03T13:34:35.907ZApplied to files:
🪛 Ruff (0.14.8)temoa/_internal/run_actions.py174-174: Boolean-typed positional argument in function definition (FBT001) 174-174: Boolean default positional argument in function definition (FBT002) 251-251: Redundant exception object included in (TRY401) 252-255: Use Replace with (TRY400) 259-259: Do not catch blind exception: (BLE001) 261-264: Use Replace with (TRY400) 265-265: Avoid specifying long messages outside the exception class (TRY003) 387-387: Logging (G201) ⏰ 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). (4)
🔇 Additional comments (9)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
temoa/_internal/run_actions.py (1)
178-277: Harden error-path logging for solver status extractionThe solver configuration branches (cbc/cplex/gurobi/appsi_highs), suffix validation, and
task_timerwrapper all look reasonable. However, the current error handling path has a robustness gap:if result: logger.error( 'Solver reported termination condition (if any): %s', result['Solution'].Status, )This direct access to
result['Solution'].Statuswill raiseKeyError/TypeError/AttributeErrorif the result object lacks a'Solution'entry or is not dictionary-like (e.g., APPSI results or partially populated legacy results). This masks the original solver failure.Since
check_solve_statusalready exists and safely handles both legacy and APPSI results, use it defensively here:if result: try: _ok, status_msg = check_solve_status(result) except Exception: status_msg = '<unable to extract status>' logger.error( 'Solver reported termination/status (if any): %s', status_msg, )Also, the docstring mentions
'duals'(plural) as a supported suffix, butlegit_suffixesuses'dual'(singular). Align the documentation with the actual allowed suffix names to prevent user confusion.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
temoa/_internal/run_actions.py(10 hunks)temoa/cli.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
temoa/_internal/run_actions.py (3)
temoa/extensions/monte_carlo/mc_run.py (1)
model(185-192)temoa/_internal/table_writer.py (1)
write_results(91-135)temoa/data_processing/db_to_excel.py (1)
make_excel(25-241)
🪛 Ruff (0.14.7)
temoa/_internal/run_actions.py
34-34: Boolean-typed positional argument in function definition
(FBT001)
34-34: Boolean default positional argument in function definition
(FBT002)
96-100: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
181-181: Boolean-typed positional argument in function definition
(FBT001)
181-181: Boolean default positional argument in function definition
(FBT002)
268-268: 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). (4)
- GitHub Check: setup and test (macos-latest, 3.12)
- GitHub Check: setup and test (macos-latest, 3.13)
- GitHub Check: setup and test (windows-latest, 3.12)
- GitHub Check: setup and test (windows-latest, 3.13)
🔇 Additional comments (5)
temoa/cli.py (1)
48-55: RichHandler timestamp enabling aligns with new timing logsSetting
show_time=Truewithlog_time_format='[%X]'is consistent with the new timer-based INFO logs and respects the existingdebug/silentlevel handling; I don’t see regressions here.temoa/_internal/run_actions.py (4)
127-161: Model-building timer and stats changes look goodWrapping
model.create_instanceintask_timer('Creating model instance', ...)and computing constraint/variable counts viasum(len(...))overcomponent_objectsis a sensible refactor and should behave well for typical Pyomo models. The final info log on variable/constraint counts is clear.
280-333: check_solve_status helper is a nice, solver-agnostic improvementUsing
check_optimal_termination(result)plus the layered extraction oftermination_conditionand status (legacyresult['Solution'].Status, then APPSI-style attributes, then a string fallback) makes status reporting much more robust across different solver plugins.The final return of
(False, f'{status_msg} was returned from solve')for non‑optimal cases is clear and should integrate well with callers. I don’t see issues here; just ensure you exercise this with both classic Pyomo solvers andappsi_highsin tests to validate the logging output.
335-371: Result processing timer and Excel naming changes look consistent
- Wrapping result processing in
task_timer('Processing results', silent=config.silent)is straightforward and keeps logging consistent with the rest of the pipeline.- Passing
results_with_duals=resultsonly whenconfig.save_dualsmatches the intendedTableWriter.write_resultscontract.- The Excel branch’s
scenario_name = f'{config.scenario}-{iteration}' if iteration is not None else config.scenariocombined withtemp_scenario = {scenario_name}andexcel_filename = config.output_path / scenario_nameshould give distinct Excel files per iteration, assuming the DB uses the same scenario naming convention.The updated total-cost log (
'Total Cost value: %0.2f') is fine and keeps the message explicit. Please verify one multi‑iteration run end‑to‑end (including Excel and Graphviz) to confirm scenario naming is aligned across DB, Excel, and plots.
164-176: Current implementation is correct for actual usage patterns; the concern about file path handling is unfounded.All verified call sites pass directory paths to
save_lp():
sv_mga_sequencer.py:self.config.output_path / 'base_model'sv_mga_sequencer.py:self.config.output_path / 'option_model'temoa_sequencer.py:self.config.output_pathmyopic_sequencer.py:self.config.output_path / 'LP...'No existing callers pass full file paths (e.g.,
/path/to/model.lp), so the regression scenario described in the review comment does not occur. The current implementation correctly treatslp_pathas a directory root and appendsmodel.lp, which aligns with all actual usage in the codebase. The suggested diff to support both patterns is unnecessary.Likely an incorrect or invalid review comment.
ParticularlyPythonicBS
force-pushed
the
ref/task_timer_context_manager
branch
from
c01cc6a to
54bb577
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
temoa/_internal/run_actions.py(9 hunks)temoa/cli.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-03T13:34:35.907Z
Learnt from: ParticularlyPythonicBS
Repo: TemoaProject/temoa PR: 184
File: temoa/data_processing/database_util.py:109-118
Timestamp: 2025-11-03T13:34:35.907Z
Learning: In the Temoa project, SQL injection vulnerabilities in database utility classes (such as temoa/data_processing/database_util.py) are considered low concern due to the data access patterns and should not be flagged during code reviews. Technical debt for SQL injection hardening should be tracked separately as issues rather than blocking PRs.
Applied to files:
temoa/_internal/run_actions.py
📚 Learning: 2025-11-03T13:34:35.907Z
Learnt from: ParticularlyPythonicBS
Repo: TemoaProject/temoa PR: 184
File: temoa/data_processing/database_util.py:109-118
Timestamp: 2025-11-03T13:34:35.907Z
Learning: In the Temoa project, SQL injection vulnerabilities in the database utility classes (such as temoa/data_processing/database_util.py) are considered low concern due to the data access patterns and should not be flagged during reviews. Technical debt for SQL injection hardening should be tracked separately as issues rather than blocking PRs.
Applied to files:
temoa/_internal/run_actions.py
🧬 Code graph analysis (1)
temoa/_internal/run_actions.py (2)
temoa/_internal/table_writer.py (1)
write_results(91-135)temoa/data_processing/db_to_excel.py (1)
make_excel(25-241)
🪛 Ruff (0.14.7)
temoa/_internal/run_actions.py
179-179: Boolean-typed positional argument in function definition
(FBT001)
179-179: Boolean default positional argument in function definition
(FBT002)
256-256: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
257-260: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
264-264: Do not catch blind exception: Exception
(BLE001)
266-269: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
270-270: 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). (4)
- GitHub Check: setup and test (windows-latest, 3.12)
- GitHub Check: setup and test (windows-latest, 3.13)
- GitHub Check: setup and test (macos-latest, 3.13)
- GitHub Check: setup and test (macos-latest, 3.12)
🔇 Additional comments (7)
temoa/cli.py (1)
49-55: LGTM!Enabling
show_time=Truecorrectly activates the timestamp display that was already configured vialog_time_format. This aligns well with the newtask_timercontext manager to provide consistent timing information in CLI output.temoa/_internal/run_actions.py (6)
33-50: LGTM!The
task_timercontext manager is well-implemented with:
- Correct use of
perf_counterfor accurate timing- Keyword-only
silentparameter (as suggested in past review)finallyblock ensures elapsed time is logged even when exceptions occur
87-107: Database version check improvements look good.The defensive
int()casting (lines 93-94) and explicit handling of both input and output databases are solid improvements. Thefinallyblock ensures connections are properly closed.
147-159: LGTM!Clean integration of
task_timerfor instance creation timing, and thesum()with generator expressions is memory-efficient for counting model components.
282-334: LGTM!The solver-agnostic status extraction with cascading fallbacks for legacy and APPSI formats is well-designed. The defensive approach handles various solver result object structures gracefully.
374-403: LGTM!Good defensive error handling for Graphviz generation. Since this is supplementary output, catching exceptions and logging with
exc_info=Trueprevents optional visualization failures from crashing the main workflow.
209-230: Solver configuration values are current and match PyPSA defaults.The parameter values for both
cplexandgurobiare verified against PyPSA documentation:
- CPLEX:
lpmethod=4(barrier),solutiontype=2(non-basic, disables crossover),barrier convergetol=1.0e-5,feasopt tolerance=1.0e-6— all align with PyPSA's cplex-default configuration.- Gurobi:
Method=2(barrier),Crossover=0(no crossover),BarConvTol=1.0e-5,FeasibilityTol=1.0e-6— all match documented PyPSA patterns and solver API expectations.The explicit
passstatements forcbcandappsi_highsappropriately indicate no additional configuration is needed for these solvers.
ParticularlyPythonicBS
force-pushed
the
ref/task_timer_context_manager
branch
from
54bb577 to
323da03
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
temoa/_internal/run_actions.py(9 hunks)temoa/cli.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-03T13:34:35.907Z
Learnt from: ParticularlyPythonicBS
Repo: TemoaProject/temoa PR: 184
File: temoa/data_processing/database_util.py:109-118
Timestamp: 2025-11-03T13:34:35.907Z
Learning: In the Temoa project, SQL injection vulnerabilities in database utility classes (such as temoa/data_processing/database_util.py) are considered low concern due to the data access patterns and should not be flagged during code reviews. Technical debt for SQL injection hardening should be tracked separately as issues rather than blocking PRs.
Applied to files:
temoa/_internal/run_actions.py
📚 Learning: 2025-11-03T13:34:35.907Z
Learnt from: ParticularlyPythonicBS
Repo: TemoaProject/temoa PR: 184
File: temoa/data_processing/database_util.py:109-118
Timestamp: 2025-11-03T13:34:35.907Z
Learning: In the Temoa project, SQL injection vulnerabilities in the database utility classes (such as temoa/data_processing/database_util.py) are considered low concern due to the data access patterns and should not be flagged during reviews. Technical debt for SQL injection hardening should be tracked separately as issues rather than blocking PRs.
Applied to files:
temoa/_internal/run_actions.py
🪛 Ruff (0.14.7)
temoa/_internal/run_actions.py
178-178: Boolean-typed positional argument in function definition
(FBT001)
178-178: Boolean default positional argument in function definition
(FBT002)
255-255: Redundant exception object included in logging.exception call
(TRY401)
256-259: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
263-263: Do not catch blind exception: Exception
(BLE001)
265-268: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
269-269: 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). (4)
- GitHub Check: setup and test (windows-latest, 3.13)
- GitHub Check: setup and test (macos-latest, 3.13)
- GitHub Check: setup and test (windows-latest, 3.12)
- GitHub Check: setup and test (macos-latest, 3.12)
🔇 Additional comments (10)
temoa/cli.py (1)
49-55: LGTM!Enabling
show_time=Truealigns console output with the newtask_timerlogging approach, providing consistent timestamps for all logged operations. The[%X]format displays time in locale's appropriate representation.temoa/_internal/run_actions.py (9)
5-11: LGTM!The new imports support the
task_timercontext manager (contextmanager,Generator,perf_counter) and the updatedsolve_instancesignature (Iterable).
33-50: LGTM!The
task_timercontext manager is well-implemented withsilentas keyword-only (addressing prior feedback). Thefinallyblock correctly ensures elapsed time is logged even when exceptions occur within the timed block.
147-158: LGTM!The
task_timerintegration is clean, and the stats gathering usingsum()with generator expressions is idiomatic and efficient.
162-172: LGTM!The directory creation logic is now idempotent with
mkdir(parents=True, exist_ok=True), and the redundantexists()check has been removed (addressing prior feedback).
231-243: LGTM!The suffix validation logic correctly filters invalid suffixes using set operations and provides helpful warning messages. The type change from
list[str]toIterable[str]increases flexibility while maintaining safety through internal conversion.
281-333: LGTM!The
check_solve_statusfunction provides robust solver-agnostic status extraction with sensible fallback hierarchy. The multiple attribute checks handle both legacy Pyomo solver results and APPSI solver formats gracefully.
343-351: LGTM!The
write_resultscall is now simplified using the conditional expression forresults_with_duals(addressing prior feedback), eliminating the duplicate branch.
365-394: LGTM!The Graphviz generation is properly wrapped in a try/except with
exc_info=Truefor full traceback logging, ensuring plotting failures don't crash the main workflow.
208-229: Solver configuration parameters verified as correct.The CPLEX and Gurobi parameter values match PyPSA-Eur defaults exactly. CPLEX parameters use space-separated interactive names (
barrier convergetol,feasopt tolerance) which is the correct convention for Pyomo's CPLEX solver interface. CBC andappsi_highsbranches are appropriately left as no-ops.
|
A couple nitpicky requests:
|
It is possible to report it for every step, currently it only displays if there is a change in the displayed time, so when the second changes, it is shown. I thought it would be easier to spot the changes that way. The log files have the time info on every line this is a console output customization, that I can change if preferred
Sounds good can change the wording there, changed to |
ParticularlyPythonicBS
force-pushed
the
ref/task_timer_context_manager
branch
from
323da03 to
c3e1347
Compare
|
OK, that sounds great, thanks! |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
temoa/_internal/run_actions.py (1)
175-279: Solver configuration and status handling are solid; address minor suffix/docs/logging nitsThe refactored
solve_instancepluscheck_solve_statusprovide clearer solver‑specific configuration, suffix whitelisting, and robust status extraction across legacy and APPSI-style results. A few small consistency issues are worth tightening up:
Suffix name/doc mismatch and warning clarity (minor functional/doc issue)
The docstring states that'duals'is supported, butlegit_suffixescontains'dual','slack', and'rc'. Callers passing'duals'would be warned and filtered out despite the docstring. Consider aligning the docs and warning with the actual accepted set, for example:
- :param solver_suffixes: iterable of string names for suffixes. See pyomo dox. right now, only
- 'duals' is supported in the Temoa Framework. Some solvers may not support duals.
- :param solver_suffixes: iterable of string names for suffixes. See Pyomo docs. Right now,
- 'dual', 'slack', and 'rc' are recognized in the Temoa framework. Some solvers may not support
- all suffixes.
And make the warning more explicit: ```diff - if bad_apples: - logger.warning( - 'Solver suffix %s is not in pyomo standards (see pyomo dox). Removed', - bad_apples, - ) + if bad_apples: + logger.warning( + 'Ignoring unsupported solver suffix(es) %s; supported suffixes are %s', + bad_apples, + legit_suffixes, + )
Redundant exception argument to
logger.exception(already flagged by tooling)
On the RuntimeError path,logger.exception('Solver failed to solve and returned an error: %s', error)passes the caught exception as a formatting argument, butlogger.exceptionwill already attach the traceback and exception info. You can drop the%sand argument:
except RuntimeError as error:logger.exception('Solver failed to solve and returned an error: %s', error)
except RuntimeError as error:logger.exception('Solver failed to solve and returned an error')This also satisfies Ruff’s TRY401 warning.
Optional: make
silentkeyword‑only to mirrortask_timerand appease FBT
If you want to follow the same pattern astask_timerand Ruff’s FBT rules, you could makesilentkeyword‑only insolve_instance(after auditing call sites):
- def solve_instance(
instance: TemoaModel,solver_name: str,silent: bool = False,solver_suffixes: Iterable[str] | None = None,- ) -> tuple[TemoaModel, SolverResults]:
- def solve_instance(
instance: TemoaModel,solver_name: str,*,silent: bool = False,solver_suffixes: Iterable[str] | None = None,- ) -> tuple[TemoaModel, SolverResults]:
Since this function lives under `_internal`, the break risk is limited but still worth checking for any positional callers.Overall, the solve/timing and status‑logging behavior looks correct; these are just polish items.
Also applies to: 281-334
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
temoa/_internal/run_actions.py(9 hunks)temoa/cli.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-03T13:34:35.907Z
Learnt from: ParticularlyPythonicBS
Repo: TemoaProject/temoa PR: 184
File: temoa/data_processing/database_util.py:109-118
Timestamp: 2025-11-03T13:34:35.907Z
Learning: In the Temoa project, SQL injection vulnerabilities in database utility classes (such as temoa/data_processing/database_util.py) are considered low concern due to the data access patterns and should not be flagged during code reviews. Technical debt for SQL injection hardening should be tracked separately as issues rather than blocking PRs.
Applied to files:
temoa/_internal/run_actions.py
📚 Learning: 2025-11-03T13:34:35.907Z
Learnt from: ParticularlyPythonicBS
Repo: TemoaProject/temoa PR: 184
File: temoa/data_processing/database_util.py:109-118
Timestamp: 2025-11-03T13:34:35.907Z
Learning: In the Temoa project, SQL injection vulnerabilities in the database utility classes (such as temoa/data_processing/database_util.py) are considered low concern due to the data access patterns and should not be flagged during reviews. Technical debt for SQL injection hardening should be tracked separately as issues rather than blocking PRs.
Applied to files:
temoa/_internal/run_actions.py
🧬 Code graph analysis (1)
temoa/_internal/run_actions.py (1)
temoa/_internal/table_writer.py (2)
TableWriter(77-658)write_results(91-135)
🪛 Ruff (0.14.7)
temoa/_internal/run_actions.py
178-178: Boolean-typed positional argument in function definition
(FBT001)
178-178: Boolean default positional argument in function definition
(FBT002)
255-255: Redundant exception object included in logging.exception call
(TRY401)
256-259: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
263-263: Do not catch blind exception: Exception
(BLE001)
265-268: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
269-269: 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). (4)
- GitHub Check: setup and test (windows-latest, 3.12)
- GitHub Check: setup and test (macos-latest, 3.12)
- GitHub Check: setup and test (macos-latest, 3.13)
- GitHub Check: setup and test (windows-latest, 3.13)
🔇 Additional comments (4)
temoa/cli.py (1)
49-55: RichHandler timestamp configuration aligns with requested CLI formatEnabling
show_time=Truewithlog_time_format='[%X]'should emit timestamps like[HH:MM:SS] INFO …on every console log line, matching the reviewer’s request while keeping file logging unchanged. Please run a simple command (e.g.,temoa --help) to visually confirm the Rich output on your terminal.temoa/_internal/run_actions.py (3)
33-50: task_timer is exception-safe and matches the new logging patternUsing
perf_counter()with atry/finallyensures the “Finished … (Time taken: …s)” message is logged even if the block raises, whilesilentcleanly gates both start/finish INFO logs. This pairs well with the Rich console timestamps configured incli.pyto produce consistent, per-step timing output. Please run a representative workflow to confirm the emitted lines look as expected with your terminal’s Rich formatting.
73-81: Database version check now closes connections reliably; confirm metadata schemaWrapping each
(con, name)pair in atry/…/finallywithcon.close()ensures both input and (optional) output database connections are closed regardless of query success or failure, resolving prior leak concerns. The queries now assume ametadatatable withvalueentries forDB_MAJORandDB_MINOR; please confirm this matches the actual schema for both input and output databases so version checking behaves as intended.Also applies to: 87-104
343-360: Result processing refactor and logging improvements look correctWrapping result handling in
task_timer('Processing results', silent=config.silent)plus the unifiedwrite_resultscall withresults_with_duals=results if config.save_duals else Nonecleanly ties dual writing to the config flag and matches theTableWriter.write_resultsAPI. The revisedscenario_name/temp_scenariologic for Excel output and the more explicitTotal Cost value: …INFO log are also consistent and low‑risk. Please confirm thatmake_exceland downstream tooling expect the scenario container and filename in this shape (single‑element set and path without extension).Also applies to: 361-364
ParticularlyPythonicBS
force-pushed
the
ref/task_timer_context_manager
branch
from
c3e1347 to
f4e7082
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
temoa/_internal/run_actions.py (2)
247-278: Redundant exception parameter in logging.Line 255 passes the caught
errortologger.exception, butlogging.exceptionautomatically includes the exception and traceback. A past review comment indicated this was addressed in commit c3e1347, but the redundant parameter is still present.Apply this fix:
except RuntimeError as error: - logger.exception('Solver failed to solve and returned an error: %s', error) + logger.exception('Solver failed to solve and returned an error') logger.error(The subsequent
logger.errorcalls (lines 256-259, 265-268) are appropriate for adding context without re-logging the traceback. The blindExceptioncatch at line 263 is acceptable for robust status extraction.
178-178: Consider makingsilentkeyword-only for consistency.The
silentboolean parameter is positional, whiletask_timer(line 34) uses a keyword-onlysilentparameter (via*). Makingsilentkeyword-only here would maintain API consistency and avoid boolean trap anti-patterns flagged by static analysis.def solve_instance( instance: TemoaModel, solver_name: str, + *, silent: bool = False, solver_suffixes: Iterable[str] | None = None, ) -> tuple[TemoaModel, SolverResults]:All current call sites should already use
silent=by name, so this would be non-breaking if the codebase follows that convention.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
temoa/_internal/run_actions.py(9 hunks)temoa/cli.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-03T13:34:35.907Z
Learnt from: ParticularlyPythonicBS
Repo: TemoaProject/temoa PR: 184
File: temoa/data_processing/database_util.py:109-118
Timestamp: 2025-11-03T13:34:35.907Z
Learning: In the Temoa project, SQL injection vulnerabilities in database utility classes (such as temoa/data_processing/database_util.py) are considered low concern due to the data access patterns and should not be flagged during code reviews. Technical debt for SQL injection hardening should be tracked separately as issues rather than blocking PRs.
Applied to files:
temoa/_internal/run_actions.py
📚 Learning: 2025-11-03T13:34:35.907Z
Learnt from: ParticularlyPythonicBS
Repo: TemoaProject/temoa PR: 184
File: temoa/data_processing/database_util.py:109-118
Timestamp: 2025-11-03T13:34:35.907Z
Learning: In the Temoa project, SQL injection vulnerabilities in the database utility classes (such as temoa/data_processing/database_util.py) are considered low concern due to the data access patterns and should not be flagged during reviews. Technical debt for SQL injection hardening should be tracked separately as issues rather than blocking PRs.
Applied to files:
temoa/_internal/run_actions.py
🪛 Ruff (0.14.8)
temoa/_internal/run_actions.py
178-178: Boolean-typed positional argument in function definition
(FBT001)
178-178: Boolean default positional argument in function definition
(FBT002)
255-255: Redundant exception object included in logging.exception call
(TRY401)
256-259: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
263-263: Do not catch blind exception: Exception
(BLE001)
265-268: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
269-269: 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). (4)
- GitHub Check: setup and test (macos-latest, 3.12)
- GitHub Check: setup and test (macos-latest, 3.13)
- GitHub Check: setup and test (windows-latest, 3.12)
- GitHub Check: setup and test (windows-latest, 3.13)
🔇 Additional comments (7)
temoa/cli.py (1)
53-53: Console timestamps enabled as requested.The addition of
show_time=Truealigns with the PR objectives and user feedback to display timestamps in square brackets for every step in console output. This makes console behavior consistent with the "Time taken:" labels emitted by the newtask_timercontext manager.temoa/_internal/run_actions.py (6)
33-50: task_timer implementation is solid.The context manager correctly uses
perf_counterfor timing, ensures the elapsed time is logged even when exceptions occur (viafinally), and the keyword-onlysilentparameter prevents boolean traps. The log format matches the PR objectives.
147-159: Model building with task_timer looks good.The
task_timerwrapper provides consistent "Started"/"Finished" logging for model instantiation, and usingsum()with generator expressions for constraint/variable counting is both concise and efficient. The aggregate log message gives clear build metrics.
162-173: LP file saving is correct.The idempotent
mkdir(parents=True, exist_ok=True)call ensures the directory structure exists before writing, and thestr(filename)cast accommodates Pyomo's API requirements.
208-243: Solver configuration and suffix validation are well-structured.The explicit solver configuration branches for CBC, CPLEX, Gurobi, and APPSI HiGHS are clear and well-documented with parameter sources. The suffix validation logic (lines 232-243) correctly filters invalid suffixes against the legitimate set
{'dual', 'slack', 'rc'}and warns about removals, providing good user feedback.
281-333: Robust solver-agnostic status extraction.The
check_solve_statusfunction provides excellent compatibility across solver interfaces with multiple fallback strategies: it tries legacyresult['Solution'].Status, then APPSI-style attributes (status,problem_status,solver_status), and finally a string representation fallback. The safe attribute checking withhasattrprevents crashes on unexpected result formats.
336-394: Results handling refactored cleanly.The
task_timerwrapper provides consistent timing for result processing, and the conditionalresults_with_duals=results if config.save_duals else Noneparameter elegantly handles both cases without code duplication. The Graphviz generation is appropriately wrapped in try/except (lines 393-394) to ensure optional visualization failures don't crash the main workflow.
ParticularlyPythonicBS
force-pushed
the
ref/task_timer_context_manager
branch
2 times, most recently
from
230986a to
4eefc48
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
temoa/_internal/run_actions.py (1)
361-387: Potential resource leak in Graphviz generation.If an exception occurs between
graph_gen.connect()(line 376) andgraph_gen.close()(line 386), the connection may not be closed. Consider using a context manager pattern or explicittry/finally.Suggested fix:
graph_gen = GraphvizDiagramGenerator( db_file=str(config.output_database), scenario=config.scenario, out_dir=out_dir, verbose=0, # Less verbose for integrated run ) graph_gen.connect() - - # Get periods from the model instance - periods = sorted(instance.time_optimize) - - for period in periods: - # Generate main results diagram for the period - # We pass None for region to generate for all/default - graph_gen.create_main_results_diagram(period=period, region=None) - - graph_gen.close() + try: + # Get periods from the model instance + periods = sorted(instance.time_optimize) + + for period in periods: + # Generate main results diagram for the period + # We pass None for region to generate for all/default + graph_gen.create_main_results_diagram(period=period, region=None) + finally: + graph_gen.close() logger.info('Graphviz plots generated in %s', graph_gen.out_dir)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
temoa/_internal/run_actions.py(10 hunks)temoa/cli.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-03T13:34:35.907Z
Learnt from: ParticularlyPythonicBS
Repo: TemoaProject/temoa PR: 184
File: temoa/data_processing/database_util.py:109-118
Timestamp: 2025-11-03T13:34:35.907Z
Learning: In the Temoa project, SQL injection vulnerabilities in database utility classes (such as temoa/data_processing/database_util.py) are considered low concern due to the data access patterns and should not be flagged during code reviews. Technical debt for SQL injection hardening should be tracked separately as issues rather than blocking PRs.
Applied to files:
temoa/_internal/run_actions.py
📚 Learning: 2025-11-03T13:34:35.907Z
Learnt from: ParticularlyPythonicBS
Repo: TemoaProject/temoa PR: 184
File: temoa/data_processing/database_util.py:109-118
Timestamp: 2025-11-03T13:34:35.907Z
Learning: In the Temoa project, SQL injection vulnerabilities in the database utility classes (such as temoa/data_processing/database_util.py) are considered low concern due to the data access patterns and should not be flagged during reviews. Technical debt for SQL injection hardening should be tracked separately as issues rather than blocking PRs.
Applied to files:
temoa/_internal/run_actions.py
🧬 Code graph analysis (1)
temoa/_internal/run_actions.py (1)
temoa/_internal/table_writer.py (1)
write_results(231-274)
🪛 Ruff (0.14.8)
temoa/_internal/run_actions.py
174-174: Boolean-typed positional argument in function definition
(FBT001)
174-174: Boolean default positional argument in function definition
(FBT002)
251-251: Redundant exception object included in logging.exception call
(TRY401)
252-255: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
259-259: Do not catch blind exception: Exception
(BLE001)
261-264: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
265-265: 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). (4)
- GitHub Check: setup and test (windows-latest, 3.12)
- GitHub Check: setup and test (macos-latest, 3.13)
- GitHub Check: setup and test (macos-latest, 3.12)
- GitHub Check: setup and test (windows-latest, 3.13)
🔇 Additional comments (8)
temoa/cli.py (1)
49-55: LGTM! Console timestamps enabled as requested.The
show_time=Truesetting combined with the existinglog_time_format='[%X]'will produce timestamps in[HH:MM:SS]format to the left of the log level, aligning with the PR objectives for consistent CLI output.temoa/_internal/run_actions.py (7)
33-50: LGTM! Clean task_timer implementation.The context manager correctly uses
perf_counterfor timing, makessilentkeyword-only (addressing prior feedback), and always logs elapsed time in thefinallyblock even when exceptions occur. The "Time taken:" wording aligns with the PR discussion.
80-103: Connection lifecycle handling is correct.Each database connection is now properly scoped within the loop iteration's
try/finallyblock, ensuring connections are closed even if queries fail. This addresses the previously identified edge-case leak concern.
143-155: LGTM! Clean integration of task_timer with model building.The
task_timerwrapper provides standardized timing, and the aggregate counts usingsum(len(...))over component objects give clear, low-overhead build metrics.
158-168: LGTM!The
mkdir(parents=True, exist_ok=True)call is now idempotent without the redundantexists()check, addressing previous feedback. The defensiveif not lp_pathguard is reasonable even if callers currently validate.
204-239: Solver configuration and suffix handling look good.The solver-specific options are well-documented with references to PyPSA. The suffix validation correctly filters invalid entries and warns users, preventing silent failures when requesting unsupported suffixes.
277-329: LGTM! Robust solver-agnostic status extraction.The function gracefully handles both legacy and APPSI solver result formats with multiple fallback strategies, ensuring status information is always available for logging even when result object structures vary between solvers.
339-347: LGTM! Simplified write_results call.The conditional
results_with_duals=results if config.save_duals else Noneconsolidates the previous two-branch logic into a single call, as suggested in prior review feedback.
| except RuntimeError as error: | ||
| logger.error('Solver failed to solve and returned an error: %s', error) | ||
| logger.exception('Solver failed to solve and returned an error: %s', error) | ||
| logger.error( | ||
| 'This may be due to asking for suffixes (duals) for an incompatible solver. ' | ||
| "Try de-selecting 'save_duals' in the config. (see note in run_actions.py code)" | ||
| ) | ||
| if result: | ||
| try: | ||
| _ok, status_msg = check_solve_status(result) | ||
| except Exception: | ||
| status_msg = '<unable to extract status>' | ||
| logger.error( | ||
| 'Solver reported termination condition (if any): %s', result['Solution'].Status | ||
| 'Solver reported termination/status (if any): %s', | ||
| status_msg, | ||
| ) | ||
| sys.stderr.write('solver failure. See log file.') | ||
| sys.exit(-1) | ||
| raise RuntimeError('Solver failure. See log file.') from error |
There was a problem hiding this comment.
Redundant exception object in logger.exception call.
On line 251, logger.exception already captures and logs the exception automatically, so passing error as a formatting argument is redundant. This was flagged in a past review as addressed but appears to persist.
Suggested fix:
except RuntimeError as error:
- logger.exception('Solver failed to solve and returned an error: %s', error)
+ logger.exception('Solver failed to solve and returned an error')
logger.error(The bare Exception catch at line 259 is acceptable here since it's defensive status extraction within an already-failed code path.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except RuntimeError as error: | |
| logger.error('Solver failed to solve and returned an error: %s', error) | |
| logger.exception('Solver failed to solve and returned an error: %s', error) | |
| logger.error( | |
| 'This may be due to asking for suffixes (duals) for an incompatible solver. ' | |
| "Try de-selecting 'save_duals' in the config. (see note in run_actions.py code)" | |
| ) | |
| if result: | |
| try: | |
| _ok, status_msg = check_solve_status(result) | |
| except Exception: | |
| status_msg = '<unable to extract status>' | |
| logger.error( | |
| 'Solver reported termination condition (if any): %s', result['Solution'].Status | |
| 'Solver reported termination/status (if any): %s', | |
| status_msg, | |
| ) | |
| sys.stderr.write('solver failure. See log file.') | |
| sys.exit(-1) | |
| raise RuntimeError('Solver failure. See log file.') from error | |
| except RuntimeError as error: | |
| logger.exception('Solver failed to solve and returned an error') | |
| logger.error( | |
| 'This may be due to asking for suffixes (duals) for an incompatible solver. ' | |
| "Try de-selecting 'save_duals' in the config. (see note in run_actions.py code)" | |
| ) | |
| if result: | |
| try: | |
| _ok, status_msg = check_solve_status(result) | |
| except Exception: | |
| status_msg = '<unable to extract status>' | |
| logger.error( | |
| 'Solver reported termination/status (if any): %s', | |
| status_msg, | |
| ) | |
| raise RuntimeError('Solver failure. See log file.') from error |
🧰 Tools
🪛 Ruff (0.14.8)
251-251: Redundant exception object included in logging.exception call
(TRY401)
252-255: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
259-259: Do not catch blind exception: Exception
(BLE001)
261-264: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
265-265: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In temoа/_internal/run_actions.py around lines 250 to 265, the logger.exception
call includes the caught exception as a formatting argument which is redundant
because logger.exception already logs the traceback and exception; remove the
extra "%s" and the error argument and instead call logger.exception with a plain
descriptive message (e.g. "Solver failed to solve and returned an error") so the
exception is still logged with traceback, leaving the following logger.error
messages and the raised RuntimeError intact.
ParticularlyPythonicBS
force-pushed
the
ref/task_timer_context_manager
branch
from
4eefc48 to
e637077
Compare
Standardizes the logging and timing mechanisms using a reusable context manager to handle operation timing.
New sample output
Current output:
Summary by CodeRabbit
New Features
Bug Fixes
Quality
✏️ Tip: You can customize this high-level summary in your review settings.