Skip to content

Warn user about precompilation of symbolic units #58

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Warn user about precompilation of symbolic units #58

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
689a723
Warn user about precompilation of symbolic units
MilesCranmer Oct 13, 2023
cc9694c
Merge branch 'main' into safe-precompile
MilesCranmer Oct 22, 2023
8520362
Merge branch 'main' into safe-precompile
MilesCranmer Nov 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion src/symbolic_dimensions.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,8 @@ module SymbolicUnitsParse
import ...DEFAULT_VALUE_TYPE
import ...DEFAULT_DIM_BASE_TYPE

_is_precompiling() = ccall(:jl_generating_output, Cint, ()) == 1

# Lazily create unit symbols (since there are so many)
module Constants
import ..CONSTANT_SYMBOLS
Expand All @@ -290,12 +292,14 @@ module SymbolicUnitsParse
import ..Quantity
import ..DEFAULT_VALUE_TYPE
import ..DEFAULT_DIM_BASE_TYPE
import .._is_precompiling

import ...Constants as EagerConstants

const CONSTANT_SYMBOLS_EXIST = Ref{Bool}(false)
const CONSTANT_SYMBOLS_LOCK = Threads.SpinLock()
function _generate_unit_symbols()
_is_precompiling() && return nothing
CONSTANT_SYMBOLS_EXIST[] || lock(CONSTANT_SYMBOLS_LOCK) do
CONSTANT_SYMBOLS_EXIST[] && return nothing
for unit in setdiff(CONSTANT_SYMBOLS, SYMBOL_CONFLICTS)
Expand All @@ -314,7 +318,19 @@ module SymbolicUnitsParse

const UNIT_SYMBOLS_EXIST = Ref{Bool}(false)
const UNIT_SYMBOLS_LOCK = Threads.SpinLock()
function _generate_unit_symbols()
function _generate_unit_symbols(; testing=Val(false))
Copy link

Choose a reason for hiding this comment

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

Just an FYI — the changes in this PR completely address what I observed in #51! I checked out this branch with my AstrophysicalCalculations package, and found the expected warning. Then I changed my code to use the Quantity(..., SymbolicDimensions; pc=1) constructor, as instructed by the warning, and my package precompiled as expected.

Copy link
Member Author

@MilesCranmer MilesCranmer Nov 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Awesome!

The only reason I'm hesitating to merge is I'm not sure there are any cases where you may actually need to run through that function during precompilation. I ran some testing and it seemed to hit that error even without necessarily precompiling a module with it... So maybe we could just leave the warning, but still allow it to execute as normal?

Copy link

@cadojo cadojo Nov 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I ran some testing and it seemed to hit that error even without necessarily precompiling a module with it...

That's interesting. I don't know enough about Julia's stages of compilation / precompilation. I think the warning is great; if I saw it before making #51, I would have used the Quantity constructor instead of filing an issue. Of course, no rush merging anything from my point of view; AstrophysicalCalculations is a "brush up on physics before graduate school" kind of package, not a serious kind of package. 😉 Plus the constructor is a great solution as-is.

if _is_precompiling() || testing === Val(true)
@warn (
"Creating SymbolicDimensions objects during precompilation is not allowed, "
* "as symbolic units are not created until the first runtime call. "
* "You should use regular `Dimensions` instead for computations, "
* "and generally use `SymbolicDimensions` for display purposes. "
* "If needed, you can manually create `SymbolicDimensions` by calling "
* "the constructor explicitly, such as `Quantity(1.5, SymbolicDimensions; km=1, s=-1)`, "
* "which is equivalent to `us\"km/s\"`."
)
return nothing
end
UNIT_SYMBOLS_EXIST[] || lock(UNIT_SYMBOLS_LOCK) do
UNIT_SYMBOLS_EXIST[] && return nothing
for unit in UNIT_SYMBOLS
Expand Down
8 changes: 8 additions & 0 deletions test/unittests.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -638,6 +638,14 @@ end
VERSION >= v"1.8" &&
@test_throws "rad is not available as a symbol" sym5.rad

# Extra test coverage
@test_warn(
"Creating SymbolicDimensions objects",
DynamicQuantities.SymbolicUnitsParse._generate_unit_symbols(
testing=Val(true)
)
)

# Test deprecated method
q = 1.5us"km/s"
@test expand_units(q) == uexpand(q)
Expand Down