SymbolicML / DynamicQuantities.jl

Efficient and type-stable physical quantities in Julia
https://symbolicml.org/DynamicQuantities.jl/dev/
Apache License 2.0
132 stars 17 forks source link

Apparent Issue with Precompilation for Dependent Packages #51

Closed cadojo closed 9 months ago

cadojo commented 1 year ago

Edit: Actually, this issue happens even if I don't use Revise.jl! I think the @eval line somehow breaks precompilation?

I'm developing a new package, AstrophysicalCalculations, which depends on DynamicQuantities, and I'm using Revise to assist with that development. When I enter using Revise, AstrophysicalCalculations, I get the error shown below. I believe that the @eval line in symbolic_dimensions.jl:215 makes using Revise impossible for dependent packages. I could be wrong / using this incorrectly, and this issue may be unresolvable, but I wanted to post this issue for awareness. Thank you for DynamicQuantities.jl!

The line where my package fails: src/src/StellarObservations.jl:111.

``` ERROR: LoadError: Evaluation into the closed module `SymbolicUnitsParse` breaks incremental compilation because the side effects will not be permanent. This is likely due to some other module mutating `SymbolicUnitsParse` with `eval` during precompilation - don't do this. Stacktrace: [1] eval @ ./boot.jl:370 [inlined] [2] (::DynamicQuantities.SymbolicUnitsParse.var"#1#2")() @ DynamicQuantities.SymbolicUnitsParse ~/.julia/packages/DynamicQuantities/AS62k/src/symbolic_dimensions.jl:162 [3] lock(f::DynamicQuantities.SymbolicUnitsParse.var"#1#2", l::Base.Threads.SpinLock) @ Base ./lock.jl:229 [4] _generate_unit_symbols @ ~/.julia/packages/DynamicQuantities/AS62k/src/symbolic_dimensions.jl:159 [inlined] [5] sym_uparse(raw_string::String) @ DynamicQuantities.SymbolicUnitsParse ~/.julia/packages/DynamicQuantities/AS62k/src/symbolic_dimensions.jl:186 [6] var"@us_str"(__source__::LineNumberNode, __module__::Module, s::Any) @ DynamicQuantities ~/.julia/packages/DynamicQuantities/AS62k/src/symbolic_dimensions.jl:215 [7] #macroexpand#63 @ ./expr.jl:119 [inlined] [8] macroexpand @ ./expr.jl:117 [inlined] [9] docm(source::LineNumberNode, mod::Module, meta::Any, ex::Any, define::Bool) @ Base.Docs ./docs/Docs.jl:539 [10] docm(source::LineNumberNode, mod::Module, meta::Any, ex::Any) @ Base.Docs ./docs/Docs.jl:539 [11] (::DocStringExtensions.var"#35#36"{typeof(DocStringExtensions.template_hook)})(::LineNumberNode, ::Vararg{Any}) @ DocStringExtensions ~/.julia/packages/DocStringExtensions/JVu77/src/templates.jl:11 [12] var"@doc"(::LineNumberNode, ::Module, ::String, ::Vararg{Any}) @ Core ./boot.jl:539 [13] include(mod::Module, _path::String) @ Base ./Base.jl:457 [14] include(x::String) @ AstrophysicalCalculations ~/Projects/Science/AstrophysicalCalculations.jl/src/AstrophysicalCalculations.jl:36 [15] top-level scope @ ~/Projects/Science/AstrophysicalCalculations.jl/src/AstrophysicalCalculations.jl:61 [16] include @ ./Base.jl:457 [inlined] [17] 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:2049 [18] top-level scope @ stdin:3 in expression starting at /Users/joey/Projects/Science/AstrophysicalCalculations.jl/src/src/StellarObservations.jl:31 in expression starting at /Users/joey/Projects/Science/AstrophysicalCalculations.jl/src/src/StellarObservations.jl:1 in expression starting at /Users/joey/Projects/Science/AstrophysicalCalculations.jl/src/AstrophysicalCalculations.jl:1 in expression starting at stdin:3 ERROR: Failed to precompile AstrophysicalCalculations [fd9cccc2-a1d0-4073-bc0f-e265a877cbf5] to "/Users/joey/.julia/compiled/v1.9/AstrophysicalCalculations/jl_DJD5DI". 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:2300 [3] compilecache @ ./loading.jl:2167 [inlined] [4] _require(pkg::Base.PkgId, env::String) @ Base ./loading.jl:1805 [5] _require_prelocked(uuidkey::Base.PkgId, env::String) @ Base ./loading.jl:1660 [6] macro expansion @ ./loading.jl:1648 [inlined] [7] macro expansion @ ./lock.jl:267 [inlined] [8] require(into::Module, mod::Symbol) @ Base ./loading.jl:1611 ```
MilesCranmer commented 1 year ago

Hi @cadojo,

Thanks for the report. Excited to hear about the package you are making!

Is this Julia 1.10? Does it go away on Julia 1.9?

One thing to note is that the SymbolicDimensions symbols (e.g., us"m") don’t exist during precompilation. The reason for this is that there are so many, it makes startup time slow unless you are using them. So I wonder if it’s related to that.

Best, Miles

MilesCranmer commented 1 year ago

The quick fix would be to change

+ flux_distance(m, M) = 10^((m - M + 5) // 5) * u"Constants.pc"
- flux_distance(m, M) = 10^((m - M + 5) // 5) * us"Constants.pc"

us (SymbolicDimensions) is intended to be for display, whereas u (Dimensions) is intended to be for calculations.

us would not be available during precompilation because the variables are created upon first call. But maybe we can change that.

cadojo commented 1 year ago

Thanks Miles! I did change all instances of us"Constants.pc" to just Constants.pc, and that worked also. I'm using Julia 1.9 — my versioninfo() is shown below.

Julia Version 1.9.3
Commit bed2cd540a1 (2023-08-24 14:43 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: macOS (arm64-apple-darwin22.4.0)
  CPU: 8 × Apple M1
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-14.0.6 (ORCJIT, apple-m1)
  Threads: 1 on 4 virtual cores
MilesCranmer commented 1 year ago

Awesome. Okay I think we should go with that, what do you think? Then for display, I think @gaurav-arya has a good strategy proposed in https://github.com/SymbolicML/DynamicQuantities.jl/pull/48

Basically, one can display units with as_u (name pending) e.g.,

u"Constants.pc" / 0.05 |> as_u(us"Constants.kpc)

which would display 0.02 kpc, even though the calculation happens with the faster Dimensions.

cadojo commented 1 year ago

Works for me, and I like the unit conversion feature and syntax. The |> is convenient. I also like the Unitful.uconvert syntax, I see some benefit to having multiple different methods for as_u so there are different options for converting one quantity's units to another: as_u(u"km", 100u"m") or 100u"m" |> as_u(u"km").

So all of this is good for display and for my purposes, but could this issue pop up again? If I understand correctly, symbolic units can't be used in any Julia package, because they're created on-the-fly. With native code cacheing in Julia v1.9 and later, I wonder how much slower the package would be if all of the symbolic units are created at once? If no one has done that experiment yet, and if there's interest in the future, I'd be interested in trying that out.

MilesCranmer commented 8 months ago

Okay now this issue is fixed for real – please try it out! You can do, e.g.,

const my_unit = us"ohm"

just fine, and precompilation should be good