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.

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

Jump to bottom

@eval considered harmful #3555

Open
vchuravy opened this issue Apr 17, 2024 · 3 comments · May be fixed by #3556
Open

@eval considered harmful #3555

vchuravy opened this issue Apr 17, 2024 · 3 comments · May be fixed by #3556
Labels
performance 🏍️ So we can get the wrong answer even faster

Comments

@vchuravy
Copy link
Collaborator

Trying to use OceanScalingTest.jl from @simone-silvestri as a benchmark for TTFTS (Time-To-First-Time-Step) I came across this delightful error:

ERROR: LoadError: Evaluation into the closed module `Grids` breaks incremental compilation because the side effects will not be permanent. This is likely due to some other module mutating `Grids` with `eval` during precompilation - don't do this.
Stacktrace:
  [1] eval
    @ ./boot.jl:428 [inlined]
  [2] allocate_metrics(grid::Oceananigans.Grids.LatitudeLongitudeGrid{Float64, Oceananigans.Grids.Periodic, Oceananigans.Grids.Bounded, Oceananigans.Grids.Bounded, Nothing, Nothing, Float64, Float64, OffsetArrays.OffsetVector{Float64, CUDA.CuArray{Float64, 1, CUDA.Mem.DeviceBuffer}}, OffsetArrays.OffsetVector{Float64, StepRangeLen{Float64, Base.TwicePrecision{Float64}, Base.TwicePrecision{Float64}, Int64}}, OffsetArrays.OffsetVector{Float64, StepRangeLen{Float64, Base.TwicePrecision{Float64}, Base.TwicePrecision{Float64}, Int64}}, OffsetArrays.OffsetVector{Float64, CUDA.CuArray{Float64, 1, CUDA.Mem.DeviceBuffer}}, Oceananigans.DistributedComputations.Distributed{Oceananigans.Architectures.GPU, false, Oceananigans.DistributedComputations.Partition{Int64, Int64, Int64}, Tuple{Int64, Int64, Int64}, Int64, Tuple{Int64, Int64, Int64}, Oceananigans.DistributedComputations.RankConnectivity{Nothing, Nothing, Nothing, Nothing, Nothing, Nothing, Nothing, Nothing}, MPI.Comm, Vector{MPI.Request}, Base.RefValue{Int64}}})
    @ Oceananigans.Grids ~/.julia/packages/Oceananigans/kBe5X/src/Grids/latitude_longitude_grid.jl:554
  [3] with_precomputed_metrics(grid::Oceananigans.Grids.LatitudeLongitudeGrid{Float64, Oceananigans.Grids.Periodic, Oceananigans.Grids.Bounded, Oceananigans.Grids.Bounded, Nothing, Nothing, Float64, Float64, OffsetArrays.OffsetVector{Float64, CUDA.CuArray{Float64, 1, CUDA.Mem.DeviceBuffer}}, OffsetArrays.OffsetVector{Float64, StepRangeLen{Float64, Base.TwicePrecision{Float64}, Base.TwicePrecision{Float64}, Int64}}, OffsetArrays.OffsetVector{Float64, StepRangeLen{Float64, Base.TwicePrecision{Float64}, Base.TwicePrecision{Float64}, Int64}}, OffsetArrays.OffsetVector{Float64, CUDA.CuArray{Float64, 1, CUDA.Mem.DeviceBuffer}}, Oceananigans.DistributedComputations.Distributed{Oceananigans.Architectures.GPU, false, Oceananigans.DistributedComputations.Partition{Int64, Int64, Int64}, Tuple{Int64, Int64, Int64}, Int64, Tuple{Int64, Int64, Int64}, Oceananigans.DistributedComputations.RankConnectivity{Nothing, Nothing, Nothing, Nothing, Nothing, Nothing, Nothing, Nothing}, MPI.Comm, Vector{MPI.Request}, Base.RefValue{Int64}}})
    @ Oceananigans.Grids ~/.julia/packages/Oceananigans/kBe5X/src/Grids/latitude_longitude_grid.jl:222
  [4] Oceananigans.Grids.LatitudeLongitudeGrid(arch::Oceananigans.DistributedComputations.Distributed{Oceananigans.Architectures.GPU, false, Oceananigans.DistributedComputations.Partition{Int64, Int64, Int64}, Tuple{Int64, Int64, Int64}, Int64, Tuple{Int64, Int64, Int64}, Oceananigans.DistributedComputations.RankConnectivity{Nothing, Nothing, Nothing, Nothing, Nothing, Nothing, Nothing, Nothing}, MPI.Comm, Vector{MPI.Request}, Base.RefValue{Int64}}, FT::DataType; precompute_metrics::Bool, size::Tuple{Int64, Int64, Int64}, latitude::Tuple{Int64, Int64}, longitude::Tuple{Int64, Int64}, z::Vector{Float64}, topology::Nothing, radius::Float64, halo::Tuple{Int64, Int64, Int64})
    @ Oceananigans.DistributedComputations ~/.julia/packages/Oceananigans/kBe5X/src/DistributedComputations/distributed_grids.jl:160
  [5] LatitudeLongitudeGrid
    @ ~/.julia/packages/Oceananigans/kBe5X/src/DistributedComputations/distributed_grids.jl:106 [inlined]
  [6] macro expansion
    @ ./show.jl:1230 [inlined]
  [7] load_balanced_grid(arch::Oceananigans.DistributedComputations.Distributed{Oceananigans.Architectures.GPU, false, Oceananigans.DistributedComputations.Partition{Int64, Int64, Int64}, Tuple{Int64, Int64, Int64}, Int64, Tuple{Int64, Int64, Int64}, Oceananigans.DistributedComputations.RankConnectivity{Nothing, Nothing, Nothing, Nothing, Nothing, Nothing, Nothing, Nothing}, MPI.Comm, Vector{MPI.Request}, Base.RefValue{Int64}}, precision::Type, N::Tuple{Int64, Int64, Int64}, latitude::Tuple{Int64, Int64}, z_faces::Vector{Float64}, resolution::Int64, ::Val{false}, ::Val{:DoubleDrake}; Bottom::Type{Oceananigans.ImmersedBoundaries.GridFittedBottom})
    @ OceanScalingTests ~/src/OceanScalingTests.jl/src/grid_load_balance.jl:21
  [8] load_balanced_grid(arch::Oceananigans.DistributedComputations.Distributed{Oceananigans.Architectures.GPU, false, Oceananigans.DistributedComputations.Partition{Int64, Int64, Int64}, Tuple{Int64, Int64, Int64}, Int64, Tuple{Int64, Int64, Int64}, Oceananigans.DistributedComputations.RankConnectivity{Nothing, Nothing, Nothing, Nothing, Nothing, Nothing, Nothing, Nothing}, MPI.Comm, Vector{MPI.Request}, Base.RefValue{Int64}}, precision::Type, N::Tuple{Int64, Int64, Int64}, latitude::Tuple{Int64, Int64}, z_faces::Vector{Float64}, resolution::Int64, ::Val{false}, ::Val{:DoubleDrake})
    @ OceanScalingTests ~/src/OceanScalingTests.jl/src/grid_load_balance.jl:16
  [9] scaling_test_simulation(resolution::Int64, ranks::Tuple{Int64, Int64, Int64}, Δt::Tuple{Int64, Float64}, stop_time::Float64; child_arch::Oceananigans.Architectures.GPU, experiment::Symbol, Depth::Int64, latitude::Tuple{Int64, Int64}, restart::String, z_faces_function::typeof(OceanScalingTests.exponential_z_faces), Nz::Int64, profile::Bool, with_fluxes::Bool, with_restoring::Bool, loadbalance::Bool, precision::Type, boundary_layer_parameterization::Oceananigans.TurbulenceClosures.RiBasedVerticalDiffusivity{Oceananigans.TurbulenceClosures.VerticallyImplicitTimeDiscretization, Float64, Oceananigans.TurbulenceClosures.HyperbolicTangentRiDependentTapering})
    @ OceanScalingTests ~/src/OceanScalingTests.jl/src/near_global_simulation.jl:64
 [10] macro expansion
    @ ~/src/OceanScalingTests.jl/src/OceanScalingTests.jl:54 [inlined]
 [11] macro expansion
    @ ~/.julia/packages/PrecompileTools/L8A3n/src/workloads.jl:78 [inlined]
 [12] macro expansion
    @ ~/src/OceanScalingTests.jl/src/OceanScalingTests.jl:53 [inlined]
 [13] macro expansion
    @ ~/.julia/packages/PrecompileTools/L8A3n/src/workloads.jl:140 [inlined]
 [14] top-level scope
    @ ~/src/OceanScalingTests.jl/src/OceanScalingTests.jl:32
 [15] include
    @ ./Base.jl:556 [inlined]
 [16] 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:2664
 [17] top-level scope
    @ stdin:4
in expression starting at /home/vchuravy/src/OceanScalingTests.jl/src/OceanScalingTests.jl:1
in expression starting at stdin:

Caused by @eval. Note that @eval uses the current module and not the module the user is calling this function from.
This means we are trying to modify the Oceananigans after it has already been closed. This is imcompatible with precompilation since we are unable to track and restore this modification.

My intuition is that you probably just want a dictionary for these kind of globals, maybe even within the model?
Instead of using global variables.

Oceananigans.jl/src/Grids/latitude_longitude_grid.jl

Lines 554 to 555 in 00f028b

@eval $parentM = zeros($FT, $metric_size...)
@eval $metric = OffsetArray(on_architecture($arch, $parentM), $offsets...)

The use-case is shown in simone-silvestri/OceanScalingTests.jl#8 where one wants to use PrecompileTools to cache important functions.
@simone-silvestri simone-silvestri linked a pull request Apr 18, 2024 that will close this issue
Open
@glwagner
Copy link
Member

glwagner commented Apr 19, 2024 •
edited

@simone-silvestri are you sure it's a good idea to use OceanScalingTests for this purpose? It might be better to put together something longer term in ClimaOcean

@vchuravy
Copy link
Collaborator Author

I just needed an example and I have some old data from last year. I am purely interested in the time it takes until the first time step completes.

The issues I identified so far with the use of eval would occur for any other example

@navidcy navidcy added the performance 🏍️ So we can get the wrong answer even faster label Apr 22, 2024
@glwagner
Copy link
Member

Ok if its just for a quick test rather than extended work, that makes sense

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
performance 🏍️ So we can get the wrong answer even faster
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove performance-/precompilation-time harmful @eval CliMA/Oceananigans.jl
3 participants
@vchuravy @navidcy @glwagner