Comments (9)
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:
- Locally apply any breaking changes to DynamicPPL fixing Turing's src accordingly
- Copy and paste all of Turing's src to DynamicPPL's test folder and make an "update tests" commit
- Make sure DynamicPPL tests pass
- 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
intotest/test_utils
- Fix absolute module imports (
using Turing
->using .Turing
) in at leasttest_utils/{AllUtils,ad_utils,testing_functions}.jl
.
from dynamicppl.jl.
Related Issues (20)
- Derived variables from data on the LHS of tilde HOT 10
- Decouple from Distributions.jl HOT 8
- ThreadSafeVarInfo(::SimpleVarInfo) bug with ForwardDiff HOT 1
- Need alternative to `NamedTuple` for `SimpleVarInfo` HOT 12
- Storing returned values in `VarInfo`.
- `BangBang.setindex!!(vi::SimpleVarInfo{<:AbstractDict}, val, vn::VarName)` returns eltype Any arrays when `vn` is multidimensional HOT 1
- Remove `tonamedtuple`
- Can't use `@info` in models HOT 1
- Move TestUtils to an extension
- `SimpleVarInfo` sometimes returns a `SimpleVarInfo` with `Int`-type `logp` and creates down-stream errors HOT 1
- Roadmap for removal of `Selector` shenanigans HOT 2
- change defaults: @addlogprob! not included in PriorContext() HOT 5
- Type-instability when using type-parameters HOT 1
- Type instability in unflatten for medium / large tuples HOT 6
- DynamicPPL..jl tests 23 Broken. HOT 3
- Introduce a convenience function, `AutoType`, to hide `TypeWrap`.
- Issue with ReverseDiff and undef on Julia v1.7 HOT 1
- Add an exported function to get variable names for a model
- Replace uage of `typeof` with `Core.Typeof` HOT 2
- `minibatch` / `stochastic_gradient` operation on models HOT 5
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from dynamicppl.jl.