From 3201b00079dd0b17fd404fbe09b2ebad6fbea264 Mon Sep 17 00:00:00 2001 From: Richard O'Shaughnessy Date: Thu, 13 Aug 2026 03:15:34 -0700 Subject: [PATCH] portfolio: make the driver fail loudly instead of silently substituting a sampler Driver-side half of making --sampler-method portfolio usable on rift_O4c. The module-side half -- guarding the unguarded entry-point plugin load, so one missing optional dependency does not make mcsamplerPortfolio unimportable -- is PR #168's first commit (aaa8a6e1) and is deliberately NOT duplicated here. Neither half is sufficient alone; #168 does not touch this file. Four defects, all in the sampler-construction chain, all of the same kind: a request the driver cannot honour is answered with something other than an error. 1. `elif opts.sampler_method == "portfolio" and mcsampler_Portfolio_ok:` made the very next statement -- `if not(mcsampler_Portfolio_ok): raise` -- unreachable, and sent an unavailable portfolio down the chain to the terminal `else`, which prints " ILE: **original sampler** " and proceeds with the plain mcsampler.MCSampler constructed before the chain. A run that asked for the portfolio would have integrated with a different sampler. Drop the ok-flag from the test so the existing raise becomes reachable and does its job. 2. That terminal `else` then dereferences `mcsamplerPortfolio.known_pipelines` in its own diagnostic print -- but the name is only bound if the import succeeded, so the diagnostic for a failed import raised NameError itself. This is what '--sampler-method portfolio' actually produced on a torch-free container: NameError: name 'mcsamplerPortfolio' is not defined 3. The plugin-pipeline branch had the same unguarded dereference in its `elif` test. Gate it on mcsampler_Portfolio_ok. 4. --sampler-portfolio is action='append' while its help documents a comma-separated list, and the member loop had no else clause. So the documented invocation '--sampler-portfolio AV,GMM' arrived as the single member name "AV,GMM", matched no branch, and appended whatever `sampler` happened to hold -- the plain MCSampler from before the chain, or on later iterations the PREVIOUS member. The portfolio then ran with a member nobody asked for and died later and elsewhere with a misleading "no attribute 'draw_simplified'". Accept both spellings (and a mix), and make an unrecognized member name an error naming the known members. The committed demo harness already works around this by splitting on commas in shell before invoking the driver (extrinsic_collapse_demo/run_demo.sh:58), which is why the append form is the only one that had been exercised. rift_O4d carries the same defect 4; it is fixed here rather than backported. Co-Authored-By: Claude Opus 5 --- .../integrate_likelihood_extrinsic_batchmode | 31 ++++++++++++++++--- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/MonteCarloMarginalizeCode/Code/bin/integrate_likelihood_extrinsic_batchmode b/MonteCarloMarginalizeCode/Code/bin/integrate_likelihood_extrinsic_batchmode index 2c4b83e14..356fe0ff3 100755 --- a/MonteCarloMarginalizeCode/Code/bin/integrate_likelihood_extrinsic_batchmode +++ b/MonteCarloMarginalizeCode/Code/bin/integrate_likelihood_extrinsic_batchmode @@ -287,7 +287,7 @@ integration_params.add_option("--manual-logarithm-offset",type=float,default=0,h integration_params.add_option("--auto-logarithm-offset",action='store_true',help="Use the 'guess_snr' field returned in the precompute stage to change --manual-logarithm-offset for each event.") integration_params.add_option("--internal-use-lnL",action='store_true',help="likelihood returns lnL, and integrator integrates lnL") integration_params.add_option("--sampler-method",default="adaptive_cartesian_gpu",help="adaptive_cartesian|GMM|adaptive_cartesian_gpu") -integration_params.add_option("--sampler-portfolio",default=None,action='append',type=str,help="comma-separated strings, matching sampler methods other than portfolio") +integration_params.add_option("--sampler-portfolio",default=None,action='append',type=str,help="Portfolio member sampler, one of AV / GMM / AC (adaptive_cartesian_gpu) or a discovered plugin. Repeat the option per member, or give one comma-separated list; both forms may be mixed. An unrecognized name is an error.") integration_params.add_option("--sampler-portfolio-args",default=None, action='append', type=str, help='eval-able dictionaryo to be passed to that sampler') integration_params.add_option("--sampler-xpy",default=None,help="numpy|cupy if the adaptive_cartesian_gpu sampler is active, use that.") integration_params.add_option("--supplementary-likelihood-factor-code", default=None,type=str,help="Import a module (in your pythonpath!) containing a supplementary factor for the likelihood. Used to impose supplementary external priors of arbitrary complexity and external dependence (e.g., EM observations). EXPERTS-ONLY") @@ -804,13 +804,25 @@ elif opts.sampler_method == 'AV': mcsampler.set_xpy_to_numpy() sampler.xpy= numpy sampler.identity_convert= lambda x: x -elif opts.sampler_method == "portfolio" and mcsampler_Portfolio_ok: +elif opts.sampler_method == "portfolio": + # NB the `and mcsampler_Portfolio_ok` that used to be part of this test made the raise + # below dead code AND sent an unavailable portfolio to the `else` fallback at the end of + # this chain, which silently runs the plain mcsampler.MCSampler instead. Requesting a + # sampler that cannot be built must fail, not quietly become a different sampler. if not(mcsampler_Portfolio_ok): raise Exception(" Portfolio integrator requested but not available") use_portfolio=True opts.internal_use_lnL=True # required, we only implement those scenarios right now sampler_list = [] - sampler_types = opts.sampler_portfolio + # The option is action='append', but its help has always documented a comma-separated + # list. Accept both (and a mix), because the undocumented half silently misbehaved: + # '--sampler-portfolio AV,GMM' arrived as the single member name "AV,GMM", matched no + # branch in the loop below, and -- the loop having no else -- appended whatever `sampler` + # happened to hold, i.e. the plain MCSampler constructed before this chain. The run then + # died later and elsewhere with a misleading "no attribute 'draw_simplified'". + sampler_types = [_n.strip() for _entry in (opts.sampler_portfolio or []) for _n in str(_entry).split(',') if _n.strip()] + if not sampler_types: + raise Exception(" --sampler-method portfolio requires at least one --sampler-portfolio member") # prep xpy, etc my_xpy = xpy_default @@ -835,6 +847,12 @@ elif opts.sampler_method == "portfolio" and mcsampler_Portfolio_ok: mcsampler = mcsamplerGPU # force use of routines in that file, for properly configured GPU-accelerated code as needed elif name in mcsamplerPortfolio.known_pipelines: # everything else, including nflow sampler = mcsamplerPortfolio.known_pipelines[name]() + else: + # No else clause here meant an unrecognized name left `sampler` bound to its + # previous value -- the plain MCSampler built before this chain, or, on the second + # and later iterations, the PREVIOUS member -- and appended it silently. The + # portfolio then ran with a member the user never asked for. + raise Exception(" --sampler-portfolio: unknown member '{}'. Known: AV, GMM, AC/adaptive_cartesian_gpu, {}".format(name, sorted(mcsamplerPortfolio.known_pipelines))) print('PORTFOLIO: adding {} '.format(name)) # enable xpy for low level sampler as needed if hasattr(sampler, 'xpy'): @@ -847,7 +865,7 @@ elif opts.sampler_method == "portfolio" and mcsampler_Portfolio_ok: sampler.identity_convert= my_identity_convert sampler.identity_convert_togpu= my_identity_convert_togpu # sampler weights will be CPU-typed, so don't change them -elif opts.sampler_method in mcsamplerPortfolio.known_pipelines: # access from plugins +elif mcsampler_Portfolio_ok and opts.sampler_method in mcsamplerPortfolio.known_pipelines: # access from plugins sampler = mcsamplerPortfolio.known_pipelines[opts.sampler_method]() # prep xpy, etc my_xpy = xpy_default @@ -858,7 +876,10 @@ elif opts.sampler_method in mcsamplerPortfolio.known_pipelines: # access from pl sampler.identity_convert_togpu= my_identity_convert_togpu else: print(" ILE: **original sampler** ") - print(" ILE requested: {}".format(opts.sampler_method), " compare to ", mcsamplerPortfolio.known_pipelines) + # mcsamplerPortfolio is only bound if its import succeeded; reaching this line with a + # failed import used to raise NameError from the diagnostic itself. + print(" ILE requested: {}".format(opts.sampler_method), " compare to ", + sorted(mcsamplerPortfolio.known_pipelines) if mcsampler_Portfolio_ok else "") # # Psi -- polarization angle