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

RFC: only auto precompile the added package (+ deps) after an add operation #3848

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

Conversation

KristofferC
Copy link
Member

Many times in the global env I add some small package to try it out and then Pkg starts precompiling everything (even things I did not intend to load). This means I often have to interrupt it (which is a bit brittle) and it feels like it is doing something I didn't ask it to do.

With this, it will only compile the deps graph of the added packages. If a user want to load some other package later, the load precompile step will pick that up.

@KristofferC KristofferC added the precompile Pkg.precompile label Mar 18, 2024
@IanButterworth
Copy link
Member

The current approach aims at having the only thing that should cause load-time precompilation being changes to dev-ed packages. This moves more precompilation back to load time, when deps of other packages are changed by an add, which IMO would be a UX regression. But perhaps this is popular?

Personally I prefer the current arrangement and using the tiered_installed resolve strategy. Less changes, precompilation stays at install time.

@KristofferC
Copy link
Member Author

KristofferC commented Mar 18, 2024

The current approach aims at having the only thing that should cause load-time precompilation being changes to dev-ed packages.

Yeah, of course, if you precompile the world once you don't have to precompile it later.

which IMO would be a UX regression

Well, it is not a straight-up regression, it is a trade-off. It is a big win in the cases where you do add Foo; using Foo; Foo.do_what_I_want() in that it takes 10s, and not 10 minutes because you happen to have OrdinaryDiffEq also in the env (which I wasn't going to use).


To be honest, with load time precompilation I probably don't see much personal value in the add precompile. Maybe I'll just turn it off 🤷‍♂️ .

@IanButterworth
Copy link
Member

I think we might have opposite opinions on this. Might be worth fielding opinions on slack?

@IanButterworth
Copy link
Member

A fancy solution would be to be specific like this PR, but spawn off the rest of the Project work as a background thing afterwards.

The UI of that needs figuring out, but it might not be too tricky..

  • notify that the rest of the project is being precompiled in the background. Use pkg> precompile --stop to interrupt
  • always be silent, don't report success or errors

Pidfile locking should protect against races and we already report if the current process is holding the lock in an async task.

@KristofferC
Copy link
Member Author

Right now, there is no way to turn off full precompilation of env on add while keeping precompilation of test env. :(

@KristofferC
Copy link
Member Author

KristofferC commented Jul 29, 2024

I must say this is sooo annoying.

(@v1.12) pkg> add Cthulhu
   Resolving package versions...
   Installed AbstractTrees ── v0.4.5
   Installed TypedSyntax ──── v1.3.1
   Installed WidthLimitedIO ─ v1.0.1
   Installed FoldingTrees ─── v1.2.1
   Installed JuliaSyntax ──── v0.4.9
   Installed Cthulhu ──────── v2.12.7
    Updating `~/.julia/environments/v1.12/Project.toml`
  [f68482b8] + Cthulhu v2.12.7
    Updating `~/.julia/environments/v1.12/Manifest.toml`
  [1520ce14] + AbstractTrees v0.4.5
  [f68482b8] + Cthulhu v2.12.7
  [1eca21be] + FoldingTrees v1.2.1
  [70703baa] + JuliaSyntax v0.4.9
  [d265eb64] + TypedSyntax v1.3.1
  [b8c1c048] + WidthLimitedIO v1.0.1
Precompiling all packages...
  Progress [==================================>      ]  237/287

Argh!! All the time.

And then I have to cancel it:

Precompiling all packages...
^CProgress [=====================================>   ]  263/287
[3454925] signal (2): Interrupt
in expression starting at /home/maleadt/manyjulias/bin/julia.jl:169
epoll_wait at /lib/x86_64-linux-gnu/libc.so.6 (unknown line)t
uv__io_poll at /workspace/srcdir/libuv/src/unix/epoll.c:236
uv_run at /workspace/srcdir/libuv/src/unix/core.c:400
ijl_task_get_next at /cache/build/default-amdci4-6/julialang/julia-release-1-dot-9/src/partr.c:390
 Interrupted: Exiting precompilation...

  4 dependencies had output during precompilation:
┌ SciMLBase
│  [3462422] signal 2: Interrupt
│  in expression starting at /home/kristofferc/.julia/packages/SciMLBase/hq1ku/src/callbacks.jl:158
│  trace_globals at /source/src/flisp/flisp.c:438
│  trace_globals at /source/src/flisp/flisp.c:440
│  trace_globals at /source/src/flisp/flisp.c:440
│  trace_globals at /source/src/flisp/flisp.c:440
│  trace_globals at /source/src/flisp/flisp.c:440
│  trace_globals at /source/src/flisp/flisp.c:440
│  trace_globals at /source/src/flisp/flisp.c:440
│  trace_globals at /source/src/flisp/flisp.c:440
│  trace_globals at /source/src/flisp/flisp.c:440
│  trace_globals at /source/src/flisp/flisp.c:440 [inlined]
│  gc at /source/src/flisp/flisp.c:582
│  apply_cl at /source/src/flisp/flisp.c:1360
│  _applyn at /source/src/flisp/flisp.c:679
│  fl_map1 at /source/src/flisp/flisp.c:2204 [inlined]
│  fl_map1 at /source/src/flisp/flisp.c:2192
│  apply_cl at /source/src/flisp/flisp.c:1224
│  _applyn at /source/src/flisp/flisp.c:679
│  fl_map1 at /source/src/flisp/flisp.c:2204 [inlined]
│  fl_map1 at /source/src/flisp/flisp.c:2192
│  apply_cl at /source/src/flisp/flisp.c:1224
│  _applyn at /source/src/flisp/flisp.c:679
│  fl_map1 at /source/src/flisp/flisp.c:2217 [inlined]
│  fl_map1 at /source/src/flisp/flisp.c:2192
│  apply_cl at /source/src/flisp/flisp.c:1224
│  _applyn at /source/src/flisp/flisp.c:679
└
┌ LoopVectorization
│  [3462400] signal 2: Interrupt
│  in expression starting at /home/kristofferc/.julia/packages/LoopVectorization/tIJUA/src/precompile.jl:4
│  _ZNK4llvm12MachineInstr12getSpillSizeEPKNS_15TargetInstrInfoE at /tmp/jl_BB7T81/bin/../lib/julia/libLLVM-17jl.so (unknown line)
└
┌ ClimaUtilities → ClimaUtilitiesClimaCoreInterpolationsExt
│  [3462490] signal 2: Interrupt
│  in expression starting at /home/kristofferc/.julia/packages/ClimaUtilities/zYtvC/ext/InterpolationsRegridderExt.jl:5
│  gc_mark_outrefs at /source/src/gc.c:2616 [inlined]
│  gc_mark_loop_serial_ at /source/src/gc.c:2891
│  gc_mark_loop_serial at /source/src/gc.c:2914
│  gc_mark_loop at /source/src/gc.c:3102 [inlined]
│  _jl_gc_collect at /source/src/gc.c:3474

and it is pages of errors.

@IanButterworth
Copy link
Member

IMO it's an issue for people that work on master. We could turn it off for non tagged builds?

@KristofferC
Copy link
Member Author

If add Foo precompiles everything why doesn't dev ./Foo precompile anything? Why would I want to be more likely to want to precompile IrrelevantPackage.jl if I do an add on some other package vs a dev?

@IanButterworth
Copy link
Member

Why does everything else need re-precompiling? Because you're changing julia version a lot, no?

@IanButterworth
Copy link
Member

Otherwise it's handled at install time for those packages.

@IanButterworth
Copy link
Member

And IMO we should #3905 to help everyone avoid re-precompilation they don't really need

@IanButterworth
Copy link
Member

Perhaps this could be merged behind a switch?

One reason I don't like it is that it dislocates cause and effect for precompilation activity. Like, add a package, updates a dep that is shared by another top level package. With this change the user will hit precompilation at a later stage and not know why, whereas currently they can see that a dep changed. For normal non package dev users, it reverts to the olden days of Pkg not really fully finishing the installation.

@IanButterworth
Copy link
Member

IanButterworth commented Oct 31, 2024

What about we do this but also add something that detects if you're using a new julia version compared to the manifest (wouldn't happen for nightly because we don't record the DEV number) and asks if you'd like to precompile the entire env?

A prompt with a shortish timeout would be best.

@KristofferC
Copy link
Member Author

How about this:

  • Keep the same behavior for package projects
  • Do this behavior for non-package projects (like the default environment).

@IanButterworth
Copy link
Member

IanButterworth commented Oct 31, 2024

I see your suggestion and raise you:

  • Keep the same behavior for package projects
  • Do this behavior for non-package projects (like the default environment), but if the manifest version has changed prompt with a brief timeout to ask if the full env should be precompiled. Default timeout prompt would be [n]

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

Successfully merging this pull request may close these issues.

2 participants