Skip to content

fix: fetch and store duals only when save_duals is on - #390

Open
SutubraResearch wants to merge 2 commits into
TemoaProject:unstablefrom
SutubraResearch:fix/save-duals
Open

SutubraResearch wants to merge 2 commits into
TemoaProject:unstablefrom
SutubraResearch:fix/save-duals

Conversation

@SutubraResearch

@SutubraResearch SutubraResearch commented Oct 16, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #389.

What happens now

build_instance always declares model.dual as an import suffix, and Pyomo asks the solver for
every active import suffix on the model, whatever suffix list it is passed, and adds it to that
list. So with save_duals = false, every solve through solve_instance still asks the solver for
all duals and loads them into the model. Then, because 'dual' is now in the list Temoa passed,
store_to copies every variable value and dual into the results object. Nothing uses them.

What this changes

  • solve_instance (temoa/_internal/run_actions.py): when 'dual' is not in the requested
    suffixes, the model's dual suffix is set to Suffix.LOCAL for the solve and set back
    afterwards. Pyomo then does not ask for duals, does not load them, and does not add 'dual' to
    the list, so store_to is not called. This is the same thing the appsi_highs branch just
    above already does.
  • Myopic sequencer: passes solver_suffixes=['dual'] if self.config.save_duals else None to
    solve_instance, as the perfect-foresight and single-vector MGA sequencers already do. It used
    to get its duals only through the behaviour above. Without this line, a myopic run with
    save_duals = true would write no duals and give no error.

Measured saving

A representative-day capacity expansion model (2.4 million variables, 2.8 million constraints),
solver = "gurobi" (LP file), save_duals = false, 32 cores, same node, one run each.
PYTHONHASHSEED=0 in both, so both runs build the same model:

before after
store_to 54.3 s not called
Loading the solution into the model 16.3 s 5.0 s
Gurobi's solution to Python, Pyomo's solution object 9.4 s 7.5 s
Whole solve step 235 s 170 s
Whole run 461 s (7:41) 402 s (6:42)
Peak memory 9.3 GiB 8.2 GiB

Gurobi received the same model in both runs (same fingerprint, same 66 barrier iterations, same
objective). All output tables were identical row for row: every column, exact values, including
2,005,248 rows in each flow table. Writing results took 8 s longer in the second run, in code
this change does not touch. Gurobi itself also varied by 3 s on the same work, so I read that as
run-to-run variation, but with one run each I can't show it.

What doesn't change

  • With save_duals = true: the same suffix list reaches Pyomo, the solver is asked for the same
    duals, store_to runs, and the dual table is identical row for row. Checked on utopia in
    perfect foresight with gurobi and gurobi_direct, and in myopic mode.
  • The model returned by build_instance is unchanged and still has model.dual.
  • The appsi_highs branch is unchanged.
  • MGA and Monte Carlo call the solver directly rather than through solve_instance, so this
    change does not affect them. Method of Morris does call solve_instance, without asking for
    duals (it turns save_duals off), so it now skips them too; I haven't run it.

Testing

  • New test, tests/test_save_duals.py. It runs utopia in myopic mode (two steps) with
    solver = "gurobi", once with save_duals = true and once with false.
    • With it on, both steps fetch duals and all of them are written to output_dual_variable
      (931 and 887).
    • With it off, neither step loads any duals into the model or copies the solution into the
      results object, and no duals are written.
    • On unstable without this change, the false case fails (931 duals in the first step). With
      only the run_actions.py change, the true case fails (no duals): that is the myopic line's
      job. With the whole change both pass.
    • It uses gurobi because appsi_highs does not return duals. Utopia fits in the size-limited
      licence that comes with gurobipy, which CI installs through the solver extra; both cases
      pass under that licence. Without gurobi the test is skipped.
    • I ran it on macOS with Python 3.12 and 3.14, not on CI's Linux and Windows runners or
      Python 3.13.
  • pytest on v4.1.0 with this change: 291 passed (289 before it, plus the 2 new ones). On
    unstable with this change, in a fresh environment (uv sync --extra solver, Python 3.14.3):
    291 passed. ruff and mypy pass on the three files.
  • Utopia, before and after, with the hash seed fixed: perfect foresight with gurobi,
    gurobi_direct and appsi_highs, and myopic with gurobi, each with save_duals off and on
    where supported. All output tables are identical row for row. With save_duals off, no duals
    are now requested or stored. With it on, the same 1,499 duals (1,818 in myopic) are written as
    before. Done on the test suite's copy of utopia, and again on the database temoa tutorial
    makes (every case except appsi_highs), with the same result.

Summary by CodeRabbit

  • Bug Fixes
    • Corrected dual-result handling for supported solver workflows: requested duals are retained, while unrequested duals are no longer implicitly requested.
    • Myopic runs now consistently save duals when that option is enabled. When it is disabled, duals are not loaded or written to the output database.

@coderabbitai

coderabbitai Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: TemoaProject/temoa/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 108c8365-5016-486b-9856-03e7cc31dea1
📥 Commits

Reviewing files that changed from the base of the PR and between d5148db and c866d4e.

📒 Files selected for processing: 3
  • temoa/_internal/run_actions.py
  • temoa/extensions/myopic/myopic_sequencer.py
  • tests/test_save_duals.py

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.

Walkthrough

The solve path prevents non-appsi_highs solvers from importing duals unless requested. The myopic sequencer requests duals when save_duals is enabled. Integration tests cover dual and solution handling with dual saving enabled and disabled.

Changes

Dual suffix handling

Layer / File(s) Summary
Filter dual imports during solve
temoa/_internal/run_actions.py
For non-appsi_highs solvers, solve_instance temporarily disables the dual import suffix when duals are not requested. It restores the original suffix direction after solving, including when solving fails.
Request duals for myopic output
temoa/extensions/myopic/myopic_sequencer.py, tests/test_save_duals.py
The myopic sequencer requests duals when save_duals is enabled and passes no solver suffixes otherwise. Integration tests check dual and solution handling for both settings.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to c866d

No actionable merge-blocking issue is established. The reported Gurobi test coverage remains limited to the environments where it was run.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: duals are fetched and stored only when save_duals is enabled.
Linked Issues check Passed Issue #389 is an active direct issue. In solve_instance, the change sets instance.dual.direction to Suffix.LOCAL when dual is not requested and restores the original direction in finally. Th…
Out of Scope Changes check Passed The changed production code directly implements Issue #389 in run_actions.py and the myopic sequencer. The new integration tests verify the requested behavior. No unrelated production behavior or un…
Full details: Linked Issues check

Explanation

Issue #389 is an active direct issue. In solve_instance, the change sets instance.dual.direction to Suffix.LOCAL when dual is not requested and restores the original direction in finally. This prevents implicit dual fetching and keeps the suffix list empty, so store_to is not called. The myopic sequencer explicitly requests ['dual'] when save_duals is enabled. tests/test_save_duals.py covers both settings and checks model duals, result solutions, and database output.

Full details: Out of Scope Changes check

Explanation

The changed production code directly implements Issue #389 in run_actions.py and the myopic sequencer. The new integration tests verify the requested behavior. No unrelated production behavior or unrelated files are shown in the reviewed changes.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Oct 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.93939% with 4 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
tests/test_save_duals.py 92.85% 2 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant