typing: adding types to extensions outside of breakeven#251
typing: adding types to extensions outside of breakeven#251ParticularlyPythonicBS wants to merge 1 commit intounstablefrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds comprehensive type hints, TYPE_CHECKING guards, and future annotations across many temoa extensions; tightens function signatures and public attributes, renames variables to snake_case, fixes a mypy exclude pattern, and makes minor runtime-safety adjustments without changing core algorithms. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
temoa/extensions/single_vector_mga/sv_mga_sequencer.py (1)
203-209: Theverboseparameter is declared but never used.Static analysis correctly flags that
verbose(line 208) is unused in the function body (ARG004). The docstring references it at line 215, but there's no actual implementation using it.Either remove the unused parameter or implement the intended verbose behavior (e.g., conditional print/log statements).
🔧 Option 1: Remove unused parameter
@staticmethod def construct_obj( model: TemoaModel, emission_labels: Iterable[str], capacity_labels: Iterable[str], activity_labels: Iterable[str], - verbose: bool = True, ) -> Expression | int: """ Construct an alternative OBJ statement from the config data Specifically, locate the labels passed in within the related variables and kluge together an objective to be minimized from them. - :param verbose: If True, report to console during construction... :param M: The basis model to search🔧 Option 2: Use the parameter (if verbose output is intended)
# handle emissions... for label in emission_labels: idxs = [idx for idx in model.emission_activity if idx[1] == label] logger.debug('Located %d items for emission label: %s', len(idxs), label) + if verbose: + print(f'Located {len(idxs)} items for emission label: {label}')Additionally, if you keep the parameter, consider making it keyword-only to address the boolean positional argument warnings (FBT001/FBT002):
- verbose: bool = True, + *, + verbose: bool = True,temoa/extensions/stochastics/scenario_creator.py (1)
21-33: Consider narrowing the return type toTemoaModel.The static analysis flags
Anyusage (ANN401). WhileAnyin**kwargsis acceptable given mpi-sppy's untyped callback interface, the return type could potentially beTemoaModelsincebuild_instanceexplicitly returns that type. However, sinceattach_root_nodemutates the instance with mpi-sppy-specific attributes, keepingAnyis also a defensible choice.♻️ Optional: Narrow return type
-def scenario_creator(scenario_name: str, **kwargs: Any) -> Any: +def scenario_creator(scenario_name: str, **kwargs: Any) -> TemoaModel:This would require adding
TemoaModelto the TYPE_CHECKING imports:if TYPE_CHECKING: from temoa.core.config import TemoaConfig from temoa.data_io.hybrid_loader import LoadItem from temoa.extensions.stochastics.stochastic_config import StochasticConfig from temoa.temoa_model.temoa_model import TemoaModeltemoa/extensions/myopic/myopic_progress_mapper.py (1)
52-83: Consider usingLiteralfor thestatusparameter.The function validates that
statusmust be one of{'load', 'solve', 'report', 'check'}. Using aLiteraltype would catch invalid values at static analysis time rather than runtime.♻️ Optional improvement
+from typing import Literal + +StatusType = Literal['load', 'solve', 'report', 'check'] + class MyopicProgressMapper: ... - def report(self, mi: MyopicIndex, status: str) -> None: + def report(self, mi: MyopicIndex, status: StatusType) -> None:temoa/extensions/get_comm_tech.py (1)
12-44: SQL injection vulnerability inget_tperiods.Line 35 interpolates
y(derived from database content) directly into the SQL query using an f-string. Whileyoriginates from prior query results (not direct user input), this pattern is fragile and could be exploited if the data source is compromised.🔒 Proposed fix using parameterized query
for y in x: cur.execute( - f"SELECT DISTINCT period FROM output_flow_out WHERE scenario is '{y}'" + 'SELECT DISTINCT period FROM output_flow_out WHERE scenario = ?', + (y,) )temoa/extensions/modeling_to_generate_alternatives/worker.py (1)
35-59: Add return type annotation to__init__.Per Ruff ANN204, the
__init__method should have a-> Nonereturn type annotation for completeness with the typing pass.Proposed fix
def __init__( self, model_queue: Queue[Any], results_queue: Queue[Any], log_root_name: str, log_queue: Queue[logging.LogRecord], log_level: int = logging.INFO, solver_name: str = 'appsi_highs', solver_options: dict[str, Any] | None = None, solver_log_path: Path | None = None, - ): + ) -> None:temoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.py (1)
298-302: Fix incorrect string formatting in RuntimeError.
RuntimeErrordoesn't support%-style formatting in the constructor. Thevar_namewon't be interpolated into the message.Proposed fix
if not isinstance(var, Var): raise RuntimeError( - 'Failed to retrieve a named variable from the model: %s', var_name + f'Failed to retrieve a named variable from the model: {var_name}' )temoa/extensions/method_of_morris/morris.py (1)
40-49: Function relies on module-level globalsconfiganddb_file.The
evaluatefunction referencesconfig(line 42, 46) anddb_file(line 49) which are defined later in the module during script execution. This works because the function is only called after these globals are initialized, but it makes the function harder to test and reuse. Consider passing these as parameters, similar to howmorris_evaluate.pyhandles this.Note: The related
morris_evaluate.pyfile passesconfigas a parameter toevaluate, which is a cleaner pattern.temoa/extensions/monte_carlo/mc_worker.py (1)
46-67: Add return type annotation-> Nonefor__init__.Per static analysis hint (ANN204), special methods like
__init__should have explicit return type annotations for consistency with the typing effort in this PR.Suggested fix
def __init__( self, dp_queue: Queue[Any], results_queue: Queue[DataBrick | str], log_root_name: str, log_queue: Queue[logging.LogRecord], log_level: int = logging.INFO, solver_log_path: Path | None = None, **kwargs: Any, - ): + ) -> None:temoa/extensions/monte_carlo/mc_sequencer.py (1)
55-108: Add return type annotation-> Nonefor__init__.Per static analysis hint (ANN204),
__init__should have an explicit return type annotation.Suggested fix
- def __init__(self, config: TemoaConfig): + def __init__(self, config: TemoaConfig) -> None:temoa/extensions/monte_carlo/mc_run.py (1)
43-54: Add return type annotation-> Nonefor__init__methods.The
__init__methods forTweak,TweakFactory,MCRun, andMCRunFactoryclasses are all missing the-> Nonereturn type annotation. For consistency with the typing effort in this PR, consider adding them.Suggested fixes for all __init__ methods
# Tweak.__init__ (line 43) - def __init__(self, param_name: str, indices: tuple[Any, ...], adjustment: str, value: float): + def __init__(self, param_name: str, indices: tuple[Any, ...], adjustment: str, value: float) -> None: # TweakFactory.__init__ (line 70) - def __init__(self, data_store: dict[str, Any]): + def __init__(self, data_store: dict[str, Any]) -> None: # MCRun.__init__ (line 175) def __init__( self, scenario_name: str, run_index: int, data_store: dict[str, Any], included_tweaks: dict[Tweak, list[ChangeRecord]], - ): + ) -> None: # MCRunFactory.__init__ (line 224) - def __init__(self, config: TemoaConfig, data_store: dict[str, Any]): + def __init__(self, config: TemoaConfig, data_store: dict[str, Any]) -> None:
🤖 Fix all issues with AI agents
In @temoa/extensions/method_of_morris/morris_evaluate.py:
- Around line 65-67: The format string stored in setting_entry is never
interpolated before being appended to log_entry, so the log prints the raw
"%d/%s/%f" placeholders; update the code where setting_entry is appended to
instead append a formatted string (e.g., use setting_entry % (run_number,
param_name, param_index, param_value) or construct an f-string) and ensure the
variables used match the expected types, then call logger.debug on the joined
log_entry as before.
In @temoa/extensions/method_of_morris/morris_sequencer.py:
- Around line 88-96: The cast('Any', pert) followed by float() is unnecessary
and unconventional; update the assignment for self.mm_perturbation to
convert/match the expected numeric type directly (e.g., self.mm_perturbation =
float(pert) if you trust morris_inputs, or use self.mm_perturbation =
float(cast(float, pert)) to keep an explicit type cast) when reading pert from
morris_inputs.get('perturbation'), leaving the default fallback logic and logger
calls unchanged.
In @temoa/extensions/method_of_morris/morris.py:
- Around line 24-25: The evaluate function has an unused parameter k; rename it
to _k in the function signature (def evaluate(param_names: dict[int, list[Any]],
param_values: Any, data: dict[str, Any], _k: int) -> list[Any]) to follow Python
convention for intentionally unused parameters and update any internal
references to use _k if present.
- Line 55: SQL queries use the wrong table name `output_emissionn`; update the
SQL in the cur.execute calls to use the correct table `output_emission` (e.g.,
replace the query string in the cur.execute(...) call found in morris.py and the
analogous cur.execute in clear_db_outputs.py) so the database selects run
against the actual table name.
In @temoa/extensions/modeling_to_generate_alternatives/hull.py:
- Line 28: The __init__ method in class Hull declares an unused **kwargs
parameter; either remove it if not needed or rename it to **_kwargs to signal
intentional non-use and silence the Ruff ARG002 warning (update the signature of
Hull.__init__ accordingly and adjust any callers if you remove it).
- Around line 99-107: In the except Exception as e block that currently logs the
failure and calls logger.error(e) before raising a RuntimeError, replace the
second logger.error(e) with logger.exception(...) (or combine into a single
logger.exception call) so the traceback is included in the logs; keep the
existing descriptive message about hull construction failure and re-raise the
RuntimeError('Hull construction from vectors failed. See log file') from e.
In @temoa/extensions/modeling_to_generate_alternatives/mga_sequencer.py:
- Around line 127-133: The code uses cast('Any', ...) which defeats type
checking; replace these with casts to the actual target types or remove casts
and rely on direct conversion: for example, change cast('Any',
all_options.get('num_workers', 1)) to either cast(int,
all_options.get('num_workers', 1)) or simply int(all_options.get('num_workers',
1)), and do the same for iteration_limit (int), time_limit_hrs (float), and
cost_epsilon (float) when reading from mga_inputs; alternatively, annotate
mga_inputs/all_options with a TypedDict or a typed config class so you can avoid
casts altogether and use strongly typed access for num_workers, iteration_limit,
time_limit_hrs, and cost_epsilon.
In
@temoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.py:
- Around line 147-148: The method group_variable_names currently ignores its
tech parameter and returns all keys from category_mapping; update it to filter
and return only the variable names for the specified technology (use the tech
argument to look up the appropriate entry in self.category_mapping, e.g.,
self.category_mapping[tech] or iterate items and select keys whose value matches
tech), and return their string names; if the intended behavior is to
deliberately ignore tech, rename the parameter to _tech and keep current
behavior while adding a clarifying comment.
In @temoa/extensions/monte_carlo/example_builds/scenario_analyzer.py:
- Line 4: The importlib.resources call using
resources.files('data_files.example_dbs') fails because data_files and its
subdirectory are not importable packages and the code also references a
non-existent file 'utopia.sqlite'; either make data_files and
data_files/example_dbs into proper packages by adding __init__.py files so
resources.files('data_files.example_dbs') can locate assets, or move the example
DB files into the temoa package (and update pyproject.toml) and change the
resources.files argument to the new package name; alternatively, replace
resources.files(...) with a filesystem-relative loader (using
Path(__file__).parent / 'data_files' / 'example_dbs' / 'utopia.sql' or similar)
and correct the filename to 'utopia.sql' where the code currently expects
'utopia.sqlite'.
In @temoa/extensions/monte_carlo/mc_run.py:
- Around line 224-228: In MCRunFactory.__init__, validate
self.config.monte_carlo_inputs before using it: check that
self.config.monte_carlo_inputs is not None and contains the 'run_settings' key
(mirroring the MCSequencer.__init__ pattern) and raise a clear ValueError or
TypeError if missing; only then set self.settings_file =
Path(self.config.monte_carlo_inputs['run_settings']) and remove the # type:
ignore. Ensure the check references monte_carlo_inputs and the run_settings key
and updates the constructor to fail fast with a descriptive error message when
inputs are absent.
In @temoa/extensions/myopic/myopic_sequencer.py:
- Around line 570-576: The DELETE uses f-string interpolation of the table
identifier in myopic_sequencer.py (loop over self.tables_with_period) which
risks SQL injection; instead validate the table variable against a strict
allowlist (e.g., confirm table in self.tables_with_period or a dedicated
constant) before constructing the query and raise/skip if not allowed, then
build the SQL using the validated table name only; keep parameterized
placeholders for period and scenario (execute(sql, (period, scenario_name))) so
only the identifier is validated while values remain parameterized.
In @temoa/extensions/single_vector_mga/output_summary.py:
- Around line 38-50: The delta calculation in the Emission loop uses an
unnecessary float() wrapper for row_delta while the Activity and Capacity loops
compute row_delta without it; remove the float(...) around ((option - orig) /
orig * 100) in the Emission block so row_delta is computed consistently (as in
the Activity and Capacity loops) when all((orig, option)) is truthy; update the
expression where row_delta is set in the Emission loop (the line that currently
reads row_delta = float((option - orig) / orig * 100) if all((orig, option))
else None) to match the others.
- Around line 31-52: The code incorrectly treats zero values as missing by using
all((orig, option)); update the polling functions (poll_emission, poll_activity,
poll_capacity) to return None for missing data (float | None) and change the
checks in summarize to explicit None checks (e.g., if orig is not None and
option is not None) before computing row_delta; ensure you compute row_delta as
((option - orig) / orig * 100) with a float conversion where needed and leave
row_delta as None when either value is None, then append records as before.
- Around line 31-47: The cast('Iterable[Any]', ...) calls around
emission_labels, activity_labels, and capacity_labels are unnecessary and
verbose; remove the casts in the three sorted(...) iterations (the lines that
call sorted(cast('Iterable[Any]', emission_labels)) and similar for
activity_labels and capacity_labels) and simply call sorted(emission_labels),
sorted(activity_labels), and sorted(capacity_labels) instead, or alternatively
tighten the type of svmga_inputs so those variables are already typed as
Sequence/Iterable[str] to avoid needing casts; keep the existing logic using
poll_emission and poll_activity unchanged.
In @temoa/extensions/stochastics/scenario_creator.py:
- Around line 43-50: The loop contains a standalone type annotation "item:
LoadItem" before the for loop which is unconventional; remove that line and
either rely on the type of hybrid_loader.manifest so the loop variable item is
inferred (for item in hybrid_loader.manifest) or, if necessary, annotate the
manifest's type so item is typed implicitly; update the code around
table_index_map and the for loop to drop the separate "item: LoadItem"
declaration.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (23)
pyproject.tomltemoa/extensions/get_comm_tech.pytemoa/extensions/method_of_morris/morris.pytemoa/extensions/method_of_morris/morris_evaluate.pytemoa/extensions/method_of_morris/morris_sequencer.pytemoa/extensions/modeling_to_generate_alternatives/hull.pytemoa/extensions/modeling_to_generate_alternatives/manager_factory.pytemoa/extensions/modeling_to_generate_alternatives/mga_sequencer.pytemoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.pytemoa/extensions/modeling_to_generate_alternatives/vector_manager.pytemoa/extensions/modeling_to_generate_alternatives/worker.pytemoa/extensions/monte_carlo/example_builds/scenario_analyzer.pytemoa/extensions/monte_carlo/mc_run.pytemoa/extensions/monte_carlo/mc_sequencer.pytemoa/extensions/monte_carlo/mc_worker.pytemoa/extensions/myopic/myopic_index.pytemoa/extensions/myopic/myopic_progress_mapper.pytemoa/extensions/myopic/myopic_sequencer.pytemoa/extensions/single_vector_mga/output_summary.pytemoa/extensions/single_vector_mga/sv_mga_sequencer.pytemoa/extensions/stochastics/scenario_creator.pytemoa/extensions/stochastics/stochastic_config.pytemoa/extensions/stochastics/stochastic_sequencer.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ParticularlyPythonicBS
Repo: TemoaProject/temoa PR: 177
File: temoa/model_checking/commodity_network.py:26-33
Timestamp: 2025-10-27T15:53:41.829Z
Learning: The Temoa project requires Python 3.12 or above as the minimum supported version, so PEP 695 `type` syntax for type aliases is appropriate and preferred over `TypeAlias`.
Learnt from: ParticularlyPythonicBS
Repo: TemoaProject/temoa PR: 165
File: temoa/components/time.py:229-236
Timestamp: 2025-10-18T15:54:24.846Z
Learning: In PR #165 (typing first pass), unused parameter renaming (e.g., `p` → `_p`) is intentionally deferred to a future PR. The current PR focuses exclusively on adding type annotations, not on parameter naming conventions or marking unused parameters.
📚 Learning: 2025-11-03T22:55:02.519Z
Learnt from: ParticularlyPythonicBS
Repo: TemoaProject/temoa PR: 186
File: temoa/_internal/temoa_sequencer.py:60-63
Timestamp: 2025-11-03T22:55:02.519Z
Learning: In `TemoaSequencer.__init__`, when a `mode_override` is provided, it must be written back to `self.config.scenario_mode` (not just stored in `self.temoa_mode`) because `HybridLoader.create_data_dict()` has strict consistency checks that validate `config.scenario_mode` matches the presence/absence of `myopic_index`. Without this synchronization, MYOPIC mode would fail with a RuntimeError when the loader is instantiated.
Applied to files:
temoa/extensions/modeling_to_generate_alternatives/mga_sequencer.py
🧬 Code graph analysis (11)
temoa/extensions/myopic/myopic_progress_mapper.py (1)
temoa/extensions/myopic/myopic_index.py (1)
MyopicIndex(9-29)
temoa/extensions/single_vector_mga/output_summary.py (1)
temoa/core/config.py (1)
TemoaConfig(32-344)
temoa/extensions/method_of_morris/morris_sequencer.py (2)
temoa/_internal/table_writer.py (1)
TableWriter(74-774)temoa/core/config.py (1)
TemoaConfig(32-344)
temoa/extensions/stochastics/scenario_creator.py (2)
temoa/_internal/run_actions.py (1)
build_instance(121-155)temoa/data_io/loader_manifest.py (1)
LoadItem(26-59)
temoa/extensions/modeling_to_generate_alternatives/vector_manager.py (1)
temoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.py (4)
group_members(243-244)group_variable_names(147-148)process_results(206-234)finalize_tracker(374-381)
temoa/extensions/myopic/myopic_sequencer.py (4)
temoa/extensions/myopic/myopic_index.py (1)
MyopicIndex(9-29)temoa/extensions/myopic/myopic_progress_mapper.py (2)
MyopicProgressMapper(10-83)report(52-83)temoa/core/config.py (1)
TemoaConfig(32-344)temoa/_internal/table_writer.py (1)
TableWriter(74-774)
temoa/extensions/method_of_morris/morris_evaluate.py (2)
temoa/core/config.py (1)
TemoaConfig(32-344)temoa/extensions/method_of_morris/morris.py (1)
evaluate(24-63)
temoa/extensions/modeling_to_generate_alternatives/worker.py (3)
temoa/extensions/monte_carlo/mc_worker.py (1)
run(69-184)temoa/extensions/modeling_to_generate_alternatives/hull.py (1)
update(79-117)temoa/extensions/monte_carlo/mc_run.py (1)
model(202-209)
temoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.py (3)
temoa/extensions/modeling_to_generate_alternatives/hull.py (2)
Hull(15-169)update(79-117)temoa/extensions/modeling_to_generate_alternatives/mga_constants.py (1)
MgaWeighting(15-17)temoa/extensions/modeling_to_generate_alternatives/vector_manager.py (8)
VectorManager(16-76)expired(47-52)group_variable_names(55-57)random_input_vector_model(60-62)process_results(70-71)groups(36-38)group_members(41-43)finalize_tracker(74-76)
temoa/extensions/modeling_to_generate_alternatives/manager_factory.py (3)
temoa/extensions/modeling_to_generate_alternatives/mga_constants.py (2)
MgaAxis(7-11)MgaWeighting(15-17)temoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.py (1)
TechActivityVectorManager(46-381)temoa/extensions/modeling_to_generate_alternatives/vector_manager.py (1)
VectorManager(16-76)
temoa/extensions/modeling_to_generate_alternatives/mga_sequencer.py (2)
temoa/extensions/modeling_to_generate_alternatives/vector_manager.py (2)
VectorManager(16-76)process_results(70-71)temoa/extensions/modeling_to_generate_alternatives/mga_constants.py (2)
MgaAxis(7-11)MgaWeighting(15-17)
🪛 Ruff (0.14.10)
temoa/extensions/method_of_morris/morris_sequencer.py
138-138: Dynamically typed expressions (typing.Any) are disallowed in start
(ANN401)
204-204: Dynamically typed expressions (typing.Any) are disallowed in mm_samples
(ANN401)
205-205: Dynamically typed expressions (typing.Any) are disallowed in process_results
(ANN401)
temoa/extensions/stochastics/scenario_creator.py
21-21: Dynamically typed expressions (typing.Any) are disallowed in **kwargs
(ANN401)
21-21: Dynamically typed expressions (typing.Any) are disallowed in scenario_creator
(ANN401)
temoa/extensions/method_of_morris/morris.py
24-24: Dynamically typed expressions (typing.Any) are disallowed in param_values
(ANN401)
25-25: Unused function argument: k
(ARG001)
temoa/extensions/modeling_to_generate_alternatives/vector_manager.py
70-70: Dynamically typed expressions (typing.Any) are disallowed in process_results
(ANN401)
temoa/extensions/myopic/myopic_sequencer.py
97-97: Avoid specifying long messages outside the exception class
(TRY003)
101-101: Avoid specifying long messages outside the exception class
(TRY003)
121-121: Avoid specifying long messages outside the exception class
(TRY003)
186-186: Avoid specifying long messages outside the exception class
(TRY003)
198-198: Avoid specifying long messages outside the exception class
(TRY003)
200-200: Avoid specifying long messages outside the exception class
(TRY003)
560-560: Avoid specifying long messages outside the exception class
(TRY003)
574-574: Possible SQL injection vector through string-based query construction
(S608)
temoa/extensions/method_of_morris/morris_evaluate.py
23-23: Dynamically typed expressions (typing.Any) are disallowed in log_queue
(ANN401)
38-38: Dynamically typed expressions (typing.Any) are disallowed in mm_sample
(ANN401)
39-39: Dynamically typed expressions (typing.Any) are disallowed in log_queue
(ANN401)
61-61: Avoid specifying long messages outside the exception class
(TRY003)
63-63: Avoid specifying long messages outside the exception class
(TRY003)
100-103: Avoid specifying long messages outside the exception class
(TRY003)
temoa/extensions/single_vector_mga/sv_mga_sequencer.py
208-208: Boolean-typed positional argument in function definition
(FBT001)
208-208: Boolean default positional argument in function definition
(FBT002)
208-208: Unused static method argument: verbose
(ARG004)
temoa/extensions/monte_carlo/mc_run.py
43-43: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
70-70: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
139-139: Avoid specifying long messages outside the exception class
(TRY003)
144-144: Avoid specifying long messages outside the exception class
(TRY003)
175-175: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
224-224: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
temoa/extensions/get_comm_tech.py
16-16: Create your own exception
(TRY002)
16-16: Avoid specifying long messages outside the exception class
(TRY003)
19-19: Create your own exception
(TRY002)
19-19: Avoid specifying long messages outside the exception class
(TRY003)
35-35: Possible SQL injection vector through string-based query construction
(S608)
51-51: Create your own exception
(TRY002)
51-51: Avoid specifying long messages outside the exception class
(TRY003)
54-54: Create your own exception
(TRY002)
54-54: Avoid specifying long messages outside the exception class
(TRY003)
74-74: Boolean-typed positional argument in function definition
(FBT001)
142-142: Boolean-typed positional argument in function definition
(FBT001)
209-209: Do not catch blind exception: Exception
(BLE001)
263-263: Dynamically typed expressions (typing.Any) are disallowed in get_info
(ANN401)
temoa/extensions/modeling_to_generate_alternatives/worker.py
35-35: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
117-117: datetime.datetime.now() called without a tz argument
(DTZ005)
temoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.py
147-147: Unused method argument: tech
(ARG002)
temoa/extensions/modeling_to_generate_alternatives/hull.py
28-28: Unused method argument: kwargs
(ARG002)
28-28: Dynamically typed expressions (typing.Any) are disallowed in **kwargs
(ANN401)
101-105: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
106-106: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
107-107: Avoid specifying long messages outside the exception class
(TRY003)
temoa/extensions/modeling_to_generate_alternatives/manager_factory.py
24-24: Dynamically typed expressions (typing.Any) are disallowed in **kwargs
(ANN401)
33-33: Avoid specifying long messages outside the exception class
(TRY003)
temoa/extensions/monte_carlo/mc_sequencer.py
55-55: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
temoa/extensions/monte_carlo/mc_worker.py
46-46: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
54-54: Dynamically typed expressions (typing.Any) are disallowed in **kwargs
(ANN401)
159-159: datetime.datetime.now() called without a tz argument
(DTZ005)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: setup and test (macos-latest, 3.13)
- GitHub Check: setup and test (windows-latest, 3.12)
- GitHub Check: setup and test (macos-latest, 3.12)
- GitHub Check: setup and test (windows-latest, 3.13)
🔇 Additional comments (79)
temoa/extensions/myopic/myopic_index.py (1)
19-19: LGTM!The
-> Nonereturn type annotation is correct for__post_init__. This is a straightforward and accurate type annotation addition consistent with the PR's typing objectives.temoa/extensions/single_vector_mga/sv_mga_sequencer.py (4)
12-12: LGTM! Proper use of TYPE_CHECKING for import.Moving
DataPortalinto theTYPE_CHECKINGblock avoids a runtime import dependency while preserving type information for static analysis. This is the recommended pattern.Also applies to: 26-28
52-53: LGTM! Defensive configuration access with explicit type conversion.The fallback to an empty dict and explicit
float()cast ensures robustness againstNonevalues and incorrect types in the config.
58-58: LGTM!Adding
-> Nonereturn type annotation is good practice for methods that don't return a value.
182-184: LGTM! Clear type annotations for the return structure.The signature accurately describes the function's contract: accepting a model and a tuple, returning two lists of tuples.
temoa/extensions/monte_carlo/example_builds/scenario_analyzer.py (1)
17-19: LGTM - Good extraction to a named variable.Extracting the values into
obj_values_tupleimproves readability and avoids repeating the generator expression. The histogram invocation is correct.Minor consideration: if the query returns no results,
len(obj_values_tuple)will be 0, resulting inbins=0. While this is unlikely in practice for example code,plt.histmay raise a warning or behave unexpectedly with zero bins.temoa/extensions/stochastics/scenario_creator.py (4)
1-18: LGTM!The imports are well-organized with appropriate use of
TYPE_CHECKINGguards for type-only imports, and the# type: ignore[import-untyped]comment is justified since mpi-sppy lacks type stubs.
75-92: LGTM!Good use of
strict=Trueinzip()to catch potential length mismatches betweenindex_colsandidx_tuple. The perturbation logic is clear and handles all action types appropriately.
94-133: LGTM!The instance building logic is well-structured with appropriate validation. The check for empty
first_stage_vars(lines 117-124) prevents cryptic downstream errors in mpi-sppy, and the probability fallback with warning is a sensible default.
57-62: Appropriate use ofcast()for type narrowing.The cast appropriately narrows the type from the untyped
data_dict.get()return value. The string literal'dict[Any, Any] | None'is valid withfrom __future__ import annotations.pyproject.toml (1)
144-144: LGTM!The mypy exclusion pattern is correctly narrowed to only exclude the breakeven extension, enabling type checking for all other typed extensions in this PR.
temoa/extensions/stochastics/stochastic_sequencer.py (2)
1-12: LGTM!The typing scaffolding is correctly implemented:
from __future__ import annotationsenables forward referencesTYPE_CHECKINGguard properly isolatesTemoaConfigimport to avoid runtime import cycles while still supporting type hints
44-44: LGTM!The
type: ignore[import-untyped]comment is appropriate for thempi-sppylibrary which lacks type stubs.temoa/extensions/stochastics/stochastic_config.py (2)
1-9: LGTM!The typing setup is correct:
from __future__ import annotationsenables postponed evaluation, soPathin the type hint (line 38) is treated as a string at runtimePathunderTYPE_CHECKINGis only needed for static analysisSelfis appropriate for the classmethod return type
38-38: Good use ofSelffor the classmethod return type.Using
Selfinstead of the hardcoded'StochasticConfig'string allows proper typing if this class is subclassed, which is the idiomatic pattern for factory classmethods in Python 3.11+.temoa/extensions/myopic/myopic_progress_mapper.py (1)
11-19: LGTM!The type annotations are correct and use modern Python 3.9+ generic syntax (
list[int]), which is appropriate given the project's Python 3.12 minimum requirement.temoa/extensions/single_vector_mga/output_summary.py (2)
1-16: LGTM!The typing scaffolding follows the project's established patterns with
TYPE_CHECKINGguards and appropriatetype: ignorefor the untypedtabulatelibrary.
64-94: LGTM!The
poll_*functions have propersqlite3.Connectiontyping and safeNonehandling for database results.temoa/extensions/myopic/myopic_sequencer.py (6)
66-76: LGTM on class attribute declarations.The explicit class attribute type hints improve code clarity and enable better static analysis.
78-113: LGTM on__init__typing enhancements.The explicit
-> Nonereturn type and properly annotated instance variables align with Python typing best practices.
120-121: Guard added for uninitialized config.Good defensive check before accessing
configattributes.
185-200: Appropriate runtime validation for None states.These guards prevent potential
AttributeErrororTypeErrorwhenconfig,optimization_periods, oridxare uninitialized during the control loop.
418-433: Consistentscenario_namederivation pattern.Using
self.config.scenario if self.config else Noneconsistently handles the case where config may be None.
559-560: Good validation foroptimization_periods.This guard ensures the method fails fast with a clear error message if called before initialization.
temoa/extensions/get_comm_tech.py (5)
1-9: LGTM on imports and future annotations.The
from __future__ import annotationsenables PEP 563 postponed evaluation, which works well with Python 3.12+.
47-71: Type annotations correctly applied toget_scenario.The function signature and internal type hints are appropriate.
74-76: Type hints added forget_commandget_tech.The internal variable annotations (
dict[str, str],set[str]) improve clarity.Also applies to: 142-144
203-210: Return type and exception handling foris_db_overwritten.The
-> boolreturn type is correct. The bareexcept Exceptionat line 209 is broad but acceptable here since it's catching database connection failures.
263-264: Return typeAnyis appropriate forget_info.Given that this function returns different types (
OrderedDict,dict) based on flags,Anyis a reasonable choice. AUnionorTypeVarcould be more precise but would add complexity.temoa/extensions/modeling_to_generate_alternatives/manager_factory.py (3)
1-14: LGTM on TYPE_CHECKING scaffolding.Imports under
TYPE_CHECKINGcorrectly avoid runtime circular dependencies while preserving type information for static analysis.
19-36: Well-structured factory function with proper type safety.The explicit return type
VectorManager, runtime validation forNoneconnection, and**kwargs: Anypattern are appropriate for this factory function.
37-38: Default case ensures exhaustive axis handling.The
case _withNotImplementedErrorprovides clear feedback when unsupported axes are requested.temoa/extensions/modeling_to_generate_alternatives/vector_manager.py (4)
1-14: LGTM on ABC typing setup.The
TYPE_CHECKINGblock correctly guards imports that are only needed for type annotations, avoiding runtime overhead while enabling static type checking.
16-32: Abstract__init__with explicit return type.The
-> Nonereturn type on the abstract initializer aligns with typing best practices.
40-57: Abstract method return types properly annotated.The
list[str]return types forgroup_membersandgroup_variable_namesmatch the concrete implementations inTechActivityVectorManager.
69-76: Return typeAnyforprocess_resultsis pragmatic.While
list[float]would be more precise based onTechActivityVectorManager,Anyallows flexibility for future implementations that may return different types. Thefinalize_trackerwithpassbody after@abstractmethodis valid Python for ABCs.temoa/extensions/modeling_to_generate_alternatives/mga_sequencer.py (5)
4-26: LGTM on imports and TYPE_CHECKING setup.The
TYPE_CHECKINGblock properly guards imports needed only for type annotations, includingBaseProcess,Results,DataPortal, and internal types.
54-69: Class attribute declarations improve type safety.Explicit class-level type hints enable better IDE support and static analysis for this complex sequencer class.
222-228: Queue type annotations withAnyare appropriate.Given the queues transport heterogeneous data (models, shutdown signals, log records),
Queue[Any]is a reasonable choice.
270-270: Keyword argumentmodel=aligns withVectorManager.process_resultssignature.The explicit keyword argument improves readability and matches the abstract method's parameter name.
Also applies to: 314-314
152-152: Explicit-> Nonereturn types on lifecycle methods.The
start,process_solve_results, and__del__methods now have explicit return types, improving consistency across the codebase.Also applies to: 365-365, 380-380
temoa/extensions/modeling_to_generate_alternatives/worker.py (6)
4-11: LGTM! Future annotations and typing imports.The
from __future__ import annotationsimport enables postponed evaluation of annotations (PEP 563), which is appropriate for forward references and cleaner type hints.
22-33: LGTM! Class-level attribute annotations.The explicit attribute declarations improve type checking and documentation. The types align with the corresponding
__init__parameters and usage throughout the class.
61-68: LGTM! Typed run method and logger.The explicit
-> Nonereturn type andlogger: logging.Loggerannotation improve type safety. This pattern aligns with themc_worker.pyimplementation.
84-87: LGTM! Variable rename for clarity.Renaming to
log_location_strclearly indicates the string conversion purpose before passing to solver options. This improves readability.
103-116: LGTM! Typed solve result with consistent naming.The
solve_res: SolverResults | Noneannotation and assignment toNoneon exception aligns with the pattern inmc_worker.py, providing consistent error handling across workers.
120-137: LGTM! Updated references to usesolve_res.The termination check and status extraction correctly use the renamed
solve_resvariable, maintaining consistency with the exception handling above.temoa/extensions/modeling_to_generate_alternatives/hull.py (5)
4-10: LGTM! Future annotations and typing imports.The
# type: ignore[import-untyped]comment for scipy.spatial is appropriate since SciPy doesn't provide complete type stubs.
16-26: LGTM! Class-level attribute annotations.The explicit attribute declarations with proper types (
np.ndarray | None,Anyfor cv_hull) provide clear documentation of the class state.
75-77: LGTM! Division-by-zero guard.Good defensive addition to return
0.0whennorms_checked == 0, preventing potentialZeroDivisionError.
137-144: LGTM! Defensive null check inget_norm.The added guard
if self._valid_norms is not Noneprevents potentialTypeErrorwhen accessing an uninitialized array.
148-152: LGTM! Defensive null check inget_all_norms.The guard ensures safe slicing and returns an empty array when no norms are available.
temoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.py (8)
1-24: LGTM! Well-organized TYPE_CHECKING imports.The deferred imports under
TYPE_CHECKINGavoid circular imports and runtime overhead while enabling full type checking. Thecollections.abctypes (Iterable,Iterator,Mapping,Sequence) are appropriate for type hints.
34-38: LGTM! Return type annotations for__str__and__repr__.Adding explicit
-> strreturn types completes the type annotations forDefaultItem.
46-62: LGTM! Comprehensive class-level attribute annotations.The explicit type declarations provide clear documentation of the class state and enable better static analysis.
64-109: LGTM! Typed__init__with proper defaults.The
-> Nonereturn type and explicit initialization ofhull_points = Noneandhull = Nonealign with the class attribute declarations.
133-140: LGTM! Safe iteration withor set()pattern.Using
or set()provides a safe fallback whenactive_flow_rpsditvooractive_flow_rpitvomight beNone, preventing iteration errors.
307-322: LGTM! Defensive None guard inregenerate_hull.The early return with a warning when
hull_points is Noneprevents attempting hull construction with no data.
331-360: LGTM! Typed static method withMapping/Sequencegenerics.Using
Mapping[Any, Sequence[str]]andMapping[str, int]for parameters allows flexibility while maintaining type safety.
362-372: LGTM! Defensive None guards intracker.The combined check
if self.hull is not None and self.hull_points is not Noneensures the tracker only operates when valid data exists.temoa/extensions/method_of_morris/morris_evaluate.py (3)
5-20: LGTM on typing imports and TYPE_CHECKING pattern.The use of
TYPE_CHECKINGto guard theTemoaConfigimport avoids circular imports at runtime while still enabling static type checking. This is the correct pattern for forward references.
23-35: LGTM on type annotations.The
Anytype forlog_queueis appropriate here since multiprocessing queue types vary and aren't well-typed in the standard library.
91-107: LGTM on variable renaming and type consistency.The snake_case naming convention aligns with Python standards, and the explicit
float()casts ensure the return typelist[float]is honored even when database values might be integers.temoa/extensions/method_of_morris/morris_sequencer.py (5)
17-21: LGTM ontype: ignorecomments for untyped libraries.The
# type: ignore[import-untyped]annotations are appropriate for external libraries (joblib, SALib) that lack type stubs. This silences type checker warnings while maintaining type safety in the rest of the codebase.
138-202: LGTM onstartmethod typing.The
-> Anyreturn type is pragmatic given that the return value comes from SALib'smorris.analyze(), which is untyped. Documenting the actual return structure in a docstring would be helpful for consumers but isn't required.
204-205: LGTM onprocess_resultstype hints.The
mm_samples: Anyandmorris_results: list[Any]types are appropriate given these come from SALib's sample function and the evaluate function respectively.
276-349: LGTM ongather_parametersreturn type.The return type
dict[int, list[Any]]accurately reflects the data structure being built and is consistent with theparam_infoparameter type inmorris_evaluate.py.
351-352: LGTM on__del__return type.Adding
-> Noneto__del__follows the project's typing conventions as seen inTableWriter.__del__in the relevant code snippets.temoa/extensions/method_of_morris/morris.py (3)
1-7: LGTM on import additions.The new imports support the type annotations and are properly organized with
from __future__ import annotationsat the top.
54-63: LGTM on variable naming updates.The snake_case naming (
y_of,y_cumulative_co2,morris_objectives) is consistent with Python conventions and aligns with the naming inmorris_evaluate.py.
160-203: LGTM on analysis result variable renaming.Renaming to
si_ofandsi_cumulative_co2provides clearer semantics (sensitivity indices) and follows snake_case conventions.temoa/extensions/monte_carlo/mc_worker.py (2)
131-163: LGTM on solve result handling.The
solve_resvariable is properly typed asSolverResults | Noneand all code paths (success branches and exception handler) assign to it before use. Thetry/except AttributeErrorblock at lines 162-183 correctly guards againstNonebeing passed tocheck_optimal_termination.
34-44: Good use of class-level attribute annotations.The explicit attribute declarations improve IDE support and static analysis. This pattern aligns well with the typing effort across the Monte Carlo modules.
temoa/extensions/monte_carlo/mc_sequencer.py (3)
269-277: LGTM on cast usage in shutdown handling.The guard at line 275 (
next_result is not None and next_result != 'COYOTE') correctly ensures thatnext_resultis aDataBrickbefore the cast on line 277. This aligns with the queue typeQueue[DataBrick | str]where the only string value is the'COYOTE'shutdown signal.
24-32: Good use ofTYPE_CHECKINGfor deferred imports.Moving
DataPortal,DataBrick,TemoaConfig, and multiprocessing types underTYPE_CHECKINGcorrectly avoids runtime import overhead and potential circular dependencies while maintaining full type annotation support.
143-152: Well-typed queue and worker container declarations.The explicit type annotations for
work_queue,result_queue,log_queue, andworkersimprove code clarity and enable better static analysis across the multiprocessing workflow.temoa/extensions/monte_carlo/mc_run.py (3)
253-264: LGTM on Generator type annotations.The
Generator[tuple[int, str]]return type correctly uses thecollections.abc.Generatorshorthand form, which is valid when only specifying the yield type (with implicitNonefor send and return types).
15-21: Good use ofTYPE_CHECKINGfor type-only imports.The
Generator,DataPortal, andTemoaConfigimports underTYPE_CHECKINGcorrectly avoid runtime import overhead while enabling full type annotation support. This is consistent with the pattern used inmc_worker.pyandmc_sequencer.py.
362-368: Improved data access pattern with intermediate variable.Extracting
param_vals = data_store[tweak.param_name]into a local variable improves readability and reduces repeated dictionary lookups when accessing and modifying the same parameter data.
| setting_entry = 'run # %d: Setting param %s[%s] to value: %f' | ||
| log_entry.append(setting_entry) | ||
| logger.debug('\n '.join(log_entry)) |
There was a problem hiding this comment.
Log format string is never interpolated with actual values.
The setting_entry format string contains placeholders (%d, %s, %s, %f) but is appended to log_entry without being formatted. The debug log will output the raw format string instead of the actual parameter values.
🐛 Proposed fix
- setting_entry = 'run # %d: Setting param %s[%s] to value: %f'
- log_entry.append(setting_entry)
+ setting_entry = f'run # {i + 1}: Setting param {param_name}[{set_idx_tuple}] to value: {mm_sample[j]}'
+ log_entry.append(setting_entry)🤖 Prompt for AI Agents
In @temoa/extensions/method_of_morris/morris_evaluate.py around lines 65 - 67,
The format string stored in setting_entry is never interpolated before being
appended to log_entry, so the log prints the raw "%d/%s/%f" placeholders; update
the code where setting_entry is appended to instead append a formatted string
(e.g., use setting_entry % (run_number, param_name, param_index, param_value) or
construct an f-string) and ensure the variables used match the expected types,
then call logger.debug on the joined log_entry as before.
| def evaluate(param_names: dict[int, list[Any]], param_values: Any, | ||
| data: dict[str, Any], k: int) -> list[Any]: |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unused parameter k should be prefixed with underscore.
The k parameter is not used in the function body. Per Python convention, prefix it with _ to indicate it's intentionally unused.
♻️ Proposed fix
def evaluate(param_names: dict[int, list[Any]], param_values: Any,
- data: dict[str, Any], k: int) -> list[Any]:
+ data: dict[str, Any], _k: int) -> list[Any]:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def evaluate(param_names: dict[int, list[Any]], param_values: Any, | |
| data: dict[str, Any], k: int) -> list[Any]: | |
| def evaluate(param_names: dict[int, list[Any]], param_values: Any, | |
| data: dict[str, Any], _k: int) -> list[Any]: |
🧰 Tools
🪛 Ruff (0.14.10)
24-24: Dynamically typed expressions (typing.Any) are disallowed in param_values
(ANN401)
25-25: Unused function argument: k
(ARG001)
🤖 Prompt for AI Agents
In @temoa/extensions/method_of_morris/morris.py around lines 24 - 25, The
evaluate function has an unused parameter k; rename it to _k in the function
signature (def evaluate(param_names: dict[int, list[Any]], param_values: Any,
data: dict[str, Any], _k: int) -> list[Any]) to follow Python convention for
intentionally unused parameters and update any internal references to use _k if
present.
| for row in output_query: | ||
| Y_OF = row[-1] | ||
| y_of = row[-1] | ||
| cur.execute("SELECT emis_comm, SUM(emission) FROM output_emissionn WHERE emis_comm='co2'") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for references to output_emission variants in the codebase
rg -n "output_emission" --type py -C 2Repository: TemoaProject/temoa
Length of output: 8184
🏁 Script executed:
# Also check for SQL files or database schema definitions
fd -e sql -e db -e schema | head -20Repository: TemoaProject/temoa
Length of output: 845
🏁 Script executed:
# Search more broadly for table definitions
rg -n "CREATE TABLE.*emission" --type-listRepository: TemoaProject/temoa
Length of output: 451
🏁 Script executed:
# Search for the actual table definition in schema files
rg -n "CREATE TABLE.*output_emission" temoa/db_schema/ data_files/Repository: TemoaProject/temoa
Length of output: 661
🏁 Script executed:
# Also check the clear_db_outputs file to see if it has the same typo
cat -n temoa/utilities/clear_db_outputs.py | head -20Repository: TemoaProject/temoa
Length of output: 631
Fix typos in table name: output_emissionn should be output_emission.
The SQL query references output_emissionn (with double 'n'), which will cause a runtime error when the table is not found. The actual table name in the database schema is output_emission (single 'n'). This typo appears in at least two places:
temoa/extensions/method_of_morris/morris.py:55temoa/utilities/clear_db_outputs.py:15
🐛 Proposed fixes
For temoa/extensions/method_of_morris/morris.py:
- cur.execute("SELECT emis_comm, SUM(emission) FROM output_emissionn WHERE emis_comm='co2'")
+ cur.execute("SELECT emis_comm, SUM(emission) FROM output_emission WHERE emis_comm='co2'")For temoa/utilities/clear_db_outputs.py:
- 'output_emissionn',
+ 'output_emission',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cur.execute("SELECT emis_comm, SUM(emission) FROM output_emissionn WHERE emis_comm='co2'") | |
| cur.execute("SELECT emis_comm, SUM(emission) FROM output_emission WHERE emis_comm='co2'") |
🤖 Prompt for AI Agents
In @temoa/extensions/method_of_morris/morris.py at line 55, SQL queries use the
wrong table name `output_emissionn`; update the SQL in the cur.execute calls to
use the correct table `output_emission` (e.g., replace the query string in the
cur.execute(...) call found in morris.py and the analogous cur.execute in
clear_db_outputs.py) so the database selects run against the actual table name.
| except Exception as e: | ||
| # scipy.spatial._qhull.QhullError may not be easily found by mypy | ||
| logger.error( | ||
| 'Attempt at hull construction from basis vectors failed.' | ||
| '\nMay be non-recoverable. Possibly try a set of random vectors to initialize the ' | ||
| 'Hull.' | ||
| ) | ||
| logger.error(e) | ||
| raise RuntimeError('Hull construction from vectors failed. See log file') | ||
| raise RuntimeError('Hull construction from vectors failed. See log file') from e |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use logger.exception instead of logger.error for exception logging.
logger.exception() automatically includes the traceback, providing more diagnostic information. Per Ruff TRY400, this is the recommended pattern when logging within an exception handler.
Proposed fix
except Exception as e:
# scipy.spatial._qhull.QhullError may not be easily found by mypy
- logger.error(
+ logger.exception(
'Attempt at hull construction from basis vectors failed.'
'\nMay be non-recoverable. Possibly try a set of random vectors to initialize the '
'Hull.'
)
- logger.error(e)
raise RuntimeError('Hull construction from vectors failed. See log file') from e🧰 Tools
🪛 Ruff (0.14.10)
101-105: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
106-106: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
107-107: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In @temoa/extensions/modeling_to_generate_alternatives/hull.py around lines 99 -
107, In the except Exception as e block that currently logs the failure and
calls logger.error(e) before raising a RuntimeError, replace the second
logger.error(e) with logger.exception(...) (or combine into a single
logger.exception call) so the traceback is included in the logs; keep the
existing descriptive message about hull construction failure and re-raise the
RuntimeError('Hull construction from vectors failed. See log file') from e.
| for item in sorted(cast('Iterable[Any]', emission_labels)): | ||
| orig = poll_emission( | ||
| conn, | ||
| scenarios[0], | ||
| item, | ||
| ) | ||
| option = poll_emission(conn, scenarios[1], item) | ||
| delta = float((option - orig) / orig * 100) if all((orig, option)) else None | ||
| records.append(['Emission', item, orig, option, delta]) | ||
| for item in sorted(activity_labels): | ||
| row_delta = float((option - orig) / orig * 100) if all((orig, option)) else None | ||
| records.append(['Emission', item, orig, option, row_delta]) | ||
| for item in sorted(cast('Iterable[Any]', activity_labels)): | ||
| orig = poll_activity(conn, scenarios[0], item) | ||
| option = poll_activity(conn, scenarios[1], item) | ||
| delta = (option - orig) / orig * 100 if all((orig, option)) else None | ||
| row_delta = (option - orig) / orig * 100 if all((orig, option)) else None | ||
|
|
||
| records.append(['Activity', item, orig, option, delta]) | ||
| records.append(['Activity', item, orig, option, row_delta]) | ||
|
|
||
| for item in sorted(capacity_labels): | ||
| for item in sorted(cast('Iterable[Any]', capacity_labels)): | ||
| orig = poll_capacity(conn, scenarios[0], item) | ||
| option = poll_capacity(conn, scenarios[1], item) | ||
| delta = (option - orig) / orig * 100 if all((orig, option)) else None | ||
| row_delta = (option - orig) / orig * 100 if all((orig, option)) else None | ||
|
|
||
| records.append(['Capacity', item, orig, option, delta]) | ||
| records.append(['Capacity', item, orig, option, row_delta]) |
There was a problem hiding this comment.
Zero values are incorrectly treated as missing data.
The all((orig, option)) check treats 0.0 as falsy, so legitimate zero emissions/activity/capacity values will result in row_delta = None instead of computing the percentage change. Since the poll_* functions return 0.0 for missing data, there's no way to distinguish between "no data" and "actual zero".
🐛 Suggested fix using explicit None checks
Modify the poll_* functions to return float | None and check for None explicitly:
-def poll_emission(conn: sqlite3.Connection, scenario: str, label: str) -> float:
+def poll_emission(conn: sqlite3.Connection, scenario: str, label: str) -> float | None:
...
row = conn.execute(...).fetchone()
- return float(row[0] if row and row[0] is not None else 0.0)
+ return float(row[0]) if row and row[0] is not None else NoneThen in summarize:
- row_delta = float((option - orig) / orig * 100) if all((orig, option)) else None
+ row_delta = (option - orig) / orig * 100 if orig is not None and option is not None and orig != 0 else None🤖 Prompt for AI Agents
In @temoa/extensions/single_vector_mga/output_summary.py around lines 31 - 52,
The code incorrectly treats zero values as missing by using all((orig, option));
update the polling functions (poll_emission, poll_activity, poll_capacity) to
return None for missing data (float | None) and change the checks in summarize
to explicit None checks (e.g., if orig is not None and option is not None)
before computing row_delta; ensure you compute row_delta as ((option - orig) /
orig * 100) with a float conversion where needed and leave row_delta as None
when either value is None, then append records as before.
| for item in sorted(cast('Iterable[Any]', emission_labels)): | ||
| orig = poll_emission( | ||
| conn, | ||
| scenarios[0], | ||
| item, | ||
| ) | ||
| option = poll_emission(conn, scenarios[1], item) | ||
| delta = float((option - orig) / orig * 100) if all((orig, option)) else None | ||
| records.append(['Emission', item, orig, option, delta]) | ||
| for item in sorted(activity_labels): | ||
| row_delta = float((option - orig) / orig * 100) if all((orig, option)) else None | ||
| records.append(['Emission', item, orig, option, row_delta]) | ||
| for item in sorted(cast('Iterable[Any]', activity_labels)): | ||
| orig = poll_activity(conn, scenarios[0], item) | ||
| option = poll_activity(conn, scenarios[1], item) | ||
| delta = (option - orig) / orig * 100 if all((orig, option)) else None | ||
| row_delta = (option - orig) / orig * 100 if all((orig, option)) else None | ||
|
|
||
| records.append(['Activity', item, orig, option, delta]) | ||
| records.append(['Activity', item, orig, option, row_delta]) | ||
|
|
||
| for item in sorted(capacity_labels): | ||
| for item in sorted(cast('Iterable[Any]', capacity_labels)): |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
The cast('Iterable[Any]', ...) calls are verbose and could be simplified.
Since svmga_inputs.get('emission_labels', []) returns a list (which is already Iterable), and you're iterating over it with sorted(), the cast adds verbosity without benefit. Consider typing svmga_inputs more precisely or removing the casts.
♻️ Alternative: type the labels directly
- emission_labels = svmga_inputs.get('emission_labels', [])
- capacity_labels = svmga_inputs.get('capacity_labels', [])
- activity_labels = svmga_inputs.get('activity_labels', [])
+ emission_labels: list[str] = svmga_inputs.get('emission_labels', [])
+ capacity_labels: list[str] = svmga_inputs.get('capacity_labels', [])
+ activity_labels: list[str] = svmga_inputs.get('activity_labels', [])
...
- for item in sorted(cast('Iterable[Any]', emission_labels)):
+ for item in sorted(emission_labels):🤖 Prompt for AI Agents
In @temoa/extensions/single_vector_mga/output_summary.py around lines 31 - 47,
The cast('Iterable[Any]', ...) calls around emission_labels, activity_labels,
and capacity_labels are unnecessary and verbose; remove the casts in the three
sorted(...) iterations (the lines that call sorted(cast('Iterable[Any]',
emission_labels)) and similar for activity_labels and capacity_labels) and
simply call sorted(emission_labels), sorted(activity_labels), and
sorted(capacity_labels) instead, or alternatively tighten the type of
svmga_inputs so those variables are already typed as Sequence/Iterable[str] to
avoid needing casts; keep the existing logic using poll_emission and
poll_activity unchanged.
| row_delta = float((option - orig) / orig * 100) if all((orig, option)) else None | ||
| records.append(['Emission', item, orig, option, row_delta]) | ||
| for item in sorted(cast('Iterable[Any]', activity_labels)): | ||
| orig = poll_activity(conn, scenarios[0], item) | ||
| option = poll_activity(conn, scenarios[1], item) | ||
| delta = (option - orig) / orig * 100 if all((orig, option)) else None | ||
| row_delta = (option - orig) / orig * 100 if all((orig, option)) else None | ||
|
|
||
| records.append(['Activity', item, orig, option, delta]) | ||
| records.append(['Activity', item, orig, option, row_delta]) | ||
|
|
||
| for item in sorted(capacity_labels): | ||
| for item in sorted(cast('Iterable[Any]', capacity_labels)): | ||
| orig = poll_capacity(conn, scenarios[0], item) | ||
| option = poll_capacity(conn, scenarios[1], item) | ||
| delta = (option - orig) / orig * 100 if all((orig, option)) else None | ||
| row_delta = (option - orig) / orig * 100 if all((orig, option)) else None |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent float() wrapper in delta calculations.
Line 38 wraps the result in float() while lines 43 and 50 do not. Since division in Python 3 always returns float, the wrapper is unnecessary but the inconsistency is confusing.
♻️ Remove redundant float() for consistency
- row_delta = float((option - orig) / orig * 100) if all((orig, option)) else None
+ row_delta = (option - orig) / orig * 100 if all((orig, option)) else None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| row_delta = float((option - orig) / orig * 100) if all((orig, option)) else None | |
| records.append(['Emission', item, orig, option, row_delta]) | |
| for item in sorted(cast('Iterable[Any]', activity_labels)): | |
| orig = poll_activity(conn, scenarios[0], item) | |
| option = poll_activity(conn, scenarios[1], item) | |
| delta = (option - orig) / orig * 100 if all((orig, option)) else None | |
| row_delta = (option - orig) / orig * 100 if all((orig, option)) else None | |
| records.append(['Activity', item, orig, option, delta]) | |
| records.append(['Activity', item, orig, option, row_delta]) | |
| for item in sorted(capacity_labels): | |
| for item in sorted(cast('Iterable[Any]', capacity_labels)): | |
| orig = poll_capacity(conn, scenarios[0], item) | |
| option = poll_capacity(conn, scenarios[1], item) | |
| delta = (option - orig) / orig * 100 if all((orig, option)) else None | |
| row_delta = (option - orig) / orig * 100 if all((orig, option)) else None | |
| row_delta = (option - orig) / orig * 100 if all((orig, option)) else None | |
| records.append(['Emission', item, orig, option, row_delta]) | |
| for item in sorted(cast('Iterable[Any]', activity_labels)): | |
| orig = poll_activity(conn, scenarios[0], item) | |
| option = poll_activity(conn, scenarios[1], item) | |
| row_delta = (option - orig) / orig * 100 if all((orig, option)) else None | |
| records.append(['Activity', item, orig, option, row_delta]) | |
| for item in sorted(cast('Iterable[Any]', capacity_labels)): | |
| orig = poll_capacity(conn, scenarios[0], item) | |
| option = poll_capacity(conn, scenarios[1], item) | |
| row_delta = (option - orig) / orig * 100 if all((orig, option)) else None |
🤖 Prompt for AI Agents
In @temoa/extensions/single_vector_mga/output_summary.py around lines 38 - 50,
The delta calculation in the Emission loop uses an unnecessary float() wrapper
for row_delta while the Activity and Capacity loops compute row_delta without
it; remove the float(...) around ((option - orig) / orig * 100) in the Emission
block so row_delta is computed consistently (as in the Activity and Capacity
loops) when all((orig, option)) is truthy; update the expression where row_delta
is set in the Emission loop (the line that currently reads row_delta =
float((option - orig) / orig * 100) if all((orig, option)) else None) to match
the others.
| table_index_map: dict[str, list[str]] = {} | ||
| item: LoadItem | ||
| for item in hybrid_loader.manifest: | ||
| if item.table not in table_index_map and item.columns: | ||
| table_index_map[item.table] = item.columns[:-1] | ||
| table_index_map[item.table] = list(item.columns[:-1]) | ||
| except Exception as e: | ||
| logger.exception('Failed to connect to database %s', temoa_config.input_database) | ||
| raise RuntimeError(f'Failed to connect to database {temoa_config.input_database}') from e |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider inline type annotation for loop variable.
The standalone type annotation item: LoadItem before the for loop (line 44) is valid but unconventional. A more common pattern is to either annotate the manifest's type so item is inferred, or use an inline comment if needed. That said, this is a minor style preference and the current approach works correctly.
♻️ Alternative style (optional)
table_index_map: dict[str, list[str]] = {}
- item: LoadItem
- for item in hybrid_loader.manifest:
+ for item in hybrid_loader.manifest: # item: LoadItemOr simply remove the annotation if hybrid_loader.manifest is properly typed, letting the type checker infer item's type.
🧰 Tools
🪛 Ruff (0.14.10)
50-50: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In @temoa/extensions/stochastics/scenario_creator.py around lines 43 - 50, The
loop contains a standalone type annotation "item: LoadItem" before the for loop
which is unconventional; remove that line and either rely on the type of
hybrid_loader.manifest so the loop variable item is inferred (for item in
hybrid_loader.manifest) or, if necessary, annotate the manifest's type so item
is typed implicitly; update the code around table_index_map and the for loop to
drop the separate "item: LoadItem" declaration.
ParticularlyPythonicBS
force-pushed
the
typing/extensions
branch
from
74c5f32 to
ab26865
Compare
There was a problem hiding this comment.
Actionable comments posted: 26
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (32)
temoa/extensions/myopic/myopic_progress_mapper.py (1)
11-19: Guard against emptysorted_future_years(currently crashes inmax(...)).
Even if “shouldn’t happen”, failing with a targeted error makes debugging/config issues much easier.Proposed fix
class MyopicProgressMapper: def __init__(self, sorted_future_years: list[int]) -> None: + if not sorted_future_years: + raise ValueError('sorted_future_years must be non-empty') self.leader = '--' self.trailer = ''.join(reversed(self.leader)) self.years = sorted_future_years self.tag_width = max(len(str(t)) for t in sorted_future_years) + 2 * len(self.leader)temoa/extensions/myopic/myopic_sequencer.py (1)
154-160: Bug:start()’sself.config is Noneguard is too late to prevent crashes.
start()callsself.characterize_run()before the guard; withconfig=Nonethis can fail via missingcursor/view_depth/etc.long before your intendedRuntimeError.Proposed fix (move guard to the top of `start()`)
def start(self) -> None: + if self.config is None: + raise RuntimeError('Config is not initialized') # noqa: TRY003 + # load up the instance queue self.characterize_run() @@ - if self.config is None: - raise RuntimeError('Config is not initialized') - logger.info('Starting Myopic Sequence')Also applies to: 185-187
temoa/extensions/monte_carlo/mc_sequencer.py (2)
110-231: Fix unhandledStopIterationwhen primingrun_gen(can crashstart()and leak workers).
mc_run: MCRun = next(run_gen)(Line 179) will raise ifrun_generator()yields nothing (e.g., all runs aborted due to “no good tweaks”), but workers have already been spawned (Lines 161-174). This can terminate the sequencer without sending shutdown signals / joining processes.Proposed fix (guard initial next() and avoid starting workers when there are no runs)
@@ - # 3. set up the run generator - run_gen = mc_factory.run_generator() + # 3. set up the run generator + run_gen = mc_factory.run_generator() + + # Prime generator before starting workers so we can exit cleanly if there are no runs. + try: + mc_run: MCRun = next(run_gen) + except StopIteration: + logger.warning("Monte Carlo run generator produced no runs; nothing to do.") + return @@ - # 4. Set up the workers + # 4. Set up the workers import multiprocessing ctx = multiprocessing.get_context('spawn') @@ - # pull the first instance - mc_run: MCRun = next(run_gen) + # first instance already pulled above
142-261: Make shutdown signaling non-blocking/timeout-bounded to avoid hangs whenwork_queueis full.
work_queue.put('ZEBRA')(Line 259) can block indefinitely because the queue has a small maxsize and may still contain pending work items at shutdown time. If any worker is stalled/crashed, this can deadlock the manager.Proposed fix (timeout + retry)
@@ - for _ in workers: + for _ in workers: if self.verbose: print('shutdown sent') - work_queue.put('ZEBRA') # shutdown signal + # Avoid potential deadlock if the work queue is full. + while True: + try: + work_queue.put('ZEBRA', timeout=1.0) + break + except queue.Full: + time.sleep(0.1) logger.debug('Put "ZEBRA" on work queue (shutdown signal)')temoa/extensions/monte_carlo/mc_worker.py (3)
106-184: Harden solve-result handling: don’t callcheck_optimal_termination()onNone, and avoidsolve_res['Solver']access.
Right now, a failed solve setssolve_res = None(Line 158) but still flows intocheck_optimal_termination(solve_res)(Line 163) and later triessolve_res['Solver']...(Lines 175-181). This can throw (and is currently swallowed by theexcept AttributeError: pass), causing silent loss of failure diagnostics.Proposed fix (explicit None-guard + use standard `SolverResults` attributes)
@@ - # guard against a bad "res" object... - try: - good_solve = check_optimal_termination(solve_res) - if good_solve: + # Guard against missing/invalid solve results. + if solve_res is None: + continue + + try: + good_solve = check_optimal_termination(solve_res) + if good_solve: data_brick = data_brick_factory(model) self.results_queue.put(data_brick) logger.info( 'Worker %d solved a model in %0.2f minutes', self.worker_number, (toc - tic).total_seconds() / 60, ) if verbose: print(f'Worker {self.worker_number} completed a successful solve') else: - if solve_res is not None: - status = solve_res['Solver'].termination_condition - logger.info( - 'Worker %d did not solve. Results status: %s', - self.worker_number, - status, - ) - except AttributeError: - pass + status = getattr(solve_res.solver, "termination_condition", None) + logger.info( + 'Worker %d did not solve. Results termination condition: %s', + self.worker_number, + status, + ) + except Exception: + logger.exception("Unexpected error while interpreting solver results")
46-67: Add__init__ -> Noneand consider replacing**kwargs: Anywith explicit args.
This is both a Ruff violation (ANN204, ANN401) and improves the public surface for type-safety.Proposed fix (minimal: add return type; better: make args explicit)
@@ def __init__( self, dp_queue: Queue[Any], results_queue: Queue[DataBrick | str], log_root_name: str, log_queue: Queue[logging.LogRecord], log_level: int = logging.INFO, solver_log_path: Path | None = None, - **kwargs: Any, - ): + solver_name: str = "", + solver_options: dict[str, Any] | None = None, + ) -> None: @@ - self.solver_name = kwargs['solver_name'] - self.solver_options = kwargs['solver_options'] + self.solver_name = solver_name + self.solver_options = solver_options or {}
106-115: Guardself.opt.options = ...assignment against appsi solvers.Line 107 applies the legacy
.optionsAPI unconditionally, but appsi solvers (appsi_ipopt, appsi_highs, etc.) do not support this interface and will fail or ignore it. This causes the assignment to be ineffective for appsi backends and risks errors in multiprocessing workers. Wrap line 107 withhasattr(self.opt, 'options')or a try/except block. The subsequent per-key config mapping (lines 110–115) is correct but serves only as a partial workaround; the initial unguarded assignment must be skipped for appsi solvers entirely.Note: Update the comment on line 103—appsi solvers do not accept options via
.optionsat all; use.configfor solver-independent settings and.ipopt_options/.highs_options/.solver_optionsfor solver-specific parameters.temoa/extensions/monte_carlo/mc_run.py (5)
201-209: FixMCRun.model: it passes a tuple intocreate_instance()(likely runtime failure).
model_dpreturns(name, dp)(Lines 195-200), butmodelassignsdp = self.model_dp(Line 203) and callscreate_instance(data=dp)(Line 205). Thatdpis atuple[str, DataPortal], not aDataPortal.Proposed fix
@@ def model(self) -> TemoaModel: - dp = self.model_dp + _, dp = self.model_dp model = TemoaModel() instance = model.create_instance(data=dp)
230-251: Don’t useassertfor input validation inprescreen_input_file()(can be optimized away).
Withpython -O, the header check disappears; invalid files will slip through and fail later in less actionable ways.Proposed fix
@@ with open(self.settings_file) as f: header = f.readline().strip() - assert header == 'run,param,index,mod,value,notes', ( - 'header should be: run,param,index,mod,value,notes' - ) + if header != 'run,param,index,mod,value,notes': + raise ValueError('header should be: run,param,index,mod,value,notes')
293-325: Guardelement_locator()against empty param dicts (current code canIndexError).
Ifparam_datais{}(present but empty),tuple(param_data.keys())[0](Line 313) crashes.Proposed fix
@@ param_data = data_store.get(param) if not param_data: return [] + if not getattr(param_data, "keys", None) or not param_data: + return [] @@ - first_index = tuple(param_data.keys())[0] + first_index = next(iter(param_data.keys()))
266-291: Handle empty run-settings files intweak_set_generator()(unguardednext(rows)).
If the CSV has only a header,next(rows)(Line 274) raisesStopIterationand will currently crash the generator consumer (e.g., the sequencer).Proposed fix
@@ rows = self._next_row_generator() empty = False run_tweaks = [] - row_number, next_row = next(rows) + try: + row_number, next_row = next(rows) + except StopIteration: + return
43-55: Add missing__init__ -> Nonereturn types (Ruff ANN204).
Applies toTweak.__init__(Line 43),TweakFactory.__init__(Line 70),MCRun.__init__(Line 175),MCRunFactory.__init__(Line 224).Proposed fix (example pattern)
@@ - def __init__(self, param_name: str, indices: tuple[Any, ...], adjustment: str, value: float): + def __init__( + self, param_name: str, indices: tuple[Any, ...], adjustment: str, value: float + ) -> None: @@ - def __init__(self, data_store: dict[str, Any]): + def __init__(self, data_store: dict[str, Any]) -> None: @@ def __init__( @@ - ): + ) -> None: @@ - def __init__(self, config: TemoaConfig, data_store: dict[str, Any]): + def __init__(self, config: TemoaConfig, data_store: dict[str, Any]) -> None:Also applies to: 70-78, 175-186, 224-229
temoa/extensions/method_of_morris/morris_sequencer.py (2)
86-131: Fix Morris option parsing: falsy values + missing int coercions can break runtime.
if pert:/if levels:/if traj:will treat0/0.0as “not provided”. Alsocoresisn’t coerced toint, so'0'or'4'will misbehave ('0' == 0is False; joblibn_jobs='4'will error).Proposed fix
- pert = morris_inputs.get('perturbation') - if pert: - self.mm_perturbation = float(cast(str | float, pert)) + pert = morris_inputs.get('perturbation') + if pert is not None: + self.mm_perturbation = float(pert) logger.info('Morris perturbation: %0.2f', self.mm_perturbation) else: self.mm_perturbation = 0.10 logger.warning( 'No value received for perturbation, using default: %0.2f', self.mm_perturbation ) - levels = morris_inputs.get('levels') - if levels: - self.num_levels = int(cast('Any', levels)) + levels = morris_inputs.get('levels') + if levels is not None: + self.num_levels = int(levels) logger.info('Morris levels: %d', self.num_levels) else: self.num_levels = ( 8 # the number of levels to divide the param range into (must be even number) ) logger.warning('No value received for levels, using default: %d', self.num_levels) - traj = morris_inputs.get('trajectories') - if traj: - self.trajectories = int(cast('Any', traj)) + traj = morris_inputs.get('trajectories') + if traj is not None: + self.trajectories = int(traj) logger.info('Morris trajectories: %d', self.trajectories) else: self.trajectories = 4 # number of morris trajectories to generate logger.warning( 'No value received for trajectories, using default: %d', self.trajectories ) seed = morris_inputs.get('seed') - self.seed = int(cast('Any', seed)) if seed else None + self.seed = int(seed) if seed is not None else None logger.info('Morris Seed (None indicates system generated): %s', self.seed) - self.num_cores = morris_inputs.get('cores', 0) - if self.num_cores == 0: + self.num_cores = int(morris_inputs.get('cores', 0) or 0) + if self.num_cores == 0: self.num_cores = multiprocessing.cpu_count()
138-203: Ensure QueueListener/Manager are cleaned up on exceptions (avoid leaked threads/processes).If any worker raises,
log_listener.stop()won’t run, and themultiprocessing.Manager()server process may linger.Proposed fix
- m = multiprocessing.Manager() - log_queue = m.Queue() # Queue() - - log_listener = QueueListener(log_queue, *logging.root.handlers) - log_level = logger.getEffectiveLevel() - log_listener.start() + with multiprocessing.Manager() as m: + log_queue = m.Queue() + log_listener = QueueListener(log_queue, *logging.root.handlers) + log_level = logger.getEffectiveLevel() + log_listener.start() + try: + # 5. Run the processing: + if not self.config.silent: + msg = f'Starting {len(mm_samples)} MM runs on {self.num_cores} cores.\n' + sys.stdout.write(msg) + sys.stdout.write('=' * (len(msg) - 1) + '\n') + sys.stdout.flush() + morris_results = Parallel(n_jobs=self.num_cores)( + delayed(evaluate)( + param_names, mm_samples[i, :], data, i, self.config, log_queue, log_level + ) + for i in range(0, len(mm_samples)) + ) + finally: + log_listener.stop() - - # 5. Run the processing: - if not self.config.silent: - msg = f'Starting {len(mm_samples)} MM runs on {self.num_cores} cores.\n' - sys.stdout.write(msg) - sys.stdout.write('=' * (len(msg) - 1) + '\n') - sys.stdout.flush() - morris_results = Parallel(n_jobs=self.num_cores)( - delayed(evaluate)( - param_names, mm_samples[i, :], data, i, self.config, log_queue, log_level - ) - for i in range(0, len(mm_samples)) - ) - log_listener.stop()temoa/extensions/method_of_morris/morris_evaluate.py (2)
23-35: Logger handler detection is too broad (hasHandlers()checks ancestors).
hasHandlers()can return True due to parent/root handlers, which can prevent attaching theQueueHandlerand silently drop worker logs into the queue. Prefer checkingworker_logger.handlers(and setpropagate = False) if the intent is “ensure this logger has a QueueHandler”.
79-107: Handle empty objective rows and NULL SUM emissions to avoid runtime crashes.
- If
output_objectivereturns 0 rows,output_query[0][0]raisesIndexError.SELECT SUM(emission) ...commonly returns 1 row withNonewhen there are no matches;float(None)will raise.Proposed fix
cur.execute( 'SELECT total_system_cost FROM output_objective where scenario = ?', (scenario_name,) ) output_query = cur.fetchall() - if len(output_query) > 1: + if len(output_query) == 0: + raise RuntimeError( + 'No output found in output_objective table matching scenario name. Model write failed.' + ) + if len(output_query) > 1: raise RuntimeError( 'Multiple outputs found in Objective table matching scenario name. Coding error.' ) else: y_of = output_query[0][0] ... output_query = cur.fetchall() - if len(output_query) == 0: - y_cumulative_co2 = 0.0 + if len(output_query) == 0: + y_cumulative_co2 = 0.0 elif len(output_query) > 1: raise RuntimeError( 'Multiple outputs found in output_emissions table matching scenario name. Coding ' 'error.' ) else: - y_cumulative_co2 = output_query[0][0] + y_cumulative_co2 = output_query[0][0] or 0.0 morris_objectives = [float(y_of), float(y_cumulative_co2)]temoa/extensions/stochastics/scenario_creator.py (2)
53-56: Avoid reusingpfor both “perturbation” and “period” loop variables.This currently works, but it’s easy to misread and can lead to accidental bugs when this function is edited.
Proposed diff
- for p in stoch_config.perturbations: - if p.scenario != scenario_name: + for perturbation in stoch_config.perturbations: + if perturbation.scenario != scenario_name: continue @@ - target_param = cast(dict[Any, Any] | None, data_dict.get(p.table)) + target_param = cast(dict[Any, Any] | None, data_dict.get(perturbation.table)) @@ - for r, t, p in instance.v_new_capacity: - if p == first_period: - first_stage_vars.append(instance.v_new_capacity[r, t, p]) + for r, t, period in instance.v_new_capacity: + if period == first_period: + first_stage_vars.append(instance.v_new_capacity[r, t, period])Also applies to: 113-116
21-34: UseTypedDict+Unpackto replaceAnyfor**kwargsand specifyTemoaModelas return type.
scenario_creatorhas a stable kwargs contract; encoding it withTypedDict+Unpackimproves type safety, fixes Ruff ANN401, and eliminates the need for runtime validation. The function already returns aTemoaModelfrombuild_instance().Proposed diff
from __future__ import annotations import logging import sqlite3 -from typing import TYPE_CHECKING, Any, cast +from typing import TYPE_CHECKING, TypedDict, cast +from typing import Unpack from mpisppy.utils.sputils import attach_root_node # type: ignore[import-untyped] from temoa._internal.run_actions import build_instance +from temoa.core.model import TemoaModel from temoa.components.costs import period_cost_rule from temoa.data_io.hybrid_loader import HybridLoader if TYPE_CHECKING: from temoa.core.config import TemoaConfig from temoa.data_io.hybrid_loader import LoadItem from temoa.extensions.stochastics.stochastic_config import StochasticConfig logger = logging.getLogger(__name__) +class _ScenarioCreatorKwargs(TypedDict): + temoa_config: TemoaConfig + stoch_config: StochasticConfig + -def scenario_creator(scenario_name: str, **kwargs: Any) -> Any: +def scenario_creator(scenario_name: str, **kwargs: Unpack[_ScenarioCreatorKwargs]) -> TemoaModel: """ Creator for mpi-sppy scenarios. @@ - if 'temoa_config' not in kwargs or 'stoch_config' not in kwargs: - raise ValueError("scenario_creator requires 'temoa_config' and 'stoch_config' in kwargs") - - temoa_config: TemoaConfig = kwargs['temoa_config'] - stoch_config: StochasticConfig = kwargs['stoch_config'] + temoa_config = kwargs["temoa_config"] + stoch_config = kwargs["stoch_config"]mpi-sppy is fully compatible with this narrower return type annotation.
temoa/extensions/single_vector_mga/output_summary.py (1)
18-62: Fix divide-by-zero and “0.0 means missing” delta logic.
orig_cost == 0will crash; andall((orig, option))suppresses legitimate deltas whenever either side is0.0.Proposed tweak
- conn = sqlite3.connect(config.output_database) - records: list[list[Any]] = [['Category', 'Label', 'Original', 'Option', 'Delta [%]']] - total_delta = (option_cost - orig_cost) / orig_cost * 100 - records.append(['Cost', 'Total Cost', orig_cost, option_cost, total_delta]) + with sqlite3.connect(config.output_database) as conn: + records: list[list[Any]] = [['Category', 'Label', 'Original', 'Option', 'Delta [%]']] + total_delta = ((option_cost - orig_cost) / orig_cost * 100) if orig_cost != 0 else None + records.append(['Cost', 'Total Cost', orig_cost, option_cost, total_delta]) @@ - for item in sorted(cast('Iterable[Any]', emission_labels)): + for item in sorted(cast(Iterable[Any], emission_labels)): orig = poll_emission( conn, scenarios[0], item, ) option = poll_emission(conn, scenarios[1], item) - row_delta = float((option - orig) / orig * 100) if all((orig, option)) else None - records.append(['Emission', item, orig, option, row_delta]) + row_delta = ((option - orig) / orig * 100) if orig != 0 else None + records.append(['Emission', item, orig, option, row_delta]) @@ - conn.close() + # connection closed by context managertemoa/extensions/get_comm_tech.py (2)
12-40: Replace f-string SQL with parameterized query (and use=notIS).Even if the DB is “local”, treating it as untrusted avoids easy footguns.
Proposed fix
- cur.execute( - f"SELECT DISTINCT period FROM output_flow_out WHERE scenario is '{y}'" - ) + cur.execute( + 'SELECT DISTINCT period FROM output_flow_out WHERE scenario = ?', + (y,), + )
263-324: Fix incorrect error message variable + narrowget_inforeturn type (avoidAny).Line 311 currently formats
{file_ty}(a match object / None), not the user-provided filename.Proposed tweak
-def get_info(inputs: dict[str, str]) -> Any: +def get_info( + inputs: dict[str, str], +) -> OrderedDict[str, str] | dict[str, str] | dict[str, list[int]]: @@ - if not file_ty: - raise Exception(f'The file type {file_ty} is not recognized.') + if not file_ty: + raise Exception(f'The file type {inp_file} is not recognized.')temoa/extensions/modeling_to_generate_alternatives/worker.py (3)
35-60: Add__init__return type and keepself.opttyped consistently.Proposed tweak
def __init__( @@ - solver_log_path: Path | None = None, - ): + solver_log_path: Path | None = None, + ) -> None: @@ - self.opt = None # Initialize in run() + self.opt: Any = None # Initialize in run()
76-105: Fix solver log filename off-by-one (first solve currently writes..._0.log).One possible fix
- if self.solver_log_path: + if self.solver_log_path: # add the solver log path to options, if one is provided log_location = Path( self.solver_log_path, - f'solver_log_{str(self.worker_number)}_{self.solve_count}.log', + f'solver_log_{self.worker_number}_{self.solve_count + 1}.log', ) log_location_str = str(log_location)
103-138: HandleNoneresults explicitly and apply missing appsi_highs config setup.The code relies on catching
AttributeErrorto handleNoneresults, but should branch explicitly before callingcheck_optimal_termination(solve_res). This is clearer and more robust than catching exceptions.Additionally, unlike
mc_worker.py, this worker lacks the appsi-specific config setup for appsi_highs. After settingself.opt.options = self.solver_optionson line 93, add the config assignment loop:self.opt.options = self.solver_options + if self.solver_name.startswith('appsi_'): + for k, v in self.solver_options.items(): + try: + setattr(self.opt.config, k, v) + except (ValueError, AttributeError): + passWithout this, appsi_highs solver options won't be applied (appsi solvers primarily use
self.opt.config, not theoptionsattribute).For the None handling, add an explicit check before the try block at line 120:
toc = datetime.now() + if solve_res is None: + continue try:pyproject.toml (1)
86-92: Ruff excludes all oftemoa/extensions/while mypy checks most extensions with strict typing enabled — causing a linting gap.Mypy is configured to check all extensions except
breakeven(method_of_morris, modeling_to_generate_alternatives, monte_carlo, myopic, single_vector_mga, stochastics) with strict type checking. However, ruff excludes the entiretemoa/extensions/directory. This inconsistency means potential issues like unused imports, naming violations, and other style problems go undetected in the typed extensions. The presence of per-file-ignores for stochastics (line 120) further suggests these modules were intended to be linted.temoa/extensions/modeling_to_generate_alternatives/hull.py (3)
79-108: Uselogger.exception(...)once; avoid double-logging and keep stack traces.
The currentlogger.error(...); logger.error(e)loses traceback context and is noisier than needed.Proposed fix
try: @@ - except Exception as e: + except Exception as e: # scipy.spatial._qhull.QhullError may not be easily found by mypy - logger.error( - 'Attempt at hull construction from basis vectors failed.' - '\nMay be non-recoverable. Possibly try a set of random vectors to initialize the ' - 'Hull.' - ) - logger.error(e) - raise RuntimeError('Hull construction from vectors failed. See log file') from e + logger.exception( + "Attempt at hull construction from basis vectors failed. " + "May be non-recoverable; possibly try random vectors to initialize the Hull." + ) + raise RuntimeError("Hull construction from vectors failed") from e
119-131: Wrong-dimension points should raise immediately (don’t proceed to append).
As written, you log the error but still append; that can corrupt the hull state or crash onvstack.Proposed fix
def add_point(self, point: np.ndarray) -> None: if len(point) != self.dim: - logger.error( + logger.error( 'Tried adding a point to hull (dim: %d) with wrong dimensions %d. Point: %s', self.dim, len(point), point, ) + raise ValueError(f"Point dimension {len(point)} does not match hull dimension {self.dim}") if self.all_points is None: self.all_points = np.atleast_2d(point) else: self.all_points = np.vstack((self.all_points, point))
146-153: Return a consistent 2D empty array fromget_all_norms().
np.array([])is shape(0,), which is easy to misuse; returning(0, self.dim)is safer.Proposed fix
def get_all_norms(self) -> np.ndarray: @@ - return np.array([]) + return np.empty((0, self.dim))temoa/extensions/single_vector_mga/sv_mga_sequencer.py (2)
182-200: Tightenreitvotyping (it’s always length-6 here) or add a defensive check.
r, _, i, t, v, o = reitvowill throw if config/model feeds unexpected shapes.Proposed fix
def flow_idxs_from_eac_idx( - model: TemoaModel, reitvo: tuple[Any, ...] + model: TemoaModel, reitvo: tuple[Any, Any, Any, Any, Any, Any] ) -> tuple[list[tuple[Any, ...]], list[tuple[Any, ...]]]: @@ - r, _, i, t, v, o = reitvo + r, _, i, t, v, o = reitvo
203-209:verboseparameter is dead; either remove it or honor it (and make it keyword-only).
Today it’s a public API arg that does nothing, andprint(...)happens unconditionally.Proposed fix (keyword-only + gate prints)
def construct_obj( model: TemoaModel, emission_labels: Iterable[str], capacity_labels: Iterable[str], activity_labels: Iterable[str], - verbose: bool = True, + *, + verbose: bool = True, ) -> Expression | int: @@ - logger.warning(msg) - print(msg) + logger.warning(msg) + if verbose: + print(msg)Also applies to: 233-245
temoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.py (2)
266-290: Guard againstsum(coeffs) == 0when normalizing objective coefficients.
Hull normals (and especially mixed-sign vectors) can sum to ~0, producing inf/NaN coefficients and unstable solves.Proposed fix
coeffs = np.array(coeffs_list) - coeffs /= np.sum(coeffs) # normalize + denom = float(np.sum(coeffs)) + if np.isclose(denom, 0.0): + denom = float(np.sum(np.abs(coeffs))) + if np.isclose(denom, 0.0): + logger.warning("Cannot normalize coefficient vector (zero denominator); skipping") + return None + coeffs /= denom
331-360: Guard_generate_basis_coefficients()against categories with zero active variables (divide-by-zero).
If a category’s technologies have no entries intechnology_size, you’ll build an all-zero vector and normalize by 0.Proposed fix
for selected_cat in category_mapping: res = [] if selected_cat == default_cat: continue for cat in category_mapping: num_marks = sum(technology_size[tech] for tech in category_mapping[cat]) if cat == selected_cat: @@ res.extend(marks) + if not res: + continue entry = np.array(res) - entry = entry / np.array(np.sum(entry)) + s = float(np.sum(entry)) + if np.isclose(s, 0.0): + continue + entry = entry / s q.put(entry) # high value q.put(-entry) # low value
🤖 Fix all issues with AI agents
In @pyproject.toml:
- Around line 143-145: The mypy exclude regex in pyproject.toml only matches
paths with a trailing slash for the breakeven directory; update the exclude
entry (the string assigned to exclude) so the ^temoa/extensions/breakeven part
allows either a trailing slash or end-of-string (i.e., make the trailing slash
optional using a group like a non-capturing (?:/|$) after breakeven) so both
breakeven and breakeven/ are excluded.
In @temoa/extensions/get_comm_tech.py:
- Around line 12-19: The function get_tperiods uses generic Exception for input
validation; replace those raises with ValueError (or a module-specific custom
exception) to clearly signal invalid arguments: update the two raises in
get_tperiods (the "file type not recognized" and the "Please specify a
database..." cases) to raise ValueError (or a defined custom exception class)
and apply the same change to the similar checks at the other location referenced
(the block around lines 47-55) so argument errors consistently use the
appropriate exception type.
- Around line 74-76: Change the get_comm signature to require db_dat as a
keyword-only argument (e.g., def get_comm(inp_f: str, *, db_dat: bool) ->
OrderedDict[str, str]) so callers must use db_dat=...; apply the same change to
the other functions in this file that accept db_dat (the ones around lines
142-145) and update any local callers to pass db_dat by name rather than
position. Ensure only the signature is changed (keep type hints and return type)
and run tests to catch any missed call sites.
- Around line 203-210: In is_db_overwritten, replace the broad except Exception
around sqlite3.connect(db_file) with explicit exception handling for
sqlite3.Error (and optionally OSError) so only DB/connect related errors are
caught; open the connection in a try block and catch sqlite3.Error (and/or
OSError) to return False, and ensure the connection is closed or use a context
manager for the subsequent DB usage tied to the sqlite3.connect call.
In @temoa/extensions/method_of_morris/morris_evaluate.py:
- Around line 56-68: The debug log currently appends the literal template in
setting_entry instead of a formatted message; update the loop so that you build
the formatted message using the actual values (e.g., use param_info,
mm_sample[j], set_idx_tuple and the loop index j) and append that formatted
string to log_entry before calling logger.debug (replace the unformatted
setting_entry with a formatted string built via f-string or .format or %
formatting and ensure set_idx_tuple is rendered as a string or joined indices).
In @temoa/extensions/method_of_morris/morris_sequencer.py:
- Around line 351-352: The __del__ method currently calls self.con.close()
directly which can raise during garbage collection; wrap the close operation in
a try/except that catches sqlite3.Error (and optionally Exception) to
suppress/log errors, or remove __del__ and implement an explicit close method
(e.g., close or shutdown) that calls self.con.close() with proper error handling
and call that where the object is managed; update the class to use the new close
method and ensure any callers/integration points call it instead of relying on
__del__.
- Around line 5-31: Remove the unnecessary "# type: ignore[import-untyped]"
comments from the SALib imports (the lines importing morris, sample,
compute_groups_matrix, read_param_file) so their inline type hints are used, and
replace the joblib import annotation strategy: either install and rely on the
community "joblib-stubs" package so you can remove "# type:
ignore[import-untyped]" from the "from joblib import Parallel, delayed" import,
or explicitly annotate uses of joblib with types from those stubs; additionally,
replace broad Any return/type annotations in functions that consume SALib
outputs (e.g., functions referencing evaluate, sample, morris results) with
concrete types such as numpy.typing.NDArray and Dict[str, numpy.typing.NDArray]
to satisfy stricter linters.
In @temoa/extensions/method_of_morris/morris.py:
- Around line 24-26: The evaluate function currently relies on module globals
(config and db_file) created under resources.as_file and takes an unused
parameter k; change evaluate(param_names, param_values, data, k) to accept
explicit dependencies (e.g., add parameters config and db_path:
evaluate(param_names, param_values, data, k, *, config, db_path)) and stop using
the global db_file path so it uses the stable db_path string passed in; remove
or mark k as unused (or drop it) to fix the ARG001 warning. Then update all call
sites using delayed(evaluate)(...) to pass the new config= and db_path=
arguments (ensuring db_path is a persistent string, not a Path from a
short-lived resources.as_file context) and remove any reliance on module-level
config/db_file.
In @temoa/extensions/modeling_to_generate_alternatives/hull.py:
- Line 28: The __init__ signature in the Hull class currently accepts an unused
**kwargs: Any which violates lint/type rules; remove the **kwargs parameter from
the __init__ declarations (both occurrences referenced) so the constructors
become def __init__(self, points: np.ndarray) -> None: and update any docstrings
or type hints if present, ensuring all call sites remain unchanged since they
already pass only points.
In @temoa/extensions/modeling_to_generate_alternatives/mga_sequencer.py:
- Around line 358-360: The return statement using a backslash continuation is
fragile; replace it with a single parenthesized expression so it reads as:
return (status == pyo.TerminationCondition.optimal or str(status) ==
'convergenceCriteriaSatisfied'), ensuring the same logic and avoiding
trailing-whitespace issues—update the return that compares to
pyo.TerminationCondition.optimal and the 'convergenceCriteriaSatisfied' string
accordingly.
In
@temoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.py:
- Around line 147-149: group_variable_names currently ignores the tech parameter
and returns category_mapping keys; change it to return the variable names for
the given tech from variable_index_mapping (i.e., the keys of
variable_index_mapping[tech]) and handle missing tech safely (return an empty
list if tech not present). Update the implementation of group_variable_names to
look up variable_index_mapping for the provided tech and return its variable
name keys rather than using category_mapping.
In @temoa/extensions/modeling_to_generate_alternatives/vector_manager.py:
- Around line 73-76: The abstract method finalize_tracker currently contains a
silent pass which can hide implementation omissions; replace the pass with
raising NotImplementedError (e.g., raise NotImplementedError("finalize_tracker
must be implemented by subclasses")) in the finalize_tracker method of the
abstract base so callers get a clear error, or alternatively remove the
@abstractmethod decorator if you intend it to be optional/implemented in the
base class.
- Around line 70-76: Replace the Any return on the abstract base with a generic
TypeVar: add TResult = TypeVar("TResult") and make the class Generic[TResult]
(e.g., class VectorManager(ABC, Generic[TResult])) and change
process_results(self, model: TemoaModel) -> TResult to use that TypeVar; update
subclasses such as TechActivityVectorManager to declare the concrete result type
(e.g., VectorManager[list[float]] or similar) so implementations keep their
list[float] signature and Ruff ANN401 is satisfied.
In @temoa/extensions/monte_carlo/example_builds/scenario_analyzer.py:
- Around line 14-17: The SQL uses f-string interpolation which can cause SQL
injection and quoting issues; change the cur.execute call to use a parameterized
query (use "SELECT total_system_cost FROM output_objective WHERE scenario LIKE
?") and pass a single parameter like f"{scenario_name}-%" as the second argument
to cur.execute, then keep building obj_values_tuple from the fetched rows;
reference cur.execute, scenario_name, obj_values_tuple and the output_objective
table when making the change.
- Around line 17-20: The code builds obj_values_tuple and calls plt.hist with
bins=int(sqrt(len(obj_values_tuple))) which will raise if obj_values_tuple is
empty; update the plotting block to handle empty result sets by checking
len(obj_values_tuple) (or len(obj_values)) before calling plt.hist, and either
skip plotting (no-op) or use a safe fallback (e.g., bins=1) and/or log a
warning; make this change around the obj_values_tuple creation and the plt.hist
call so the empty-case is handled before invoking plt.show().
- Line 6: The code currently imports and/or constructs sqlite3.Connection
directly (symbol: Connection); replace that with the sqlite3.connect factory:
remove "from sqlite3 import Connection", import the sqlite3 module (or reference
sqlite3) and change any direct instantiation of Connection(...) to
sqlite3.connect(db_path, ...) so connections are created via sqlite3.connect
instead of direct construction (also update the other occurrence noted around
line 12).
In @temoa/extensions/monte_carlo/mc_sequencer.py:
- Line 55: The __init__ method in the class (def __init__(self, config:
TemoaConfig)) is missing an explicit return type; update the signature to
include -> None to satisfy Ruff and typing conventions (i.e., change def
__init__(self, config: TemoaConfig) to def __init__(self, config: TemoaConfig)
-> None) and run type/flake checks to confirm the warning is resolved.
In @temoa/extensions/myopic/myopic_progress_mapper.py:
- Around line 52-55: The report method currently accepts a plain str and
performs a runtime validation; change the signature of
MyopicProgressMapper.report to use
typing.Literal['load','solve','report','check'] for the status parameter so
invalid states are caught by mypy, add the necessary import for Literal (from
typing import Literal), and remove the redundant runtime ValueError check (or
keep it as an assert if you want a runtime guard) in the
MyopicProgressMapper.report implementation.
In @temoa/extensions/myopic/myopic_sequencer.py:
- Around line 66-77: Class attributes output_con, cursor, table_writer,
view_depth, step_size (and similar members declared non-optional) can be left
unset when config is None; change their declarations to optional (e.g., append |
None) and initialize them to None in the class or __init__, or alternatively
ensure they are always set in __init__ when config is None by providing sensible
defaults; update types for instance_queue/progress_mapper/config if needed to
accurately reflect possible None values and adjust any uses to handle None
safely (refer to output_con, cursor, table_writer, view_depth, step_size,
instance_queue, progress_mapper, config, and __init__).
In @temoa/extensions/single_vector_mga/output_summary.py:
- Around line 4-15: The code currently uses a string-based cast like
cast('Iterable[Any]', ...) and only imports Iterable under TYPE_CHECKING;
instead import Iterable directly from collections.abc at top-level and change
the cast to use the real type (cast(Iterable[Any], ...)) so you avoid string
type names; since from __future__ import annotations is enabled, a runtime
import of Iterable is fine—update the imports (remove Iterable from the
TYPE_CHECKING block) and replace any cast('Iterable[Any]', ...) occurrences with
cast(Iterable[Any], ...).
In @temoa/extensions/stochastics/stochastic_sequencer.py:
- Around line 43-47: The ImportError is logged but the original exception
message is dropped when re-raising; update the exception raised in the
try/except around importing from mpisppy.opt.ef (where ExtensiveForm is imported
and logger.exception is called) to include the original ImportError message
(e.g., incorporate the caught exception 'e' into the RuntimeError message) and
still use "raise ... from e" to preserve the traceback.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (23)
pyproject.tomltemoa/extensions/get_comm_tech.pytemoa/extensions/method_of_morris/morris.pytemoa/extensions/method_of_morris/morris_evaluate.pytemoa/extensions/method_of_morris/morris_sequencer.pytemoa/extensions/modeling_to_generate_alternatives/hull.pytemoa/extensions/modeling_to_generate_alternatives/manager_factory.pytemoa/extensions/modeling_to_generate_alternatives/mga_sequencer.pytemoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.pytemoa/extensions/modeling_to_generate_alternatives/vector_manager.pytemoa/extensions/modeling_to_generate_alternatives/worker.pytemoa/extensions/monte_carlo/example_builds/scenario_analyzer.pytemoa/extensions/monte_carlo/mc_run.pytemoa/extensions/monte_carlo/mc_sequencer.pytemoa/extensions/monte_carlo/mc_worker.pytemoa/extensions/myopic/myopic_index.pytemoa/extensions/myopic/myopic_progress_mapper.pytemoa/extensions/myopic/myopic_sequencer.pytemoa/extensions/single_vector_mga/output_summary.pytemoa/extensions/single_vector_mga/sv_mga_sequencer.pytemoa/extensions/stochastics/scenario_creator.pytemoa/extensions/stochastics/stochastic_config.pytemoa/extensions/stochastics/stochastic_sequencer.py
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: ParticularlyPythonicBS
Repo: TemoaProject/temoa PR: 177
File: temoa/model_checking/commodity_network.py:26-33
Timestamp: 2025-10-27T15:53:41.829Z
Learning: The Temoa project requires Python 3.12 or above as the minimum supported version, so PEP 695 `type` syntax for type aliases is appropriate and preferred over `TypeAlias`.
📚 Learning: 2025-11-03T22:55:02.519Z
Learnt from: ParticularlyPythonicBS
Repo: TemoaProject/temoa PR: 186
File: temoa/_internal/temoa_sequencer.py:60-63
Timestamp: 2025-11-03T22:55:02.519Z
Learning: In `TemoaSequencer.__init__`, when a `mode_override` is provided, it must be written back to `self.config.scenario_mode` (not just stored in `self.temoa_mode`) because `HybridLoader.create_data_dict()` has strict consistency checks that validate `config.scenario_mode` matches the presence/absence of `myopic_index`. Without this synchronization, MYOPIC mode would fail with a RuntimeError when the loader is instantiated.
Applied to files:
temoa/extensions/myopic/myopic_sequencer.pytemoa/extensions/modeling_to_generate_alternatives/mga_sequencer.py
📚 Learning: 2025-11-03T13:34:35.907Z
Learnt from: ParticularlyPythonicBS
Repo: TemoaProject/temoa PR: 184
File: temoa/data_processing/database_util.py:109-118
Timestamp: 2025-11-03T13:34:35.907Z
Learning: In the Temoa project, SQL injection vulnerabilities in database utility classes (such as temoa/data_processing/database_util.py) are considered low concern due to the data access patterns and should not be flagged during code reviews. Technical debt for SQL injection hardening should be tracked separately as issues rather than blocking PRs.
Applied to files:
temoa/extensions/myopic/myopic_sequencer.py
📚 Learning: 2025-11-03T13:34:35.907Z
Learnt from: ParticularlyPythonicBS
Repo: TemoaProject/temoa PR: 184
File: temoa/data_processing/database_util.py:109-118
Timestamp: 2025-11-03T13:34:35.907Z
Learning: In the Temoa project, SQL injection vulnerabilities in the database utility classes (such as temoa/data_processing/database_util.py) are considered low concern due to the data access patterns and should not be flagged during reviews. Technical debt for SQL injection hardening should be tracked separately as issues rather than blocking PRs.
Applied to files:
temoa/extensions/myopic/myopic_sequencer.py
🧬 Code graph analysis (12)
temoa/extensions/method_of_morris/morris_sequencer.py (1)
temoa/core/config.py (1)
TemoaConfig(32-344)
temoa/extensions/myopic/myopic_progress_mapper.py (1)
temoa/extensions/myopic/myopic_index.py (1)
MyopicIndex(9-29)
temoa/extensions/method_of_morris/morris_evaluate.py (2)
temoa/core/config.py (1)
TemoaConfig(32-344)temoa/extensions/method_of_morris/morris.py (1)
evaluate(24-63)
temoa/extensions/monte_carlo/mc_worker.py (1)
temoa/_internal/data_brick.py (2)
DataBrick(23-83)data_brick_factory(86-117)
temoa/extensions/modeling_to_generate_alternatives/vector_manager.py (2)
temoa/core/model.py (1)
TemoaModel(93-1137)temoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.py (4)
group_members(243-244)group_variable_names(147-148)process_results(206-234)finalize_tracker(374-381)
temoa/extensions/modeling_to_generate_alternatives/manager_factory.py (4)
temoa/extensions/modeling_to_generate_alternatives/mga_constants.py (2)
MgaAxis(7-11)MgaWeighting(15-17)temoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.py (1)
TechActivityVectorManager(46-381)temoa/core/model.py (1)
TemoaModel(93-1137)temoa/extensions/modeling_to_generate_alternatives/vector_manager.py (1)
VectorManager(16-76)
temoa/extensions/stochastics/scenario_creator.py (3)
temoa/_internal/run_actions.py (1)
build_instance(121-155)temoa/data_io/hybrid_loader.py (1)
HybridLoader(70-930)temoa/data_io/loader_manifest.py (1)
LoadItem(26-59)
temoa/extensions/monte_carlo/mc_run.py (3)
temoa/data_io/hybrid_loader.py (1)
HybridLoader(70-930)temoa/core/config.py (1)
TemoaConfig(32-344)tests/test_mc_run.py (1)
tweak_factory(7-11)
temoa/extensions/method_of_morris/morris.py (3)
temoa/_internal/table_writer.py (2)
TableWriter(74-774)close(120-128)temoa/core/config.py (2)
TemoaConfig(32-344)build_config(201-253)temoa/extensions/method_of_morris/morris_evaluate.py (1)
evaluate(38-111)
temoa/extensions/stochastics/stochastic_sequencer.py (2)
temoa/extensions/stochastics/stochastic_config.py (1)
StochasticConfig(31-92)temoa/core/config.py (1)
TemoaConfig(32-344)
temoa/extensions/single_vector_mga/output_summary.py (1)
temoa/core/config.py (1)
TemoaConfig(32-344)
temoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.py (2)
temoa/extensions/modeling_to_generate_alternatives/hull.py (2)
Hull(15-169)update(79-117)temoa/extensions/modeling_to_generate_alternatives/vector_manager.py (8)
VectorManager(16-76)expired(47-52)group_variable_names(55-57)random_input_vector_model(60-62)process_results(70-71)groups(36-38)group_members(41-43)finalize_tracker(74-76)
🪛 Ruff (0.14.10)
temoa/extensions/monte_carlo/mc_sequencer.py
55-55: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
temoa/extensions/get_comm_tech.py
16-16: Create your own exception
(TRY002)
16-16: Avoid specifying long messages outside the exception class
(TRY003)
19-19: Create your own exception
(TRY002)
19-19: Avoid specifying long messages outside the exception class
(TRY003)
35-35: Possible SQL injection vector through string-based query construction
(S608)
51-51: Create your own exception
(TRY002)
51-51: Avoid specifying long messages outside the exception class
(TRY003)
54-54: Create your own exception
(TRY002)
54-54: Avoid specifying long messages outside the exception class
(TRY003)
74-74: Boolean-typed positional argument in function definition
(FBT001)
142-142: Boolean-typed positional argument in function definition
(FBT001)
209-209: Do not catch blind exception: Exception
(BLE001)
263-263: Dynamically typed expressions (typing.Any) are disallowed in get_info
(ANN401)
temoa/extensions/method_of_morris/morris_sequencer.py
138-138: Dynamically typed expressions (typing.Any) are disallowed in start
(ANN401)
204-204: Dynamically typed expressions (typing.Any) are disallowed in mm_samples
(ANN401)
205-205: Dynamically typed expressions (typing.Any) are disallowed in process_results
(ANN401)
temoa/extensions/modeling_to_generate_alternatives/hull.py
28-28: Unused method argument: kwargs
(ARG002)
28-28: Dynamically typed expressions (typing.Any) are disallowed in **kwargs
(ANN401)
101-105: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
106-106: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
107-107: Avoid specifying long messages outside the exception class
(TRY003)
temoa/extensions/method_of_morris/morris_evaluate.py
23-23: Dynamically typed expressions (typing.Any) are disallowed in log_queue
(ANN401)
38-38: Dynamically typed expressions (typing.Any) are disallowed in mm_sample
(ANN401)
39-39: Dynamically typed expressions (typing.Any) are disallowed in log_queue
(ANN401)
61-61: Avoid specifying long messages outside the exception class
(TRY003)
63-63: Avoid specifying long messages outside the exception class
(TRY003)
100-103: Avoid specifying long messages outside the exception class
(TRY003)
temoa/extensions/monte_carlo/mc_worker.py
46-46: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
54-54: Dynamically typed expressions (typing.Any) are disallowed in **kwargs
(ANN401)
159-159: datetime.datetime.now() called without a tz argument
(DTZ005)
temoa/extensions/modeling_to_generate_alternatives/vector_manager.py
70-70: Dynamically typed expressions (typing.Any) are disallowed in process_results
(ANN401)
temoa/extensions/modeling_to_generate_alternatives/manager_factory.py
24-24: Dynamically typed expressions (typing.Any) are disallowed in **kwargs
(ANN401)
33-33: Avoid specifying long messages outside the exception class
(TRY003)
temoa/extensions/stochastics/scenario_creator.py
21-21: Dynamically typed expressions (typing.Any) are disallowed in **kwargs
(ANN401)
21-21: Dynamically typed expressions (typing.Any) are disallowed in scenario_creator
(ANN401)
temoa/extensions/myopic/myopic_sequencer.py
97-97: Avoid specifying long messages outside the exception class
(TRY003)
101-101: Avoid specifying long messages outside the exception class
(TRY003)
121-121: Avoid specifying long messages outside the exception class
(TRY003)
186-186: Avoid specifying long messages outside the exception class
(TRY003)
198-198: Avoid specifying long messages outside the exception class
(TRY003)
200-200: Avoid specifying long messages outside the exception class
(TRY003)
560-560: Avoid specifying long messages outside the exception class
(TRY003)
574-574: Possible SQL injection vector through string-based query construction
(S608)
temoa/extensions/monte_carlo/mc_run.py
43-43: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
70-70: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
139-139: Avoid specifying long messages outside the exception class
(TRY003)
144-144: Avoid specifying long messages outside the exception class
(TRY003)
175-175: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
224-224: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
temoa/extensions/method_of_morris/morris.py
24-24: Dynamically typed expressions (typing.Any) are disallowed in param_values
(ANN401)
25-25: Unused function argument: k
(ARG001)
temoa/extensions/modeling_to_generate_alternatives/worker.py
35-35: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
117-117: datetime.datetime.now() called without a tz argument
(DTZ005)
temoa/extensions/single_vector_mga/sv_mga_sequencer.py
208-208: Boolean-typed positional argument in function definition
(FBT001)
208-208: Boolean default positional argument in function definition
(FBT002)
208-208: Unused static method argument: verbose
(ARG004)
temoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.py
147-147: Unused method argument: tech
(ARG002)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: setup and test (macos-latest, 3.12)
- GitHub Check: setup and test (windows-latest, 3.13)
- GitHub Check: setup and test (windows-latest, 3.12)
- GitHub Check: setup and test (macos-latest, 3.13)
| # Exclude specific directories from type checking will try to add them back gradually | ||
| exclude = "(?x)(^temoa/extensions/|^temoa/utilities/|^stubs/)" | ||
| exclude = "(?x)(^temoa/utilities/|^stubs/|^temoa/extensions/breakeven/)" | ||
|
|
There was a problem hiding this comment.
Tighten the mypy exclude regex to match breakeven both with and without trailing slash.
Current regex only matches ^temoa/extensions/breakeven/… paths.
Proposed tweak
-exclude = "(?x)(^temoa/utilities/|^stubs/|^temoa/extensions/breakeven/)"
+exclude = "(?x)(^temoa/utilities/|^stubs/|^temoa/extensions/breakeven(?:/|$))"🤖 Prompt for AI Agents
In @pyproject.toml around lines 143 - 145, The mypy exclude regex in
pyproject.toml only matches paths with a trailing slash for the breakeven
directory; update the exclude entry (the string assigned to exclude) so the
^temoa/extensions/breakeven part allows either a trailing slash or end-of-string
(i.e., make the trailing slash optional using a group like a non-capturing
(?:/|$) after breakeven) so both breakeven and breakeven/ are excluded.
| def get_tperiods(inp_f: str) -> dict[str, list[int]]: | ||
| file_ty = re.search(r'(\w+)\.(\w+)\b', inp_f) # Extract the input filename and extension | ||
|
|
||
| if not file_ty: | ||
| raise 'The file type %s is not recognized.' % inp_f | ||
| raise Exception(f'The file type {inp_f} is not recognized.') | ||
|
|
||
| elif file_ty.group(2) not in ('db', 'sqlite', 'sqlite3', 'sqlitedb'): | ||
| raise Exception('Please specify a database for finding scenarios') |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer ValueError/custom exceptions over generic Exception for argument validation.
Also applies to: 47-55
🧰 Tools
🪛 Ruff (0.14.10)
16-16: Create your own exception
(TRY002)
16-16: Avoid specifying long messages outside the exception class
(TRY003)
19-19: Create your own exception
(TRY002)
19-19: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In @temoa/extensions/get_comm_tech.py around lines 12 - 19, The function
get_tperiods uses generic Exception for input validation; replace those raises
with ValueError (or a module-specific custom exception) to clearly signal
invalid arguments: update the two raises in get_tperiods (the "file type not
recognized" and the "Please specify a database..." cases) to raise ValueError
(or a defined custom exception class) and apply the same change to the similar
checks at the other location referenced (the block around lines 47-55) so
argument errors consistently use the appropriate exception type.
| def get_comm(inp_f: str, db_dat: bool) -> OrderedDict[str, str]: | ||
| comm_list: dict[str, str] = {} | ||
| comm_set: set[str] = set() |
There was a problem hiding this comment.
Boolean positional params (db_dat: bool) are hard to read at call sites—prefer keyword-only usage.
Also applies to: 142-145
🧰 Tools
🪛 Ruff (0.14.10)
74-74: Boolean-typed positional argument in function definition
(FBT001)
🤖 Prompt for AI Agents
In @temoa/extensions/get_comm_tech.py around lines 74 - 76, Change the get_comm
signature to require db_dat as a keyword-only argument (e.g., def
get_comm(inp_f: str, *, db_dat: bool) -> OrderedDict[str, str]) so callers must
use db_dat=...; apply the same change to the other functions in this file that
accept db_dat (the ones around lines 142-145) and update any local callers to
pass db_dat by name rather than position. Ensure only the signature is changed
(keep type hints and return type) and run tests to catch any missed call sites.
| def is_db_overwritten(db_file: str, inp_dat_file: str) -> bool: | ||
| if os.path.basename(db_file) == '0': | ||
| return False | ||
|
|
||
| try: | ||
| con = sqlite3.connect(db_file) | ||
| except: | ||
| except Exception: | ||
| return False |
There was a problem hiding this comment.
Don’t catch broad Exception on sqlite connect; catch sqlite3.Error (or OSError) explicitly.
Proposed tweak
- try:
- con = sqlite3.connect(db_file)
- except Exception:
+ try:
+ con = sqlite3.connect(db_file)
+ except sqlite3.Error:
return False📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def is_db_overwritten(db_file: str, inp_dat_file: str) -> bool: | |
| if os.path.basename(db_file) == '0': | |
| return False | |
| try: | |
| con = sqlite3.connect(db_file) | |
| except: | |
| except Exception: | |
| return False | |
| def is_db_overwritten(db_file: str, inp_dat_file: str) -> bool: | |
| if os.path.basename(db_file) == '0': | |
| return False | |
| try: | |
| con = sqlite3.connect(db_file) | |
| except sqlite3.Error: | |
| return False |
🧰 Tools
🪛 Ruff (0.14.10)
209-209: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In @temoa/extensions/get_comm_tech.py around lines 203 - 210, In
is_db_overwritten, replace the broad except Exception around
sqlite3.connect(db_file) with explicit exception handling for sqlite3.Error (and
optionally OSError) so only DB/connect related errors are caught; open the
connection in a try block and catch sqlite3.Error (and/or OSError) to return
False, and ensure the connection is closed or use a context manager for the
subsequent DB usage tied to the sqlite3.connect call.
| def evaluate(param_info: dict[int, list[Any]], mm_sample: Any, data: dict[str, Any], | ||
| i: int, config: TemoaConfig, log_queue: Any, log_level: int) -> list[float]: | ||
| """ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the morris_evaluate.py file structure
cat -n temoa/extensions/method_of_morris/morris_evaluate.py | head -100Repository: TemoaProject/temoa
Length of output: 4600
🏁 Script executed:
# Check the TableWriter and sqlite3 connection patterns
rg -n "def write_mm_results|write_mm_results\(" -A 5 temoa/ --type=pyRepository: TemoaProject/temoa
Length of output: 1191
🏁 Script executed:
# Search for sqlite3.connect calls in the codebase
rg -n "sqlite3\.connect\(" -B 2 -A 5 temoa/ --type=pyRepository: TemoaProject/temoa
Length of output: 23652
🏁 Script executed:
# Look for PRAGMA settings and busy_timeout configurations
rg -n "PRAGMA|busy_timeout|timeout" -B 2 -A 2 temoa/ --type=pyRepository: TemoaProject/temoa
Length of output: 11727
🏁 Script executed:
# Check table_writer.py specifically for database write handling
cat -n temoa/_internal/table_writer.pyRepository: TemoaProject/temoa
Length of output: 33661
Verify SQLite concurrency strategy for parallel workers—no timeouts/WAL configured.
Each worker calls TableWriter.write_mm_results(...) (which writes to output_database with PRAGMA foreign_keys = OFF only) and also opens its own sqlite3.connect(config.input_database) for reading. With multiple workers running in parallel, SQLite can hit "database is locked" errors unless you configure WAL mode (PRAGMA journal_mode = WAL) and/or set explicit busy timeouts (e.g., connection.execute('PRAGMA busy_timeout = 5000')). The default 5-second timeout on sqlite3.connect() may be insufficient under concurrent load.
Also applies to: 77-80
🧰 Tools
🪛 Ruff (0.14.10)
38-38: Dynamically typed expressions (typing.Any) are disallowed in mm_sample
(ANN401)
39-39: Dynamically typed expressions (typing.Any) are disallowed in log_queue
(ANN401)
| from __future__ import annotations | ||
|
|
||
| import sqlite3 | ||
| from sqlite3 import Connection | ||
| from typing import TYPE_CHECKING, Any, cast | ||
|
|
||
| import tabulate | ||
| if TYPE_CHECKING: | ||
| from collections.abc import Iterable | ||
|
|
||
| from temoa.core.config import TemoaConfig | ||
| from temoa.core.config import TemoaConfig | ||
|
|
||
|
|
||
| import tabulate # type: ignore[import-untyped] |
There was a problem hiding this comment.
Avoid string-based cast('Iterable[Any]', ...); import Iterable normally (cheaper + clearer).
With postponed annotations already enabled, a runtime import from collections.abc is fine and keeps cast() idiomatic.
Proposed tweak
-from typing import TYPE_CHECKING, Any, cast
+from collections.abc import Iterable
+from typing import TYPE_CHECKING, Any, cast
@@
- for item in sorted(cast('Iterable[Any]', emission_labels)):
+ for item in sorted(cast(Iterable[Any], emission_labels)):
@@
- for item in sorted(cast('Iterable[Any]', activity_labels)):
+ for item in sorted(cast(Iterable[Any], activity_labels)):
@@
- for item in sorted(cast('Iterable[Any]', capacity_labels)):
+ for item in sorted(cast(Iterable[Any], capacity_labels)):🤖 Prompt for AI Agents
In @temoa/extensions/single_vector_mga/output_summary.py around lines 4 - 15,
The code currently uses a string-based cast like cast('Iterable[Any]', ...) and
only imports Iterable under TYPE_CHECKING; instead import Iterable directly from
collections.abc at top-level and change the cast to use the real type
(cast(Iterable[Any], ...)) so you avoid string type names; since from __future__
import annotations is enabled, a runtime import of Iterable is fine—update the
imports (remove Iterable from the TYPE_CHECKING block) and replace any
cast('Iterable[Any]', ...) occurrences with cast(Iterable[Any], ...).
| svmga_inputs = config.svmga_inputs or {} | ||
| self.cost_epsilon: float = float(svmga_inputs.get('cost_epsilon', 0.05)) # type: ignore[arg-type] | ||
| logger.info('Set SVMGA cost (relaxation) epsilon to: %0.3f', self.cost_epsilon) |
There was a problem hiding this comment.
Stop papering over config types with type: ignore[arg-type]; normalize inputs once.
Right now, you ignore typing for cost_epsilon and label lists at call sites, which makes the “typed extensions” story fragile.
Proposed fix (normalize + validate)
svmga_inputs = config.svmga_inputs or {}
- self.cost_epsilon: float = float(svmga_inputs.get('cost_epsilon', 0.05)) # type: ignore[arg-type]
+ cost_eps_raw = svmga_inputs.get("cost_epsilon", 0.05)
+ self.cost_epsilon = float(cost_eps_raw)
@@
svmga_inputs = self.config.svmga_inputs or {}
- emission_labels = svmga_inputs.get('emission_labels', [])
- capacity_labels = svmga_inputs.get('capacity_labels', [])
- activity_labels = svmga_inputs.get('activity_labels', [])
+ emission_labels = [str(x) for x in svmga_inputs.get("emission_labels", [])]
+ capacity_labels = [str(x) for x in svmga_inputs.get("capacity_labels", [])]
+ activity_labels = [str(x) for x in svmga_inputs.get("activity_labels", [])]
new_obj = SvMgaSequencer.construct_obj(
instance,
- emission_labels, # type: ignore[arg-type]
- capacity_labels, # type: ignore[arg-type]
- activity_labels, # type: ignore[arg-type]
+ emission_labels,
+ capacity_labels,
+ activity_labels,
)Also applies to: 125-134
| table_index_map: dict[str, list[str]] = {} | ||
| item: LoadItem | ||
| for item in hybrid_loader.manifest: | ||
| if item.table not in table_index_map and item.columns: | ||
| table_index_map[item.table] = item.columns[:-1] | ||
| table_index_map[item.table] = list(item.columns[:-1]) | ||
| except Exception as e: |
There was a problem hiding this comment.
Fix cast type + harden index mapping against non-tuple keys / length mismatch.
cast('dict[Any, Any] | None', ...)passes a string as the type argument.zip(..., strict=True)can raise and abort the entire scenario ifidx_tupleisn’t a tuple (or lengths don’t match); this is especially risky in perturbation code.
Proposed diff
- target_param = cast('dict[Any, Any] | None', data_dict.get(p.table))
+ target_param = cast(dict[Any, Any] | None, data_dict.get(p.table))
@@
- for idx_tuple, current_val in list(target_param.items()):
+ for idx_tuple, current_val in list(target_param.items()):
# Map index tuple to names based on table manifest
- index_map = dict(zip(index_cols, idx_tuple, strict=True))
+ key = idx_tuple if isinstance(idx_tuple, tuple) else (idx_tuple,)
+ if len(key) != len(index_cols):
+ logger.warning(
+ "Index/key length mismatch for table %s in scenario %s: expected %d cols %s, got key %r",
+ p.table,
+ scenario_name,
+ len(index_cols),
+ index_cols,
+ idx_tuple,
+ )
+ continue
+ index_map = dict(zip(index_cols, key, strict=True))Also applies to: 57-58, 75-78
| try: | ||
| from mpisppy.opt.ef import ExtensiveForm | ||
| from mpisppy.opt.ef import ExtensiveForm # type: ignore[import-untyped] | ||
| except ImportError as e: | ||
| logger.exception('mpi-sppy is not installed. Please install it to use stochastic mode.') | ||
| raise RuntimeError('mpi-sppy not found') from e |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n temoa/extensions/stochastics/stochastic_sequencer.py | sed -n '40,50p'Repository: TemoaProject/temoa
Length of output: 575
Include the original ImportError message in the RuntimeError for better debugging.
The current code logs the exception but discards the original error message when re-raising. Since ImportError messages vary (e.g., "No module named 'mpisppy'" vs. submodule issues), including this detail in the RuntimeError message improves visibility without adding complexity.
Suggested change
except ImportError as e:
logger.exception('mpi-sppy is not installed. Please install it to use stochastic mode.')
- raise RuntimeError('mpi-sppy not found') from e
+ raise RuntimeError(f'mpi-sppy not found: {e}') from e🧰 Tools
🪛 Ruff (0.14.10)
47-47: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In @temoa/extensions/stochastics/stochastic_sequencer.py around lines 43 - 47,
The ImportError is logged but the original exception message is dropped when
re-raising; update the exception raised in the try/except around importing from
mpisppy.opt.ef (where ExtensiveForm is imported and logger.exception is called)
to include the original ImportError message (e.g., incorporate the caught
exception 'e' into the RuntimeError message) and still use "raise ... from e" to
preserve the traceback.
ParticularlyPythonicBS
force-pushed
the
typing/extensions
branch
from
ab26865 to
e0d8c6b
Compare
1 participant
Adds typing for all extensions except breakeven.
Summary by CodeRabbit
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.