Add --iter0-from-pickle; has_variable_probability unpickle-safety; ADMM pickling docs#656
Open
DLWoodruff wants to merge 34 commits intoPyomo:mainfrom
Open
Add --iter0-from-pickle; has_variable_probability unpickle-safety; ADMM pickling docs#656DLWoodruff wants to merge 34 commits intoPyomo:mainfrom
DLWoodruff wants to merge 34 commits intoPyomo:mainfrom
Conversation
Register the stage2 EF solver option in xhatshuffle_args() so it is available as --stage2-ef-solver-name from the generic_cylinders CLI. Rename the option from stage2EFsolvern to stage2_ef_solver_name to follow project naming conventions. Relax the hard assertion requiring this option for multistage xhatshuffle to a warning, since the regular multi-stage xhat path also works. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
PR Pyomo#651 relaxed a hard assert into a warnings.warn call when multistage xhatshuffle is used without --stage2-ef-solver-name. Without a test, silent drift (warning removed, branch not taken) would go unnoticed. Covers both cases: warning fires when missing, does not fire when set. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Stoch_AdmmWrapper.assign_variable_probs synthesizes dummy nonants for subproblems that don't own a given consensus variable, renaming them (e.g. y[F1_1] -> y__F1_1__) to distinguish the fixed-at-zero dummies from real consensus vars. Across ADMM subproblem scenarios this causes the post-Pyomo#634 nonant name validator to trip. Same auto-disable pattern used for proper_bundles. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ef_options was hard-coded to emit options={"solver": ...} only, so the
auto-disable we set on cfg for --admm/--stoch-admm runs never reached
SPBase._verify_nonant_names and the EF path still tripped on the dummy
nonant name mismatch. Mirror cfg_vanilla.shared_options, honoring both
cfg.turn_off_names_check and proper-bundles. Regression test added.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The merge of main into merge-651-652 kept both the pre-split unindented test list and the indented if-block version from Pyomo#654, causing every test to run twice on each runall CI job. Drop the unindented duplicate block and port the --stage2EFsolvern -> --stage2-ef-solver-name rename (from commit e053324) onto the indented hydro line in run_second_part. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Stoch_AdmmWrapper injects fixed-to-0 dummy Vars for consensus variables that a given ADMM subproblem does not own, so every scenario's nonant_vardata_list has the same length and positional correspondence at each shared tree node. PH handles these correctly via probability 0 in var_prob_list, but EF with nonant_for_fixed_vars=True (the default via ExtensiveForm) was creating hard equality constraints tying the real owners to the zero-fixed dummies, forcing them to zero and producing noticeably worse objectives than PH for num_admm_subproblems > 2. Reported by a user running stoch_distr with 4 admm x 3 stoch. Tag the dummies post-hoc on each ScenarioNode's surrogate_vardatas so EF skips their nonant constraints and fixers ignore them, without disturbing positional alignment across scenarios (the normal surrogate_nonant_list API appends at the end, which would misalign). Also handle the iteration-order-dependent case in _create_EF_from_scen_dict where a surrogate was installed as ref before a real var arrived: upgrade the ref to the real var and drop the surrogate tag so subsequent real vars tie to it. While here, preserve user-supplied surrogate_vardatas and nonant_ef_suppl_vardata_list across Stoch_AdmmWrapper's stage-node rewrite, which had been silently dropping them. Tests: test_ef_partial_consensus drives generic_cylinders --EF on the user's 3x4 reproducer and asserts EF sits within the PH outer/inner envelope; test_preserves_user_surrogates_and_ef_suppl constructs a minimal partial-consensus scenario with user surrogates and EF-suppl vars and asserts both survive the rewrite. Both verified to fail without the corresponding change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: bknueven <30801372+bknueven@users.noreply.github.com>
The Pyomo#652 changes made admm_subproblem_names_creator and stoch_scenario_names_creator take cfg. Update the pre-pickle pipeline test (brought in from upstream/main via this merge) to match. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- stoch_admmWrapper: pass user's raw surrogate_nonant_list / nonant_ef_suppl_list into rebuilt ScenarioNodes (not the rebuilt ComponentSet/vardata lists) so ordering stays deterministic. - stoch_admmWrapper: guard the bracket-wrap fallback in assign_variable_probs so malformed vstr without '[' / ']' doesn't produce a truncated component name. - generic/parsing: give the multistage xhatshuffle stage2 warning a UserWarning category and stacklevel=2 so it points at the caller. - examples/hydro: drop the redundant stage2_ef_solver_name add_to_config calls; cfg.multistage() / xhatshuffle_args() already register it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Tier 2 of the pre-pickle preprocessing design (doc/pre_pickle_pre processing_design.md). When the user pickles with --iter0-before-pickle the variable values + duals + objective bound are baked into the pickle. With --iter0-from-pickle, PHBase.Iter0 now skips its solve_loop call and reuses those values directly: it validates that every local scenario carries pickle_metadata['iter0_before_pickle']==True (hard fails otherwise), then sets solution_available, outer_bound, and inner_bound on each scenario from pyo.value(self.saved_objectives[ sname]). The rest of Iter0 -- E1 update, feasibility check, post_iter0 extension hook, Ebound, rho_setter -- runs unchanged because PH's downstream logic only needs the variable values that the pickle already provides. Wires the new flag through cfg.pre_pickle_args and shared_options so it flows from the cylinder driver to PHBase. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds two end-to-end subprocess tests under test_pre_pickle_pipeline.py::TestEndToEnd: - test_iter0_from_pickle_skips_solve_loop pickles farmer scenarios with --iter0-before-pickle, then runs PH with --iter0-from-pickle and asserts that the run completes and the "Skipping PHBase.Iter0 solve loop" message appears in stdout. - test_iter0_from_pickle_without_pickled_iter0_fails pickles WITHOUT --iter0-before-pickle and verifies that --iter0-from-pickle hard- fails on the run side rather than fabricating solver state. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The page used to describe --iter0-from-pickle as a forthcoming flag. Tier 2 is now implemented (commit 95838a5), so reword the relevant subsection to describe the actual behavior, document the hard-fail contract (every local scenario must carry the pre-pickle iter0 flag), and add a worked pickle->run example. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The pickling_design_docs base branch added a --python-args parser shim
to test_pre_pickle_pipeline.py and threaded it into the v1 TestEndToEnd
methods so the dedicated coverage CI job can capture coverage from the
spawned subprocesses. The tier 2 test methods on this branch (added
before that change landed on the base) still hard-coded f"{python} -m
mpisppy.generic_cylinders ...", so their subprocess invocations would
not have been coverage-instrumented in the new pre-pickle-pipeline CI
job.
Mirror the v1 form so all four TestEndToEnd subprocess invocations
look the same and the new CI job picks up coverage uniformly.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When unpickling ADMM scenarios without --admm/--stoch-admm, prob_coeff/prob0_mask arrays survive because SPBase guards their init with hasattr, but the has_variable_probability flag was being clobbered to False. That trips gating logic in convergers and APH despite the arrays actually being non-uniform. Only default the flag to False if it wasn't already set. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The pre-pickle pipeline handles ADMM-wrapped scenarios correctly at the programmatic level (covered by test_pipeline_runs_on_admm_bundles in test_pre_pickle_pipeline.py). Driving pickling of --admm / --stoch-admm through generic_cylinders is still blocked: write_scenarios uses module.scenario_names_creator (not the ADMM combined names), _build_pickle_pipeline_spbase does not plumb variable_probability, and the unpickle side has no way to discover ADMM-combined names in the pickle dir. Documented in pickling.rst with cross-references from generic_admm.rst and stoch_admmWrapper.rst so readers hit the status note from either side. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #656 +/- ##
==========================================
+ Coverage 70.71% 70.78% +0.07%
==========================================
Files 153 153
Lines 19005 19061 +56
==========================================
+ Hits 13439 13492 +53
- Misses 5566 5569 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
HEY!!
Merge 653 first, even before you look at the Files changed.
Summary
--iter0-from-pickle: PH skips its iter0 solve loop when the pickled scenario metadata carriesiter0_before_pickle=True, using the pickled solution values as the iter0 warm start. Hard-fails if any local scenario is missing the metadata.test_pre_pickle_pipeline.py::TestEndToEnd._use_variable_probability_setterno longer clobbers a pickledhas_variable_probability = True, so scenarios pickled under--admm/--stoch-admmcan in principle be read back without those flags.pickling.rstgains an ADMM section;generic_admm.rstandstoch_admmWrapper.rstcross-link to it. Current status is documented explicitly — the programmatic pipeline handles ADMM (covered bytest_pipeline_runs_on_admm_bundles); CLI end-to-end remains blocked at the compatibility check, with the three plumbing items needed to unblock it spelled out.Base / stacking
This PR is stacked on #653 — the branch includes #653's commits until it merges. After #653 lands, I will rebase onto main and force-push; the diff will shrink to the 6 iter0-from-pickle commits.
Test plan
ruff check .cleanpython -m unittest mpisppy.tests.test_admm_bundler— passpython -m unittest mpisppy.tests.test_pre_pickle_pipeline— 11 pass (includes new round-trip + hard-fail)cd doc && make html— no new warnings🤖 Generated with Claude Code