avoid loading pkg until needed #1

IanButterworth · 2024-09-10T14:20:41Z

This PR

julia> @time using MicroMamba
  0.195992 seconds (343.78 k allocations: 18.925 MiB, 19.03% gc time, 78.26% compilation time: 99% of which was recompilation)

julia> @time @time_imports using MicroMamba
      0.6 ms  Printf
     15.5 ms  Dates
      0.4 ms  Scratch
     10.9 ms  LazyArtifacts
      0.6 ms  TOML
      7.8 ms  Preferences
      0.4 ms  JLLWrappers
               ┌ 120.3 ms micromamba_jll.__init__() 99.79% compilation time (98% recompilation)
    222.6 ms  micromamba_jll 99.59% compilation time (99% recompilation)
      0.4 ms  MicroMamba
  0.271243 seconds (808.59 k allocations: 43.782 MiB, 14.78% gc time, 82.66% compilation time: 99% of which was recompilation)

julia master

julia> @time using MicroMamba
  0.355640 seconds (610.25 k allocations: 39.037 MiB, 13.60% gc time, 14.79% compilation time: 100% of which was recompilation)

julia> @time @time_imports using MicroMamba
      0.5 ms  Printf
     11.7 ms  Dates
      0.3 ms  Scratch
      0.3 ms  TOML
               ┌ 0.0 ms NetworkOptions.__init__()
      1.0 ms  NetworkOptions
               ┌ 0.8 ms MbedTLS_jll.__init__()
      2.6 ms  MbedTLS_jll
               ┌ 0.3 ms LibSSH2_jll.__init__()
      2.0 ms  LibSSH2_jll
               ┌ 0.6 ms LibGit2_jll.__init__()
      2.1 ms  LibGit2_jll
      6.9 ms  LibGit2
      5.5 ms  ArgTools
               ┌ 0.3 ms nghttp2_jll.__init__()
      1.8 ms  nghttp2_jll
               ┌ 0.6 ms LibCURL_jll.__init__()
      2.3 ms  LibCURL_jll
               ┌ 0.0 ms MozillaCACerts_jll.__init__()
      1.6 ms  MozillaCACerts_jll
               ┌ 0.0 ms LibCURL.__init__()
      0.9 ms  LibCURL
               ┌ 2.9 ms Downloads.Curl.__init__()
     15.4 ms  Downloads
      0.7 ms  Tar
               ┌ 0.0 ms p7zip_jll.__init__()
      2.1 ms  p7zip_jll
      0.3 ms  UUIDs
      0.2 ms  Logging
               ┌ 0.0 ms Pkg.__init__()
    221.1 ms  Pkg
      0.5 ms  LazyArtifacts
      7.3 ms  Preferences
      0.5 ms  JLLWrappers
               ┌ 50.3 ms micromamba_jll.__init__() 99.61% compilation time (100% recompilation)
    126.1 ms  micromamba_jll 99.23% compilation time (75% recompilation)
      0.5 ms  MicroMamba
  0.447403 seconds (1.07 M allocations: 63.718 MiB, 15.97% gc time, 28.90% compilation time: 76% of which was recompilation)

giordano · 2024-09-10T14:25:00Z

Should delete

LazyArtifacts.jl/Project.toml

Line 6 in c1c1c7a

Pkg = "44cfe95a-1eb2-52ea-b672-e2afdf69b78f"

too

IanButterworth · 2024-09-10T14:25:51Z

I'm not sure about that. I think we need the Project dependency even if we're not always loading.

giordano · 2024-09-10T14:28:04Z

Ah, is that because of

LazyArtifacts.jl/src/LazyArtifacts.jl

Line 52 in 362299d

    
                   Pkg = Base.require_stdlib(Base.PkgId(Base.UUID("44cfe95a-1eb2-52ea-b672-e2afdf69b78f"), "Pkg"));

?

staticfloat · 2024-09-10T15:28:29Z

src/LazyArtifacts.jl

+    if !artifact_exists(hash)
+        # loading Pkg is a bit slow, so we only do it if we need to
+        # and do it in a subprocess to avoid precompilation complexity
+        cmd = run(pipeline(`$(Base.julia_cmd()) -E '


What's the benefit of subbing out to another process vs. doing something like Base.inference_barrier?

@vtjnash would that work? It'd be nice to not have to manage the download progress bar behavior coming from the sub process, while being able to read back the resulting path at the end

you cannot unload it without a process, so it is illegal to load during precompile

I see; yes, if this is run during precompilation, loading another module will screw up the precompilation output, so that makes sense.

src/LazyArtifacts.jl

IanButterworth · 2024-09-10T17:20:38Z

I build julia with this locally and confirmed that the progress bar shows properly with overprinting etc.

julia> using PythonCall
  Downloaded artifact: micromamba

julia>

topolarity · 2024-09-11T16:54:16Z

src/LazyArtifacts.jl

+            Pkg = Base.require_stdlib(Base.PkgId(Base.UUID("44cfe95a-1eb2-52ea-b672-e2afdf69b78f"), "Pkg"));
+            Pkg.Artifacts.try_artifact_download_sources(
+                $(repr(name)),
+                Base.$(repr(hash)),
+                $(repr(meta)),
+                $(repr(artifacts_toml));
+                platform = Base.BinaryPlatforms.$(repr(platform)),
+                verbose = $(repr(verbose)),
+                quiet_download = $(repr(quiet_download)),
+                io = stderr
+            )


Hmm, unfortunately running a sub-process with dynamic code like this is very incompatible with --trim

See #1 (comment) for some discussion

And JuliaLang/julia#55725 (comment)

Is it possible to do this only during pre-compilation? That behavior on its own is fine, but doing it at runtime is problematic for --trim

Is it possible to do this only during pre-compilation?

If we revert JuliaPackaging/JLLWrappers.jl#66 yes 😛

I don't really agree that lazy artifacts are incompatible with AOT compilation - but they are definitely incompatible with the idea of providing a self-contained executable/package

My request was for this to be branched on Base.generating_output() because the true branch is not inferable/compilable for juliac, but we have ways to let juliac know that only the false branch is ever taken at run-time.

In that case, you're still running an AOT-compiled executable and the download works - it just happens lazily.

(Didn't see Cody's reply before sending this)
I think the question here is more about legality, rather than preference that something orchestrating --trim would have for artifact downloads a la PackageCompiler.

As it is now seems more generally legal.

On second thought, I fear that Base.require_stdlib is still going to be effectively impossible for us to infer

We're not even allowed to load the stdlib for inference on its own functions, because that would break the same pre-compilation assumptions we're skirting in the other branch.

I'm a bit worried that this change just makes LazyArtifacts incompatible with --trim unfortunately

We might have to find a way to load Pkg eagerly for --trim alone (which unfortunately means that AOT-compiled applications don't get this speed up)

I don't know if you can infer it directly, but you should be able to pre-emulate it, since it is just a shim for finding the right .ji file to load, and that can be pre-computed and run directly (esp. for --trim)

Right but then I need to infer Pkg.Artifacts.try_artifact_download_sources and also ensure that its code is available in the loaded pkgimage (or our own sysimage)

topolarity · 2024-09-11T23:36:35Z

How did we get into this situation where LazyArtifacts depends on Pkg for this implementation, instead of vice-versa?

I'm probably missing some historical context..

But it feels like this would maybe better be solved by getting the code out of Pkg?

IanButterworth · 2024-09-12T00:45:29Z

Others here may be able to correct/expand.. but:

Because Pkg used to be in the sysimage so was fast, and downloading artifacts is handled by Pkg at Pkg.add etc. time, including hooking into the pkgserver setup because artifacts hosted by pkgserver-ed packages are attempted to be downloaded from the pkgserver before any declared url.

i.e.
The Artifacts stdlib doesn't do downloading and has no deps, just points to things that were installed by Pkg.
Pkg.Artifacts does downloading during Pkg.add etc.
LazyArtifacts doesn't do downloading, but needs to orchestrate it, so depends on Pkg.

I think if we simply wanted to merge Pkg.Artifacts into Artifacts it would have to take on Downloads ,Tar, TOML, LibGit2 and also take with it the Pkg.GitTools module.

Artifacts currently has no deps.

staticfloat · 2024-09-12T00:53:47Z

I'm probably missing some historical context..

Originally, everything having to do with packages was in Pkg. There was no Artifacts module, and both lazy and non-lazy artifacts were all just functionality in Pkg. Eventually, we split things out into separate modules, and we maintained the functionality within Pkg for backwards-compatibility.

IanButterworth · 2024-09-12T11:09:45Z

Before merge I should add a test for the fork done during precompilation.

topolarity · 2024-09-12T13:20:23Z

I did some experimenting - Ripping out the relevant bits doesn't look too bad: JuliaLang/julia#55755

IanButterworth mentioned this pull request Sep 10, 2024

avoid loading Pkg unless necessary JuliaLang/LazyArtifacts.jl#1

Closed

IanButterworth force-pushed the ib/dont_load_pkg branch from 1c85c60 to f66d10a Compare September 10, 2024 14:22

IanButterworth mentioned this pull request Sep 10, 2024

136x regression for LazyArtifacts JuliaLang/julia#55725

Open

avoid loading pkg until we have to

362299d

IanButterworth force-pushed the ib/dont_load_pkg branch from f66d10a to 362299d Compare September 10, 2024 14:24

tryfix

e80fa25

staticfloat reviewed Sep 10, 2024

View reviewed changes

IanButterworth added 3 commits September 10, 2024 12:49

fixes

e3fd69f

don't cache artifacts because we need to test downloading

80dd849

update CI

2a6b4e4

IanButterworth force-pushed the ib/dont_load_pkg branch from 4e56e8b to 2a6b4e4 Compare September 10, 2024 16:57

IanButterworth commented Sep 10, 2024

View reviewed changes

src/LazyArtifacts.jl Outdated Show resolved Hide resolved

fix getting output back from subprocess

3aeb8be

IanButterworth requested a review from vtjnash September 10, 2024 17:20

IanButterworth added 2 commits September 10, 2024 13:22

add comment

9999ed5

add a precompile

80b12ab

This comment was marked as resolved.

Sign in to view

IanButterworth changed the title ~~avoid loading pkg until we have to~~ avoid loading pkg until needed Sep 11, 2024

IanButterworth mentioned this pull request Sep 11, 2024

Add invokelatest barrier to string(...) in @assert JuliaLang/julia#55739

Merged

topolarity reviewed Sep 11, 2024

View reviewed changes

IanButterworth mentioned this pull request Sep 11, 2024

Add some loading / LazyArtifacts precompiles to the sysimage JuliaLang/julia#55740

Merged

IanButterworth added 3 commits September 11, 2024 13:43

only use a subprocess if generating output (precompiling)

83e3328

add invokelatest

6cde8c2

improve comments

ca58bba

vchuravy approved these changes Sep 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

avoid loading pkg until needed #1

avoid loading pkg until needed #1

IanButterworth commented Sep 10, 2024 •

edited

Loading

giordano commented Sep 10, 2024

IanButterworth commented Sep 10, 2024

giordano commented Sep 10, 2024

staticfloat Sep 10, 2024

IanButterworth Sep 10, 2024

vtjnash Sep 10, 2024

staticfloat Sep 10, 2024

IanButterworth commented Sep 10, 2024

This comment was marked as resolved.

topolarity Sep 11, 2024

IanButterworth Sep 11, 2024

IanButterworth Sep 11, 2024

topolarity Sep 11, 2024

giordano Sep 11, 2024

topolarity Sep 11, 2024

IanButterworth Sep 11, 2024 •

edited

Loading

topolarity Sep 11, 2024 •

edited

Loading

vtjnash Sep 11, 2024

topolarity Sep 11, 2024

topolarity commented Sep 11, 2024

IanButterworth commented Sep 12, 2024

staticfloat commented Sep 12, 2024

IanButterworth commented Sep 12, 2024

topolarity commented Sep 12, 2024

avoid loading pkg until needed #1

Are you sure you want to change the base?

avoid loading pkg until needed #1

Conversation

IanButterworth commented Sep 10, 2024 • edited Loading

This PR

julia master

giordano commented Sep 10, 2024

IanButterworth commented Sep 10, 2024

giordano commented Sep 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IanButterworth commented Sep 10, 2024

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IanButterworth Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

topolarity Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

topolarity commented Sep 11, 2024

IanButterworth commented Sep 12, 2024

staticfloat commented Sep 12, 2024

IanButterworth commented Sep 12, 2024

topolarity commented Sep 12, 2024

IanButterworth commented Sep 10, 2024 •

edited

Loading

IanButterworth Sep 11, 2024 •

edited

Loading

topolarity Sep 11, 2024 •

edited

Loading