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 Julia 1.11 for CI tests and docs #648

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

Conversation

ferrolho
Copy link
Member

@ferrolho ferrolho commented Nov 9, 2024

This PR is for using Julia 1.11 for CI tests and docs.

I have tested it locally and the pipelines below also seem to be happy. 🎉

Important

There's something missing from this PR that I cannot fix myself because it has to do with the repo settings regarding the secret keys for Documenter.jl (this) and the GitHub action that deploys the documentation after it has been built. You can see the error from the last deploy attempt here (or pasted below) — FYI @tkoolen @rdeits.

Please make sure you have the correct access rights
and the repository exists.
┌ Error: Git failed to fetch [email protected]:JuliaRobotics/RigidBodyDynamics.jl.git
│ This can be caused by a DOCUMENTER_KEY variable that is not correctly set up.
│ Make sure that the environment variable is properly set up as a Base64-encoded string
│ of the SSH private key. You may need to re-generate the keys with DocumenterTools.
└ @ Documenter ~/.julia/packages/Documenter/H5y27/src/Documenter.jl:652

Here's how to build the docs and serve them locally:

cd RigidBodyDynamics.jl
julia --project=docs docs/make.jl
julia -e 'using LiveServer; serve(dir="docs/build")'

I keep forgetting this and have to check it every time I do maintenance on documentation of Julia packages, so I am writing this here mostly for my future reference... 😛

Comment on lines -63 to -81

docs:
name: Documentation
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- uses: julia-actions/setup-julia@v1
with:
version: '1.6'
- run: |
julia --project=docs -e '
using Pkg
Pkg.develop(PackageSpec(path=pwd()))
Pkg.instantiate()
include("docs/make.jl")'
env:
PYTHON: ""
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
DOCUMENTER_KEY: ${{ secrets.DOCUMENTER_KEY }}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have moved this into .github/workflows/Documentation.yml.

- uses: actions/checkout@v4
- uses: julia-actions/setup-julia@v2
with:
version: '1.11'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs are now being built using Julia v1.11.

docs/make.jl Outdated
Comment on lines 14 to 16
if VERSION >= v"1.6"
push!(excludefiles, "6. Symbolics.jl")
push!(excludefiles, "7. Rigorous error bounds using IntervalArithmetic.jl")
Copy link
Member Author

@ferrolho ferrolho Nov 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit annoying... I have already spent a lot of time on this PR, but I haven't been able to fix the issues with these two notebooks.

  • For 6. Symbolics.jl, I am not sure how to fix it and there is an open issue for this (Symbolic Derivatives with SymPy do not work #630). Update: I have managed to get the 6. Symbolics.jl example notebook back up and running in 5155344.
  • For 7. Rigorous error bounds using IntervalArithmetic.jl I have already applied a few updates to use the new syntax and API of IntervalArithmetic.jl, but not all the cells of the notebook are fixed yet.

],
format = Documenter.HTML(prettyurls = parse(Bool, get(ENV, "CI", "false")))
],
warnonly = Documenter.except(),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to add this line warnonly = Documenter.except() because otherwise the build fails due to :missing_docs errors (which means not all docstrings are being used by the Docs).

Comment on lines 487 to +493
[[deps.MeshCatMechanisms]]
deps = ["ColorTypes", "CoordinateTransformations", "GeometryBasics", "InteractBase", "Interpolations", "LoopThrottle", "MechanismGeometries", "MeshCat", "OrderedCollections", "RigidBodyDynamics"]
git-tree-sha1 = "b766636d31175340e542c833cff1895c2f3f41cc"
git-tree-sha1 = "c6ca2052dc571cfe1395a12e970101c9356cf85f"
repo-rev = "hf/upstream-changes"
repo-url = "https://github.com/ferrolho/MeshCatMechanisms.jl.git"
uuid = "6ad125db-dd91-5488-b820-c1df6aab299d"
version = "0.9.0"
version = "0.9.1"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Manifest.toml files of the notebooks need to be generated again after JuliaRobotics/MeshCatMechanisms.jl#71 has been merged. Right now, they are referring to the PR branch from my fork.

@@ -30,7 +30,7 @@ include("test_simulate.jl")
include("test_mechanism_modification.jl")
include("test_pd_control.jl")

if v"1.9" <= VERSION < v"1.10"
if v"1.11" <= VERSION < v"1.12" # Test notebooks on Julia v1.11
Copy link
Member Author

@ferrolho ferrolho Nov 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for running the notebook tests and benchmarks only on Julia v1.11 runners.

repo = "github.com/JuliaRobotics/RigidBodyDynamics.jl.git"
repo="github.com/JuliaRobotics/RigidBodyDynamics.jl",
devbranch="master",
push_preview=true,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

push_preview = true turned out to be useless for this PR because it only works for PRs originating from the same repo instead of from forked repositories — see this line of the job.

Comment on lines +16 to +20
## Type piracy needed in order to make Symbolics.jl types interact well with Quaternions.jl.
## This should be avoided — see https://docs.julialang.org/en/v1/manual/style-guide/#avoid-type-piracy.
function Base.:/(q::Quaternions.Quaternion, x::Symbolics.Num)
return Quaternions.Quaternion(q.s / x.val, q.v1 / x.val, q.v2 / x.val, q.v3 / x.val)
end
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took this from https://github.com/dionysos-dev/Dionysos.jl/blob/master/BipedRobot/src/piracy.jl to check if it would fix the issue in #630 and it did! So, we can close that issue when we merge this PR.

Comment on lines +93 to +100
try
## This throws `ERROR: TypeError: non-boolean (Num) used in boolean context` because
## of `m > 0 || return zero(cache_eltype(state))` in `gravitational_potential_energy`.
## See https://docs.sciml.ai/Symbolics/stable/manual/faq/ for more details.
simplify(gravitational_potential_energy(x), expand=true)
catch e
e
end
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cell is not working because of the short-circuiting on this line:

m > 0 || return zero(cache_eltype(state))

I tried removing the short-circuiting statement and it seems to fix it, but the repercussions of doing that are not clear to me. I am afraid that not having the early return might affect performance somehow, but on the other hand I could not see it being used as part of other methods...

@tkoolen would be the best person to chip in. 😛

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.

1 participant