Extensions: make loading of extensions independent of what packages are in the sysimage #52841

KristofferC · 2024-01-10T12:10:43Z

When triggers of extension are in the sysimage it is easy to end up with cycles in package loading. Say we have a package A with exts BExt and CExt and say that B and C is in the sysimage.

Upon loading A, we will immidiately start to precompile BExt (because the trigger B is "loaded" by virtue of being in the sysimage).
BExt will load A which will cause CExt to start precompiling (again because C is in the sysimage).
CExt will load A which will now cause BExt to start loading and we get a cycle.

This is fixed in this PR by instead of looking at what modules are loaded, we look at what modules are actually required and only use that to drive the loading of extensions.

Fixes #52132.

…re in the sysimage When triggers of extension are in the sysimage it is easy to end up with cycles in package loading. Say we have a package A with exts BExt and CExt and say that B and C is in the sysimage. - Upon loading A, we will immidiately start to precompile BExt (because the trigger B is "loaded" by virtue of being in the sysimage). - BExt will load A which will cause CExt to start precompiling (again because C is in the sysimage). - CExt will load A which will now cause BExt to start loading and we get a cycle. This is fixed in this PR by instead of looking at what modules are loaded, we look at what modules are actually `require`d and only use that to drive the loading of extensions.

KristofferC · 2024-01-11T09:21:09Z

@nanosoldier runtests()

nanosoldier · 2024-01-11T17:53:33Z

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

KristofferC · 2024-01-11T21:18:17Z

Okay, this PR, as is, is incomplete because it also has to count the dependencies that are loaded. Right now it only considers the package that is explicitly loaded by the user.

vtjnash · 2024-01-12T00:13:10Z

Oh yeah, this is a very good observation. I think a better solution might be a bit different though--we should make sure instead that all of these extensions are included in the original precompile of the package that resolved them all (rather than trying to split them into separate files). The precompile code should already be perfectly happy for them to live inside the .ji file of the parent, we just need to teach loading.jl to put them there.

KristofferC · 2024-01-12T11:21:02Z

@nanosoldier runtests(["TranscodingStreams", "EulerAngles", "CodecBase", "GeneralizedSylvesterSolver", "BufferedStreams", "CodecBzip2", "CodecLz4", "CodecZlib", "CodecXz", "CodecZstd", "Sinograms", "Pseudospectra", "ImplicitDifferentiation", "StatsDiscretizations", "StipplePlotly", "MRIOperators", "CategoryData", "GR", "MRISimulation"])

KristofferC · 2024-01-12T11:32:23Z

@nanosoldier runtests(["TranscodingStreams", "EulerAngles", "CodecBase", "GeneralizedSylvesterSolver", "BufferedStreams", "CodecBzip2", "CodecLz4", "CodecZlib", "CodecXz", "CodecZstd", "Sinograms", "Pseudospectra", "ImplicitDifferentiation", "StatsDiscretizations", "StipplePlotly", "MRIOperators", "CategoryData", "GR", "MRISimulation"])

KristofferC · 2024-01-12T13:46:47Z

we should make sure instead that all of these extensions are included in the original precompile of the package that resolved them all (rather than trying to split them into separate files). The precompile code should already be perfectly happy for them to live inside the .ji file of the parent, we just need to teach loading.jl to put them there.

I don't understand this really. Let's say you have the extension BCExt for A that requires B and C. You load A and then B and then C. Now BCExt will load. Where do you want to put that cache file? In C? What happens next time when you load things in the order C, A, B? Do you reopen C to look for BCExt? What's the advantage of this?

vtjnash · 2024-01-12T15:16:02Z

it belongs in A, since B and C are already loaded (in the case where B and C are already loaded when A finishes precompiling)

KristofferC · 2024-01-12T17:10:21Z

And if they aren't and you just load A by itself (and then B and C)?

vtjnash · 2024-01-12T17:30:25Z

Then no change is needed, since there is no circularity problem?

nanosoldier · 2024-01-13T13:10:37Z

Your job failed. Consult the server logs for more details (cc @maleadt).

maleadt · 2024-01-13T13:12:54Z

This broke Nanosoldier: It hung during build (PkgEval should use a timeout) at the generate_precompile.jl stage:

Collecting and executing precompile statements
...
InitError(mod=:Base, error=UndefVarError(var=:required_modules, scope=Base))
ijl_undefined_var_error at /source/src/rtutils.c:152
ijl_get_binding_or_error at /source/src/module.c:464
__init__ at ./Base.jl:630
unknown function (ip: 0x7f6323b56cbf)
_jl_invoke at /source/src/gf.c:2921 [inlined]
ijl_apply_generic at /source/src/gf.c:3098
jl_apply at /source/src/julia.h:2153 [inlined]
jl_module_run_initializer at /source/src/toplevel.c:76
_finish_julia_init at /source/src/init.c:902
julia_init at /source/src/init.c:843
jl_repl_entrypoint at /source/src/jlapi.c:1087
main at /source/cli/loader_exe.c:58
__libc_start_main at /lib/x86_64-linux-gnu/libc.so.6 (unknown line)
unknown function (ip: 0x401098)

KristofferC · 2024-01-13T13:34:35Z

Yeah, I was a bit eager to submit that job...

nanosoldier · 2024-01-13T14:08:27Z

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

…n the sysimage

KristofferC · 2024-01-15T08:53:24Z

@nanosoldier runtests(["CodecZstd", "TranscodingStreams", "CodecBase", "CodecLz4", "CodecBzip2", "CodecZlib", "CodecXz"])

nanosoldier · 2024-01-15T22:31:00Z

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

KristofferC · 2024-01-16T12:48:27Z

I realize now that the failing packages are actually failing due to the bugfix here. They never explicitly load Random in their tests but they have an extension that requires both Random and Test to trigger. Their extension used to load due to Random being in the sysimage which isn't enough anymore.

…re in the sysimage (JuliaLang#52841) When triggers of extension are in the sysimage it is easy to end up with cycles in package loading. Say we have a package A with exts BExt and CExt and say that B and C is in the sysimage. - Upon loading A, we will immidiately start to precompile BExt (because the trigger B is "loaded" by virtue of being in the sysimage). - BExt will load A which will cause CExt to start precompiling (again because C is in the sysimage). - CExt will load A which will now cause BExt to start loading and we get a cycle. This is fixed in this PR by instead of looking at what modules are loaded, we look at what modules are actually `require`d and only use that to drive the loading of extensions. Fixes JuliaLang#52132. (cherry picked from commit 08d229f)

… extension in precompile code, this was fixed in #52841

…re in the sysimage (JuliaLang#52841) When triggers of extension are in the sysimage it is easy to end up with cycles in package loading. Say we have a package A with exts BExt and CExt and say that B and C is in the sysimage. - Upon loading A, we will immidiately start to precompile BExt (because the trigger B is "loaded" by virtue of being in the sysimage). - BExt will load A which will cause CExt to start precompiling (again because C is in the sysimage). - CExt will load A which will now cause BExt to start loading and we get a cycle. This is fixed in this PR by instead of looking at what modules are loaded, we look at what modules are actually `require`d and only use that to drive the loading of extensions. Fixes JuliaLang#52132. (cherry picked from commit 08d229f)

…#54243) Introduced in #52841.

…#54243) Introduced in #52841. (cherry picked from commit b385b4f)

…re in the sysimage (JuliaLang#52841) When triggers of extension are in the sysimage it is easy to end up with cycles in package loading. Say we have a package A with exts BExt and CExt and say that B and C is in the sysimage. - Upon loading A, we will immidiately start to precompile BExt (because the trigger B is "loaded" by virtue of being in the sysimage). - BExt will load A which will cause CExt to start precompiling (again because C is in the sysimage). - CExt will load A which will now cause BExt to start loading and we get a cycle. This is fixed in this PR by instead of looking at what modules are loaded, we look at what modules are actually `require`d and only use that to drive the loading of extensions. Fixes JuliaLang#52132. (cherry picked from commit 08d229f)

…JuliaLang#54243) Introduced in JuliaLang#52841.

…ckages are in the sysimage (#52841)" This reverts commit 08d229f.

…ckages are in the sysimage (#52841) (#56234) This reverts commit 08d229f. There are some bugs now where extensions do not load when their package has been put into the sysimage. #52841 was made because it was common to get cycles otherwise but with #55589 that should be much less of a problem. Subsumes #54750.

KristofferC added needs tests Unit tests are required for this change needs pkgeval Tests for all registered packages should be run with this change labels Jan 10, 2024

KristofferC requested review from vtjnash and IanButterworth January 10, 2024 12:10

KristofferC added the package extensions label Jan 10, 2024

jishnub mentioned this pull request Jan 10, 2024

Extension module is missing from the cache JuliaLang/Pkg.jl#3557

Open

KristofferC added the backport 1.10 Change should be backported to the 1.10 release label Jan 10, 2024

woops

be8fe90

KristofferC force-pushed the kc/pkg_callbacks_on_require branch from a7c0985 to a297cfc Compare January 12, 2024 11:30

alt implementation

6be9189

KristofferC force-pushed the kc/pkg_callbacks_on_require branch from a297cfc to 6be9189 Compare January 12, 2024 11:32

also insert into explicit_loaded_modules when require a package i…

36213eb

…n the sysimage

KristofferC added a commit that referenced this pull request Apr 9, 2024

avoid packages in sysimages from automatically count as triggering an…

57589bd

… extension in precompile code, this was fixed in #52841

emstoudenmire mentioned this pull request Apr 12, 2024

[ITensors] HDF5.jl package extension ITensor/ITensors.jl#1382

Merged

KristofferC mentioned this pull request Apr 25, 2024

prevent package_callbacks to run multiple time for a single package #54243

Merged

KristofferC added a commit that referenced this pull request May 2, 2024

prevent package_callbacks to run multiple time for a single package (…

b385b4f

…#54243) Introduced in #52841.

KristofferC added a commit that referenced this pull request May 6, 2024

prevent package_callbacks to run multiple time for a single package (…

6d84a37

…#54243) Introduced in #52841. (cherry picked from commit b385b4f)

KristofferC mentioned this pull request Jun 10, 2024

WIP: improve some cases of loading extensions in the presence of packages in the sysimage. #54750

Closed

lazarusA pushed a commit to lazarusA/julia that referenced this pull request Jul 12, 2024

prevent package_callbacks to run multiple time for a single package (…

891dc8d

…JuliaLang#54243) Introduced in JuliaLang#52841.

devmotion mentioned this pull request Aug 10, 2024

Make Test a weak dependency JuliaMath/InverseFunctions.jl#52

Merged

KristofferC added a commit that referenced this pull request Oct 18, 2024

Revert "Extensions: make loading of extensions independent of what pa…

cef7241

…ckages are in the sysimage (#52841)" This reverts commit 08d229f.

KristofferC mentioned this pull request Oct 18, 2024

Revert "Extensions: make loading of extensions independent of what packages are in the sysimage (#52841) #56234

Merged

KristofferC added a commit that referenced this pull request Oct 21, 2024

Revert "Extensions: make loading of extensions independent of what pa…

e3f2f6b

…ckages are in the sysimage (#52841)" This reverts commit 08d229f.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extensions: make loading of extensions independent of what packages are in the sysimage #52841

Extensions: make loading of extensions independent of what packages are in the sysimage #52841

KristofferC commented Jan 10, 2024

KristofferC commented Jan 11, 2024

nanosoldier commented Jan 11, 2024

KristofferC commented Jan 11, 2024

vtjnash commented Jan 12, 2024

KristofferC commented Jan 12, 2024

KristofferC commented Jan 12, 2024

KristofferC commented Jan 12, 2024

vtjnash commented Jan 12, 2024

KristofferC commented Jan 12, 2024

vtjnash commented Jan 12, 2024

nanosoldier commented Jan 13, 2024

maleadt commented Jan 13, 2024

KristofferC commented Jan 13, 2024

nanosoldier commented Jan 13, 2024

KristofferC commented Jan 15, 2024

nanosoldier commented Jan 15, 2024

KristofferC commented Jan 16, 2024

Extensions: make loading of extensions independent of what packages are in the sysimage #52841

Extensions: make loading of extensions independent of what packages are in the sysimage #52841

Conversation

KristofferC commented Jan 10, 2024

KristofferC commented Jan 11, 2024

nanosoldier commented Jan 11, 2024

KristofferC commented Jan 11, 2024

vtjnash commented Jan 12, 2024

KristofferC commented Jan 12, 2024

KristofferC commented Jan 12, 2024

KristofferC commented Jan 12, 2024

vtjnash commented Jan 12, 2024

KristofferC commented Jan 12, 2024

vtjnash commented Jan 12, 2024

nanosoldier commented Jan 13, 2024

maleadt commented Jan 13, 2024

KristofferC commented Jan 13, 2024

nanosoldier commented Jan 13, 2024

KristofferC commented Jan 15, 2024

nanosoldier commented Jan 15, 2024

KristofferC commented Jan 16, 2024