Skip to content

Bound spoke constructors are missing the communicators parameter, so SPCommunicator.options is always empty on a spoke #826

Description

@DLWoodruff

Summary

_BoundSpoke.__init__ and InnerBoundSpoke.__init__ are missing the communicators parameter that SPCommunicator.__init__ and Hub.__init__ have. As a result the cylinder list WheelSpinner passes positionally binds to their options parameter, is forwarded on into communicators, and SPCommunicator.options ends up empty on every bound spoke.

Nothing is visibly broken today, because outside Hub almost nothing reads SPCommunicator.options — but it is a trap for anyone who adds a spoke-side option, and it makes two existing reads dead code (below).

The signatures

mpisppy/cylinders/spcommunicator.py:303
    def __init__(self, spbase_object, fullcomm, strata_comm, cylinder_comm, communicators, options=None):

mpisppy/cylinders/hub.py:34
    def __init__(self, spbase_object, fullcomm, strata_comm, cylinder_comm, communicators, options=None):

mpisppy/cylinders/spoke.py:96   (_BoundSpoke)
    def __init__(self, spbase_object, fullcomm, strata_comm, cylinder_comm, options=None):

mpisppy/cylinders/spoke.py:202  (InnerBoundSpoke)
    def __init__(self, spbase_object, fullcomm, strata_comm, cylinder_comm, options=None):

Both forward positionally:

super().__init__(spbase_object, fullcomm, strata_comm, cylinder_comm, options)

so the value arrives in communicators — which is why the runs work — and the base class's options stays None, giving self.options = dict().

spin_the_wheel.py:165 constructs every cylinder the same way:

spcomm = sp_class(opt, fullcomm, strata_comm, cylinder_comm,
                  communicator_list, **sp_kwargs)

Reproducer

import inspect
from mpisppy.cylinders.hub import Hub
from mpisppy.cylinders.lagrangian_bounder import LagrangianOuterBound

for cls in (Hub, LagrangianOuterBound):
    sig = inspect.signature(cls.__init__)
    b = sig.bind(None, "opt", "full", "strata", "cyl", ["hub_dict", "spoke_dict"])
    print(cls.__name__, "->", dict(list(b.arguments.items())[4:]))
    try:
        sig.bind(None, "opt", "full", "strata", "cyl", ["hub_dict", "spoke_dict"],
                 options={"k": 1})
        print("   options= kwarg: OK")
    except TypeError as e:
        print(f"   options= kwarg: TypeError: {e}")
Hub -> {'cylinder_comm': 'cyl', 'communicators': ['hub_dict', 'spoke_dict']}
   options= kwarg: OK
LagrangianOuterBound -> {'cylinder_comm': 'cyl', 'options': ['hub_dict', 'spoke_dict']}
   options= kwarg: TypeError: multiple values for argument 'options'

Consequences

  1. self.options is always {} on a bound spoke — every _BoundSpoke / InnerBoundSpoke descendant, i.e. all the bound spokes: LagrangianOuterBound, XhatShuffleInnerBound, the xhat bounders, SubgradientOuterBound, and so on.

  2. spcomm_kwargs["options"] cannot be passed to one at all. A driver that sets it gets TypeError: __init__() got multiple values for argument 'options' at cylinder construction. cfg_vanilla only ever populates hub_kwargs["options"], so no in-tree path hits this — but it is a real wall for a custom driver.

  3. Two existing reads are dead as written. PHXFeasSpoke.main and _PHDualSpokeBase.main do

    smoothed = self.options.get('smoothed', 0)

    (mpisppy/cylinders/ph_xfeas_spoke.py:27, mpisppy/cylinders/ph_dual_spoke.py:34) — the only readers of SPCommunicator.options outside Hub. These two classes derive from Spoke, not _BoundSpoke, so they are not hit by the signature bug itself; but cfg_vanilla writes their settings into opt_kwargs["options"] (rho_factor, time_limit, PHIterLimit, ... at cfg_vanilla.py:1334 and 1373), never into spcomm_kwargs, so self.options is {} for them too and the expression can only ever yield 0. Their own update_rho, two lines above, reads self.opt.options — as does the equivalent line in subgradient_bounder.py:29:

    if self.opt.options.get("smoothed", 0) != 0:

Suggested fix

Give both spoke constructors the communicators parameter and forward it by name:

def __init__(self, spbase_object, fullcomm, strata_comm, cylinder_comm,
             communicators, options=None):
    super().__init__(spbase_object, fullcomm, strata_comm, cylinder_comm,
                     communicators, options=options)

and switch the two self.options.get('smoothed', 0) reads to self.opt.options, matching update_rho and subgradient_bounder.

Worth deciding at the same time which dict is meant to be the home for cylinder-level options. Today opt.options is the one every cylinder reliably has (it is what opt_kwargs populates) and where the cylinder-wide debug switches already live — trace_prefix in _BoundSpoke.__init__, inspect_buffers_on_shutdown in Spoke.got_kill_signal. SPCommunicator.options is effectively hub-only. If that is the intent, it deserves a docstring saying so; if not, the spoke side needs the plumbing.

How this surfaced

While reviewing #816, whose read-outcome diagnostic originally took its coherence_diagnostics_period from self.options and was therefore silently inert on exactly the bound spokes it was meant to instrument. That PR now reads the knob from opt.options, so it no longer depends on any of this; the underlying signature mismatch is untouched and filed here.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions