Make tests independent of Turing about dynamicppl.jl HOT 9 CLOSED

Let's keep this issue open for some more discussion, in case some cleverer solution coming up later.

from dynamicppl.jl.

The workflow I currently follow to avoid repeating any work and avoid inconsistencies is to:

1. Locally apply any breaking changes to DynamicPPL fixing Turing's src accordingly
2. Copy and paste all of Turing's src to DynamicPPL's test folder and make an "update tests" commit
3. Make sure DynamicPPL tests pass
4. Make a Turing PR with the new Turing

The only repetition here is that of copying and pasting code, but effort-wise there is very little overhead to this approach, other than the few seconds of copying and pasting every time I want to run DynamicPPL tests. Making the tests independent is an ideal scenario but is a little too difficult right now, definitely not on top of my priority list. Feel free to take a crack at it though.

from dynamicppl.jl.

Even something as aggressive as copying the whole Turing src folder to the DynamicPPL test folder is better than what we have now.

from dynamicppl.jl.

I ended up copying all of Turing to the tests folder and using that. As long as it is reasonably up to date with Turing master (up to breaking changes in DynamicPPL), it should be ok.

from dynamicppl.jl.

Note that this is the same problem MathOptInterface has with JuMP and solver packages, and that LineSearches has with Optim. As far as I know, there is no pretty fix. The key problem here is that we need most of Turing to test "all" of the features of DynamicPPL.

from dynamicppl.jl.

ChainRulesCore -> ChainRules has a similar situation to DynamicPPL -> Turing.

The way we sort of work-around it is:

• Every ChainRulesCore (DynamicPPL) PR also runs the ChainRules (Turing) tests. (See https://github.com/JuliaDiff/ChainRulesCore.jl/blob/master/.cirrus.yml)
• This make sure we didn't accidentally make a breaking change to ChainRulesCore (DynamicPPL).
• We use a different CI service (Cirrus) for this to the rest of the tests (Travis) to more easily see which package's tests failed. This of course relies on ChainRulesCore (DynamicPPL) having its own tests distinct from ChainRules (Turing).
• When intentionally making a breaking change to ChainRulesCore (DynamicPPL), we open a "partner" PR to ChainRules (Turing) at the same time. Then before merging we should see that
  • (i) ChainRulesCore (DynamicPPL) passes its own tests,
  • (ii) the automatic intgration tests with ChainRules (Turing) on CI will fail as expected, but
  • (iii) checking out the "partner" branch of ChainRules (Turing) alongside the ChainRulesCore (DynamicPPL) changes makes the ChainRules (Turing) tests pass. This last bit reviewers still have to do locally.
  • Example of partner PRs: JuliaDiff/ChainRules.jl#91 and JuliaDiff/ChainRulesCore.jl#30

At the very least, running Turing's tests directly could hopefully reduce maintainence burden over copy-pasting Turing tests into this package.

from dynamicppl.jl.

Thanks @nickrobinson251 for your detailed comment. Yes, running Turing tests is fine and useful. In our case, we don't need to run all of the Turing tests to test DynamicPPL, only the subset that we have in the test folder here. However, this subset uses a significant chunk of Turing's src. If the DynamicPPL PR is not breaking, we can rely on the last Turing version for that as an optional test dependency. The part I am not happy about is the last point in your comment, that is having to run the tests locally for breaking changes. I think this is anti-CI in the sense that I don't know how the 2 packages will deploy with all their dependencies downloaded from a new user's perspective. This bit is missed when we have a DynamicPPL PR that changes the API used by Turing.

Copying all of Turing's src to the test folder of DynamicPPL basically means that we use the "new" Turing directly in CI tests which will be copied to a partner PR later after the merge and release of the DynamicPPL PR. Then when we make the new partner PR in Turing, we can make sure that we use the latest DynamicPPL released by bumping the lower bound in the compat section. So even though copying the whole thing seems excessive but I think it provides the least painful path forward. So you see the problem is not copying Turing's tests, it's copying Turing's src needed in the tests.

from dynamicppl.jl.

IMO, as already mentioned in the initial comment, the proper solution to this problem would be to write new tests completely independent of Turing. If required, one should rather add some dummy implementation of a simple sampler (similar to the new tests in AbstractMCMC: https://github.com/TuringLang/AbstractMCMC.jl/blob/master/test/interface.jl) than depend on a copy of Turing's tests. I think the test errors in #34 illustrate that the current setup is not satisfying - introducing breaking changes in DynamicPPL requires fixing Turing's tests and implementation twice, once in Turing itself and in the test folder in DynamicPPL. I guess, this is both annoying and could lead to inconsistencies.

from dynamicppl.jl.

Can we have a script or better specification for this? Simply copying Turing's src is not enough. I also had to

• Update the test dependencies from Turing's Project.toml
• Update Turing/test/test_utils into test/test_utils
• Fix absolute module imports (using Turing -> using .Turing) in at least test_utils/{AllUtils,ad_utils,testing_functions}.jl.

from dynamicppl.jl.