Update monte carlo to work with v4 restructure and HiGHs defaults #244
Conversation
ParticularlyPythonicBS
added
Maintenance
|
Warning Rate limit exceeded@ParticularlyPythonicBS has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 3 minutes and 49 seconds before requesting another review. ⌛ 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. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
WalkthroughThis pull request updates Monte Carlo documentation, refactors multiprocessing architecture in the MC worker and sequencer components, standardizes output table field naming, and introduces comprehensive testing coverage for Monte Carlo functionality including configuration files and validation tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 4
🤖 Fix all issues with AI Agents
In @temoa/extensions/monte_carlo/mc_sequencer.py:
- Around line 65-71: The except FileNotFoundError block in mc_sequencer.py
currently logs via logger.error()/warning() without the traceback; update that
handler to call logger.exception (preserving the existing messages and
options_file_path usage) so the traceback is captured for diagnostics, then keep
the fallback behavior setting s_options = {} and all_options = {} unchanged;
locate the except block that handles FileNotFoundError and replace the
logger.error/logger.warning calls with logger.exception while retaining the
conditional message based on options_file_path.
In @temoa/extensions/monte_carlo/mc_worker.py:
- Line 117: The timestamp assignment uses naive datetime.now(); change it to a
timezone-aware call (e.g., datetime.now(tz=timezone.utc)) and add the necessary
import (timezone) so that the tic variable in mc_worker.py is timezone-aware;
this keeps elapsed-time calculations the same but ensures consistency for any
future uses of tic.
In @tests/test_full_runs.py:
- Line 173: The unpacked variable data_name in the tuple assignment for
system_test_run is unused; rename it to _data_name (e.g., change "data_name, _,
_, sequencer = system_test_run" to "_data_name, _, _, sequencer =
system_test_run") to indicate it is intentionally unused and satisfy the linter.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/testing_data/mc_settings_utopia.csvis excluded by!**/*.csv
📒 Files selected for processing (9)
docs/source/computational_implementation.rstdocs/source/index.rstdocs/source/monte_carlo.rsttemoa/_internal/table_writer.pytemoa/extensions/monte_carlo/mc_sequencer.pytemoa/extensions/monte_carlo/mc_worker.pytests/conftest.pytests/test_full_runs.pytests/testing_configs/config_utopia_mc.toml
🧰 Additional context used
🧬 Code graph analysis (2)
temoa/extensions/monte_carlo/mc_sequencer.py (2)
temoa/extensions/monte_carlo/mc_worker.py (1)
run(57-169)temoa/extensions/modeling_to_generate_alternatives/mga_sequencer.py (1)
start(123-318)
tests/test_full_runs.py (1)
tests/conftest.py (1)
system_test_run(189-216)
🪛 Ruff (0.14.10)
temoa/extensions/monte_carlo/mc_sequencer.py
67-67: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
tests/test_full_runs.py
173-173: Unpacked variable data_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
temoa/extensions/monte_carlo/mc_worker.py
110-110: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
117-117: 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 (macos-latest, 3.12)
- GitHub Check: setup and test (windows-latest, 3.12)
- GitHub Check: setup and test (windows-latest, 3.13)
🔇 Additional comments (14)
docs/source/index.rst (1)
7-7: LGTM!The addition of the
monte_carloentry to the toctree properly integrates the new Monte Carlo documentation into the navigation structure.docs/source/monte_carlo.rst (1)
1-124: Excellent documentation coverage!The Monte Carlo documentation is comprehensive and well-structured, covering:
- Framework overview and capabilities
- Configuration setup with TOML examples
- CSV input format with clear column descriptions
- Adjustment types (absolute, relative, substitution) with formulas
- Advanced indexing features (wildcards and multi-tokens)
- Parallel execution details
- Output structure and tweak logging
The field names in the tweak log table (lines 120-124) correctly align with the renamed fields in
table_writer.py.docs/source/computational_implementation.rst (1)
389-389: LGTM!The update to use a Sphinx doc directive (
:doc:monte_carlo``) properly creates a cross-reference to the new Monte Carlo documentation, improving navigation and documentation structure.temoa/extensions/monte_carlo/mc_worker.py (4)
31-56: Correct refactoring to support spawn-based multiprocessing.The removal of
Processinheritance and the deferred solver initialization (line 50:self.opt = None) align with the PR's objective to fix HiGHS solver initialization issues. By makingMCWorkera plain class and initializing the solver inrun()(line 70), each worker process starts with a fresh solver instance, avoiding the memory-copy issues that occur with the fork multiprocessing context.
118-128: Appropriate handling of solver-specific interfaces.The different solve approaches for
appsi_vs non-appsi solvers correctly handle the different configuration methods:
- appsi solvers use config-based settings (lines 122-123)
- Other solvers use parameter-based settings (line 128)
This ensures terminal output (
tee=True) is enabled for both solver types.
130-144: Good enhancement to error diagnostics.The addition of solver results status logging (lines 140-143) when an exception occurs provides valuable diagnostic information. The safe attribute checking with
hasattrensures this doesn't cause secondary errors.
163-165: Updated logging format is clear.The multi-line logging format for non-optimal solves improves readability while maintaining the same information content.
temoa/_internal/table_writer.py (1)
746-750: No action required. The field name changes in theoutput_mc_deltatable have been properly implemented across all integration points:
- Database schema (
temoa/extensions/monte_carlo/make_deltas_table.sql): Table definition uses the new column names (run, param, old_val, new_val)- Documentation (
docs/source/monte_carlo.rst): Updated to document the new field names- Tests (
tests/test_full_runs.py): Queries already use the new field names- Code mapping (
table_writer.pylines 746-750): Correctly maps from internal ChangeRecord fields to database columnsThe
ChangeRecordnamedtuple inmc_run.pyretains the old field names internally (param_name, old_value, new_value), but this is acceptable since the mapping to the new database column names occurs at the table writer boundary.tests/conftest.py (1)
83-83: LGTM!The new database mapping follows the established pattern and correctly enables the Monte Carlo test scenario by reusing the same
utopia_data.sqlsource file.tests/test_full_runs.py (1)
164-203: Test structure looks solid.The Monte Carlo integration test properly validates:
- The
output_mc_deltatable records parameter tweaks correctly- The
output_objectivetable contains the expected scenario variations- Database connection is properly closed
This addresses the PR objective of adding full Monte Carlo integration testing.
temoa/extensions/monte_carlo/mc_sequencer.py (3)
115-126: Spawn context correctly addresses the HiGHS solver state issue.Using
multiprocessing.get_context('spawn')ensures each worker starts in a fresh Python interpreter state, avoiding the copied solver-initialized memory that caused infeasibility when using the defaultforkcontext on Linux. The queues are correctly created via the same context.
147-149: Worker process setup is correct.Creating processes via
ctx.Process(target=w.run, daemon=True)properly separates theMCWorkerobject from the multiprocessing infrastructure. Thedaemon=Trueflag ensures workers are terminated if the main process exits unexpectedly, which is appropriate for this use case since proper shutdown is handled via the 'ZEBRA' signal.
42-51: Path resolution logic is well-implemented.The code correctly handles both absolute and relative paths for custom solver options, with relative paths resolved against the config file location. This aligns with the PR objective of exposing Monte Carlo solver options in the config.
tests/testing_configs/config_utopia_mc.toml (1)
1-16: LGTM! Configuration structure is correct.The Monte Carlo test configuration properly sets up the scenario mode, solver, and references an existing run settings file. Using
appsi_highsaligns with the PR objective to address HiGHS solver state issues.
| except FileNotFoundError: | ||
| logger.warning('Unable to find solver options toml file. Using default options.') | ||
| if options_file_path: | ||
| logger.error('Unable to find custom solver options file at: %s', options_file_path) | ||
| else: | ||
| logger.warning('Unable to find default solver options toml file.') | ||
| s_options = {} | ||
| all_options = {} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using logger.exception for better diagnostics.
When handling FileNotFoundError, using logger.exception would include the traceback, which can be helpful for debugging path resolution issues.
🔎 Proposed fix
except FileNotFoundError:
if options_file_path:
- logger.error('Unable to find custom solver options file at: %s', options_file_path)
+ logger.exception('Unable to find custom solver options file at: %s', options_file_path)
else:
logger.warning('Unable to find default solver options toml file.')
s_options = {}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except FileNotFoundError: | |
| logger.warning('Unable to find solver options toml file. Using default options.') | |
| if options_file_path: | |
| logger.error('Unable to find custom solver options file at: %s', options_file_path) | |
| else: | |
| logger.warning('Unable to find default solver options toml file.') | |
| s_options = {} | |
| all_options = {} | |
| except FileNotFoundError: | |
| if options_file_path: | |
| logger.exception('Unable to find custom solver options file at: %s', options_file_path) | |
| else: | |
| logger.warning('Unable to find default solver options toml file.') | |
| s_options = {} |
🧰 Tools
🪛 Ruff (0.14.10)
67-67: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🤖 Prompt for AI Agents
In @temoa/extensions/monte_carlo/mc_sequencer.py around lines 65 - 71, The
except FileNotFoundError block in mc_sequencer.py currently logs via
logger.error()/warning() without the traceback; update that handler to call
logger.exception (preserving the existing messages and options_file_path usage)
so the traceback is captured for diagnostics, then keep the fallback behavior
setting s_options = {} and all_options = {} unchanged; locate the except block
that handles FileNotFoundError and replace the logger.error/logger.warning calls
with logger.exception while retaining the conditional message based on
options_file_path.
| # update the solver options | ||
| 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): | ||
| # Key not defined in ConfigDict, skip it | ||
| pass | ||
|
|
||
| # Handle solver log path for appsi | ||
| if self.solver_log_path: | ||
| log_location = ( | ||
| self.solver_log_path | ||
| / f'solver_log_{self.worker_number}_{self.solve_count}.log' | ||
| ) | ||
| try: | ||
| setattr(self.opt.config, 'log_file', str(log_location)) | ||
| except (ValueError, AttributeError): | ||
| pass |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider optimizing solver options setup.
The solver options are updated on every iteration (lines 93-112). If the solver options don't change between runs, this work is redundant and could be moved to initialization.
Additionally, for appsi_ solvers, both self.opt.options (line 94) and self.opt.config attributes (lines 96-101) are being set. Verify whether setting opt.options is necessary for appsi solvers, as the subsequent config-based settings may be sufficient.
Regarding the static analysis hint about setattr at line 110: this is a false positive. Dynamic configuration setting via setattr is appropriate here since we're safely handling arbitrary solver options with try-except blocks.
💡 Optional optimization if solver options are constant
If self.solver_options doesn't change between iterations, consider moving the solver configuration to immediately after solver initialization:
self.opt = SolverFactory(self.solver_name)
+
+# Configure solver options once if they don't change between runs
+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):
+ pass
while True:
# wait for a DataPortal object to show up, then get to work
tic = datetime.now()
data = self.dp_queue.get()
toc = datetime.now()
-
- # update the solver options
- 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):
- passOnly apply this optimization if solver options don't vary per iteration.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.10)
110-110: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
| abstract_model = TemoaModel() | ||
| model: TemoaModel = abstract_model.create_instance(data=dp) | ||
| model.name = name # set the name from the input | ||
| tic = datetime.now() |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Optional: Consider timezone-aware datetime for consistency.
The static analysis tool flags datetime.now() without a tz argument. While this doesn't affect the duration calculation (which is the only use here), using timezone-aware datetimes is a general best practice for consistency.
💡 Optional improvement for timezone-aware datetime
-tic = datetime.now()
+from datetime import timezone
+tic = datetime.now(timezone.utc)However, since this is only used for calculating elapsed time and not for any time-based logic or storage, the current implementation is acceptable.
📝 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.
| tic = datetime.now() | |
| tic = datetime.now(timezone.utc) |
🧰 Tools
🪛 Ruff (0.14.10)
117-117: datetime.datetime.now() called without a tz argument
(DTZ005)
🤖 Prompt for AI Agents
In @temoa/extensions/monte_carlo/mc_worker.py at line 117, The timestamp
assignment uses naive datetime.now(); change it to a timezone-aware call (e.g.,
datetime.now(tz=timezone.utc)) and add the necessary import (timezone) so that
the tic variable in mc_worker.py is timezone-aware; this keeps elapsed-time
calculations the same but ensures consistency for any future uses of tic.
| """ | ||
| Test Monte Carlo run logic and output database contents | ||
| """ | ||
| data_name, _, _, sequencer = system_test_run |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefix unused variable with underscore.
The data_name variable is unpacked but never used. Prefix it with an underscore to indicate it's intentionally unused.
🔎 Proposed fix
- data_name, _, _, sequencer = system_test_run
+ _data_name, _, _, sequencer = system_test_run📝 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.
| data_name, _, _, sequencer = system_test_run | |
| _data_name, _, _, sequencer = system_test_run |
🧰 Tools
🪛 Ruff (0.14.10)
173-173: Unpacked variable data_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
In @tests/test_full_runs.py at line 173, The unpacked variable data_name in the
tuple assignment for system_test_run is unused; rename it to _data_name (e.g.,
change "data_name, _, _, sequencer = system_test_run" to "_data_name, _, _,
sequencer = system_test_run") to indicate it is intentionally unused and satisfy
the linter.
ParticularlyPythonicBS
force-pushed
the
tests/mc_full_run
branch
from
ea5241e to
9e8014e
Compare
On adding a full monte carlo integration test, it fail with "A feasible solution was not found" if it was executed after a standard utopia run in the same process group.
This is because:
Using the spawn start method, which starts each worker in a fresh environment, definitively resolves the issue.
To strictly enforce this process isolation, the monte carlo execution model is refactored so that
MCWorkeris a plain Python object instead of inheriting frommultiprocessing.Process.The worker initialiation logic then uses
multiprocessing.get_context('spawn').Processto truly isolate worker memory from parent memory.Additionally we now expose the monte carlo solver options in the basic config.toml with:
The default remains
temoa/extensions/monte_carlo/MC_solver_options.tomlThe docs have also been updated to detail the monte carlo feature functionality
Summary by CodeRabbit
Release Notes
Documentation
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.