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

Parallelize artifact downloads. Update progress bar styling. #3952

Merged

Conversation

IanButterworth
Copy link
Member

@IanButterworth IanButterworth commented Jul 13, 2024

This PR

% JULIA_DEPOT_PATH=$(mktemp -d): JULIA_PKG_PRECOMPILE_AUTO=0 ./julia --project=../Pkg.jl -t1 -e 'import Pkg; Pkg.activate(temp=true); @time Pkg.add("FFMPEG")'
...
 12.999424 seconds (9.81 M allocations: 915.741 MiB, 2.30% gc time, 73 lock conflicts, 11.69% compilation time: 16% of which was recompilation)

Master

% JULIA_DEPOT_PATH=$(mktemp -d): JULIA_PKG_PRECOMPILE_AUTO=0 ./julia-t1 -e 'import Pkg; Pkg.activate(temp=true); @time Pkg.add("FFMPEG")'
...
 23.540455 seconds (9.37 M allocations: 887.289 MiB, 0.82% gc time, 123 lock conflicts, 5.85% compilation time: 18% of which was recompilation)

Screen.Recording.2024-08-15.at.12.49.12.AM.mov

Todo

  • parallel progress printing
  • effective interrupt handling (proven to be hard to do because of Downloads.jl being so async)
    • needs testing
  • error handling & reporting
    • needs testing
  • code tidy and remove duplication (will implement a new MiniMultiProgressBar type in a follow-on)
  • simple printing for CI etc.
  • show progress in bytes
    • rate/s ?
    • total across all artifacts?

@IanButterworth IanButterworth force-pushed the ib/parallel_artifact_download branch from 2476ee3 to 4bb54ae Compare July 15, 2024 16:11
@IanButterworth IanButterworth force-pushed the ib/parallel_artifact_download branch from 23daab2 to 9a5ce60 Compare August 7, 2024 03:23
@IanButterworth

This comment was marked as outdated.

@IanButterworth IanButterworth changed the title HACK: parallelize artifact downloads Parallelize artifact downloads Aug 13, 2024
@IanButterworth IanButterworth force-pushed the ib/parallel_artifact_download branch from d1da835 to b6a94e7 Compare August 13, 2024 13:36
@IanButterworth IanButterworth force-pushed the ib/parallel_artifact_download branch from 54f6963 to 77e3b6a Compare August 14, 2024 16:21

errors = Any[]
# first try downloading from Pkg server
# TODO: only do this if Pkg server knows about this package
Copy link
Member

Choose a reason for hiding this comment

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

Pkg.jl/src/Operations.jl

Lines 918 to 931 in a717900

# Check if the current package is available in one of the registries being tracked by the pkg server
# In that case, download from the package server
if server_registry_info !== nothing
server, registry_info = server_registry_info
for reg in ctx.registries
if reg.uuid in keys(registry_info)
if haskey(reg, pkg.uuid)
url = "$server/package/$(pkg.uuid)/$(pkg.tree_hash)"
push!(archive_urls, url => true)
break
end
end
end
end
could be extracted into a function and used here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, but we'd have to feed the pkg info into this point. I'm unsure of the relationship between packages and artifacts and the pkgserver. Probably best as another PR.

download_states = Dict{String, Tuple{Bool,MiniProgressBar}}()
errors = Channel{Any}()
is_done = false
ansi_moveup(n::Int) = string("\e[", n, "A")
Copy link
Member

Choose a reason for hiding this comment

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

Ideally this ANSI stuff could be moved to the progress bar code where there is something like:

struct MultiProgressBar
    progress_bars::Vector{MiniProgressBar}
end

which takes care of the printing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. Will handle this in a follow-on. Added a TODO

@KristofferC
Copy link
Member

KristofferC commented Aug 15, 2024

I think the number of significant figures in Base.format_bytes is a bit high.

Edit: JuliaLang/julia#55495

@KristofferC KristofferC force-pushed the ib/parallel_artifact_download branch from fd0e9f6 to 2160523 Compare August 15, 2024 10:20
@KristofferC
Copy link
Member

I updated / modernized the progress bar styling a bit. It now looks like:

Screencast.from.08-15-2024.12.19.04.PM.webm

@IanButterworth

This comment was marked as resolved.

@IanButterworth
Copy link
Member Author

Screenshot 2024-08-16 at 2 06 58 PM

@IanButterworth IanButterworth marked this pull request as ready for review August 16, 2024 13:52
@IanButterworth IanButterworth changed the title Parallelize artifact downloads Parallelize artifact downloads. Update progress bar styling. Aug 16, 2024
@IanButterworth
Copy link
Member Author

Lets give this a go. @KristofferC and I have both been testing it out.

@IanButterworth IanButterworth merged commit d1d2fc9 into JuliaLang:master Aug 16, 2024
7 checks passed
@IanButterworth IanButterworth deleted the ib/parallel_artifact_download branch August 16, 2024 15:11
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.

2 participants