Skip to content

Commit

Permalink
Use Base.require() to load Distributed internally (#54571)
Browse files Browse the repository at this point in the history
TL;DR: we changed to loading stdlibs exclusively from the default sysimg
in #53326, and this breaks
developing Distributed.jl because the workers will load the builtin
version instead of the development version. I think this should be
backported to 1.11?

CC @jpsamaroo

---

`Base.require_stdlib()` will exclusively load a stdlib from whatever was
shipped with Julia. The problem is that when developing Distributed.jl
this will cause the workers to always load the builtin Distributed
module instead of the development version, which can break everything
because now the master and worker are running different (potentially
incompatible) versions of Distributed.

This commit changes Base and LinearAlgebra to use `Base.require()`
instead, which will respect `Base.LOAD_PATH`. I argue that this is safe
because unlike the other stdlibs like REPL or Pkg, Distributed is only
loaded if explicitly requested by the user with `-p` or through
`addprocs()` or something, so it shouldn't be possible to get into quite
the same
tools-are-broken-because-I-broke-my-tools situation that motivated using
`Base.require_stdlib()` in the first place.

An alternative design would be to:
1. Move the if-block loading Distributed in `exec_options()` below the
for-loop where it will execute `-e` options.
2. Require any implementation of `Distributed.launch(::ClusterManager)`
to pass `-e 'using Distributed'` in their command to ensure that
Distributed is loaded in a way respecting `Base.LOAD_PATH`.

This would be more consistent with how the other stdlibs must be
developed, but it requires implementers (i.e. Distributed and
ClusterManagers.jl) to opt-in to allowing development versions of
Distributed, which feels very annoying and easy to miss so I decided not
to implement that.

(cherry picked from commit 3b922b0)
  • Loading branch information
JamesWrigley authored and KristofferC committed May 27, 2024
1 parent a6796b9 commit af49d9f
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 2 deletions.
2 changes: 1 addition & 1 deletion base/client.jl
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ function exec_options(opts)
# Load Distributed module only if any of the Distributed options have been specified.
distributed_mode = (opts.worker == 1) || (opts.nprocs > 0) || (opts.machine_file != C_NULL)
if distributed_mode
let Distributed = require_stdlib(PkgId(UUID((0x8ba89e20_285c_5b6f, 0x9357_94700520ee1b)), "Distributed"))
let Distributed = require(PkgId(UUID((0x8ba89e20_285c_5b6f, 0x9357_94700520ee1b)), "Distributed"))
Core.eval(MainInclude, :(const Distributed = $Distributed))
Core.eval(Main, :(using Base.MainInclude.Distributed))
end
Expand Down
2 changes: 1 addition & 1 deletion stdlib/LinearAlgebra/src/LinearAlgebra.jl
Original file line number Diff line number Diff line change
Expand Up @@ -717,7 +717,7 @@ function peakflops(n::Integer=4096; eltype::DataType=Float64, ntrials::Integer=3
end

if parallel
let Distributed = Base.require_stdlib(Base.PkgId(
let Distributed = Base.require(Base.PkgId(
Base.UUID((0x8ba89e20_285c_5b6f, 0x9357_94700520ee1b)), "Distributed"))
nworkers = @invokelatest Distributed.nworkers()
results = @invokelatest Distributed.pmap(peakflops, fill(n, nworkers))
Expand Down

0 comments on commit af49d9f

Please sign in to comment.