ApproximateGPs.jl icon indicating copy to clipboard operation
ApproximateGPs.jl copied to clipboard

Use same ApproximateGPs version for docs and examples

Open devmotion opened this issue 3 years ago • 8 comments

Fixes #105.

devmotion avatar Jan 30 '22 12:01 devmotion

Codecov Report

Merging #106 (1f3e9e0) into master (074db1b) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #106   +/-   ##
=======================================
  Coverage   95.78%   95.78%           
=======================================
  Files           4        4           
  Lines         285      285           
=======================================
  Hits          273      273           
  Misses         12       12           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 074db1b...1f3e9e0. Read the comment docs.

codecov[bot] avatar Jan 30 '22 13:01 codecov[bot]

Using a relative path to the package makes the resulting Manifest a bit less dependent on the directory structure in the Github action ("../.." instead of something like "~/work/ApproximateGPs.jl/ApproximateGPs.jl/"). However, nevertheless it means that anyone who wants to reuse the Manifest has to ensure that the exactly same (possibly unreleased) version of ApproximateGPs.jl is located at "../.." (or some other location which however requires to update the Manifest manually).

One alternative, probably more reproducible, approach would be to use the same version as in the docs by default (as done in this PR so far) but to use the pinned version of the corresponding Github commit when docs/make.jl is executed in a Github action (by using Github-specific environment variables). In that way always the same version of ApproximateGPs would be used to build the docs and run the examples but one could just take the Manifest to reproduce the results (as long as the Github commit exists).

devmotion avatar Jan 30 '22 18:01 devmotion

Does that also work for the examples then?

theogf avatar Jan 31 '22 10:01 theogf

Hmm not sure if I understand your question correctly. This PR and the suggestions above are only concerned with the examples, the docs are built in the same standard way. The problem is just that

  • currently the examples use the latest compatible released version of the package which usually is different from what is used by the docs and doesn't allow us to see if any changes in a PR break the examples (fixed by this PR)
  • publishing Manifest files with absolute or relative paths to the package (as done in this PR) make it very difficult/impossible to reproduce the package setup and one definitely can't just take the Manifest file.

The second point would be addressed by using Github commits (on the master branch and possibly PRs from the repo) and version tags (for releases) in the Manifest file instead. The implementation would be more difficult (but possible, I played around with it) but still the correct version (same as for docs) would be used for the examples and one could just download the Manifest file and reproduce the package setup.

devmotion avatar Jan 31 '22 12:01 devmotion

FYI the PR contains some bugs but the approach was quite nicely it seems.

I used and developed this Literate setup in a bunch of personal and other packages, so I just went on and updated one of them. I ended up rewriting and improving large parts of the setup (including fixing some upstream bugs in Literate) (https://github.com/devmotion/CalibrationErrors.jl/pull/130) but finally everything seems to work: https://devmotion.github.io/CalibrationErrors.jl/dev/examples/classification/

As you can see in the Pkg status and the Manifest file, the repo is pinned to the git commit on the master branch and hence the example is fully reproducible and does not depend on the paths of the Github runners.

(Another major change is that Literate is not a dependency of the examples anymore.)

devmotion avatar Feb 11 '22 15:02 devmotion

It seems to work also for the stable docs (tagged a release yesterday for the first time since the update): https://devmotion.github.io/CalibrationErrors.jl/stable/examples/classification/

devmotion avatar Feb 15 '22 15:02 devmotion

~@devmotion is this alright to be merged then?~

EDIT: my reading comprehension is apparently limited

rossviljoen avatar Feb 27 '22 14:02 rossviljoen

No, in CalibrationErrors I used a different implementation. As said above, this PR is broken, ie contains some bugs.

devmotion avatar Feb 27 '22 14:02 devmotion