create_app: load "deps" packages besides just the app package #463
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…o helper process.
This allows users to set the debug log level in the process, not just
via the `JULIA_DEBUG=all` environment variable (we _thought_ we had
enabled the debug messages, but we had not! 😬 ).
Also, adds a log statement for precompile statements that _fail_, not
just those that error. For example, this surfaces this failure:
```julia
┌ Debug: Precompilation failed: precompile(Tuple{typeof(Base.deepcopy_internal), Any, Base.IdDict{Any, Any}})
└ @ Main.anonymous none:31
```
Which is a pattern I've noticed in our own precompilation failures,
where Julia seems to be emitting a precompile statement with an abstract
argument (`Any` in this case, but i've seen others) for a method that is
_not_ marked `@nospecialize`, which causes `precompile()` to fail, since
you cannot normally instantiate a method for abstract types.
So that's a separate issue that we need to dig into, but this PR allows
us to identify issues like these. :)
Before this commit, `create_app()` would only load the package for the
App being compiled before executing all precompile statements. This
means that if your precompile script was loading any "extra" packages,
that aren't used directly by the app's module, that they would silently
fail to statically compile.
We do this to also precompile some "helper" packages that we ship alongside
our application code, such as `Plots`.
```julia
julia> using PackageCompiler
[ Info: Precompiling PackageCompiler [9b87118b-4619-50d2-8e1e-99f35a4d4d9d]
julia> mktemp() do f, io
d = mktempdir()
mkpath("$d/Foo/src")
write("$d/Foo/src/Foo.jl", "module Foo ; julia_main()::Cint = 0 ; end")
write("$d/Foo/Project.toml", """name = "Foo"\nuuid="aa65fe97-06da-5843-b5b1-d5d13cad87d3"\n[deps]\nPProf = "e4faabce-9ead-11e9-39d9-4379958e3056"\n""") write(io, "using PProf")
close(io)
PackageCompiler.create_app("$d/Foo", "PkgsApp"; precompile_statements_file=f, cpu_target=PackageCompiler.default_app_cpu_target(), force=true)
end
```
Even in the *base system image* this seems to have fixed some errors:
```julia
[ Info: PackageCompiler: completed 253 precompile statements with 5 failures, 7 errors.
```
```julia
[ Info: PackageCompiler: completed 253 precompile statements with 5 failures, 0 errors.
```
And then, for the actual system image, it fixes the one error:
```julia
[ Info: PackageCompiler: completed 1 precompile statements with 0 failures, 1 errors.
```
```julia
[ Info: PackageCompiler: completed 1 precompile statements with 0 failures, 0 errors.
```
|
I see, yes, deterministically loading all deps makes sense to me for |
KristofferC
reviewed
Oct 23, 2020
src/PackageCompiler.jl
Outdated
| # we can load _all_ of the packages (and their recursive packages) from the project. | ||
| # This may be more than the user will actually need, but that is up to users to prune | ||
| # their Project files to only those `deps` actually in use. | ||
| packages = Symbol.(keys(Pkg.TOML.parsefile(ctx.env.project_file)["deps"])) |
There was a problem hiding this comment.
julia> ctx.env.project.deps
Dict{String,Base.UUID} with 37 entries:
"BinaryBuilder" => UUID("12aac903-9f7c-5d81-afc2-d9565ea332ae")
"Documenter" => UUID("e30172f5-a6a5-5a46-863b-614d45cd2de4")
"ForwardDiff" => UUID("f6369f11-7733-5829-9624-2563aa707210")
...
is probably better
There was a problem hiding this comment.
Suggested change
| packages = Symbol.(keys(Pkg.TOML.parsefile(ctx.env.project_file)["deps"])) | |
| packages = keys(ctx.env.project.deps) |
Awesome thanks. Does this look good? ⬆️
There was a problem hiding this comment.
Oops, I guess that should be an array of symbols:
packages = Symbol.(keys(ctx.env.project.deps))Fixed in af09bd8
KristofferC
reviewed
Oct 23, 2020
KristofferC
changed the title
Codecov Report
@@ Coverage Diff @@
## master #463 +/- ##
==========================================
- Coverage 84.31% 76.78% -7.54%
==========================================
Files 2 2
Lines 338 336 -2
==========================================
- Hits 285 258 -27
- Misses 53 78 +25
Continue to review full report at Codecov.
|
NHDaly
commented
Oct 23, 2020
src/PackageCompiler.jl
Outdated
| # we can load _all_ of the packages (and their recursive packages) from the project. | ||
| # This may be more than the user will actually need, but that is up to users to prune | ||
| # their Project files to only those `deps` actually in use. | ||
| packages = Symbol.(keys(Pkg.TOML.parsefile(ctx.env.project_file)["deps"])) |
There was a problem hiding this comment.
Suggested change
| packages = Symbol.(keys(Pkg.TOML.parsefile(ctx.env.project_file)["deps"])) | |
| packages = keys(ctx.env.project.deps) |
Awesome thanks. Does this look good? ⬆️
|
Also, this PR is currently based on #457. I guess we should review that first before merging this one. :) Thanks for your help, @KristofferC! 😁 |
|
I've also noticed this problem when creating a sysimage with Plots. If GR is set to use GR_jll it only loads GR_jll when actually executing the first plot. That means that GR_jll & dependencies will not be included in the sysimage since the sysimage only does a |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I now understand the cause of
Case 5in JuliaLang/julia#28808 (comment), and this PR is a first attempt at addressing it.Before this commit,
create_app()would only load the package for theApp being compiled before executing all precompile statements. This
means that if your precompile script was importing any "extra" packages,
that aren't used directly by the app's module, that they would silently
fail to statically compile.
We are doing this in our static compilation job to also precompile some "helper" packages that we ship alongside our application code, such as
Plots.I'm not sure what the best way to approach this fix is. Some options I see:
create_appto allow optionally specifying extra packages to include in the static compilationBase.loaded_modulesat the end of the execution, and add those to the packages.I would love your feedback! Thanks @KristofferC!