portfolio: make the driver fail loudly instead of silently substituting a sampler (driver-side complement to #168) - #172
Open
oshaughnessy-junior wants to merge 1 commit into
Conversation
…ng 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 oshaughn#168's first commit (aaa8a6e) and is deliberately NOT duplicated here. Neither half is sufficient alone; oshaughn#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 <noreply@anthropic.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
oshaughnessy-junior
had a problem deploying
to
private-review-dispatch-rift-upstream
August 13, 2026 10:23 — with
GitHub Actions
Failure
This was referenced Aug 13, 2026
Open
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
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.
Driver-side half of making
--sampler-method portfoliousable onrift_O4c. One file,+26/-5, no new dependencies, no behaviour change for any sampler other than
portfolio.Complementary to #168, not overlapping it. #168 fixes the module side
(
mcsamplerPortfolio.py): the unguarded entry-point plugin load, and the fair-draw backend.This PR fixes the driver side (
integrate_likelihood_extrinsic_batchmode), which #168 does nottouch. Neither is sufficient alone — see the measurements below. I have deliberately not
duplicated #168's plugin guard here, even though I had written an identical one, so the two
branches do not conflict;
3201b000cherry-picks onto4ddf41decleanly.Four defects, one kind
Every one of them answers a request the driver cannot honour with something other than an error.
1. A dead raise, and a silent sampler substitution.
The guard in the
elifmakes the very next statement unreachable, and routes an unavailableportfolio past the whole chain to the terminal
else, which printsILE: **original sampler**and proceeds with the plain
mcsampler.MCSamplerconstructed before the chain. A run that askedfor the portfolio would have integrated with a different sampler and reported nothing unusual.
Dropping the flag from the test makes the existing raise reachable and correct.
2. The
elsediagnostic itself raises. It formatsmcsamplerPortfolio.known_pipelines, butthat name is only bound when the import succeeded. So the branch that exists to explain a failed
import was the one that raised
NameError: name 'mcsamplerPortfolio' is not defined— which iswhat
--sampler-method portfolioactually produced on any torch-free container.3. The plugin-pipeline
elifhas the same unguarded dereference in its test. Gated on theok-flag.
4. The documented spelling of
--sampler-portfoliosilently built the wrong portfolio. Theoption is
action='append'while its help documents "comma-separated strings", and the memberloop had no
else. So--sampler-portfolio AV,GMMarrived as the single member name"AV,GMM",matched no branch, and appended whatever
samplerhappened to hold — the plainMCSamplerfrombefore the chain, or on later iterations the previous member. Measured on
4ddf41de(#168 head),rho_net 146.8:
One member, wrong type, and an error that names neither the option nor the bad value. With this
PR the same invocation gives
PORTFOLIO: adding AV/PORTFOLIO: adding GMM, and a bad name isrefused up front:
Both spellings (and a mix) are now accepted, so the help text stops lying rather than the
behaviour changing under anyone.
rift_O4dcarries the same defect 4; it is fixed here ratherthan backported.
Note the committed demo harness has been splitting on commas in shell before invoking the
driver (
extrinsic_collapse_demo/run_demo.sh:58) — a workaround for exactly this, which is whyonly the
appendspelling had ever been exercised.Measurements
Same host (ldas-pcdev2, A100), same container, same injection,
run_snr140(rho_net 146.8),--sampler-portfolio AV,GMM:4ddf41de(#168 head)"AV,GMM", diesno attribute 'draw_simplified'4ddf41de+ this PRAV,GMM, portfolio constructs, reaches the integration loop4ddf41de+ this PR, memberBOGUSEvery run logs its tree and commit; all three read
dirty=0.What this does NOT fix
Reaching the integration loop is where this PR's scope ends, and the portfolio still does not
complete on
rift_O4c. With #168 and this PR both applied it now fails one layer deeper, inthe AV member's prior evaluation:
Same class of defect as #168's fair-draw fix — allocate on one backend, operate with the other.
I chased it two sites further, and it is a chain, not a last mile:
mcsamplerAdaptiveVolume.prior_prod:286mcsamplerEnsemble.calc_pdf:202mcsamplerPortfolio.integrate_log:471all the same
TypeError, each only visible once the previous one is fixed. StandaloneAVandGMMescape 1 and 2 for different reasons (the driver rebindsmcsampler = mcsamplerAdaptiveVolumein the AV branch;self.xpyis numpy in standalone GMM), and aportfolio breaks both assumptions at once. The O4c portfolio has evidently never run on a GPU host.
rift_O4dcarries fixes for all three, and backporting the first one verbatim regresses theproduction path — so I am not proposing it here. With the O4d
prior_prodfix applied,standalone AV at rho=51.4:
4a8703f3+ O4d prior_prodTypeError: Unsupported typebecause
mcsamplerGPU.uniform_samp_psi(x, xpy=...)acceptsxpybut its body isxpy.ones(len(x))/(cupy_pi)withcupy_pi = cupy.array(np.pi)— the array honours the argument,the constant does not. The O4c and O4d bodies of that helper are byte-identical, so the O4d fix
works upstream only in combination with the rest of O4d's backend hardening
(
identity_convert*call sites:mcsamplerEnsemble5 → 26,mcsamplerPortfolio32 → 46,mcsamplerAdaptiveVolume38 → 52).So: this PR makes the portfolio selectable and honestly diagnosed. It does not make it
runnable on
rift_O4c, and that remainder is its own scoped job with its own inertness gateon standalone AV and standalone GMM — not a follow-on commit. Full measurements and the
reproduce commands:
extrinsic_collapse_demo/run/O4C_PORTFOLIO_STATUS_2026-08-13.md.