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

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

Merged
merged 5 commits into from
Jan 29, 2024

Conversation

KristofferC
Copy link
Member

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 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 KristofferC added the backport 1.10 Change should be backported to the 1.10 release label Jan 10, 2024
@KristofferC
Copy link
Member Author

@nanosoldier runtests()

@nanosoldier
Copy link
Collaborator

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

@KristofferC
Copy link
Member Author

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
Copy link
Member

vtjnash commented Jan 12, 2024

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
Copy link
Member Author

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

@KristofferC KristofferC force-pushed the kc/pkg_callbacks_on_require branch from a7c0985 to a297cfc Compare January 12, 2024 11:30
@KristofferC KristofferC force-pushed the kc/pkg_callbacks_on_require branch from a297cfc to 6be9189 Compare January 12, 2024 11:32
@KristofferC
Copy link
Member Author

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

@KristofferC
Copy link
Member Author

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
Copy link
Member

vtjnash commented Jan 12, 2024

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
Copy link
Member Author

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

@vtjnash
Copy link
Member

vtjnash commented Jan 12, 2024

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

@nanosoldier
Copy link
Collaborator

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

@maleadt
Copy link
Member

maleadt commented Jan 13, 2024

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
Copy link
Member Author

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

@nanosoldier
Copy link
Collaborator

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

@KristofferC
Copy link
Member Author

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

@nanosoldier
Copy link
Collaborator

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

@KristofferC
Copy link
Member Author

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.

DelveCI pushed a commit to RelationalAI/julia that referenced this pull request Mar 13, 2024
…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)
Drvi pushed a commit to RelationalAI/julia that referenced this pull request Apr 3, 2024
…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)
KristofferC added a commit that referenced this pull request Apr 9, 2024
… extension in precompile code, this was fixed in #52841
d-netto pushed a commit to RelationalAI/julia that referenced this pull request Apr 16, 2024
…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)
DelveCI pushed a commit to RelationalAI/julia that referenced this pull request Apr 23, 2024
…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)
DelveCI pushed a commit to RelationalAI/julia that referenced this pull request Apr 24, 2024
…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)
DelveCI pushed a commit to RelationalAI/julia that referenced this pull request Apr 30, 2024
…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)
DelveCI pushed a commit to RelationalAI/julia that referenced this pull request Apr 30, 2024
…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)
DelveCI pushed a commit to RelationalAI/julia that referenced this pull request May 2, 2024
…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)
KristofferC added a commit that referenced this pull request May 6, 2024
DelveCI pushed a commit to RelationalAI/julia that referenced this pull request May 9, 2024
…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)
DelveCI pushed a commit to RelationalAI/julia that referenced this pull request May 19, 2024
…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)
DelveCI pushed a commit to RelationalAI/julia that referenced this pull request May 26, 2024
…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)
DelveCI pushed a commit to RelationalAI/julia that referenced this pull request May 28, 2024
…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)
DelveCI pushed a commit to RelationalAI/julia that referenced this pull request May 29, 2024
…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)
Drvi pushed a commit to RelationalAI/julia that referenced this pull request Jun 7, 2024
…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)
lazarusA pushed a commit to lazarusA/julia that referenced this pull request Jul 12, 2024
KristofferC added a commit that referenced this pull request Oct 18, 2024
…ckages are in the sysimage (#52841)"

This reverts commit 08d229f.
KristofferC added a commit that referenced this pull request Oct 21, 2024
…ckages are in the sysimage (#52841)"

This reverts commit 08d229f.
KristofferC added a commit that referenced this pull request Oct 22, 2024
…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.
vtjnash pushed a commit to IanButterworth/julia that referenced this pull request Nov 6, 2024
…ckages are in the sysimage (JuliaLang#52841)"

This reverts commit 08d229f.

(cherry picked from commit e3f2f6b)
topolarity pushed a commit to topolarity/julia that referenced this pull request Nov 22, 2024
topolarity added a commit that referenced this pull request Nov 22, 2024
…ns independent of what packages are in the sysimage (#52841)" (#56658)

This is a backport of #56234

It reverts commit 08d229f (and
backports ad1dc39, which was included
in the revert PR).
KristofferC added a commit that referenced this pull request Nov 24, 2024
Backported PRs:
- [x] #56595 <!-- fix precompile(::MethodInstance) ccall signature -->
- [x] #56234 <!-- Revert "Extensions: make loading of extensions
independent of what packages are in the sysimage (#52841) -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Strange "missing from cache"/"does not support precompilation"/... errors
4 participants