uncomment test from #4297 #4302
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4302 +/- ##
=======================================
Coverage 87.69% 87.69%
=======================================
Files 88 88
Lines 14355 14356 +1
=======================================
+ Hits 12588 12590 +2
+ Misses 1767 1766 -1
|
There was a problem hiding this comment.
Looks good, though see my comment on pytest.raises vs xfail. I leave the decision up to @MarcoGorelli .
| def test_spawn_densitydist_bound_method(): | ||
| with pm.Model() as model: | ||
| mu = pm.Normal("mu", 0, 1) | ||
| normal_dist = pm.Normal.dist(mu, 1) | ||
| obs = pm.DensityDist("density_dist", normal_dist.logp, observed=np.random.randn(100)) | ||
| trace = pm.sample(draws=10, tune=10, step=pm.Metropolis(), cores=2, mp_ctx="spawn") | ||
| msg = "logp for DensityDist is a bound method, leading to RecursionError while serializing" | ||
| with pytest.raises(ValueError, match=msg): |
There was a problem hiding this comment.
I considered pytest.raises, but the code in DensityDist is not guaranteed to raise the error. It will try to serialize as normal, and only if a RecursionError occurs (at the moment, expected to happen), it's wrapped and re-raised. It's possible that a later fix in dill or so will prevent the error from happening altogether. So I decided on xfail instead.
There was a problem hiding this comment.
ah, I see - thanks for explaining!
It's possible that a later fix in dill or so will prevent the error from happening altogether
If that happens, maybe it's good that this test fails (i.e. doesn't raise a ValueError) because then we will get a loud and clear notice that the error no longer happens and then we can set a minimum dill version?
|
Thanks @MarcoGorelli! |
#4297 had a commented-out test, so here's a little PR to uncomment it
If we set a global variable
PLATFORMinpymc3/distributions/distribution.pythen we can patch it during testing without affectingsys.platformand hence not affectingtheano