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 one artifact for each deposited GAP package #1069

Merged
merged 2 commits into from
Dec 10, 2024
Merged

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Dec 7, 2024

This is using the packages for GAP 4.14.0.

Copy link

codecov bot commented Dec 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.45%. Comparing base (6f03182) to head (5d51bad).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1069      +/-   ##
==========================================
- Coverage   74.63%   74.45%   -0.18%     
==========================================
  Files          55       55              
  Lines        4585     4588       +3     
==========================================
- Hits         3422     3416       -6     
- Misses       1163     1172       +9     
Files with missing lines Coverage Δ
src/setup.jl 82.64% <100.00%> (+0.59%) ⬆️

... and 2 files with indirect coverage changes

@mohamed-barakat

This comment was marked as off-topic.

@fingolfin

This comment was marked as off-topic.

@lgoettgens
Copy link
Member

This is the debug log of the download failures that we talked about earlier today (at the example of gap_pkg_homalg which was the first one attempted by my julia 1.11.2):

(GAP) pkg> resolve
┌ Debug: Downloading artifact from Pkg server
│   name = "GAP_pkg_homalg"
│   artifacts_toml = "/home/lgoe/code/julia/GAP.jl/Artifacts.toml"
│   platform = Linux x86_64 {cxxstring_abi=cxx11, julia_version=1.11.2, libc=glibc, libgfortran_version=5.0.0, libstdcxx_version=3.4.30}
│   url = "https://pkg.julialang.org/artifact/2881b49ed9fc87cd0040df78e3c9bde8f5151f13"
└ @ Pkg.Artifacts ~/.julia/juliaup/julia-1.11.2+0.x64.linux.gnu/share/julia/stdlib/v1.11/Pkg/src/Artifacts.jl:421
  Downloading artifact: GAP_pkg_homalg
┌ Debug: download and verify failed on attempt 1/3
│   url = "https://pkg.julialang.org/artifact/2881b49ed9fc87cd0040df78e3c9bde8f5151f13"
│   dest = "/tmp/jl_WN2HIJEJQN-download.gz"
│   err = RequestError: HTTP/2 404 while requesting https://pkg.julialang.org/artifact/2881b49ed9fc87cd0040df78e3c9bde8f5151f13
└ @ Pkg.PlatformEngines ~/.julia/juliaup/julia-1.11.2+0.x64.linux.gnu/share/julia/stdlib/v1.11/Pkg/src/PlatformEngines.jl:358
┌ Debug: download and verify failed on attempt 2/3
│   url = "https://pkg.julialang.org/artifact/2881b49ed9fc87cd0040df78e3c9bde8f5151f13"
│   dest = "/tmp/jl_WN2HIJEJQN-download.gz"
│   err = RequestError: HTTP/2 404 while requesting https://pkg.julialang.org/artifact/2881b49ed9fc87cd0040df78e3c9bde8f5151f13
└ @ Pkg.PlatformEngines ~/.julia/juliaup/julia-1.11.2+0.x64.linux.gnu/share/julia/stdlib/v1.11/Pkg/src/PlatformEngines.jl:358
┌ Debug: download and verify failed on attempt 3/3
│   url = "https://pkg.julialang.org/artifact/2881b49ed9fc87cd0040df78e3c9bde8f5151f13"
│   dest = "/tmp/jl_WN2HIJEJQN-download.gz"
│   err = RequestError: HTTP/2 404 while requesting https://pkg.julialang.org/artifact/2881b49ed9fc87cd0040df78e3c9bde8f5151f13
└ @ Pkg.PlatformEngines ~/.julia/juliaup/julia-1.11.2+0.x64.linux.gnu/share/julia/stdlib/v1.11/Pkg/src/PlatformEngines.jl:358
┌ Debug: download_artifact error
│   tree_hash = SHA1("2881b49ed9fc87cd0040df78e3c9bde8f5151f13")
│   tarball_url = "https://pkg.julialang.org/artifact/2881b49ed9fc87cd0040df78e3c9bde8f5151f13"
│   tarball_hash = nothing
│   err = RequestError: HTTP/2 404 while requesting https://pkg.julialang.org/artifact/2881b49ed9fc87cd0040df78e3c9bde8f5151f13
  Failure artifact: GAP_pkg_homalg
┌ Debug: Failed to download artifact from Pkg server
│   download_success = RequestError: HTTP/2 404 while requesting https://pkg.julialang.org/artifact/2881b49ed9fc87cd0040df78e3c9bde8f5151f13
└ @ Pkg.Artifacts ~/.julia/juliaup/julia-1.11.2+0.x64.linux.gnu/share/julia/stdlib/v1.11/Pkg/src/Artifacts.jl:430
┌ Debug: Downloading artifact
│   name = "GAP_pkg_homalg"
│   artifacts_toml = "/home/lgoe/code/julia/GAP.jl/Artifacts.toml"
│   platform = Linux x86_64 {cxxstring_abi=cxx11, julia_version=1.11.2, libc=glibc, libgfortran_version=5.0.0, libstdcxx_version=3.4.30}
│   url = "https://github.com/homalg-project/homalg_project/releases/download/homalg-2024.01-01/homalg-2024.01-01.tar.gz"
└ @ Pkg.Artifacts ~/.julia/juliaup/julia-1.11.2+0.x64.linux.gnu/share/julia/stdlib/v1.11/Pkg/src/Artifacts.jl:446
  Downloading artifact: GAP_pkg_homalg
  Downloaded artifact: GAP_pkg_homalg

We can see that Pkg tries to find the artifact in the Pkg servers three times, and only then starts using your provided urls.

@lgoettgens
Copy link
Member

When downloading and installing the socrates artifact, URLs will be attempted in order until one succeeds.
(https://pkgdocs.julialang.org/v1/artifacts/#Artifact-types-and-properties)

so the order is indeed relevant

@fingolfin
Copy link
Member Author

@lgoettgens yeah, I verified all that before submitting this PR ;-) thanks for double checking, never hurts

@fingolfin fingolfin force-pushed the mh/package-artifacts branch from d29fd3d to 648bb53 Compare December 7, 2024 19:47
@fingolfin
Copy link
Member Author

Initial script for importing the artifacts data is now in place, too.

I don't understand right now why it fails to build in Julia 1.6, but I'll wait with looking into that until also the JLLs are updated.

@fingolfin fingolfin mentioned this pull request Dec 7, 2024
@fingolfin
Copy link
Member Author

Hu, weird, the Julia 1.6 failures are also on master and started yesterday (first failure on master here while it was OK the day before in this log)

@fingolfin fingolfin changed the title Replace single GAP packages artifact by one artifact per GAP package Replace single GAP packages artifact by one artifact per GAP package & update to GAP 4.14.0 Dec 8, 2024
src/GAP.jl Outdated Show resolved Hide resolved
@lgoettgens
Copy link
Member

With the change of GAP from 4.13 to 4.14.0, we should change the used GAP branch in gap.yml from stable-4.13 to stable-4.14 as well.

@fingolfin fingolfin force-pushed the mh/package-artifacts branch 4 times, most recently from a3347cd to b36956f Compare December 9, 2024 18:57
@fingolfin fingolfin changed the title Replace single GAP packages artifact by one artifact per GAP package & update to GAP 4.14.0 Update to GAP 4.14.0 and major overhaul of handling the GAP package distribution Dec 9, 2024
@fingolfin fingolfin force-pushed the mh/package-artifacts branch from b36956f to b09e19f Compare December 9, 2024 19:27
@fingolfin fingolfin force-pushed the mh/package-artifacts branch 2 times, most recently from fc46ecd to d46dad8 Compare December 10, 2024 14:42
@fingolfin fingolfin changed the title Update to GAP 4.14.0 and major overhaul of handling the GAP package distribution Use one artifact for each deposited GAP package Dec 10, 2024
@fingolfin fingolfin force-pushed the mh/package-artifacts branch from d46dad8 to e0a95be Compare December 10, 2024 14:44
@fingolfin fingolfin marked this pull request as ready for review December 10, 2024 14:45
@fingolfin
Copy link
Member Author

I've now reduced this again to just replace the single monolithic package artifact with one artifact per package. I think this was already working reasonably well (?) but we'll see.


# to separate the scratchspaces of different GAP.jl copies and Julia versions
# put the Julia version and the hash of the path to this file into the key
const scratch_key = "gap_$(string(hash(@__FILE__)))_$(VERSION.major).$(VERSION.minor)"
const scratch_key = "gap_$(hash(@__FILE__))-$(VERSION.major).$(VERSION.minor)"
Copy link
Member

Choose a reason for hiding this comment

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

IIuc this change is here to not use the same scratchspace as before as that could lead to conflicts, right? Wouldn't it make sense to either include some kind of increasing number in either the key or in the hash input, so we can bump that again for the next incompatible change in the future?

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 actually had an intermediate version where I computed a more complicated hash, which involved the paths of the various artifacts (!) as those can also change; but also checksums of e.g. setup.jl and other files, but then realized this doesn't solve all the problems either...

Here are some further alternatives:

  • as above, include more data into the hash (e.g. GAP.jl version, location of the various JLLs and artifacts we link to, ...)
  • stop using a scratch space, instead just create a fresh "mutable root dir" each time the package is loaded in a fresh temp dir (of course that means a lot of temp dirs would accumulate... we could add an atexit handler to delete them at the end?)
    • would be interesting how much time we spend creating the mutable root dir
  • we could adjust the setup code so that whenever it e.g. creates a dir or symlink it checks if there is already an object of the correct type there -- i.e. if we created a dir pkg and there is already a dir pkg, do nothing; but if we create a symlink pkg pointing to targetA and there is instead a directory pkg; or a symlink pointing to anotherTarget, then delete pkg.
    • this seems much more complicated and in every way I can think of inferior to the solution of just rebuilding it all from scratch, except that at least the path to this root dir is somewhat stable and predictable
    • strictly speaking we'd also have to scan for obsolete file system objects that should be removed... making this even more complicated...

Copy link
Member Author

Choose a reason for hiding this comment

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

See also #993

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, yeah. No strong preference between 1 and 2.
But it seems that this is not immediately needed as running this PR works locally

src/setup.jl Outdated Show resolved Hide resolved
@lgoettgens
Copy link
Member

The timeouts are probably happening as the new artifacts are not cached yet, and (even worse) Pkg will first try to download them from the Pkg servers 3 times before using the provided github source. This overhead should reduce once there is a release with these new artifacts.

Having one artifact per package instead of one single artifact has the
advantage that user's don't need to download a single 500 MB blob (which
is very annoying if one is on a flaky connection). And when updating
GAP, then only those package artifacts that actually changed need to be
re-downloaded, which reduces disk space usage.

This also paves the way for shipping fewer packages out of the box (e.g.
we could make some or all of these artifacts lazy).

Moreover the `GAP_pkg_XYZ.jl` wrapper packages use the same artifacts,
so if one uses those no extra disk or download overhead is incurred.

The packages used here are from GAP 4.14.0. The `Artifacts.toml` file
can be updated for future GAP releases by using the helper script
`etc/update_artifacts.jl`.
@fingolfin fingolfin force-pushed the mh/package-artifacts branch from e0a95be to d762fb6 Compare December 10, 2024 15:59
Co-authored-by: Lars Göttgens <[email protected]>
@fingolfin fingolfin enabled auto-merge (squash) December 10, 2024 16:59
@fingolfin fingolfin merged commit 0412fa6 into master Dec 10, 2024
21 checks passed
@fingolfin fingolfin deleted the mh/package-artifacts branch December 10, 2024 17:27
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.

4 participants