Add a framework for modular model extensions#343
Open
idelder wants to merge 4 commits into
Open
Conversation
idelder
force-pushed
the
extensions/framework
branch
from
360e30d to
21f098a
Compare
WalkthroughAdds an extension framework with runtime model and loader integration, migrates growth and degrowth limits into the optional ChangesExtension runtime and loading
Growth-rate migration
Supporting assets
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
idelder
force-pushed
the
extensions/framework
branch
from
21f098a to
e4b6a1e
Compare
idelder
force-pushed
the
extensions/framework
branch
from
e4b6a1e to
426582b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/source/extensions/growth_rates.rst`:
- Around line 87-89: Fix the typo in the degrowth-delta description by updating
the prose for limit_degrowth_new_capacity_delta in growth_rates.rst so that
“intertia” reads “inertia”. Keep the surrounding explanation unchanged and
ensure the corrected wording remains in the parameter description text.
In `@temoa/cli.py`:
- Around line 650-652: The foreign-key validation error in the tutorial data
load fallback is chained to the wrong exception via `from e`, which makes the
traceback misleading. Update the `sqlite3.IntegrityError` raise in the fallback
path to suppress the prior package-loading failure and use `from None` instead.
Keep the FK violation message in the `cli.py` tutorial-load logic, but do not
chain it to the earlier
`ModuleNotFoundError`/`FileNotFoundError`/`AttributeError`.
- Around line 590-599: The SQLite load flow should explicitly close the database
handle after the import work and avoid carrying the previous exception into the
fallback FK failure. Update the tutorial data load logic around the
sqlite3.connect context so the connection is cleanly closed once PRAGMA and
foreign_key_check are done, and in the fallback path where the IntegrityError is
raised, change the exception chaining to detach it from the earlier import/path
error by using a direct raise from None. Keep the existing fk_violations check
and error message, but ensure the cleanup and error reporting behavior is
handled in the same load routine.
In `@temoa/components/technology.py`:
- Around line 445-448: The guard in the existing-capacity handling path is
treating zero and negative adjusted capacity the same, which hides the
over-retired state. Update the logic around get_adjusted_existing_capacity in
technology.py so adjusted_cap == 0 continues to mean fully retired and can be
skipped, but adjusted_cap < 0 is detected explicitly and handled as an error or
corrective case instead of falling through. Make sure the related
adjusted_capacity_constraint() path cannot receive a negative baseline from the
same get_adjusted_existing_capacity / retired_existing_capacity flow.
In `@temoa/core/model.py`:
- Around line 449-450: `lifetime_tech` is too restrictive compared with the raw
loader contract, causing valid lifetime metadata for declared-but-unused
technologies to fail model build. Update the lifetime-tech initialization in
`TemoaModel` so `_load_lifetime_tech()` can accept rows for technologies that
are declared but not present in `efficiency` or `existing_capacity`, aligning
`self.lifetime_tech_rt` and `self.lifetime_tech` with the same sparse/lenient
handling used by `create_sparse_dicts()` and the loader path.
- Around line 224-226: `vintage_all` is incorrectly narrowed to only
`vintage_exist | time_optimize`, which drops historical vintages that still
appear in `main.efficiency` but are not in `existing_capacity`. Update the
`Model` initialization around `vintage_exist`, `vintage_optimize`, and
`vintage_all` so `vintage_all` still includes every valid historical vintage
loaded by `HybridLoader._build_efficiency_dataset()`, while preserving the
intended optimization set behavior.
In `@temoa/data_io/hybrid_loader.py`:
- Around line 697-764: The custom loaders _load_lifetime_tech,
_load_lifetime_process, and _load_lifetime_survival_curve are bypassing the
optional-table handling by querying the database directly, which can raise
sqlite3.OperationalError on older databases. Update these methods in
hybrid_loader.py to reuse the already-fetched rows passed through the
data/raw_data/filtered_data flow instead of issuing new SELECTs against
lifetime_tech, lifetime_process, and lifetime_survival_curve. Keep the existing
viability and myopic filtering logic, but apply it to the fetched rows so
optional tables remain safely skipped when absent.
In `@temoa/data_io/loader_manifest.py`:
- Line 53: Propagate the new index_length field through manifest consumers so
key handling matches HybridLoader behavior. Update scenario_creator (and any
related manifest parsing/mapping logic) to use the item’s index_length instead
of hardcoding item.columns[:-1], so growth-rate items with shorter stored keys
don’t break strict zipping. Use the LoaderManifest and scenario_creator symbols
to locate the mapping logic and ensure downstream perturbation code derives
column/key pairs from the declared index_length.
In `@temoa/extensions/growth_rates/components/growth_capacity.py`:
- Around line 102-110: The first-period fallback in the growth capacity logic
unconditionally uses model.time_exist.last(), which breaks when there is no
historical period. Update the constraint/rule in growth_capacity.py around the p
== model.time_optimize.first() branch to guard for an empty model.time_exist set
before reading last(), and either skip the fallback or use an explicit
zero-history baseline consistently with the other time-boundary handling in this
module. Keep the fix localized to the capacity_prev calculation so the
growth_rates constraint construction does not fail on models without historical
data.
In `@temoa/extensions/growth_rates/components/growth_new_capacity_delta.py`:
- Around line 111-123: The initialization logic in growth_new_capacity_delta for
the first optimization period assumes two historical entries exist, but
model.time_exist.prev(p_prev) will fail when the history is shorter. Update the
first-period branch to explicitly check the length of model.time_exist before
using prev(), and in the one-entry or zero-entry case use a safe fallback such
as skipping the delta calculation or setting a zero baseline. Keep the change
localized around the p == model.time_optimize.first() block and the
p_prev/p_prev2 handling.
In `@temoa/extensions/growth_rates/components/growth_new_capacity.py`:
- Around line 103-109: The first-period baseline in the growth capacity logic is
still using raw model.existing_capacity, which skips retirement adjustments and
can produce an incorrect bound in myopic runs. Update the p ==
model.time_optimize.first() branch in growth_new_capacity so new_cap_prev is
built from get_adjusted_existing_capacity() instead of
value(model.existing_capacity[...]), keeping the same region/tech/time filtering
and using the adjusted capacity for the baseline calculation.
In `@temoa/extensions/stochastics/scenario_creator.py`:
- Line 99: Extension-aware stochastic loading is still assuming the last column
is the only value column, which breaks multi-column items. Update the table
indexing logic in scenario_creator.py so the `table_index_map` construction and
any downstream stochastic filter setup use each `LoadItem`’s `index_length`
instead of `item.columns[:-1]`, and make sure `build_instance(...)` paths that
process extension manifest items preserve the correct index/value split. Use the
existing `LoadItem.index_length`, `table_index_map`, and `build_instance`
symbols to locate and align the index handling.
In `@temoa/extensions/template/components/example_limit.py`:
- Around line 51-56: The example_limit aggregation is scanning every
v_new_capacity key, which should be avoided in this template. Update the logic
in the example_limit component to mirror the core limit rule by iterating over
regions and techs directly and filtering via process_periods, using the same
symbols model.v_new_capacity, regions, techs, and p to keep the pattern
efficient for downstream extensions.
In `@temoa/extensions/template/tables.sql`:
- Around line 6-14: The composite primary key on example_new_capacity_limit is
still nullable in SQLite, so update the table definition to make both region and
tech_or_group explicitly NOT NULL while keeping the existing PRIMARY KEY
(region, tech_or_group) constraint. Use the example_new_capacity_limit CREATE
TABLE statement in tables.sql and ensure the scaffold prevents malformed key
rows from being inserted.
In `@tests/conftest.py`:
- Around line 61-67: The extension fixture setup in conftest.py loads
extension-owned tables before foreign key enforcement is enabled, so invalid
rows can slip through unnoticed. After the fixture-loading loop that iterates
over get_known_extension_specs() and runs executescript(), add a foreign key
validation step using PRAGMA foreign_key_check on the same connection before the
test DB is considered ready. Keep the existing foreign_keys setting, but ensure
the check runs after all extension schema/data setup so fixture corruption is
caught consistently with the CLI build path.
In `@tests/testing_configs/config_myopic_capacities.toml`:
- Around line 4-5: The myopic test config is pointing both input_database and
output_database at the shared fixture SQLite file, so MyopicSequencer will write
in place and leak state across parametrized runs. Update this config to use a
per-test copy for output_database (while keeping input_database on the fixture)
so each run writes to its own database; if needed, align the test setup around
TemoaConfig.build_config() and MyopicSequencer so the output path is isolated
from the shared fixture.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c8e8bbef-60d4-49de-8a4c-ba07b0f332b1
📒 Files selected for processing (59)
.pre-commit-config.yamldocs/source/computational_implementation.rstdocs/source/extensions.rstdocs/source/extensions/growth_rates.rstdocs/source/index.rstdocs/source/mathematical_formulation.rstdocs/source/param_desc_and_tables.rsttemoa/_internal/run_actions.pytemoa/_internal/temoa_sequencer.pytemoa/cli.pytemoa/components/capacity.pytemoa/components/limits.pytemoa/components/technology.pytemoa/components/time.pytemoa/components/utils.pytemoa/core/config.pytemoa/core/model.pytemoa/data_io/component_manifest.pytemoa/data_io/hybrid_loader.pytemoa/data_io/loader_manifest.pytemoa/db_schema/temoa_schema_v4.sqltemoa/extensions/framework.pytemoa/extensions/growth_rates/__init__.pytemoa/extensions/growth_rates/components/__init__.pytemoa/extensions/growth_rates/components/growth_capacity.pytemoa/extensions/growth_rates/components/growth_new_capacity.pytemoa/extensions/growth_rates/components/growth_new_capacity_delta.pytemoa/extensions/growth_rates/core/__init__.pytemoa/extensions/growth_rates/core/model.pytemoa/extensions/growth_rates/data_manifest.pytemoa/extensions/growth_rates/extension.pytemoa/extensions/growth_rates/tables.sqltemoa/extensions/method_of_morris/morris.pytemoa/extensions/method_of_morris/morris_evaluate.pytemoa/extensions/modeling_to_generate_alternatives/mga_sequencer.pytemoa/extensions/myopic/myopic_sequencer.pytemoa/extensions/single_vector_mga/sv_mga_sequencer.pytemoa/extensions/stochastics/scenario_creator.pytemoa/extensions/template/__init__.pytemoa/extensions/template/components/__init__.pytemoa/extensions/template/components/example_limit.pytemoa/extensions/template/core/__init__.pytemoa/extensions/template/core/model.pytemoa/extensions/template/data_manifest.pytemoa/extensions/template/extension.pytemoa/extensions/template/tables.sqltemoa/tutorial_assets/config_sample.tomltemoa/tutorial_assets/utopia.sqltests/conftest.pytests/legacy_test_values.pytests/test_cli.pytests/test_full_runs.pytests/testing_configs/config_myopic_capacities.tomltests/testing_data/mediumville.sqltests/testing_data/mediumville_sets.jsontests/testing_data/myopic_capacities.sqltests/testing_data/test_system_sets.jsontests/testing_data/utopia_sets.jsontests/testing_data/utopia_v3.sql
💤 Files with no reviewable changes (4)
- docs/source/param_desc_and_tables.rst
- tests/testing_data/mediumville.sql
- docs/source/mathematical_formulation.rst
- temoa/db_schema/temoa_schema_v4.sql
idelder
commented
Jul 6, 2026
idelder
force-pushed
the
extensions/framework
branch
from
426582b to
57639c9
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (7)
temoa/extensions/template/tables.sql (1)
6-14: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMake the primary-key columns explicitly
NOT NULL.SQLite still accepts
NULLin a compositePRIMARY KEY, so malformed rows can slip into this scaffold. This was already flagged previously.🧩 Proposed fix
CREATE TABLE IF NOT EXISTS example_new_capacity_limit ( - region TEXT, - tech_or_group TEXT, + region TEXT NOT NULL, + tech_or_group TEXT NOT NULL, value REAL NOT NULL DEFAULT 0, units TEXT, notes TEXT, PRIMARY KEY (region, tech_or_group) );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@temoa/extensions/template/tables.sql` around lines 6 - 14, The composite primary key in example_new_capacity_limit should be made explicitly NOT NULL on both key columns, since SQLite can otherwise allow NULLs in a PRIMARY KEY. Update the CREATE TABLE definition for region and tech_or_group so the primary-key fields are declared NOT NULL while keeping the existing PRIMARY KEY (region, tech_or_group) constraint.tests/testing_configs/config_myopic_capacities.toml (1)
4-5: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep the myopic output off the shared fixture.
This still writes back into
tests/testing_outputs/myopic_capacities.sqlite, so parametrized runs can leak state across cases. This is the same issue flagged in the previous review.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/testing_configs/config_myopic_capacities.toml` around lines 4 - 5, The myopic capacities test config is still pointing both input_database and output_database at the shared fixture, which can leak state between parametrized runs. Update the configuration used by this test so the output_database no longer writes back to tests/testing_outputs/myopic_capacities.sqlite, and instead uses a separate per-run or isolated output target while keeping the relevant config keys in config_myopic_capacities.toml consistent.temoa/cli.py (1)
641-654: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winSame
conn.close()-inside-withbug in the fallback path.Identical issue as the packaged-resource path above:
conn.close()at line 650 runs before the enclosingwithblock's__exit__, which will then attemptcommit()/rollback()on an already-closed connection and raisesqlite3.ProgrammingError.🐛 Proposed fix
- with sqlite3.connect(target_database) as conn: + import contextlib + with contextlib.closing(sqlite3.connect(target_database)) as conn: conn.executescript(fallback_schema.read_text(encoding='utf-8')) conn.execute('PRAGMA foreign_keys = OFF;') conn.executescript(sql_content) conn.execute('PRAGMA foreign_keys = ON;') + conn.commit() fk_violations = conn.execute('PRAGMA foreign_key_check;').fetchall() - conn.close() if fk_violations: raise sqlite3.IntegrityError( f'Foreign key check failed after tutorial data load: {fk_violations[:5]}' ) from NoneBased on learnings, prefer
with contextlib.closing(sqlite3.connect(...)) as con:for explicit connection closing; same root cause as the packaged-resource path flagged above.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@temoa/cli.py` around lines 641 - 654, The fallback database load path in the sqlite3 connection block has the same premature-close problem as the packaged-resource path: `conn.close()` is called inside the `with sqlite3.connect(...) as conn:` scope, causing the context manager’s exit to operate on an already-closed connection. Update this code in the fallback path to use the same connection-lifecycle pattern as the other path (for example, `contextlib.closing` around `sqlite3.connect(...)` or otherwise remove the manual close and let the context manager manage it) in the `cli.py` database generation logic around `conn`, `fk_violations`, and the `PRAGMA foreign_key_check` flow.Source: Learnings
temoa/extensions/growth_rates/components/growth_new_capacity.py (1)
103-109: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFirst-period baseline still bypasses
get_adjusted_existing_capacity, and lacks a guard for emptytime_exist.Lines 105-109 sum
model.existing_capacity[...]directly, so myopic runs with early retirements compute the growth/degrowth bound from pre-retirement capacity. Additionally, line 104'smodel.time_exist.last()has no guard for an emptytime_existset (same class of issue raised ingrowth_capacity.py).🐛 Proposed fix
-from temoa.components.utils import Operator, operator_expression +from temoa.components.utils import ( + Operator, + get_adjusted_existing_capacity, + operator_expression, +) @@ if p == model.time_optimize.first(): - p_prev = model.time_exist.last() - new_cap_prev = sum( - value(model.existing_capacity[_r, _t, _v]) - for _r, _t, _v in model.existing_capacity.sparse_keys() - if _r in regions and _t in techs and _v == p_prev - ) + if len(model.time_exist) == 0: + new_cap_prev = 0 + else: + p_prev = model.time_exist.last() + new_cap_prev = sum( + get_adjusted_existing_capacity(model, _r, _t, _v) + for _r, _t, _v in model.existing_capacity.sparse_keys() + if _r in regions and _t in techs and _v == p_prev + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@temoa/extensions/growth_rates/components/growth_new_capacity.py` around lines 103 - 109, The first-period baseline in growth_new_capacity still reads directly from existing_capacity and skips get_adjusted_existing_capacity, so update the p == model.time_optimize.first() branch in growth_new_capacity logic to derive new_cap_prev from the adjusted existing-capacity helper instead of raw values. Also add the same empty-time_exist guard used elsewhere before calling model.time_exist.last(), so the first-period path handles an empty time_exist safely and consistently.temoa/extensions/growth_rates/components/growth_new_capacity_delta.py (1)
111-136: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnguarded
time_existboundary calls plus unadjusted existing capacity in the first-period fallback.Line 118's
model.time_exist.prev(p_prev)still fails iftime_existhas fewer than two entries (and line 112's.last()fails if it has none). Separately, lines 114, 120, and 130 use rawvalue(model.existing_capacity[...]), bypassingget_adjusted_existing_capacity, so myopic early-retirement adjustments are ignored in this constraint's baseline — the same gap already flagged/fixed ingrowth_capacity.py.🛡️ Proposed fix
-from temoa.components.utils import Operator, operator_expression +from temoa.components.utils import ( + Operator, + get_adjusted_existing_capacity, + operator_expression, +) @@ if p == model.time_optimize.first(): - p_prev = model.time_exist.last() - new_cap_prev = sum( - value(model.existing_capacity[_r, _t, _v]) - for _r, _t, _v in model.existing_capacity.sparse_keys() - if _r in regions and _t in techs and _v == p_prev - ) - p_prev2 = model.time_exist.prev(p_prev) - new_cap_prev2 = sum( - value(model.existing_capacity[_r, _t, _v]) - for _r, _t, _v in model.existing_capacity.sparse_keys() - if _r in regions and _t in techs and _v == p_prev2 - ) + if len(model.time_exist) < 2: + return Constraint.Skip + p_prev = model.time_exist.last() + new_cap_prev = sum( + get_adjusted_existing_capacity(model, _r, _t, _v) + for _r, _t, _v in model.existing_capacity.sparse_keys() + if _r in regions and _t in techs and _v == p_prev + ) + p_prev2 = model.time_exist.prev(p_prev) + new_cap_prev2 = sum( + get_adjusted_existing_capacity(model, _r, _t, _v) + for _r, _t, _v in model.existing_capacity.sparse_keys() + if _r in regions and _t in techs and _v == p_prev2 + ) else: p_prev = model.time_optimize.prev(p) new_cap_prev = sum(new_cap_rtv[_r, _t, _v] for _r, _t, _v in cap_rtv if _v == p_prev) if p == model.time_optimize.at(2): - p_prev2 = model.time_exist.last() - new_cap_prev2 = sum( - value(model.existing_capacity[_r, _t, _v]) - for _r, _t, _v in model.existing_capacity.sparse_keys() - if _r in regions and _t in techs and _v == p_prev2 - ) + if len(model.time_exist) == 0: + new_cap_prev2 = 0 + else: + p_prev2 = model.time_exist.last() + new_cap_prev2 = sum( + get_adjusted_existing_capacity(model, _r, _t, _v) + for _r, _t, _v in model.existing_capacity.sparse_keys() + if _r in regions and _t in techs and _v == p_prev2 + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@temoa/extensions/growth_rates/components/growth_new_capacity_delta.py` around lines 111 - 136, The fallback path in growth_new_capacity_delta is still using unguarded time_exist lookups and raw existing-capacity values. Update the first-period and p==at(2) branches in the constraint logic that uses model.time_exist.last() and model.time_exist.prev(...) so they safely handle cases where time_exist has fewer than two periods, and replace the direct value(model.existing_capacity[...]) baseline sums with get_adjusted_existing_capacity so the existing-capacity baseline matches the myopic early-retirement adjustment used elsewhere.temoa/extensions/growth_rates/components/growth_capacity.py (1)
102-111: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winStill missing guard for empty
time_existbefore calling.last().Line 103 unconditionally calls
model.time_exist.last(); on a model with no historical periods this raises during constraint construction rather than falling back cleanly.🛡️ Proposed fix
if p == model.time_optimize.first(): - p_prev = model.time_exist.last() - capacity_prev = sum( - get_adjusted_existing_capacity(model, _r, _t, _v) - * value(model.process_life_frac[_r, p_prev, _t, _v]) - for _r, _t, _v in model.existing_capacity.sparse_keys() - if _r in regions - and _t in techs - and _v + value(model.lifetime_process[_r, _t, _v]) > p_prev - ) + if len(model.time_exist) == 0: + capacity_prev = 0 + else: + p_prev = model.time_exist.last() + capacity_prev = sum( + get_adjusted_existing_capacity(model, _r, _t, _v) + * value(model.process_life_frac[_r, p_prev, _t, _v]) + for _r, _t, _v in model.existing_capacity.sparse_keys() + if _r in regions + and _t in techs + and _v + value(model.lifetime_process[_r, _t, _v]) > p_prev + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@temoa/extensions/growth_rates/components/growth_capacity.py` around lines 102 - 111, The capacity calculation in growth_capacity.py still assumes historical periods exist, because the branch in the growth constraint uses model.time_exist.last() unconditionally when p equals model.time_optimize.first(). Update the constraint logic around this first-period case to guard against an empty model.time_exist set before calling .last(), and short-circuit or fall back safely when there is no prior period so the constraint can be built without raising. Use the existing growth capacity constraint block and the p == model.time_optimize.first() branch as the place to apply the check.temoa/data_io/hybrid_loader.py (1)
697-764: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winOptional lifetime tables aren't actually optional in the loaders — unresolved from prior review.
_load_lifetime_tech,_load_lifetime_process, and_load_lifetime_survival_curveare registered withis_table_required=False, but they re-query their tables directly viacur.execute(...)without checking existence. On a database missing one of these tables,_fetch_data()already safely returns[]forraw_data, but these loaders ignore it and will raisesqlite3.OperationalErrorinstead of a graceful skip.🛡️ Proposed fix: reuse fetched rows
def _load_lifetime_tech( self, data: dict[str, object], raw_data: Sequence[tuple[object, ...]], filtered_data: Sequence[tuple[object, ...]], ) -> None: """Loads the lifetime_tech component.""" - model = TemoaModel() - cur = self.con.cursor() - rows_to_load = cur.execute('SELECT region, tech, lifetime FROM lifetime_tech').fetchall() + model = self.model + rows_to_load = list(raw_data) rt_getter = itemgetter(0, 1) ... def _load_lifetime_process(...): """Loads the lifetime_process component.""" - model = TemoaModel() - cur = self.con.cursor() mi = self.myopic_index - if mi: - rows_to_load = cur.execute( - 'SELECT region, tech, vintage, lifetime FROM lifetime_process WHERE vintage <= ?', - (mi.last_demand_year,), - ).fetchall() - else: - rows_to_load = cur.execute( - 'SELECT region, tech, vintage, lifetime FROM lifetime_process' - ).fetchall() + model = self.model + rows_to_load = list(raw_data) + if mi: + rows_to_load = [row for row in rows_to_load if row[2] <= mi.last_demand_year] ... def _load_lifetime_survival_curve(...): """Loads the lifetime_survival_curve component.""" - model = TemoaModel() - cur = self.con.cursor() mi = self.myopic_index - if mi: - rows_to_load = cur.execute( - 'SELECT region, period, tech, vintage, fraction FROM lifetime_survival_curve ' - 'WHERE vintage <= ?', - (mi.last_demand_year,), - ).fetchall() - else: - rows_to_load = cur.execute( - 'SELECT region, period, tech, vintage, fraction FROM lifetime_survival_curve' - ).fetchall() + model = self.model + rows_to_load = list(raw_data) + if mi: + rows_to_load = [row for row in rows_to_load if row[3] <= mi.last_demand_year] ...🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@temoa/data_io/hybrid_loader.py` around lines 697 - 764, The optional lifetime loaders still bypass the safe fetched data path and query SQLite directly, so missing tables can raise OperationalError. Update _load_lifetime_tech, _load_lifetime_process, and _load_lifetime_survival_curve to reuse the rows already fetched by _fetch_data() from raw_data/filtered_data instead of calling cur.execute on the tables again. Keep the existing viability and myopic filtering logic, but apply it to the passed-in rows so these optional tables can be skipped gracefully when absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@temoa/cli.py`:
- Around line 590-600: The tutorial database load logic in the sqlite3
connection block is closing the connection manually even though the context
manager on sqlite3.connect already handles cleanup. Remove the explicit
conn.close() in this block and the matching close in the fallback path so the
remaining FK check and block exit can complete normally; use the existing conn
variable in the load flow and let the context manager manage commit/rollback and
closure.
In `@temoa/data_io/hybrid_loader.py`:
- Around line 659-764: The new loaders are creating throwaway TemoaModel
instances just to access component names, which is unnecessary overhead. Update
_load_retired_existing_capacity, _load_lifetime_tech, _load_lifetime_process,
and _load_lifetime_survival_curve to use self.model instead of instantiating
TemoaModel(), matching the pattern already used in create_data_dict and
_load_regional_global_indices. Keep the existing cursor/query logic unchanged
and pass the component objects from self.model into _load_component_data.
In `@temoa/extensions/framework.py`:
- Around line 186-195: Harden _table_has_rows in framework.py by validating
table_name before using it in the f-string SQL query. Keep the existing
sqlite_master existence check, then ensure the identifier is limited to the
trusted owned-table set from spec.owned_tables or otherwise matches the
validated sqlite_master result before constructing SELECT 1 FROM
main.{table_name} LIMIT 1. Make the validation part of _table_has_rows so the
query only interpolates a trusted table name.
- Around line 159-167: The prompt in the `should_apply` flow can break
non-interactive runs because `input()` may raise `EOFError` when `silent` is
false and stdin has no TTY. Update the prompt handling in this branch to either
skip prompting when stdin is not interactive or catch `EOFError` and fall back
to the existing non-apply path, using the `spec.extension_id` prompt logic as
the place to apply the guard.
---
Duplicate comments:
In `@temoa/cli.py`:
- Around line 641-654: The fallback database load path in the sqlite3 connection
block has the same premature-close problem as the packaged-resource path:
`conn.close()` is called inside the `with sqlite3.connect(...) as conn:` scope,
causing the context manager’s exit to operate on an already-closed connection.
Update this code in the fallback path to use the same connection-lifecycle
pattern as the other path (for example, `contextlib.closing` around
`sqlite3.connect(...)` or otherwise remove the manual close and let the context
manager manage it) in the `cli.py` database generation logic around `conn`,
`fk_violations`, and the `PRAGMA foreign_key_check` flow.
In `@temoa/data_io/hybrid_loader.py`:
- Around line 697-764: The optional lifetime loaders still bypass the safe
fetched data path and query SQLite directly, so missing tables can raise
OperationalError. Update _load_lifetime_tech, _load_lifetime_process, and
_load_lifetime_survival_curve to reuse the rows already fetched by _fetch_data()
from raw_data/filtered_data instead of calling cur.execute on the tables again.
Keep the existing viability and myopic filtering logic, but apply it to the
passed-in rows so these optional tables can be skipped gracefully when absent.
In `@temoa/extensions/growth_rates/components/growth_capacity.py`:
- Around line 102-111: The capacity calculation in growth_capacity.py still
assumes historical periods exist, because the branch in the growth constraint
uses model.time_exist.last() unconditionally when p equals
model.time_optimize.first(). Update the constraint logic around this
first-period case to guard against an empty model.time_exist set before calling
.last(), and short-circuit or fall back safely when there is no prior period so
the constraint can be built without raising. Use the existing growth capacity
constraint block and the p == model.time_optimize.first() branch as the place to
apply the check.
In `@temoa/extensions/growth_rates/components/growth_new_capacity_delta.py`:
- Around line 111-136: The fallback path in growth_new_capacity_delta is still
using unguarded time_exist lookups and raw existing-capacity values. Update the
first-period and p==at(2) branches in the constraint logic that uses
model.time_exist.last() and model.time_exist.prev(...) so they safely handle
cases where time_exist has fewer than two periods, and replace the direct
value(model.existing_capacity[...]) baseline sums with
get_adjusted_existing_capacity so the existing-capacity baseline matches the
myopic early-retirement adjustment used elsewhere.
In `@temoa/extensions/growth_rates/components/growth_new_capacity.py`:
- Around line 103-109: The first-period baseline in growth_new_capacity still
reads directly from existing_capacity and skips get_adjusted_existing_capacity,
so update the p == model.time_optimize.first() branch in growth_new_capacity
logic to derive new_cap_prev from the adjusted existing-capacity helper instead
of raw values. Also add the same empty-time_exist guard used elsewhere before
calling model.time_exist.last(), so the first-period path handles an empty
time_exist safely and consistently.
In `@temoa/extensions/template/tables.sql`:
- Around line 6-14: The composite primary key in example_new_capacity_limit
should be made explicitly NOT NULL on both key columns, since SQLite can
otherwise allow NULLs in a PRIMARY KEY. Update the CREATE TABLE definition for
region and tech_or_group so the primary-key fields are declared NOT NULL while
keeping the existing PRIMARY KEY (region, tech_or_group) constraint.
In `@tests/testing_configs/config_myopic_capacities.toml`:
- Around line 4-5: The myopic capacities test config is still pointing both
input_database and output_database at the shared fixture, which can leak state
between parametrized runs. Update the configuration used by this test so the
output_database no longer writes back to
tests/testing_outputs/myopic_capacities.sqlite, and instead uses a separate
per-run or isolated output target while keeping the relevant config keys in
config_myopic_capacities.toml consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 25f6efc9-bf87-4daa-9deb-bd5d0444ec90
📒 Files selected for processing (59)
.pre-commit-config.yamldocs/source/computational_implementation.rstdocs/source/extensions.rstdocs/source/extensions/growth_rates.rstdocs/source/index.rstdocs/source/mathematical_formulation.rstdocs/source/param_desc_and_tables.rsttemoa/_internal/run_actions.pytemoa/_internal/temoa_sequencer.pytemoa/cli.pytemoa/components/capacity.pytemoa/components/limits.pytemoa/components/technology.pytemoa/components/time.pytemoa/components/utils.pytemoa/core/config.pytemoa/core/model.pytemoa/data_io/component_manifest.pytemoa/data_io/hybrid_loader.pytemoa/data_io/loader_manifest.pytemoa/db_schema/temoa_schema_v4.sqltemoa/extensions/framework.pytemoa/extensions/growth_rates/__init__.pytemoa/extensions/growth_rates/components/__init__.pytemoa/extensions/growth_rates/components/growth_capacity.pytemoa/extensions/growth_rates/components/growth_new_capacity.pytemoa/extensions/growth_rates/components/growth_new_capacity_delta.pytemoa/extensions/growth_rates/core/__init__.pytemoa/extensions/growth_rates/core/model.pytemoa/extensions/growth_rates/data_manifest.pytemoa/extensions/growth_rates/extension.pytemoa/extensions/growth_rates/tables.sqltemoa/extensions/method_of_morris/morris.pytemoa/extensions/method_of_morris/morris_evaluate.pytemoa/extensions/modeling_to_generate_alternatives/mga_sequencer.pytemoa/extensions/myopic/myopic_sequencer.pytemoa/extensions/single_vector_mga/sv_mga_sequencer.pytemoa/extensions/stochastics/scenario_creator.pytemoa/extensions/template/__init__.pytemoa/extensions/template/components/__init__.pytemoa/extensions/template/components/example_limit.pytemoa/extensions/template/core/__init__.pytemoa/extensions/template/core/model.pytemoa/extensions/template/data_manifest.pytemoa/extensions/template/extension.pytemoa/extensions/template/tables.sqltemoa/tutorial_assets/config_sample.tomltemoa/tutorial_assets/utopia.sqltests/conftest.pytests/legacy_test_values.pytests/test_cli.pytests/test_full_runs.pytests/testing_configs/config_myopic_capacities.tomltests/testing_data/mediumville.sqltests/testing_data/mediumville_sets.jsontests/testing_data/myopic_capacities.sqltests/testing_data/test_system_sets.jsontests/testing_data/utopia_sets.jsontests/testing_data/utopia_v3.sql
💤 Files with no reviewable changes (4)
- docs/source/mathematical_formulation.rst
- tests/testing_data/mediumville.sql
- temoa/db_schema/temoa_schema_v4.sql
- docs/source/param_desc_and_tables.rst
idelder
force-pushed
the
extensions/framework
branch
from
57639c9 to
78760c3
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
temoa/extensions/stochastics/scenario_creator.py (1)
38-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNit: move
import contextlibto top-level imports.Inline import works but is inconsistent with the module-level import block at Lines 1-17.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@temoa/extensions/stochastics/scenario_creator.py` at line 38, Move the contextlib import into the module’s top-level import block to match the existing import style. Update the imports in scenario_creator.py so contextlib is grouped with the other module imports near the top, and remove the inline import from the local scope.temoa/cli.py (1)
587-600: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract shared helper for schema+data DB build to remove duplication.
The primary and fallback branches duplicate the same open-connection / executescript(schema) / toggle-FK / executescript(data) / foreign_key_check sequence almost verbatim. A future fix to one path (e.g., error handling, FK-check batching) risks being missed in the other.
♻️ Proposed refactor
+def _build_tutorial_database(target_database: Path, schema_content: str, sql_content: str) -> None: + with contextlib.closing(sqlite3.connect(target_database)) as conn: + conn.executescript(schema_content) + conn.execute('PRAGMA foreign_keys = OFF;') + conn.executescript(sql_content) + conn.execute('PRAGMA foreign_keys = ON;') + fk_violations = conn.execute('PRAGMA foreign_key_check;').fetchall() + if fk_violations: + raise sqlite3.IntegrityError( + f'Foreign key check failed after tutorial data load: {fk_violations[:5]}' + ) + ... - with contextlib.closing(sqlite3.connect(target_database)) as conn: - conn.executescript(schema_content) - conn.execute('PRAGMA foreign_keys = OFF;') - conn.executescript(sql_content) - conn.execute('PRAGMA foreign_keys = ON;') - fk_violations = conn.execute('PRAGMA foreign_key_check;').fetchall() - if fk_violations: - raise sqlite3.IntegrityError( - f'Foreign key check failed after tutorial data load: {fk_violations[:5]}' - ) + _build_tutorial_database(target_database, schema_content, sql_content) ... - with contextlib.closing(sqlite3.connect(target_database)) as conn: - conn.executescript(fallback_schema.read_text(encoding='utf-8')) - conn.execute('PRAGMA foreign_keys = OFF;') - conn.executescript(sql_content) - conn.execute('PRAGMA foreign_keys = ON;') - fk_violations = conn.execute('PRAGMA foreign_key_check;').fetchall() - if fk_violations: - raise sqlite3.IntegrityError( - f'Foreign key check failed after tutorial data load: {fk_violations[:5]}' - ) from None + try: + _build_tutorial_database( + target_database, fallback_schema.read_text(encoding='utf-8'), sql_content + ) + except sqlite3.IntegrityError as fk_e: + raise sqlite3.IntegrityError(str(fk_e)) from NoneAlso applies to: 628-653
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@temoa/cli.py` around lines 587 - 600, The schema-and-data database build logic is duplicated between the primary and fallback paths, so extract the repeated sqlite3 connection/executescript/foreign-key-toggle/foreign_key_check sequence into a shared helper. Update the relevant code in cli.py around the existing tutorial load flow and the fallback branch to call that helper, preserving the current behavior in both paths while keeping the foreign-key validation and error handling centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@temoa/cli.py`:
- Around line 587-600: The schema-and-data database build logic is duplicated
between the primary and fallback paths, so extract the repeated sqlite3
connection/executescript/foreign-key-toggle/foreign_key_check sequence into a
shared helper. Update the relevant code in cli.py around the existing tutorial
load flow and the fallback branch to call that helper, preserving the current
behavior in both paths while keeping the foreign-key validation and error
handling centralized.
In `@temoa/extensions/stochastics/scenario_creator.py`:
- Line 38: Move the contextlib import into the module’s top-level import block
to match the existing import style. Update the imports in scenario_creator.py so
contextlib is grouped with the other module imports near the top, and remove the
inline import from the local scope.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5622942e-0645-4b76-b0cb-cb6542474514
📒 Files selected for processing (3)
temoa/cli.pytemoa/data_io/loader_manifest.pytemoa/extensions/stochastics/scenario_creator.py
idelder
force-pushed
the
extensions/framework
branch
from
78760c3 to
6405a22
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
temoa/extensions/growth_rates/components/growth_new_capacity_delta.py (1)
1-149: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftExtract shared logic across the three growth-rate constraint builders.
growth_capacity.py,growth_new_capacity.py, andgrowth_new_capacity_delta.pyduplicate near-identical index-building, growth/degrowth wrapper dispatch, and first/second-period existing-capacity fallback logic. This duplication is the direct cause of the inconsistencies found in this review — one file correctly appliesget_adjusted_existing_capacity()and guardstime_existboundaries, the other two don't. Consolidating the shared "resolve previous-period capacity (adjusted, with time_exist boundary guards)" logic and the growth/degrowth index/rule dispatch pattern into a common helper module would prevent this class of divergence going forward.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@temoa/extensions/growth_rates/components/growth_new_capacity_delta.py` around lines 1 - 149, The growth-rate constraint builders are duplicating the same index/rule dispatch and previous-capacity fallback logic, which has already caused inconsistent handling of existing capacity and time_exist boundaries. Refactor the shared pieces used by limit_growth_new_capacity_delta_indices, limit_degrowth_new_capacity_delta_indices, limit_growth_new_capacity_delta_constraint_rule, and limit_degrowth_new_capacity_delta_constraint_rule into a common helper so the previous-period capacity resolution is centralized. Move the first/second-period lookup and adjusted existing-capacity handling into that shared helper, then have limit_growth_new_capacity_delta delegate to it for both growth and degrowth paths.
♻️ Duplicate comments (2)
temoa/data_io/hybrid_loader.py (2)
697-764: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winOptional lifetime tables still crash on older schemas.
_load_lifetime_tech,_load_lifetime_process, and_load_lifetime_survival_curveare declaredis_table_required=Falsein the manifest but re-query the tables unconditionally viacur.execute(...), ignoring the already-fetchedraw_data/filtered_data. If an older database lacks one of these optional tables, this raisessqlite3.OperationalErrorinstead of gracefully skipping, as the manifest contract promises.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@temoa/data_io/hybrid_loader.py` around lines 697 - 764, The optional lifetime loaders are re-querying the database and can still fail on missing tables even though the manifest marks them optional. Update _load_lifetime_tech, _load_lifetime_process, and _load_lifetime_survival_curve to use the already provided raw_data/filtered_data inputs instead of calling cur.execute against the table names, and keep the existing myopic/viable filtering logic applied to those in-memory rows. This will let the optional-table path in the HybridLoader methods skip gracefully on older schemas without raising sqlite3.OperationalError.
659-764: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winThrowaway
TemoaModel()instantiation in new loaders.
_load_retired_existing_capacityand the three lifetime loaders each construct a freshTemoaModel()just to read.name, whileself.model(already extension-aware, used elsewhere in this same diff, e.g.create_data_dict) provides the same names without rebuilding the entire Sets/Params graph.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@temoa/data_io/hybrid_loader.py` around lines 659 - 764, The new loader methods are creating unnecessary throwaway TemoaModel instances just to access component names. Update _load_retired_existing_capacity, _load_lifetime_tech, _load_lifetime_process, and _load_lifetime_survival_curve to use the existing self.model instead of instantiating TemoaModel(), matching the extension-aware pattern already used elsewhere like create_data_dict. Keep the existing query/filter/load logic unchanged and just switch the model reference used for model.retired_existing_capacity, model.lifetime_tech, model.lifetime_process, and model.lifetime_survival_curve.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@temoa/cli.py`:
- Around line 587-601: The database initialization flow in the tutorial data
load block relies on implicit persistence; add an explicit commit on the sqlite3
connection in the schema/sql load sequence before the contextlib.closing block
ends. Update the logic around sqlite3.connect(target_database),
conn.executescript, and the foreign_key_check handling so the successful load is
made durable directly, while preserving the existing IntegrityError path for
fk_violations.
In `@temoa/components/technology.py`:
- Around line 445-459: The validator in technology handling currently downgrades
a missing active existing-capacity case to logger.warning, which allows invalid
non-myopic inputs through. Update the logic around
get_adjusted_existing_capacity, model.time_optimize.first(), and
model.process_periods so that myopic runs still only warn, but the
base/non-myopic path raises an exception when surviving capacity should extend
into future periods yet the process is not active. Keep the warning message text
for the myopic branch, and make the non-myopic branch fail fast with the same
context.
In `@temoa/components/time.py`:
- Around line 192-198: The special-case in param_period_length is still needed
for process_life_frac and technology.model_process_life_indices, not just the
growth_rates extension, so update the stale comment to reflect that dependency.
Harden the non-last time_exist branch in param_period_length by replacing the -1
sentinel with a failure path that raises if an existing period other than the
last is ever requested, so future callers don’t silently rely on an invalid
value. Keep the logic keyed to param_period_length, time_exist, and
process_life_frac so it’s easy to find if the period handling is refactored
later.
In `@temoa/data_io/hybrid_loader.py`:
- Around line 249-252: The reshape in hybrid_loader’s indexed-row handling is
wrapping a single trailing value column into a 1-element tuple, which later
makes _load_component_data store tuples instead of plain values. Update the
raw_data reshaping logic so it only packs trailing columns into a tuple when
there are 2 or more trailing value columns, and leaves the last column as a
scalar when index_length is len(item.columns) - 1. Use the existing
item.index_length and the indexed-Param path in _load_component_data to verify
the shape stays compatible with t[:-1]: t[-1].
In `@temoa/data_io/loader_manifest.py`:
- Around line 35-36: Clarify the `index_length` contract in `LoaderManifest` so
callers know it must leave more than one trailing value column, not just a
single trailing value. Update the `LoaderManifest` docstring and, if
appropriate, add validation in the dataclass initializer or `__post_init__` to
reject `index_length == len(columns) - 1`. Use the `index_length` field and the
`LoaderManifest` class as the primary places to fix this.
In `@temoa/extensions/growth_rates/components/growth_new_capacity_delta.py`:
- Around line 113-136: The fallback baselines in the growth-rate delta logic are
still using raw existing capacity instead of the retirement-adjusted totals.
Update the first-period and p == model.time_optimize.at(2) branches in
growth_new_capacity_delta.py so new_cap_prev and new_cap_prev2 are computed from
the same retirement-adjusted capacity source used elsewhere (for example the
cap_rtv/new_cap_rtv path), matching the existing_capacity-based logic only where
no retired capacity adjustment is needed.
In `@temoa/extensions/growth_rates/components/growth_new_capacity.py`:
- Around line 103-104: The first-period branch in growth_new_capacity’s
constraint logic should not call model.time_exist.last() without checking that
the set has entries, because it can crash during constraint construction. Update
the logic around the p == model.time_optimize.first() case to guard against an
empty model.time_exist before assigning p_prev, and choose a safe fallback or
skip the lookup when the set is empty. Keep the fix localized to the
growth_new_capacity constraint code path so it matches the similar handling
needed in growth_capacity.
In `@temoa/tutorial_assets/config_sample.toml`:
- Around line 21-26: The sample extensions example is misleading because it
references an unregistered extension id, which can cause Unknown extension id(s)
if copied as-is. Update the example in config_sample.toml to only show
registered extensions, and align the comment/example with the actual IDs
returned by get_known_extension_specs so users only see valid extension names.
---
Outside diff comments:
In `@temoa/extensions/growth_rates/components/growth_new_capacity_delta.py`:
- Around line 1-149: The growth-rate constraint builders are duplicating the
same index/rule dispatch and previous-capacity fallback logic, which has already
caused inconsistent handling of existing capacity and time_exist boundaries.
Refactor the shared pieces used by limit_growth_new_capacity_delta_indices,
limit_degrowth_new_capacity_delta_indices,
limit_growth_new_capacity_delta_constraint_rule, and
limit_degrowth_new_capacity_delta_constraint_rule into a common helper so the
previous-period capacity resolution is centralized. Move the first/second-period
lookup and adjusted existing-capacity handling into that shared helper, then
have limit_growth_new_capacity_delta delegate to it for both growth and degrowth
paths.
---
Duplicate comments:
In `@temoa/data_io/hybrid_loader.py`:
- Around line 697-764: The optional lifetime loaders are re-querying the
database and can still fail on missing tables even though the manifest marks
them optional. Update _load_lifetime_tech, _load_lifetime_process, and
_load_lifetime_survival_curve to use the already provided raw_data/filtered_data
inputs instead of calling cur.execute against the table names, and keep the
existing myopic/viable filtering logic applied to those in-memory rows. This
will let the optional-table path in the HybridLoader methods skip gracefully on
older schemas without raising sqlite3.OperationalError.
- Around line 659-764: The new loader methods are creating unnecessary throwaway
TemoaModel instances just to access component names. Update
_load_retired_existing_capacity, _load_lifetime_tech, _load_lifetime_process,
and _load_lifetime_survival_curve to use the existing self.model instead of
instantiating TemoaModel(), matching the extension-aware pattern already used
elsewhere like create_data_dict. Keep the existing query/filter/load logic
unchanged and just switch the model reference used for
model.retired_existing_capacity, model.lifetime_tech, model.lifetime_process,
and model.lifetime_survival_curve.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 632c2a54-fab4-40ad-b9a9-40c96a0c8b5c
📒 Files selected for processing (59)
.pre-commit-config.yamldocs/source/computational_implementation.rstdocs/source/extensions.rstdocs/source/extensions/growth_rates.rstdocs/source/index.rstdocs/source/mathematical_formulation.rstdocs/source/param_desc_and_tables.rsttemoa/_internal/run_actions.pytemoa/_internal/temoa_sequencer.pytemoa/cli.pytemoa/components/capacity.pytemoa/components/limits.pytemoa/components/technology.pytemoa/components/time.pytemoa/components/utils.pytemoa/core/config.pytemoa/core/model.pytemoa/data_io/component_manifest.pytemoa/data_io/hybrid_loader.pytemoa/data_io/loader_manifest.pytemoa/db_schema/temoa_schema_v4.sqltemoa/extensions/framework.pytemoa/extensions/growth_rates/__init__.pytemoa/extensions/growth_rates/components/__init__.pytemoa/extensions/growth_rates/components/growth_capacity.pytemoa/extensions/growth_rates/components/growth_new_capacity.pytemoa/extensions/growth_rates/components/growth_new_capacity_delta.pytemoa/extensions/growth_rates/core/__init__.pytemoa/extensions/growth_rates/core/model.pytemoa/extensions/growth_rates/data_manifest.pytemoa/extensions/growth_rates/extension.pytemoa/extensions/growth_rates/tables.sqltemoa/extensions/method_of_morris/morris.pytemoa/extensions/method_of_morris/morris_evaluate.pytemoa/extensions/modeling_to_generate_alternatives/mga_sequencer.pytemoa/extensions/myopic/myopic_sequencer.pytemoa/extensions/single_vector_mga/sv_mga_sequencer.pytemoa/extensions/stochastics/scenario_creator.pytemoa/extensions/template/__init__.pytemoa/extensions/template/components/__init__.pytemoa/extensions/template/components/example_limit.pytemoa/extensions/template/core/__init__.pytemoa/extensions/template/core/model.pytemoa/extensions/template/data_manifest.pytemoa/extensions/template/extension.pytemoa/extensions/template/tables.sqltemoa/tutorial_assets/config_sample.tomltemoa/tutorial_assets/utopia.sqltests/conftest.pytests/legacy_test_values.pytests/test_cli.pytests/test_full_runs.pytests/testing_configs/config_myopic_capacities.tomltests/testing_data/mediumville.sqltests/testing_data/mediumville_sets.jsontests/testing_data/myopic_capacities.sqltests/testing_data/test_system_sets.jsontests/testing_data/utopia_sets.jsontests/testing_data/utopia_v3.sql
💤 Files with no reviewable changes (4)
- temoa/db_schema/temoa_schema_v4.sql
- docs/source/mathematical_formulation.rst
- docs/source/param_desc_and_tables.rst
- tests/testing_data/mediumville.sql
idelder
force-pushed
the
extensions/framework
branch
from
6405a22 to
640771d
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
temoa/cli.py (1)
587-601: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOptional: extract the schema+data DB-build into a helper to remove duplication.
Lines 587-601 and the fallback at Lines 642-655 duplicate the full build sequence (connect → executescript schema → FK OFF → executescript data → FK ON →
foreign_key_check→ commit). A small helper keeps the FK-toggle/integrity logic in one place and avoids the two paths drifting.♻️ Sketch
def _build_tutorial_db(target_database: Path, schema_content: str, sql_content: str) -> None: with contextlib.closing(sqlite3.connect(target_database)) as conn: conn.executescript(schema_content) conn.execute('PRAGMA foreign_keys = OFF;') conn.executescript(sql_content) conn.execute('PRAGMA foreign_keys = ON;') fk_violations = conn.execute('PRAGMA foreign_key_check;').fetchall() if fk_violations: raise sqlite3.IntegrityError( f'Foreign key check failed after tutorial data load: {fk_violations[:5]}' ) conn.commit()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@temoa/cli.py` around lines 587 - 601, The tutorial DB build sequence is duplicated in the main path and the fallback, so extract the shared connect/executescript/foreign_keys toggle/foreign_key_check/commit flow into a helper such as _build_tutorial_db in temoa/cli.py. Update both call sites to use that helper with the existing schema_resource and sql_resource content so the integrity logic stays in one place and the two paths cannot drift.
♻️ Duplicate comments (2)
temoa/data_io/hybrid_loader.py (1)
697-764: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winOptional lifetime tables are queried unconditionally.
_load_lifetime_tech,_load_lifetime_process, and_load_lifetime_survival_curveissueSELECT ... FROM lifetime_*directly without atable_existsguard (contrast_load_retired_existing_capacityat Line 668). If these optional tables are absent in an older schema,cur.execute(...)raisessqlite3.OperationalErrorinstead of skipping cleanly. Reuse the already-fetchedraw_data/filtered_data, or guard withtable_exists, so absent optional tables are handled gracefully.#!/bin/bash # Confirm these components are declared optional in the manifest rg -nP -C3 "lifetime_tech|lifetime_process|lifetime_survival_curve" temoa/data_io/component_manifest.py🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@temoa/data_io/hybrid_loader.py` around lines 697 - 764, The optional lifetime loaders currently query lifetime_tech, lifetime_process, and lifetime_survival_curve directly, so missing tables can raise sqlite3.OperationalError. Update _load_lifetime_tech, _load_lifetime_process, and _load_lifetime_survival_curve in HybridLoader to first check table_exists (or otherwise reuse the already available raw_data/filtered_data presence) before executing SELECT statements, and skip loading when the table is absent. Keep the existing viability filtering and _load_component_data calls intact for the table-present path.temoa/extensions/growth_rates/components/growth_new_capacity_delta.py (1)
114-127: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
time_exist.prev()still crashes on a single-entry history.The
if model.time_exist:guard only rules out an empty set. Whentime_existhas exactly one element,p_prev = model.time_exist.last()is that sole element andmodel.time_exist.prev(p_prev)(Line 122) raises during constraint construction. Guard forlen(model.time_exist) >= 2before computingp_prev2, leavingnew_cap_prev2at its0.0baseline otherwise.🛡️ Proposed guard
if p == model.time_optimize.first(): if model.time_exist: p_prev = model.time_exist.last() new_cap_prev = quicksum( value(model.existing_capacity[_r, _t, _v]) for _r, _t, _v in model.existing_capacity.sparse_keys() if _r in regions and _t in techs and _v == p_prev ) - p_prev2 = model.time_exist.prev(p_prev) - new_cap_prev2 = quicksum( - value(model.existing_capacity[_r, _t, _v]) - for _r, _t, _v in model.existing_capacity.sparse_keys() - if _r in regions and _t in techs and _v == p_prev2 - ) + if len(model.time_exist) >= 2: + p_prev2 = model.time_exist.prev(p_prev) + new_cap_prev2 = quicksum( + value(model.existing_capacity[_r, _t, _v]) + for _r, _t, _v in model.existing_capacity.sparse_keys() + if _r in regions and _t in techs and _v == p_prev2 + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@temoa/extensions/growth_rates/components/growth_new_capacity_delta.py` around lines 114 - 127, In growth_new_capacity_delta’s constraint logic, the current model.time_exist check only covers non-empty history, so model.time_exist.prev(p_prev) can still fail when there is only one historical period. Update the branch around p == model.time_optimize.first() to require at least two entries in time_exist before computing p_prev2, and otherwise leave new_cap_prev2 at its existing 0.0 default; use the existing symbols model.time_exist, p_prev, p_prev2, and new_cap_prev2 to place the guard correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@temoa/cli.py`:
- Around line 587-601: The tutorial DB build sequence is duplicated in the main
path and the fallback, so extract the shared connect/executescript/foreign_keys
toggle/foreign_key_check/commit flow into a helper such as _build_tutorial_db in
temoa/cli.py. Update both call sites to use that helper with the existing
schema_resource and sql_resource content so the integrity logic stays in one
place and the two paths cannot drift.
---
Duplicate comments:
In `@temoa/data_io/hybrid_loader.py`:
- Around line 697-764: The optional lifetime loaders currently query
lifetime_tech, lifetime_process, and lifetime_survival_curve directly, so
missing tables can raise sqlite3.OperationalError. Update _load_lifetime_tech,
_load_lifetime_process, and _load_lifetime_survival_curve in HybridLoader to
first check table_exists (or otherwise reuse the already available
raw_data/filtered_data presence) before executing SELECT statements, and skip
loading when the table is absent. Keep the existing viability filtering and
_load_component_data calls intact for the table-present path.
In `@temoa/extensions/growth_rates/components/growth_new_capacity_delta.py`:
- Around line 114-127: In growth_new_capacity_delta’s constraint logic, the
current model.time_exist check only covers non-empty history, so
model.time_exist.prev(p_prev) can still fail when there is only one historical
period. Update the branch around p == model.time_optimize.first() to require at
least two entries in time_exist before computing p_prev2, and otherwise leave
new_cap_prev2 at its existing 0.0 default; use the existing symbols
model.time_exist, p_prev, p_prev2, and new_cap_prev2 to place the guard
correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 80c0f9b1-80df-4dfc-869b-6f70d0a889b6
📒 Files selected for processing (9)
temoa/cli.pytemoa/components/limits.pytemoa/components/utils.pytemoa/data_io/hybrid_loader.pytemoa/data_io/loader_manifest.pytemoa/extensions/growth_rates/components/growth_capacity.pytemoa/extensions/growth_rates/components/growth_new_capacity.pytemoa/extensions/growth_rates/components/growth_new_capacity_delta.pytemoa/extensions/stochastics/scenario_creator.py
idelder
force-pushed
the
extensions/framework
branch
from
640771d to
6dabfb1
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
temoa/data_io/hybrid_loader.py (1)
697-764: 🩺 Stability & Availability | 🟠 MajorOptional lifetime tables can still raise
OperationalErrorin custom loaders.
_load_lifetime_tech,_load_lifetime_process, and_load_lifetime_survival_curvere-querylifetime_tech/lifetime_process/lifetime_survival_curvedirectly and unconditionally. When these optional tables are absent (older schema),_fetch_data()safely returns[], but the custom loader still issues its ownSELECT, raisingsqlite3.OperationalError. Reuse the already-fetchedraw_data/filtered_data(applying the viability + myopic filters to those rows) so absent optional tables remain skipped.#!/bin/bash # Confirm these lifetime LoadItems are declared optional (is_table_required=False) rg -nP -C4 "lifetime_tech|lifetime_process|lifetime_survival_curve" temoa/data_io/component_manifest.py temoa/extensions/growth_rates/data_manifest.py🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@temoa/data_io/hybrid_loader.py` around lines 697 - 764, The optional lifetime loaders are re-querying the database directly, which can still throw OperationalError when the tables are missing in older schemas. Update _load_lifetime_tech, _load_lifetime_process, and _load_lifetime_survival_curve to consume the already supplied raw_data/filtered_data instead of executing new SELECTs on self.con, then apply the same viability and myopic filtering to those in-memory rows before passing them into _load_component_data. Keep the logic aligned with TemoaModel and the existing itemgetter-based filters so these optional components stay safely skipped when absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@temoa/data_io/hybrid_loader.py`:
- Around line 697-764: The optional lifetime loaders are re-querying the
database directly, which can still throw OperationalError when the tables are
missing in older schemas. Update _load_lifetime_tech, _load_lifetime_process,
and _load_lifetime_survival_curve to consume the already supplied
raw_data/filtered_data instead of executing new SELECTs on self.con, then apply
the same viability and myopic filtering to those in-memory rows before passing
them into _load_component_data. Keep the logic aligned with TemoaModel and the
existing itemgetter-based filters so these optional components stay safely
skipped when absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6e79c36a-1652-4e74-a2bf-a93b03b091d5
📒 Files selected for processing (9)
temoa/cli.pytemoa/components/limits.pytemoa/components/utils.pytemoa/data_io/hybrid_loader.pytemoa/data_io/loader_manifest.pytemoa/extensions/growth_rates/components/growth_capacity.pytemoa/extensions/growth_rates/components/growth_new_capacity.pytemoa/extensions/growth_rates/components/growth_new_capacity_delta.pytemoa/extensions/stochastics/scenario_creator.py
Collaborator
|
@idelder -- Thanks! This is a great idea, which can help reduce bloat in the core model. How do you envision tracking changes to the extensions over time? Do they accumulate in the extensions folder and get included with regular Temoa releases? |
Collaborator
Author
|
Yeah I was envisioning having them be tracked just like any other feature additions, except they don't bloat the database/core model. Realistically, building the extensions usually does require some minor changes to the core model code (refactoring things to be used by the extension, injecting one or two things into the model instantiation queue if the extension is active) but they should be designed to have absolutely zero impact on core runtime if inactive. |
Collaborator
Author
|
Hm wasn't expecting any merge conflicts. I'll rebase locally and repush |
Signed-off-by: Davey Elder <iandavidelder@gmail.com>
Signed-off-by: Davey Elder <iandavidelder@gmail.com>
Signed-off-by: Davey Elder <iandavidelder@gmail.com>
Signed-off-by: Davey Elder <iandavidelder@gmail.com>
idelder
force-pushed
the
extensions/framework
branch
from
6dabfb1 to
1fbc8c8
Compare
Collaborator
Author
|
Recording this for next time 
|
Collaborator
|
@idelder Yes looked like there were some direct conflicts that needed to get sorted during the rebase. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
temoa/data_io/component_manifest.py (1)
26-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocstring doesn't mention the new
extension_idsparameter.Consider documenting
extension_idsin the docstringArgssection for API clarity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@temoa/data_io/component_manifest.py` around lines 26 - 39, The docstring for build_manifest does not document the extension_ids parameter. Add an Args entry describing extension_ids, including its purpose and the behavior when it is None, while preserving the existing documentation.
♻️ Duplicate comments (8)
temoa/extensions/template/tables.sql (1)
6-14: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMake composite key columns
NOT NULL.SQLite allows
NULLin compositePRIMARY KEYcolumns, so malformed(region, tech_or_group)rows can slip into this scaffold. This was previously raised and remains unaddressed.🛡️ Proposed fix
CREATE TABLE IF NOT EXISTS example_new_capacity_limit ( - region TEXT, - tech_or_group TEXT, + region TEXT NOT NULL, + tech_or_group TEXT NOT NULL, value REAL NOT NULL DEFAULT 0, units TEXT, notes TEXT, PRIMARY KEY (region, tech_or_group) );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@temoa/extensions/template/tables.sql` around lines 6 - 14, Update the example_new_capacity_limit table definition so both composite primary-key columns, region and tech_or_group, are explicitly declared NOT NULL before the PRIMARY KEY constraint.tests/testing_configs/config_myopic_capacities.toml (1)
4-5: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winInput and output database still point to the same file.
MyopicSequencerwrites in place wheninput_databaseandoutput_databasetarget the same SQLite file, so parametrized runs will mutate the shared fixture and leak state into later test cases. This was flagged in a previous review and remains unresolved.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/testing_configs/config_myopic_capacities.toml` around lines 4 - 5, Update config_myopic_capacities.toml so input_database and output_database reference distinct SQLite files, preventing MyopicSequencer runs from mutating the shared input fixture and leaking state across parametrized tests.temoa/tutorial_assets/config_sample.toml (1)
21-26: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winExample still references a non-existent extension.
"unit_commitment"is not a registered extension — onlygrowth_ratesis wired intoget_known_extension_specs(). Copying this example verbatim will raiseUnknown extension id(s). This was flagged in a previous review and remains unresolved.📝 Proposed fix
-# Example: extensions = ["unit_commitment", "growth_rates"] +# Example: extensions = ["growth_rates"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@temoa/tutorial_assets/config_sample.toml` around lines 21 - 26, Update the extensions example comment near the extensions configuration to remove the unregistered “unit_commitment” identifier and reference only the supported “growth_rates” extension, matching get_known_extension_specs().tests/conftest.py (1)
61-67: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winStill missing
PRAGMA foreign_key_checkafter extension fixture loading.
PRAGMA foreign_keys = ONat Line 79 does not validate rows inserted while enforcement was off, so a bad row in any extension-owned table will still produce a "successful" test DB. This was flagged in a previous review and remains unresolved.Suggested fix
# 4. Turn foreign keys back on con.execute('PRAGMA foreign_keys = ON') + fk_violations = con.execute('PRAGMA foreign_key_check').fetchall() + if fk_violations: + raise sqlite3.IntegrityError( + f'Foreign key check failed after test DB load: {fk_violations[:5]}' + ) con.commit()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/conftest.py` around lines 61 - 67, Add a `PRAGMA foreign_key_check` validation immediately after the extension schemas and fixture data are loaded in the test database setup, using the connection variable `con`; ensure violations raise or fail setup rather than allowing an invalid database to proceed.temoa/extensions/framework.py (1)
186-195: 🔒 Security & Privacy | 🟡 Minor | 💤 Low valueQuote the interpolated identifier in
_table_has_rows.
table_nameis trusted today (sourced fromspec.owned_tables), but the f-string interpolation is fragile if extension metadata ever becomes less trusted. Quote the identifier defensively; the precedingsqlite_mastercheck already validates existence.♻️ Suggested hardening
- query = f'SELECT 1 FROM main.{table_name} LIMIT 1' + query = f'SELECT 1 FROM main."{table_name}" LIMIT 1' return cur.execute(query).fetchone() is not None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@temoa/extensions/framework.py` around lines 186 - 195, In _table_has_rows, defensively quote the interpolated table_name identifier in the SELECT query, using SQLite identifier-quoting with proper escaping, while preserving the existing sqlite_master existence check and row-existence behavior.Source: Linters/SAST tools
temoa/data_io/hybrid_loader.py (2)
659-764: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueReuse
self.modelinstead of instantiating throwawayTemoaModel()instances.
_load_retired_existing_capacity,_load_lifetime_tech,_load_lifetime_process, and_load_lifetime_survival_curveeach build a freshTemoaModel()(Lines 675, 704, 720, 746) purely to access a component's.name.self.modelis already available, extension-aware, and used elsewhere in this file (e.g.create_data_dict,_load_regional_global_indices).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@temoa/data_io/hybrid_loader.py` around lines 659 - 764, Replace the throwaway TemoaModel() instances in _load_retired_existing_capacity, _load_lifetime_tech, _load_lifetime_process, and _load_lifetime_survival_curve with self.model, using the corresponding component attributes when calling _load_component_data. Preserve the existing query and filtering behavior while relying on the extension-aware model already attached to the loader.
697-764: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCustom lifetime loaders bypass the optional-table safety net.
These loaders are marked
is_table_required=Falsein the manifest, but re-querylifetime_tech/lifetime_process/lifetime_survival_curvedirectly rather than reusing the already-fetchedraw_data, so a database missing one of these optional tables will raisesqlite3.OperationalErrorinstead of being skipped gracefully (as_fetch_dataintends).🛡️ Proposed fix: reuse fetched rows instead of re-querying
def _load_lifetime_tech( self, data: dict[str, object], raw_data: Sequence[tuple[object, ...]], filtered_data: Sequence[tuple[object, ...]], ) -> None: """Loads the lifetime_tech component.""" model = self.model - cur = self.con.cursor() - rows_to_load = cur.execute('SELECT region, tech, lifetime FROM lifetime_tech').fetchall() + rows_to_load = list(raw_data) rt_getter = itemgetter(0, 1) if self.viable_rt: valid_rt = self.viable_rt.members | self.viable_existing_rt rows_to_load = [item for item in rows_to_load if rt_getter(item) in valid_rt] self._load_component_data(data, model.lifetime_tech, rows_to_load)Apply the analogous change (filtering
raw_data/filtered_databymi.last_demand_yearin Python) to_load_lifetime_processand_load_lifetime_survival_curve.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@temoa/data_io/hybrid_loader.py` around lines 697 - 764, Update _load_lifetime_tech, _load_lifetime_process, and _load_lifetime_survival_curve to stop querying SQLite directly and instead use the rows supplied through raw_data or filtered_data by _fetch_data. Preserve the existing viable_rt/viable_rtv filtering, and apply the myopic_index last_demand_year cutoff in Python for process and survival-curve rows so missing optional tables are skipped safely.Source: Linters/SAST tools
temoa/data_io/loader_manifest.py (1)
35-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocstring could still clarify the single-trailing-value case.
The underlying data-corruption bug this comment referenced was fixed in
hybrid_loader.py(reshape only triggers when there are 2+ trailing columns), so this is now purely a documentation clarity nit rather than a correctness issue.Based on a previous review comment on this file recommending the docstring "state that
index_lengthmust leave more than one trailing value column."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@temoa/data_io/loader_manifest.py` around lines 35 - 36, Update the `index_length` parameter documentation in the loader manifest docstring to explicitly state that it must leave more than one trailing value column, clarifying that a single trailing value column does not require this setting. Preserve the existing explanation about index columns and multi-value-column tables.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/source/extensions/growth_rates.rst`:
- Around line 95-99: Update the growth-rates documentation to reference every
registered constraint rule using its actual `*_constraint_rule` symbol from
`temoa.extensions.growth_rates.core.model`; replace the three incorrect
non-degrowth autodoc targets and add entries for all three corresponding
degrowth constraints, covering all six registered rules.
In `@temoa/extensions/framework.py`:
- Around line 159-167: Guard the interactive prompt in the missing-table
handling logic so non-interactive execution cannot crash. In the block
controlled by `if not silent` within the relevant extension validation function,
either check `sys.stdin.isatty()` before calling `input()` or catch `EOFError`;
on non-interactive input or EOF, leave `should_apply` as `False` so the existing
missing-tables `RuntimeError` path executes.
In `@temoa/extensions/growth_rates/components/growth_new_capacity_delta.py`:
- Around line 78-88: Update the constraint docstring in the growth
capacity-delta component so its Θ set references use the actual parameter names:
replace the growth set with Θ_limit_growth_new_capacity_delta and the degrowth
set with Θ_limit_degrowth_new_capacity_delta, preserving the surrounding
equations.
In `@temoa/extensions/growth_rates/components/growth_new_capacity.py`:
- Around line 76-84: Update the mathematical docstring in the growth-capacity
constraint documentation to replace the incorrect set names
limit_growth_capacity and limit_degrowth_capacity with the actual parameter set
names limit_growth_new_capacity and limit_degrowth_new_capacity, without
changing the constraint logic.
---
Outside diff comments:
In `@temoa/data_io/component_manifest.py`:
- Around line 26-39: The docstring for build_manifest does not document the
extension_ids parameter. Add an Args entry describing extension_ids, including
its purpose and the behavior when it is None, while preserving the existing
documentation.
---
Duplicate comments:
In `@temoa/data_io/hybrid_loader.py`:
- Around line 659-764: Replace the throwaway TemoaModel() instances in
_load_retired_existing_capacity, _load_lifetime_tech, _load_lifetime_process,
and _load_lifetime_survival_curve with self.model, using the corresponding
component attributes when calling _load_component_data. Preserve the existing
query and filtering behavior while relying on the extension-aware model already
attached to the loader.
- Around line 697-764: Update _load_lifetime_tech, _load_lifetime_process, and
_load_lifetime_survival_curve to stop querying SQLite directly and instead use
the rows supplied through raw_data or filtered_data by _fetch_data. Preserve the
existing viable_rt/viable_rtv filtering, and apply the myopic_index
last_demand_year cutoff in Python for process and survival-curve rows so missing
optional tables are skipped safely.
In `@temoa/data_io/loader_manifest.py`:
- Around line 35-36: Update the `index_length` parameter documentation in the
loader manifest docstring to explicitly state that it must leave more than one
trailing value column, clarifying that a single trailing value column does not
require this setting. Preserve the existing explanation about index columns and
multi-value-column tables.
In `@temoa/extensions/framework.py`:
- Around line 186-195: In _table_has_rows, defensively quote the interpolated
table_name identifier in the SELECT query, using SQLite identifier-quoting with
proper escaping, while preserving the existing sqlite_master existence check and
row-existence behavior.
In `@temoa/extensions/template/tables.sql`:
- Around line 6-14: Update the example_new_capacity_limit table definition so
both composite primary-key columns, region and tech_or_group, are explicitly
declared NOT NULL before the PRIMARY KEY constraint.
In `@temoa/tutorial_assets/config_sample.toml`:
- Around line 21-26: Update the extensions example comment near the extensions
configuration to remove the unregistered “unit_commitment” identifier and
reference only the supported “growth_rates” extension, matching
get_known_extension_specs().
In `@tests/conftest.py`:
- Around line 61-67: Add a `PRAGMA foreign_key_check` validation immediately
after the extension schemas and fixture data are loaded in the test database
setup, using the connection variable `con`; ensure violations raise or fail
setup rather than allowing an invalid database to proceed.
In `@tests/testing_configs/config_myopic_capacities.toml`:
- Around line 4-5: Update config_myopic_capacities.toml so input_database and
output_database reference distinct SQLite files, preventing MyopicSequencer runs
from mutating the shared input fixture and leaking state across parametrized
tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 08b439ca-57b0-47d4-84f3-df4bf45d4394
📒 Files selected for processing (51)
docs/source/computational_implementation.rstdocs/source/extensions.rstdocs/source/extensions/growth_rates.rstdocs/source/index.rstdocs/source/mathematical_formulation.rstdocs/source/param_desc_and_tables.rsttemoa/_internal/run_actions.pytemoa/_internal/temoa_sequencer.pytemoa/cli.pytemoa/components/limits.pytemoa/components/utils.pytemoa/core/config.pytemoa/core/model.pytemoa/data_io/component_manifest.pytemoa/data_io/hybrid_loader.pytemoa/data_io/loader_manifest.pytemoa/db_schema/temoa_schema_v4.sqltemoa/extensions/framework.pytemoa/extensions/growth_rates/__init__.pytemoa/extensions/growth_rates/components/__init__.pytemoa/extensions/growth_rates/components/growth_capacity.pytemoa/extensions/growth_rates/components/growth_new_capacity.pytemoa/extensions/growth_rates/components/growth_new_capacity_delta.pytemoa/extensions/growth_rates/core/__init__.pytemoa/extensions/growth_rates/core/model.pytemoa/extensions/growth_rates/data_manifest.pytemoa/extensions/growth_rates/extension.pytemoa/extensions/growth_rates/tables.sqltemoa/extensions/method_of_morris/morris.pytemoa/extensions/method_of_morris/morris_evaluate.pytemoa/extensions/modeling_to_generate_alternatives/mga_sequencer.pytemoa/extensions/myopic/myopic_sequencer.pytemoa/extensions/single_vector_mga/sv_mga_sequencer.pytemoa/extensions/stochastics/scenario_creator.pytemoa/extensions/template/__init__.pytemoa/extensions/template/components/__init__.pytemoa/extensions/template/components/example_limit.pytemoa/extensions/template/core/__init__.pytemoa/extensions/template/core/model.pytemoa/extensions/template/data_manifest.pytemoa/extensions/template/extension.pytemoa/extensions/template/tables.sqltemoa/tutorial_assets/config_sample.tomltemoa/tutorial_assets/utopia.sqltests/conftest.pytests/test_cli.pytests/testing_configs/config_myopic_capacities.tomltests/testing_data/mediumville_sets.jsontests/testing_data/test_system_sets.jsontests/testing_data/utopia_sets.jsontests/testing_data/utopia_v3.sql
💤 Files with no reviewable changes (3)
- temoa/db_schema/temoa_schema_v4.sql
- docs/source/mathematical_formulation.rst
- docs/source/param_desc_and_tables.rst
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
NOTE: Currently based on top of PR #340. These changes begin with commit
Move schema components out of tutorial database for SSOTCreates a framework so that new model extensions can be developed that append components onto Temoa including new parameters, variables, database tables, and constraints. These extensions are take-or-leave, where Temoa runs as normal without them but they can be easily included in the config file to add new capabilities to the model. Documentation has been added to explain how this all works, which I'll just include below. I have also moved the growth rate constraints to an extension because they were a bit bulky in the database and not typically used in base models.
.. _extensions:
Extension Framework
Temoa includes a lightweight framework for adding optional model components
without modifying the core model. An extension can contribute its own database
tables, Pyomo components (sets, parameters, and constraints), and data-loading
rules. Extensions are declared once and then enabled per run through
configuration.
This page describes how the framework works and how to author a new extension.
A ready-to-copy scaffold lives at
temoa/extensions/template.Overview
Use an extension when you want to add modeling capability that is:
If a feature is fundamental to every Temoa run, it belongs in
temoa/components(a core component) instead of an extension.Each extension is described by a single :class:
ExtensionSpec(declarativemetadata plus hook functions). The spec is registered with the framework, after
which users can enable the extension by id.
Lifecycle
When a run is configured with
extensions = ["..."], the framework threads theenabled specs through configuration, model construction, and data loading:
.. code-block:: text
config: extensions = ["my_ext"]
|
v
resolve_extension_specs() validate ids -> ExtensionSpec list
|
v
TemoaModel(extensions=[...])
|
+--> apply_model_extension_hooks()
| calls spec.register_model_components(model)
| -> attaches Params / Sets / Constraints to the model
|
v
HybridLoader
+--> ensure_enabled_extension_tables_exist() (offer to append schema)
+--> assert_disabled_extension_tables_are_empty()
+--> merge_regional_group_tables()
+--> build_manifest() -> appends spec.build_manifest_items(model)
-> loads each owned table into its component
The relevant code lives in :mod:
temoa.extensions.framework,:mod:
temoa.core.model, and :mod:temoa.data_io.hybrid_loader.ExtensionSpecreference.. list-table::
:header-rows: 1
:widths: 30 70
extension_idextensions = [...].owned_tablesdisabled/enabled table guards.
regional_group_tablestable -> columnfor tables whose region column may hold aregional group name. Merged into the loader's regional-group handling.
register_model_componentsCallable[[TemoaModel], None]that attaches model components.build_manifest_itemsCallable[[TemoaModel], list[LoadItem]]describing how to load theextension's data.
schema_sql_path.sqlfile applied (with consent) when the extension isenabled but its tables are missing.
fail_if_tables_populated_when_disabledTrue, loading fails if the extension is disabled but its ownedtables contain data, preventing silently-ignored inputs.
Recommended package layout
Mirror the structure of the core model (
temoa/core+temoa/components) soextension code is organized the same way as the rest of the codebase:
.. code-block:: text
temoa/extensions/<your_extension>/
init.py # re-export the ExtensionSpec
extension.py # the ExtensionSpec definition
data_manifest.py # build_manifest_items()
tables.sql # CREATE TABLE IF NOT EXISTS for owned tables
core/
init.py
model.py # typing subtype + register_model_components()
components/
init.py
.py # one module per constraint family
Centralize the component declarations in
core/model.py(just astemoa/core/model.pydoes) and keep the index-set and constraint-rule logic incomponents/modules (just astemoa/componentsdoes)... _extensions-typing:
The typing pattern
Core model components are declared as attribute assignments inside
TemoaModel.__init__(for exampleself.time_optimize = Set(...)), which iswhy the type checker knows about them. An extension instead adds attributes from
outside the class, so without help the type checker reports
"TemoaModel" has no attribute ...and provides no autocomplete.Three rules make typing carry over cleanly:
Declare a
TYPE_CHECKING-only subtype. Incore/model.py, define asubclass of
TemoaModelthat annotates every component the extension adds.It inherits all core attributes, so component code sees both core and
extension members.
.. code-block:: python
if TYPE_CHECKING:
from temoa.core.model import TemoaModel
Annotate component functions with the subtype. Index-set and
constraint-rule functions take
model: ExampleModel. This restores bothmypy coverage and editor autocomplete.
Keep spec hooks on the base type and
castinternally. Functions storedon the
ExtensionSpec(register_model_componentsandbuild_manifest_items) must keepmodel: TemoaModelto match the hookcallable types. Narrowing the parameter is a contravariance error.
castonce at the top:
.. code-block:: python
def register_model_components(model: TemoaModel) -> None:
m = cast('ExampleModel', model)
m.example_new_capacity_limit = Param(...)
.. note::
Extension attribute names share the single
TemoaModelnamespace atruntime. Keep them unique across extensions to avoid collisions.
Adding a new extension
#. Copy the template. Duplicate
temoa/extensions/templatetotemoa/extensions/<your_extension>/.#. Rename ids and tables. Update
extension_id,owned_tables,regional_group_tables, params, sets, and constraints to your domain.#. Declare components. Add annotations to the typing subtype in
core/model.pyand create the components inregister_model_components.#. Write the constraint logic. Add index-set and rule functions under
components/and annotate them with your subtype.#. Describe data loading. Add one
LoadItemper owned table indata_manifest.py.#. Define the schema. Add a
CREATE TABLE IF NOT EXISTSper owned table totables.sql.#. Register the spec. Import your spec in
:func:
temoa.extensions.framework.get_known_extension_specsand add it to thespecslist. (Until you do this, the extension is inert -- enabling itraises an "Unknown extension id" error.)
#. Enable it. Add the id to your configuration TOML:
.. code-block:: toml
Data loading and
LoadItembuild_manifest_itemsreturns one :class:temoa.data_io.loader_manifest.LoadItemper database table the extension reads. Common fields:
.. list-table::
:header-rows: 1
:widths: 30 70
componentSetorParamto populate.tablecolumnsParamthe final column is the value.index_lengthvalidator_name/validation_mapcolumns it applies to.
is_table_requiredFalsefor optional extension inputs so a missing table is not anerror.
Verification
After authoring an extension, confirm:
#. Types --
mypy temoa/extensions/<your_extension>reports no issues.#. Imports --
python -c "import temoa.extensions.<your_extension>.extension".#. Wiring -- a model built with the extension enabled attaches the expected
components, and the test suite passes.
The template extension
temoa/extensions/templateis a complete, type-checked, but deliberatelyunregistered scaffold. Because it is not listed in
get_known_extension_specs, it cannot be enabled until you register it, so itnever affects normal runs. Copy the folder as the starting point for a new
extension; every file carries
# TEMPLATE:comments explaining what to change... _extension-catalog:
Available extensions
The extensions that ship with Temoa are documented on their own pages below.
Each page describes the extension's parameters and constraints. This list grows
as new extensions are added.
.. toctree::
:maxdepth: 1
extensions/growth_rates
Summary by CodeRabbit
New Features
growth_ratesextension, providing configurable capacity and new-capacity growth, degrowth, and acceleration limits.Changes