Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use same ApproximateGPs version for docs and examples #106

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

devmotion
Copy link
Member

Fixes #105.

@codecov
Copy link

codecov bot commented Jan 30, 2022

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.

@devmotion
Copy link
Member Author

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).

@theogf
Copy link
Member

theogf commented Jan 31, 2022

Does that also work for the examples then?

@devmotion
Copy link
Member Author

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.

docs/make.jl Outdated Show resolved Hide resolved
docs/make.jl Outdated Show resolved Hide resolved
docs/make.jl Outdated Show resolved Hide resolved
@devmotion
Copy link
Member Author

devmotion commented Feb 11, 2022

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) (devmotion/CalibrationErrors.jl#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
Copy link
Member Author

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/

@rossviljoen
Copy link
Collaborator

rossviljoen commented Feb 27, 2022

@devmotion is this alright to be merged then?

EDIT: my reading comprehension is apparently limited

@devmotion
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Examples don't use correct version of ApproximateGPs
4 participants