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

Fix type instability in PresetTimeCallback #230

Merged
merged 1 commit into from
Aug 20, 2024

Conversation

ytdHuang
Copy link
Contributor

@ytdHuang ytdHuang commented Aug 20, 2024

close #225

This PR tries to solve the type instability in PresetTimeCallback start from v3.2.0+

Checklist

@ChrisRackauckas
Copy link
Member

ERROR: LoadError: `has_newpm()` is deprecated, use `true` instead.
Stacktrace:
 [1] depwarn(msg::String, funcsym::Symbol; force::Bool)
   @ Base ./deprecated.jl:124
 [2] depwarn(msg::String, funcsym::Symbol)
   @ Base ./deprecated.jl:121
 [3] has_newpm()
   @ LLVM ./deprecated.jl:104
 [4] top-level scope
   @ ~/.julia/packages/GPUCompiler/Y4hSX/src/GPUCompiler.jl:19
 [5] include
   @ ./Base.jl:495 [inlined]
 [6] 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::String)
   @ Base ./loading.jl:2222
 [7] top-level scope
   @ stdin:3
in expression starting at /home/runner/.julia/packages/GPUCompiler/Y4hSX/src/GPUCompiler.jl:1
in expression starting at stdin:3
ERROR: LoadError: Failed to precompile GPUCompiler [61eb1bfa-7361-4325-ad38-22787b887f55] to "/home/runner/.julia/compiled/v1.10/GPUCompiler/jl_AJaXUG".
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:2468
  [3] compilecache
    @ ./loading.jl:2340 [inlined]
  [4] (::Base.var"#968#969"{Base.PkgId})()
    @ Base ./loading.jl:1974
  [5] mkpidlock(f::Base.var"#968#969"{Base.PkgId}, at::String, pid::Int32; kwopts::@Kwargs{stale_age::Int64, wait::Bool})
    @ FileWatching.Pidfile /opt/hostedtoolcache/julia/1.10.4/x64/share/julia/stdlib/v1.10/FileWatching/src/pidfile.jl:93
  [6] #mkpidlock#6
    @ /opt/hostedtoolcache/julia/1.10.4/x64/share/julia/stdlib/v1.10/FileWatching/src/pidfile.jl:88 [inlined]
  [7] trymkpidlock(::Function, ::Vararg{Any}; kwargs::@Kwargs{stale_age::Int64})
    @ FileWatching.Pidfile /opt/hostedtoolcache/julia/1.10.4/x64/share/julia/stdlib/v1.10/FileWatching/src/pidfile.jl:111
  [8] #invokelatest#2
    @ ./essentials.jl:894 [inlined]
  [9] invokelatest
    @ ./essentials.jl:889 [inlined]
 [10] maybe_cachefile_lock(f::Base.var"#968#969"{Base.PkgId}, pkg::Base.PkgId, srcpath::String; stale_age::Int64)
    @ Base ./loading.jl:2983
 [11] maybe_cachefile_lock
    @ ./loading.jl:2980 [inlined]
 [12] _require(pkg::Base.PkgId, env::String)
    @ Base ./loading.jl:1970
 [13] __require_prelocked(uuidkey::Base.PkgId, env::String)
    @ Base ./loading.jl:1812
 [14] #invoke_in_world#3
    @ ./essentials.jl:926 [inlined]
 [15] invoke_in_world
    @ ./essentials.jl:923 [inlined]
 [16] _require_prelocked(uuidkey::Base.PkgId, env::String)
    @ Base ./loading.jl:1803
 [17] macro expansion
    @ ./loading.jl:1790 [inlined]
 [18] macro expansion
    @ ./lock.jl:267 [inlined]
 [19] __require(into::Module, mod::Symbol)
    @ Base ./loading.jl:1753
 [20] #invoke_in_world#3
    @ ./essentials.jl:926 [inlined]
 [21] invoke_in_world
    @ ./essentials.jl:923 [inlined]
 [22] require(into::Module, mod::Symbol)
    @ Base ./loading.jl:1746
 [23] include(mod::Module, _path::String)
    @ Base ./Base.jl:495
 [24] include(x::String)
    @ Enzyme ~/.julia/packages/Enzyme/OOd6p/src/Enzyme.jl:1
 [25] top-level scope
    @ ~/.julia/packages/Enzyme/OOd6p/src/Enzyme.jl:46
 [26] include
    @ ./Base.jl:495 [inlined]
 [27] 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::String)
    @ Base ./loading.jl:2222
 [28] top-level scope
    @ stdin:3
in expression starting at /home/runner/.julia/packages/Enzyme/OOd6p/src/typetree.jl:6
in expression starting at /home/runner/.julia/packages/Enzyme/OOd6p/src/Enzyme.jl:1
in expression starting at stdin:3
ERROR: LoadError: Failed to precompile Enzyme [7da242da-08ed-463a-9acd-ee780be4f1d9] to "/home/runner/.julia/compiled/v1.10/Enzyme/jl_0fd1qt".

@maleadt @wsmoses your LLVM incompatibility has our CI shut down for now. How should I resolve this?

@maleadt
Copy link

maleadt commented Aug 20, 2024

Don't run on CI with --depwarn=error? That's not what deprecations are for, I frequently add them in both CUDA.jl and LLVM.jl to avoid having to tag breaking releases (given that these are fairly widely used packages).

@ChrisRackauckas
Copy link
Member

Adding depwarns is different from using deprecated code. Why isn't there an update that just avoids the depwarn in your own dependency?

@maleadt
Copy link

maleadt commented Aug 20, 2024

Adding depwarns is different from using deprecated code.

Deprecated code triggers depwarns.

Why isn't there an update that just avoids the depwarn in your own dependency?

There is; you aren't using the latest version because of some incompatibility.

@ChrisRackauckas ChrisRackauckas merged commit f1a0ff7 into SciML:master Aug 20, 2024
3 of 9 checks passed
@ytdHuang ytdHuang deleted the preset_time_type_inference branch August 20, 2024 13:23
@maleadt
Copy link

maleadt commented Aug 20, 2024

I guess what we really want is --depwarn=own-error, or something like it in order to error on deprecated functionality / depwarns triggered by the code in the package being tested, as opposed to depwarns in dependencies that you can't update.

@ChrisRackauckas
Copy link
Member

Yes. That's really what we're looking for.

@wsmoses
Copy link

wsmoses commented Aug 20, 2024

So those functions are used to determine which version of LLVM is being used (later versions of LLVM have opaque pointers/newpm). Enzyme supports 1.6+. LLVM.jl only supports 1.10+ on latest versions (and older versions support 1.6). Thus on 1.6/7/8/9 Enzyme uses older LLVM.jl's. Since LLVM.jl only supports 1.10+, I presume @maleadt wants to eventually remove the other functionality, hence the depwarn. Since we support many different LLVM's/julia versions, we need to support both sides of the conditional so to speak.

Once the LTS comes out and Enzyme drops 1.6-1.9 we can remove the code using that check.

Not sure if there's a better soltion in the interim, but thats the tl;dr.

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.

Type instabilities from v3.2.0
4 participants