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

Turn Method Overwritten Error into a PrecompileError -- turning off caching #52214

Merged
merged 5 commits into from
Nov 24, 2023

Conversation

vchuravy
Copy link
Member

Fixes #52213

Overwritting methods during cache creation is currently not something that the system
can support and can lead to surprising, counter-intuitive and fatal errors.

In 1.10 we turned it from a warning to a strong error, with this PR it remains
a strong error, but the precompilation system recognizes it and essentially sets __precompile__(false)
for this module and all modules that depend on it.

Before:

julia> using OverwriteMethodError
[ Info: Precompiling OverwriteMethodError [top-level]
WARNING: Method definition +(Bool, Bool) in module Base at bool.jl:166 overwritten in module OverwriteMethodError at /home/vchuravy/src/julia2/OverwriteMethodError.jl:2.
ERROR: LoadError: Method overwriting is not permitted during Module precompile.
Stacktrace:
 [1] top-level scope
   @ ~/src/julia2/OverwriteMethodError.jl:2
 [2] include
   @ Base ./Base.jl:489 [inlined]
 [3] include_package_for_output(pkg::Base.PkgId, input::String, depot_path::Vector{String}, dl_load_path::Vector{String}, load_path::Vector{String}, concrete_deps::Vector{Pair{Base.PkgId, UInt128}}, source::Nothing)
   @ Base ./loading.jl:2216
 [4] top-level scope
   @ stdin:3
in expression starting at /home/vchuravy/src/julia2/OverwriteMethodError.jl:1
in expression starting at stdin:3
ERROR: Failed to precompile OverwriteMethodError [top-level] to "/home/vchuravy/.julia/compiled/v1.10/jl_guiuCQ".
Stacktrace:
  [1] error(s::String)
    @ Base ./error.jl:35
  [2] compilecache(pkg::Base.PkgId, path::String, internal_stderr::IO, internal_stdout::IO, keep_loaded_modules::Bool)
    @ Base ./loading.jl:2462
  [3] compilecache
    @ Base ./loading.jl:2334 [inlined]
  [4] (::Base.var"#968#969"{Base.PkgId})()
    @ Base ./loading.jl:1968
  [5] mkpidlock(f::Base.var"#968#969"{Base.PkgId}, at::String, pid::Int32; kwopts::@Kwargs{stale_age::Int64, wait::Bool})
    @ FileWatching.Pidfile ~/.julia/juliaup/julia-1.10.0-rc1+0.x64.linux.gnu/share/julia/stdlib/v1.10/FileWatching/src/pidfile.jl:93
  [6] #mkpidlock#6
    @ FileWatching.Pidfile ~/.julia/juliaup/julia-1.10.0-rc1+0.x64.linux.gnu/share/julia/stdlib/v1.10/FileWatching/src/pidfile.jl:88 [inlined]
  [7] trymkpidlock(::Function, ::Vararg{Any}; kwargs::@Kwargs{stale_age::Int64})
    @ FileWatching.Pidfile ~/.julia/juliaup/julia-1.10.0-rc1+0.x64.linux.gnu/share/julia/stdlib/v1.10/FileWatching/src/pidfile.jl:111
  [8] #invokelatest#2
    @ Base ./essentials.jl:889 [inlined]
  [9] invokelatest
    @ Base ./essentials.jl:884 [inlined]
 [10] maybe_cachefile_lock(f::Base.var"#968#969"{Base.PkgId}, pkg::Base.PkgId, srcpath::String; stale_age::Int64)
    @ Base ./loading.jl:2977
 [11] maybe_cachefile_lock
    @ Base ./loading.jl:2974 [inlined]
 [12] _require(pkg::Base.PkgId, env::String)
    @ Base ./loading.jl:1964
 [13] __require_prelocked(uuidkey::Base.PkgId, env::String)
    @ Base ./loading.jl:1806
 [14] #invoke_in_world#3
    @ Base ./essentials.jl:921 [inlined]
 [15] invoke_in_world
    @ Base ./essentials.jl:918 [inlined]
 [16] _require_prelocked(uuidkey::Base.PkgId, env::String)
    @ Base ./loading.jl:1797
 [17] macro expansion
    @ Base ./loading.jl:1784 [inlined]
 [18] macro expansion
    @ Base ./lock.jl:267 [inlined]
 [19] __require(into::Module, mod::Symbol)
    @ Base ./loading.jl:1747
 [20] #invoke_in_world#3
    @ Base ./essentials.jl:921 [inlined]
 [21] invoke_in_world
    @ Base ./essentials.jl:918 [inlined]
 [22] require(into::Module, mod::Symbol)
    @ Base ./loading.jl:1740

After:

julia> using OverwriteMethodError
┌ Info: Precompiling OverwriteMethodError [top-level]
└ @ Base loading.jl:2486
WARNING: Method definition +(Bool, Bool) in module Base at bool.jl:166 overwritten in module OverwriteMethodError at /home/vchuravy/src/julia2/OverwriteMethodError.jl:2.
ERROR: Method overwriting is not permitted during Module precompile.
┌ Info: Skipping precompilation since __precompile__(false). Importing OverwriteMethodError [top-level].
└ @ Base loading.jl:2084

@vchuravy vchuravy added this to the 1.10 milestone Nov 17, 2023
@vchuravy vchuravy added the backport 1.10 Change should be backported to the 1.10 release label Nov 17, 2023
@vchuravy vchuravy requested a review from vtjnash November 17, 2023 18:32
@aplavin
Copy link
Contributor

aplavin commented Nov 17, 2023

May I suggest adding something like

state of your packages may depend on their loading order

to the warning to make it easier to understand?

@PallHaraldsson
Copy link
Contributor

PallHaraldsson commented Nov 17, 2023

That was quick and it's certainly better to have something shorter to be readable. But I see "Info", "WARNING" and "ERROR" for the same cause...

I'm not even sure yet if this should only be a WARNING, or an ERROR actually... If I trust you then it needs not error, by turning precompilation off, but I still think the root cause is the redefinition. If I'm wrong, can it be changed to just a WARNING?

I'm sympathetic to people wanting stuff "working" that did seemed to work before. Should this say something like:

WARNING, precompilation is turned off for module X, and this is a potential order-defendant ERROR, because of type-piracy, and in future versions of Julia this will be an ERROR.

I see Kristoffer gave thumbs down, so maybe the change you made or anything (I suggested) isn't in the cards. Could it ERROR by default, the status quo for the latest Julia, but be overridable somehow, maybe just with an ENV var to make it inconvenient... and not promise a CLI option for the future. Would that make @aplavin and @KristofferC happy?

@aplavin
Copy link
Contributor

aplavin commented Nov 17, 2023

I'd be glad with any user-based workaround/flag that doesn't require package code changes. This alone would help dealing with the breakage in 1.10.

@maleadt
Copy link
Member

maleadt commented Nov 17, 2023

FWIW, there's currently 214 packages (6% of all current failures) that fail daily PkgEval because of this error, so it would help alleviate some of the 1.11-related breakage.

@BioTurboNick
Copy link
Contributor

Automatically doing the thing that would be required by a package author to make it work is wonderful. I would much prefer it to never error in 1.x. A warning that clearly spells out the implications would be a large improvement, and might well incentivize more package authors to find alternatives. Especially if users also understand the problem, and log issues to get the warning to go away. But in the meantime, everything still works.

I would agree with PallHaraldsson that avoiding three log messages for the same event would be better.

@IanButterworth
Copy link
Member

What about a flag/env var to turn it back into a warning, which is mentioned in the error and is enabled in PkgEval

@IanButterworth
Copy link
Member

For this to not be highly confusing I think the message for PrecompilableError needs tweaking.

Skipping precompilation since __precompile__(false)

because there won't be any __precompile__(false) instances.

@IanButterworth
Copy link
Member

That type of confusion does already exist, but I think it would be unwise to add more places that throw PrecompilableError without fixing the message.

@KristofferC
Copy link
Member

#51367

@vchuravy vchuravy merged commit 9e8fe68 into master Nov 24, 2023
7 checks passed
@vchuravy vchuravy deleted the vc/methodoverwrite_into_precompile_error branch November 24, 2023 22:35
end
""")

@test Base.compilecache(Base.PkgId("OverwriteMethodError")) == Base.PrecompilableError() # due to piracy
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is noisy during CI:

precompile                                       (1) |        started at 2023-11-25T02:42:26.767
WARNING: Method definition +(Bool, Bool) in module Base at bool.jl:166 overwritten in module OverwriteMethodError at C:\Users\julia\AppData\Local\Temp\jl_shtGYs\OverwriteMethodError.jl:2.
ERROR: Method overwriting is not permitted during Module precompilation. Use `__precompile__(false)` to opt-out of precompilation.

(needs @test_warn)

@KristofferC KristofferC mentioned this pull request Nov 27, 2023
39 tasks
KristofferC pushed a commit that referenced this pull request Nov 27, 2023
…aching (#52214)

Fixes #52213

Overwritting methods during cache creation is currently not something
that the system
can support and can lead to surprising, counter-intuitive and fatal
errors.

In 1.10 we turned it from a warning to a strong error, with this PR it
remains
a strong error, but the precompilation system recognizes it and
essentially sets `__precompile__(false)`
for this module and all modules that depend on it.

Before:
```
julia> using OverwriteMethodError
[ Info: Precompiling OverwriteMethodError [top-level]
WARNING: Method definition +(Bool, Bool) in module Base at bool.jl:166 overwritten in module OverwriteMethodError at /home/vchuravy/src/julia2/OverwriteMethodError.jl:2.
ERROR: LoadError: Method overwriting is not permitted during Module precompile.
Stacktrace:
 [1] top-level scope
   @ ~/src/julia2/OverwriteMethodError.jl:2
 [2] include
   @ Base ./Base.jl:489 [inlined]
 [3] include_package_for_output(pkg::Base.PkgId, input::String, depot_path::Vector{String}, dl_load_path::Vector{String}, load_path::Vector{String}, concrete_deps::Vector{Pair{Base.PkgId, UInt128}}, source::Nothing)
   @ Base ./loading.jl:2216
 [4] top-level scope
   @ stdin:3
in expression starting at /home/vchuravy/src/julia2/OverwriteMethodError.jl:1
in expression starting at stdin:3
ERROR: Failed to precompile OverwriteMethodError [top-level] to "/home/vchuravy/.julia/compiled/v1.10/jl_guiuCQ".
Stacktrace:
  [1] error(s::String)
    @ Base ./error.jl:35
  [2] compilecache(pkg::Base.PkgId, path::String, internal_stderr::IO, internal_stdout::IO, keep_loaded_modules::Bool)
    @ Base ./loading.jl:2462
  [3] compilecache
    @ Base ./loading.jl:2334 [inlined]
  [4] (::Base.var"#968#969"{Base.PkgId})()
    @ Base ./loading.jl:1968
  [5] mkpidlock(f::Base.var"#968#969"{Base.PkgId}, at::String, pid::Int32; kwopts::@kwargs{stale_age::Int64, wait::Bool})
    @ FileWatching.Pidfile ~/.julia/juliaup/julia-1.10.0-rc1+0.x64.linux.gnu/share/julia/stdlib/v1.10/FileWatching/src/pidfile.jl:93
  [6] #mkpidlock#6
    @ FileWatching.Pidfile ~/.julia/juliaup/julia-1.10.0-rc1+0.x64.linux.gnu/share/julia/stdlib/v1.10/FileWatching/src/pidfile.jl:88 [inlined]
  [7] trymkpidlock(::Function, ::Vararg{Any}; kwargs::@kwargs{stale_age::Int64})
    @ FileWatching.Pidfile ~/.julia/juliaup/julia-1.10.0-rc1+0.x64.linux.gnu/share/julia/stdlib/v1.10/FileWatching/src/pidfile.jl:111
  [8] #invokelatest#2
    @ Base ./essentials.jl:889 [inlined]
  [9] invokelatest
    @ Base ./essentials.jl:884 [inlined]
 [10] maybe_cachefile_lock(f::Base.var"#968#969"{Base.PkgId}, pkg::Base.PkgId, srcpath::String; stale_age::Int64)
    @ Base ./loading.jl:2977
 [11] maybe_cachefile_lock
    @ Base ./loading.jl:2974 [inlined]
 [12] _require(pkg::Base.PkgId, env::String)
    @ Base ./loading.jl:1964
 [13] __require_prelocked(uuidkey::Base.PkgId, env::String)
    @ Base ./loading.jl:1806
 [14] #invoke_in_world#3
    @ Base ./essentials.jl:921 [inlined]
 [15] invoke_in_world
    @ Base ./essentials.jl:918 [inlined]
 [16] _require_prelocked(uuidkey::Base.PkgId, env::String)
    @ Base ./loading.jl:1797
 [17] macro expansion
    @ Base ./loading.jl:1784 [inlined]
 [18] macro expansion
    @ Base ./lock.jl:267 [inlined]
 [19] __require(into::Module, mod::Symbol)
    @ Base ./loading.jl:1747
 [20] #invoke_in_world#3
    @ Base ./essentials.jl:921 [inlined]
 [21] invoke_in_world
    @ Base ./essentials.jl:918 [inlined]
 [22] require(into::Module, mod::Symbol)
    @ Base ./loading.jl:1740
```

After:
```
julia> using OverwriteMethodError
┌ Info: Precompiling OverwriteMethodError [top-level]
└ @ Base loading.jl:2486
WARNING: Method definition +(Bool, Bool) in module Base at bool.jl:166 overwritten in module OverwriteMethodError at /home/vchuravy/src/julia2/OverwriteMethodError.jl:2.
ERROR: Method overwriting is not permitted during Module precompile.
┌ Info: Skipping precompilation since __precompile__(false). Importing OverwriteMethodError [top-level].
└ @ Base loading.jl:2084
```

---------

Co-authored-by: Kristoffer Carlsson <[email protected]>
(cherry picked from commit 9e8fe68)
KristofferC added a commit that referenced this pull request Dec 2, 2023
Backported PRs:
- [x] #51213 <!-- Wait for other threads to finish compiling before
exiting -->
- [x] #51520 <!-- Make allocopt respect the GC verifier rules with non
usual address spaces -->
- [x] #51598 <!-- Use a simple error when reporting sysimg load
failures. -->
- [x] #51757 <!-- fix parallel peakflop usage -->
- [x] #51781 <!-- Don't make pkgimages global editable -->
- [x] #51848 <!-- allow finalizers to take any locks and yield during
exit -->
- [x] #51847 <!-- add missing wait during Timer and AsyncCondition close
-->
- [x] #50824 <!-- Add some aliasing warnings to docstrings for mutating
functions in Base -->
- [x] #51885 <!-- remove chmodding the pkgimages -->
- [x] #50207 <!-- [devdocs] Improve documentation about building
external forks of LLVM -->
- [x] #51967 <!-- further fix to the new promoting method for
AbstractDateTime subtraction -->
- [x] #51980 <!-- macroexpand: handle const/atomic struct fields
correctly -->
- [x] #51995 <!-- [Artifacts] Pass artifacts dictionary to
`ensure_artifact_installed` dispatch -->
- [x] #52098 <!-- Fix errors in `sort` docstring -->
- [x] #52136 <!-- Bump JuliaSyntax to 0.4.7 -->
- [x] #52140 <!-- Make c func `abspath` consistent on Windows. Fix
tracking path conversion. -->
- [x] #52009 <!-- fix completion that resulted in startpos of 0 for `\\
-->
- [x] #52192 <!-- cap the number of GC threads to number of cpu cores
-->
- [x] #52206 <!-- Make have_fma consistent between interpreter and
compiled -->
- [x] #52027 <!-- fix Unicode.julia_chartransform for Julia 1.10 -->
- [x] #52217 <!-- More helpful error message for empty `cpu_target` in
`Base.julia_cmd` -->
- [x] #51371 <!-- Memoize `cwstring` when used for env lookup /
modification on Windows -->
- [x] #52214 <!-- Turn Method Overwritten Error into a PrecompileError
-- turning off caching -->
- [x] #51895 <!-- Devdocs on fixing precompile hangs, take 2 -->
- [x] #51596 <!-- Reland "Don't mark nonlocal symbols as hidden"" -->
- [x] #51834 <!-- [REPLCompletions] allow symbol completions within
incomplete macrocall expression -->
- [x] #52010 <!-- Revert "Support sorting iterators (#46104)" -->
- [x] #51430 <!-- add support for async backtraces of Tasks on any
thread -->
- [x] #51471 <!-- Fix segfault if root task is NULL -->
- [x] #52194 <!-- Fix multiversioning issues caused by the parallel llvm
work -->
- [x] #51035 <!-- refactor GC scanning code to reflect jl_binding_t are
now first class -->
- [x] #52030 <!-- Bump Statistics -->
- [x] #52189 <!-- codegen: ensure i1 bool is widened to i8 before
storing -->
- [x] #52228 <!-- Widen diagonal var during `Type` unwrapping in
`instanceof_tfunc` -->
- [x] #52182 <!-- jitlayers: replace sharedbytes intern pool with one
that respects alignment -->

Contains multiple commits, manual intervention needed:
- [ ] #51092 <!-- inference: fix bad effects for recursion -->

Non-merged PRs with backport label:
- [ ] #52196 <!-- Fix creating custom log level macros -->
- [ ] #52170 <!-- fix invalidations related to `ismutable` -->
- [ ] #51479 <!-- prevent code loading from lookin in the versioned
environment when building Julia -->
@KristofferC KristofferC removed the backport 1.10 Change should be backported to the 1.10 release label Dec 2, 2023
mkitti pushed a commit to mkitti/julia that referenced this pull request Dec 9, 2023
…aching (JuliaLang#52214)

Fixes JuliaLang#52213

Overwritting methods during cache creation is currently not something
that the system
can support and can lead to surprising, counter-intuitive and fatal
errors.

In 1.10 we turned it from a warning to a strong error, with this PR it
remains
a strong error, but the precompilation system recognizes it and
essentially sets `__precompile__(false)`
for this module and all modules that depend on it.

Before:
```
julia> using OverwriteMethodError
[ Info: Precompiling OverwriteMethodError [top-level]
WARNING: Method definition +(Bool, Bool) in module Base at bool.jl:166 overwritten in module OverwriteMethodError at /home/vchuravy/src/julia2/OverwriteMethodError.jl:2.
ERROR: LoadError: Method overwriting is not permitted during Module precompile.
Stacktrace:
 [1] top-level scope
   @ ~/src/julia2/OverwriteMethodError.jl:2
 [2] include
   @ Base ./Base.jl:489 [inlined]
 [3] include_package_for_output(pkg::Base.PkgId, input::String, depot_path::Vector{String}, dl_load_path::Vector{String}, load_path::Vector{String}, concrete_deps::Vector{Pair{Base.PkgId, UInt128}}, source::Nothing)
   @ Base ./loading.jl:2216
 [4] top-level scope
   @ stdin:3
in expression starting at /home/vchuravy/src/julia2/OverwriteMethodError.jl:1
in expression starting at stdin:3
ERROR: Failed to precompile OverwriteMethodError [top-level] to "/home/vchuravy/.julia/compiled/v1.10/jl_guiuCQ".
Stacktrace:
  [1] error(s::String)
    @ Base ./error.jl:35
  [2] compilecache(pkg::Base.PkgId, path::String, internal_stderr::IO, internal_stdout::IO, keep_loaded_modules::Bool)
    @ Base ./loading.jl:2462
  [3] compilecache
    @ Base ./loading.jl:2334 [inlined]
  [4] (::Base.var"JuliaLang#968#969"{Base.PkgId})()
    @ Base ./loading.jl:1968
  [5] mkpidlock(f::Base.var"JuliaLang#968#969"{Base.PkgId}, at::String, pid::Int32; kwopts::@kwargs{stale_age::Int64, wait::Bool})
    @ FileWatching.Pidfile ~/.julia/juliaup/julia-1.10.0-rc1+0.x64.linux.gnu/share/julia/stdlib/v1.10/FileWatching/src/pidfile.jl:93
  [6] #mkpidlock#6
    @ FileWatching.Pidfile ~/.julia/juliaup/julia-1.10.0-rc1+0.x64.linux.gnu/share/julia/stdlib/v1.10/FileWatching/src/pidfile.jl:88 [inlined]
  [7] trymkpidlock(::Function, ::Vararg{Any}; kwargs::@kwargs{stale_age::Int64})
    @ FileWatching.Pidfile ~/.julia/juliaup/julia-1.10.0-rc1+0.x64.linux.gnu/share/julia/stdlib/v1.10/FileWatching/src/pidfile.jl:111
  [8] #invokelatest#2
    @ Base ./essentials.jl:889 [inlined]
  [9] invokelatest
    @ Base ./essentials.jl:884 [inlined]
 [10] maybe_cachefile_lock(f::Base.var"JuliaLang#968#969"{Base.PkgId}, pkg::Base.PkgId, srcpath::String; stale_age::Int64)
    @ Base ./loading.jl:2977
 [11] maybe_cachefile_lock
    @ Base ./loading.jl:2974 [inlined]
 [12] _require(pkg::Base.PkgId, env::String)
    @ Base ./loading.jl:1964
 [13] __require_prelocked(uuidkey::Base.PkgId, env::String)
    @ Base ./loading.jl:1806
 [14] #invoke_in_world#3
    @ Base ./essentials.jl:921 [inlined]
 [15] invoke_in_world
    @ Base ./essentials.jl:918 [inlined]
 [16] _require_prelocked(uuidkey::Base.PkgId, env::String)
    @ Base ./loading.jl:1797
 [17] macro expansion
    @ Base ./loading.jl:1784 [inlined]
 [18] macro expansion
    @ Base ./lock.jl:267 [inlined]
 [19] __require(into::Module, mod::Symbol)
    @ Base ./loading.jl:1747
 [20] #invoke_in_world#3
    @ Base ./essentials.jl:921 [inlined]
 [21] invoke_in_world
    @ Base ./essentials.jl:918 [inlined]
 [22] require(into::Module, mod::Symbol)
    @ Base ./loading.jl:1740
```

After:
```
julia> using OverwriteMethodError
┌ Info: Precompiling OverwriteMethodError [top-level]
└ @ Base loading.jl:2486
WARNING: Method definition +(Bool, Bool) in module Base at bool.jl:166 overwritten in module OverwriteMethodError at /home/vchuravy/src/julia2/OverwriteMethodError.jl:2.
ERROR: Method overwriting is not permitted during Module precompile.
┌ Info: Skipping precompilation since __precompile__(false). Importing OverwriteMethodError [top-level].
└ @ Base loading.jl:2084
```

---------

Co-authored-by: Kristoffer Carlsson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

incremental precompile warning turned into error: breaking?
8 participants