Restore initial state of `DEPOT_PATH` after precompilation workload #51746

fatteneder · 2023-10-17T20:54:30Z

Idea came from JuliaLang/Pkg.jl#3668 (comment)

At least preserving DEPOT_PATH through precompilation workload will be essential for #49866.

Julia build will now fail, because Pkg does not pass that check until JuliaLang/Pkg.jl#3668 is merged.

And I guess this will also need a PkgEval.

DilumAluthge · 2023-10-18T00:41:04Z

base/loading.jl

@@ -2296,17 +2296,44 @@ function include_package_for_output(pkg::PkgId, input::String, depot_path::Vecto
        task_local_storage()[:SOURCE_PATH] = source
    end

+    original_depot_path = copy(Base.DEPOT_PATH)
+    original_load_path = copy(Base.LOAD_PATH)
+    original_env = copy(ENV)


Let's comment out the ENV-related checks for now?

AFAICT, changes to ENV shouldn't affect #49866, right?

DilumAluthge · 2023-10-18T00:41:55Z

This is only affected by what the package does during precompilation, right? It's not affected by anything a package does in its __init__() function?

KristofferC · 2023-10-18T14:21:33Z

Couldn't these just restore the paths instead of erroring if they are mutated?

DilumAluthge · 2023-10-18T14:31:06Z

Couldn't these just restore the paths instead of erroring if they are mutated?

I think that would be fine, although I think it would be nice to encourage package authors to clean up at the end of their precompile workload.

KristofferC · 2023-10-18T14:47:05Z

I think it would be nice to encourage package authors to clean up at the end of their precompile workload.

Okay, but these environment variables are just a few out of many. Why not compare the whole ENV dict etc? The only reason these specific ones are checked here is due to them being used later (AFAIU) so then they might just as well be set to the desired values?

fatteneder · 2023-10-18T21:11:04Z

This is only affected by what the package does during precompilation, right? It's not affected by anything a package does in its __init__() function?

Atm I think it does not, because the check actually happens too late, e.g. after serialization is done 😆. Need to figure out the correct place to intercept.

fatteneder · 2023-10-18T22:35:38Z

Regarding:

This is only affected by what the package does during precompilation, right? It's not affected by anything a package does in its init() function?

The docs say (https://docs.julialang.org/en/v1/manual/modules/)

For this purpose, Julia allows you to define an init() function in your module that executes any initialization steps that must occur at runtime. This function will not be called during compilation (--output-*).

So I think there is no danger in interfering with __init__ in any case.

Memo for myself:
IIUC the checks need to be inserted on the C side, probably in jl_write_compilter_output.
I see that there is jl_errorf for throwing errors, but perhaps its really better to just set the appropriate DEPOT_PATH and move on?
In any case, need to figure out how to get the DEPOT_PATH over to the C side, maybe via JLOptions.
The current implementation should already work, I guess my test before was wrong.

fatteneder · 2023-10-19T19:55:12Z

The only reason these specific ones are checked here is due to them being used later (AFAIU) so then they might just as well be set to the desired values?

An argument in favor for this strategy is that throwing an error after a potential long precompilation load would waste all that precious work.
I updated the branch to use that strategy now.

fatteneder · 2023-10-19T19:56:06Z

@nanosoldier runtests()

nanosoldier · 2023-10-21T12:46:32Z

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

DilumAluthge

Can you detect whether the package changed these values? And then print a warning if they did? (In addition to restoring the original values?)

DilumAluthge · 2023-10-28T05:09:25Z

base/loading.jl

+
+    # restore some globals in case they were altered in the precompilation workload
+    append!(empty!(Base.DEPOT_PATH), depot_path)
+    append!(empty!(Base.DL_LOAD_PATH), dl_load_path)


Can you add a comment explaining why it's necessary to restore Base.DL_LOAD_PATH?

None of these except DEPOT_PATH are necessary to aid the relocation.
I thought it would make sense to reset them too, because they are just set like this before running precompilation workload.

PkgEval showed a few hick ups, perhaps some where caused by these.

Hmmm. If the only thing that is necessary for the relocation is the DEPOT_PATH, then let's just do that, and get rid of the other stuff?

fatteneder · 2023-10-29T01:18:51Z

I studied the PkgEval, but could not find anything obvious that points at this PR.
All crashes and segfaults also happened on the daily PkgEvals (most of them due to LLVM/SmallVector.h related thingys; the QML test passes locally for me).

base/loading.jl

DilumAluthge · 2023-10-31T15:14:34Z

Let's do another PkgEval run, now that the PR has been simplified considerably?

@nanosoldier runtests()

base/loading.jl

nanosoldier · 2023-10-31T21:44:15Z

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

fatteneder · 2023-10-31T22:49:21Z

Three out of the four crashes were caused by the same LLVM assertion that was dominant in the first PkgEval (not sure if there is already an issue for that)

julia: /source/usr/include/llvm/ADT/SmallVector.h:273: T& llvm::SmallVectorTemplateCommon<T, <template-parameter-1-2> >::operator[](llvm::SmallVectorTemplateCommon<T, <template-parameter-1-2> >::size_type) [with T = int; <template-parameter-1-2> = void; llvm::SmallVectorTemplateCommon<T, <template-parameter-1-2> >::reference = int&; llvm::SmallVectorTemplateCommon<T, <template-parameter-1-2> >::size_type = long unsigned int]: Assertion `idx < size()' failed.

The error from the fourth pkg NumCME I have not seen before yet (but I think is also unrelated)

Info Packages marked with ⌅ have new versions available but compatibility constraints restrict them from upgrading.
     Testing Running tests...
Instruction does not dominate all uses!
  %667 = call token (...) @llvm.julia.gc_preserve_begin({} addrspace(10)* nonnull %658), !dbg !583
  call void @llvm.julia.gc_preserve_end(token %667), !dbg !583
Failed to verify function 'julia__addstates!_6216', dumping entire module!

...

The skipped tests seem to be problematic and seem to be all related to PyCall

ERROR: LoadError: Error building `PyCall`: 
ERROR: LoadError: ArgumentError: Failed to determine depot from srctext files in cache file /home/pkgeval/.julia/compiled/v1.11/Conda/WZE3U_1qfsV.ji.
- Make sure you have adjusted DEPOT_PATH in case you relocated depots.

fatteneder · 2023-11-01T20:32:31Z

I can't reproduce the PyCall failure locally.
Is there any way/chance to get the hands onto log or cache files that were generated during the PkgEval run?
I would be interested in these files

    Building WebIO ───────→ `~/.julia/scratchspaces/44cfe95a-1eb2-52ea-b672-e2afdf69b78f/0eef0765186f7452e52236fa42ca8c9b3c11c6e3/build.log`
    Building Conda ───────→ `~/.julia/scratchspaces/44cfe95a-1eb2-52ea-b672-e2afdf69b78f/8c86e48c0db1564a1d49548d3515ced5d604c408/build.log`
    Building PyCall ──────→ `~/.julia/scratchspaces/44cfe95a-1eb2-52ea-b672-e2afdf69b78f/43d304ac6f0354755f1d60730ece8c499980f7ba/build.log`

and

/home/pkgeval/.julia/compiled/v1.11/Conda/WZE3U_hi2aR.ji.

fatteneder · 2023-11-01T20:36:46Z

Let me try also resetting LOAD_PATH and do a PkgEval only with the skipped ones to see if it fixes it.

fatteneder · 2023-11-01T20:42:48Z

@nanosoldier runtests(["SymPy", "ScikitLearn", "Pandas", "QuantumESPRESSOParser","Queryverse","HiQGA","Simplices","IterativeLearningControl","PlotSeis","Scruff","SymFEL","SQLdf","Surprise","IceFloeTracker","ClimatePlots","Garlic","DCEMRI","SciPyDiffEq","CVXOPT","MTH229","PerceptualColourMaps","Gettext","Batsrus","OpenStreetMapXPlot","AnalyticComb","PlotPlants","Retriever","MusicVisualizations","GAlgebra","CometLogger","BundlerIO","CDGRNs","SciMLTutorials","GerryChain","ClimateMARGO","ChargeTransport"])

nanosoldier · 2023-11-01T21:54:43Z

The package evaluation job you requested has completed - no new issues were detected.
The full report is available.

fatteneder · 2023-11-01T22:04:12Z

Ok, somehow the LOAD_PATH is needed.
Also I realized that packages were also skipped on the daily PkgEval with the same build error, again because PyCall, RCall and friends all have build failures (and they all mention Conda.jl in the logs hmmm).

fatteneder · 2023-11-01T22:31:52Z

I wonder if its related to the deps/build.jl scripts, which was already mentioned back in #47943 (comment)

On my machine I have the following inside my Conda's cache file

(^@^@^@@depot/packages/Conda/2lg2O/deps/deps.jlÍ^@^@^@^@^@^@^@const ROOTENV = "/home/flo/wd/julia/usr/share/julia/conda/3/x86_64"
const MINICONDA_VERSION = "3"
const USE_MINIFORGE = true
const CONDA_EXE = "/home/flo/wd/julia/usr/share/julia/conda/3/x86_64/bin/conda"

which makes it clearly not relocatable.

But I don't understand why this shows up only on PkgEval and why resetting LOAD_PATH fixes that ...

fatteneder · 2023-11-02T23:01:10Z

I rebased to see if #51989 fixed the PkgEval skips.

@nanosoldier runtests(["SymPy", "ScikitLearn", "Pandas", "QuantumESPRESSOParser","Queryverse","HiQGA","Simplices","IterativeLearningControl","PlotSeis","Scruff","SymFEL","SQLdf","Surprise","IceFloeTracker","ClimatePlots","Garlic","DCEMRI","SciPyDiffEq","CVXOPT","MTH229","PerceptualColourMaps","Gettext","Batsrus","OpenStreetMapXPlot","AnalyticComb","PlotPlants","Retriever","MusicVisualizations","GAlgebra","CometLogger","BundlerIO","CDGRNs","SciMLTutorials","GerryChain","ClimateMARGO","ChargeTransport"])

fatteneder · 2023-11-03T07:57:21Z

I forgot the backticks.

@nanosoldier runtests(["SymPy", "ScikitLearn", "Pandas", "QuantumESPRESSOParser","Queryverse","HiQGA","Simplices","IterativeLearningControl","PlotSeis","Scruff","SymFEL","SQLdf","Surprise","IceFloeTracker","ClimatePlots","Garlic","DCEMRI","SciPyDiffEq","CVXOPT","MTH229","PerceptualColourMaps","Gettext","Batsrus","OpenStreetMapXPlot","AnalyticComb","PlotPlants","Retriever","MusicVisualizations","GAlgebra","CometLogger","BundlerIO","CDGRNs","SciMLTutorials","GerryChain","ClimateMARGO","ChargeTransport"])

nanosoldier · 2023-11-03T09:13:36Z

The package evaluation job you requested has completed - no new issues were detected.
The full report is available.

DilumAluthge self-requested a review October 17, 2023 20:57

fatteneder force-pushed the fa/precomp-check-depots-restored branch from 69cc535 to b8929a0 Compare October 17, 2023 21:15

DilumAluthge marked this pull request as draft October 17, 2023 22:29

DilumAluthge added the needs pkgeval Tests for all registered packages should be run with this change label Oct 17, 2023

DilumAluthge changed the title ~~Draft: Check if precompilation workload restores initial state of DEPOT_PATH etc~~ Check if precompilation workload restores initial state of DEPOT_PATH etc Oct 17, 2023

DilumAluthge mentioned this pull request Oct 17, 2023

🤖 [master] Bump the Pkg stdlib from ffb6edf03 to af5392db5 #51749

Merged

DilumAluthge reviewed Oct 18, 2023

View reviewed changes

fatteneder force-pushed the fa/precomp-check-depots-restored branch 2 times, most recently from e223b8a to 70d049f Compare October 19, 2023 19:53

fatteneder marked this pull request as ready for review October 19, 2023 19:55

DilumAluthge self-requested a review October 22, 2023 05:25

DilumAluthge added the compiler:precompilation Precompilation of modules label Oct 22, 2023

fatteneder marked this pull request as draft October 24, 2023 21:17

DilumAluthge reviewed Oct 28, 2023

View reviewed changes

fatteneder changed the title ~~Check if precompilation workload restores initial state of DEPOT_PATH etc~~ Restore initial state of DEPOT_PATH after precompilation workload Oct 28, 2023

fatteneder force-pushed the fa/precomp-check-depots-restored branch from 7f0105d to 914479b Compare October 29, 2023 00:25

fatteneder marked this pull request as ready for review October 29, 2023 01:19

DilumAluthge self-requested a review October 29, 2023 01:19

fatteneder commented Oct 29, 2023

View reviewed changes

base/loading.jl Outdated Show resolved Hide resolved

fatteneder closed this Oct 29, 2023

fatteneder reopened this Oct 29, 2023

fatteneder force-pushed the fa/precomp-check-depots-restored branch from 46831fe to d3f7672 Compare October 29, 2023 13:06

DilumAluthge approved these changes Oct 31, 2023

View reviewed changes

base/loading.jl Show resolved Hide resolved

DilumAluthge requested a review from vchuravy October 31, 2023 15:16

DilumAluthge added packages Package management and loading modules labels Oct 31, 2023

fatteneder mentioned this pull request Nov 2, 2023

Remove ArgumentError() in parse_cache_header() when @depot cannot be resolved #51989

Merged

fatteneder added 2 commits November 2, 2023 23:58

restore DEPOT_PATH etc after precompilation workload ran

20d177d

only reset DEPOT_PATH after precompilation workload ran

e5a05ed

fatteneder force-pushed the fa/precomp-check-depots-restored branch from 0dce5a3 to e5a05ed Compare November 2, 2023 23:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore initial state of `DEPOT_PATH` after precompilation workload #51746

Restore initial state of `DEPOT_PATH` after precompilation workload #51746

fatteneder commented Oct 17, 2023 •

edited by DilumAluthge

Loading

DilumAluthge Oct 18, 2023

DilumAluthge commented Oct 18, 2023

KristofferC commented Oct 18, 2023

DilumAluthge commented Oct 18, 2023

KristofferC commented Oct 18, 2023

fatteneder commented Oct 18, 2023

fatteneder commented Oct 18, 2023 •

edited

Loading

fatteneder commented Oct 19, 2023

fatteneder commented Oct 19, 2023

nanosoldier commented Oct 21, 2023

DilumAluthge left a comment

DilumAluthge Oct 28, 2023

fatteneder Oct 28, 2023

DilumAluthge Oct 29, 2023

fatteneder commented Oct 29, 2023

DilumAluthge commented Oct 31, 2023

nanosoldier commented Oct 31, 2023

fatteneder commented Oct 31, 2023

fatteneder commented Nov 1, 2023

fatteneder commented Nov 1, 2023

fatteneder commented Nov 1, 2023

nanosoldier commented Nov 1, 2023

fatteneder commented Nov 1, 2023

fatteneder commented Nov 1, 2023

fatteneder commented Nov 2, 2023

fatteneder commented Nov 3, 2023

nanosoldier commented Nov 3, 2023

Restore initial state of DEPOT_PATH after precompilation workload #51746

Are you sure you want to change the base?

Restore initial state of DEPOT_PATH after precompilation workload #51746

Conversation

fatteneder commented Oct 17, 2023 • edited by DilumAluthge Loading

DilumAluthge Oct 18, 2023

Choose a reason for hiding this comment

DilumAluthge commented Oct 18, 2023

KristofferC commented Oct 18, 2023

DilumAluthge commented Oct 18, 2023

KristofferC commented Oct 18, 2023

fatteneder commented Oct 18, 2023

fatteneder commented Oct 18, 2023 • edited Loading

fatteneder commented Oct 19, 2023

fatteneder commented Oct 19, 2023

nanosoldier commented Oct 21, 2023

DilumAluthge left a comment

Choose a reason for hiding this comment

DilumAluthge Oct 28, 2023

Choose a reason for hiding this comment

fatteneder Oct 28, 2023

Choose a reason for hiding this comment

DilumAluthge Oct 29, 2023

Choose a reason for hiding this comment

fatteneder commented Oct 29, 2023

DilumAluthge commented Oct 31, 2023

nanosoldier commented Oct 31, 2023

fatteneder commented Oct 31, 2023

fatteneder commented Nov 1, 2023

fatteneder commented Nov 1, 2023

fatteneder commented Nov 1, 2023

nanosoldier commented Nov 1, 2023

fatteneder commented Nov 1, 2023

fatteneder commented Nov 1, 2023

fatteneder commented Nov 2, 2023

fatteneder commented Nov 3, 2023

nanosoldier commented Nov 3, 2023

Restore initial state of `DEPOT_PATH` after precompilation workload #51746

Restore initial state of `DEPOT_PATH` after precompilation workload #51746

fatteneder commented Oct 17, 2023 •

edited by DilumAluthge

Loading

fatteneder commented Oct 18, 2023 •

edited

Loading